src/util.rs (1)

406-411: Same misleading error mapping for seller side

Please apply the same fix here as in the buyer branch to keep error semantics consistent.

src/main.rs (1)

75-75: Consider implementing the database connection pooling suggestion

The TODO comment correctly identifies a potential improvement. Moving the database connection pool to a shared reference-counted container would prevent database reinitialization across threads.

Consider implementing this using an Arc<Pool> stored in a static OnceLock, similar to how MOSTRO_DB_PASSWORD is managed.

src/app/take_sell.rs (1)

53-55: Consider performance impact of buyer_has_pending_order.

Based on the relevant code snippets from src/db.rs, the buyer_has_pending_order function now needs to decrypt all pending orders' master buyer pubkeys to check for matches. This could cause performance issues as the number of pending orders grows.

Consider implementing a caching mechanism similar to the one mentioned in the PR comments, or explore indexing strategies that would minimize the number of decryptions needed.

src/util.rs (1)

548-552: Left-over debugging println!

The println! statement will spam STDOUT in production.
Switch to tracing::debug! (or remove entirely).

-    println!(
-        "Inside update_order_event order_updated status {:?} - order id {:?}",
-        order_updated.status, order_updated.id,
-    );
+    tracing::debug!(
+        "update_order_event → status {:?} - order id {:?}",
+        order_updated.status,
+        order_updated.id,
+    );

src/db.rs (3)

161-201: Unnecessary confirmation prompt when password is skipped

If the admin presses to skip encryption you still prompt for confirmation, forcing the user to hit twice.

Add an early return when password.is_empty() to streamline UX:

-        if password.is_empty() {
-            println!("Password skipped!!");
-            break;
-        } else {
-            // Confirm password
-            print!("Confirm database password: ");
-        }
+        if password.is_empty() {
+            println!("Password skipped – database will stay unencrypted.");
+            break;
+        }
+        // Confirm password
+        print!("Confirm database password: ");

---

300-338: Redundant double parsing of Argon2 hash

hash_password already returns a PasswordHash string ready for storage; immediately parsing it again and converting back to string adds cost and cognitive load.

-    let key = argon2
+    let password_hash = argon2
         .hash_password(password.expose_secret().as_bytes(), &salt)?
         .to_string();
-
-    // Create a key suitable for ChaCha20Poly1305
-    let key = PasswordHash::new(&key)?.to_string();

Store password_hash directly.

---

35-50: Permission hardening bypassed on Windows

On Windows the function silently prints a warning but still returns Ok(()), leaving the DB wide-open.
Either:

1. Abort the start-up when secure permissions cannot be set, or
2. Provide an actionable message (e.g., “run icacls ...”) and clearly mark the instance as unsafe.

Fail-fast is preferable to avoid a false sense of security.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/db.rs (2)

218-236: Blocking STDIN in an async context can stall the reactor

get_user_password() performs synchronous read_password() calls inside the
async connect() path. On Tokio’s multithreaded runtime this blocks an
executor thread and may introduce latency spikes.

Consider isolating the prompt in a tokio::task::spawn_blocking closure or
moving database initialisation to a dedicated synchronous bootstrap step
executed before the async runtime starts.

---

784-816: Linear scan & decrypt in hot path may hurt performance

Both seller_has_pending_order and buyer_has_pending_order fetch all
pending orders and decrypt each key to find a match.
With a large order table this becomes O(n × decrypt_cost).

Ideas:

1. Store a deterministic, non-reversible hash (e.g. SHA-256) of the master
   pubkey next to the ciphertext and index that column; compare hashes before
   decrypting.
2. Keep an in-memory HashSet of decrypted keys populated at startup and
   updated through triggers/callbacks.

This keeps O(1) lookup per request while preserving on-disk secrecy.

src/app/rate_user.rs (1)

101-104: Avoid moving counterpart when borrowing suffices

is_user_present only needs an immutable reference to the key; passing the
owned String forces an unnecessary move.

-    let mut user_to_vote = is_user_present(pool, counterpart)
+    let mut user_to_vote = is_user_present(pool, counterpart.clone())

Minor, but keeps counterpart available for future use without another clone.

src/app/release.rs (1)

397-405: identity_pubkey is moved before reuse

identity_pubkey is moved into is_user_present; if later logic needs the
same value (e.g. for logging), it will be unavailable.

-        let user = crate::db::is_user_present(&pool, identity_pubkey)
+        let user = crate::db::is_user_present(&pool, identity_pubkey.clone())

This preserves the original string for any subsequent operations at negligible
cost.

src/app/admin_take_dispute.rs (1)

30-37: Consider caching the global password retrieval

MOSTRO_DB_PASSWORD.get() is inexpensive, but the expression appears several
times in tight loops. Binding it once improves readability and avoids repeated
calls.

let db_pwd = MOSTRO_DB_PASSWORD.get();
let master_buyer_key  = order.get_master_buyer_pubkey(db_pwd)?;
// …

Purely stylistic; feel free to skip if you prefer the explicit calls.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/db.rs (4)

53-120: Password requirements implementation follows security best practices.

The password validation logic implements strong requirements (12+ chars, mixed case, digits, special chars) with clear error messages. The implementation allows bypassing requirements for empty passwords, which appears to be an intentional design choice to make encryption optional.

Consider using a more specific check for special characters rather than !c.is_alphanumeric() which also includes spaces and other non-printing characters:

-        if self.requires_special && !password.chars().any(|c| !c.is_alphanumeric()) {
+        if self.requires_special && !password.chars().any(|c| "!@#$%^&*()_+-=[]{}|;:,.<>?/".contains(c)) {

---

273-294: Secure authentication with retry limits for existing databases.

The implementation provides a good security practice of limiting password attempts to prevent brute force attacks, with appropriate termination on repeated failures.

Consider using a cleaner loop structure for retry attempts:

-        let max_attempts = 3;
-        let mut attempts = 0;
-
-        if MOSTRO_DB_PASSWORD.get().is_none() {
-            while let Some(argon2_hash) = get_admin_password(&conn).await? {
-                // Database already exists - and yet opened
-                let parsed_hash = PasswordHash::new(&argon2_hash)
-                    .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
-                if check_password_hash(&parsed_hash).is_ok() {
-                    break;
-                } else {
-                    attempts += 1;
-                    println!("Wrong password, attempts: {}", attempts);
-                    if attempts >= max_attempts {
-                        println!("Maximum password attempts exceeded!!");
-                        std::process::exit(1);
-                    }
-                }
-            }
-        }
+        const MAX_ATTEMPTS: usize = 3;
+        
+        if MOSTRO_DB_PASSWORD.get().is_none() {
+            if let Some(argon2_hash) = get_admin_password(&conn).await? {
+                let parsed_hash = PasswordHash::new(&argon2_hash)
+                    .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
+                
+                for attempt in 1..=MAX_ATTEMPTS {
+                    if check_password_hash(&parsed_hash).is_ok() {
+                        break;
+                    }
+                    
+                    println!("Wrong password, attempts: {}/{}", attempt, MAX_ATTEMPTS);
+                    if attempt >= MAX_ATTEMPTS {
+                        println!("Maximum password attempts exceeded!!");
+                        std::process::exit(1);
+                    }
+                }
+            }
+        }

---

300-334: Strong password hashing implementation with Argon2.

The function correctly generates a random salt and uses Argon2 for secure password hashing, following cryptographic best practices.

The comment on line 322 refers to storing "the key and salt securely", but only the key (password hash) is stored. Consider updating the comment for clarity:

-    // Store the key and salt securely (e.g., in a file or database)
+    // Store the password hash securely in the database

---

959-1057: Good test coverage for encryption and decryption functionality.

The test module properly tests the encryption and decryption of stored strings in an in-memory SQLite database, which is valuable for validating the core functionality of the encryption feature.

Consider these improvements to the test:

1. Use a constant for the number of test entries rather than hardcoding the value in multiple places
2. Instead of using unwrap() which would cause panics, propagate errors consistently using the ? operator
3. Either use a consistent approach for salts (all fixed or all random) or explicitly document why you're testing both approaches

        for i in 0..total_entries {
            let value_string = format!("Entry {}", i % 5);
            println!("Inserting value {}", value_string);
-            let value_string = if i % 3 == 1 {
-                store_encrypted(&value_string, Some(&password), Some(salt))
-                    .await
-                    .unwrap() // Encrypt the string
-                              // Note: id is INTEGER PRIMARY KEY, so it will auto-increment if we don't specify it,
-                              // or we can specify it like this. Let's specify for clarity.
-            } else {
-                store_encrypted(&value_string, Some(&password), None)
-                    .await
-                    .unwrap() // Encrypt the string
-            };
+            // Test both fixed and random salts
+            let salt_option = if i % 3 == 1 { Some(salt) } else { None };
+            let value_string = store_encrypted(&value_string, Some(&password), salt_option)
+                .await
+                .map_err(|e| Error::RowNotFound)?; // Convert the error to sqlx::Error

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.522Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

src/db.rs (5)

219-221: Redundant TODO comment on restricting file permissions

The TODO comment about checking if the permission restriction works on Windows is misleading since the code already handles Windows by using cfg attributes.

-        // TODO: check if this is works on windows
         restrict_file_permissions(db_path)
             .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;

---

235-239: Error handling inconsistency with password operations

The password hash storage error is converted to a DbAccessError which isn't sufficiently specific about the actual operation failing.

Consider using a more specific error variant for password handling operations:

         if let Some(password) = MOSTRO_DB_PASSWORD.get() {
-            store_password_hash(password, &pool).await.map_err(|e| {
-                MostroInternalErr(ServiceError::DbAccessError(e.to_string()))
-            })?;
+            store_password_hash(password, &pool).await?;
         }

This assumes store_password_hash already returns an appropriate MostroError type. If not, consider adding a specific error variant for password operations.

---

282-291: Improve user feedback for password attempts

The current implementation shows "Wrong password, attempts: X" without indicating how many attempts are allowed.

Make the password attempt feedback more user-friendly:

-                    println!("Wrong password, attempts: {}", attempts);
+                    println!("Wrong password. {} of {} attempts used.", attempts, max_attempts);
                     if attempts >= max_attempts {
-                        println!("Maximum password attempts exceeded!!");
+                        println!("Maximum password attempts exceeded. Please contact the administrator.");
                         std::process::exit(1);
                     }

---

792-798: Fix typo in "Foundd" comment

There's a typo in the comment which indicates a lack of attention to detail.

            if master_pubkey_decrypted == pubkey {
-                // Foundd another waiting-buyer-invoice order with the same pubkey
+                // Found another waiting-payment order with the same pubkey
                 return Ok(true);
            }

---

845-848: Fix typo in "Foundd" comment here too

Same typo appears in another comment.

            if master_pubkey_decrypted == pubkey {
-                // Foundd another waiting-buyer-invoice order with the same pubkey
+                // Found another waiting-buyer-invoice order with the same pubkey
                 return Ok(true);
            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.522Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

src/db.rs (6)

214-267: Consider removing database file on password setup failure

If the get_user_password() or store_password_hash() functions fail after the database file is created, the file remains but might be in an inconsistent state. Consider extending the error handling to clean up the file in these cases as well.

 match sqlx::migrate!().run(&pool).await {
     Ok(_) => {
         tracing::info!(
             "Successfully created database file at {}",
             db_path.display(),
         );
-        // Get user password
-        get_user_password().await?;
-        // Save admin password hash securely
-        if let Some(password) = MOSTRO_DB_PASSWORD.get() {
-            store_password_hash(password, &pool).await.map_err(|e| {
-                MostroInternalErr(ServiceError::DbAccessError(e.to_string()))
-            })?;
-        }
-        pool
+        // Get user password
+        match get_user_password().await {
+            Ok(_) => {
+                // Save admin password hash securely
+                if let Some(password) = MOSTRO_DB_PASSWORD.get() {
+                    match store_password_hash(password, &pool).await {
+                        Ok(_) => pool,
+                        Err(e) => {
+                            if let Err(cleanup_err) = std::fs::remove_file(db_path) {
+                                tracing::error!(
+                                    error = %cleanup_err,
+                                    path = %db_path.display(),
+                                    "Failed to clean up database file after password storage failure"
+                                );
+                            }
+                            return Err(MostroInternalErr(ServiceError::DbAccessError(e.to_string())))
+                        }
+                    }
+                } else {
+                    pool
+                }
+            },
+            Err(e) => {
+                if let Err(cleanup_err) = std::fs::remove_file(db_path) {
+                    tracing::error!(
+                        error = %cleanup_err,
+                        path = %db_path.display(),
+                        "Failed to clean up database file after password setup failure"
+                    );
+                }
+                return Err(e)
+            }
+        }
     }
     Err(e) => {

---

780-813: Add caching for decrypted pubkeys in seller_has_pending_order

The function decrypts potentially many public keys in a loop, which could be inefficient. Consider implementing a cache to avoid repeated decryption of the same key, especially in cases where a seller might have multiple pending orders.

 // Check if database is encrypted
 if MOSTRO_DB_PASSWORD.get().is_some() {
+    // Cache for decrypted pubkeys to avoid repeated decryption
+    let mut decrypted_cache = std::collections::HashMap::new();
+    
     let orders_to_check: Vec<String> = sqlx::query_scalar(
         r#"
             SELECT master_seller_pubkey FROM orders WHERE status = 'waiting-payment'
         "#,
     )
     .fetch_all(&mut conn)
     .await
     .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
     // search for a waiting-buyer-invoice order with the same pubkey
     for master_key in orders_to_check {
-        // Decrypt master buyer pubkey
-        let master_pubkey_decrypted =
-            decrypt_data(master_key, MOSTRO_DB_PASSWORD.get()).map_err(MostroInternalErr)?;
+        // Check cache first, decrypt only if not found
+        let master_pubkey_decrypted = if let Some(decrypted) = decrypted_cache.get(&master_key) {
+            decrypted.clone()
+        } else {
+            let decrypted = decrypt_data(master_key.clone(), MOSTRO_DB_PASSWORD.get()).map_err(MostroInternalErr)?;
+            decrypted_cache.insert(master_key, decrypted.clone());
+            decrypted
+        };
         if master_pubkey_decrypted == pubkey {
             // Foundd another waiting-buyer-invoice order with the same pubkey
             return Ok(true);

---

831-864: Add caching for decrypted pubkeys in buyer_has_pending_order

Similar to the seller function, this function decrypts potentially many public keys in a loop. Implementing a cache would improve performance.

 // Check if database is encrypted
 if MOSTRO_DB_PASSWORD.get().is_some() {
+    // Cache for decrypted pubkeys to avoid repeated decryption
+    let mut decrypted_cache = std::collections::HashMap::new();
+    
     let orders_to_check: Vec<String> = sqlx::query_scalar(
         r#"
             SELECT master_buyer_pubkey FROM orders WHERE status = 'waiting-buyer-invoice'
         "#,
     )
     .fetch_all(&mut conn)
     .await
     .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
     // search for a waiting-buyer-invoice order with the same pubkey
     for master_key in orders_to_check {
-        // Decrypt master buyer pubkey
-        let master_pubkey_decrypted =
-            decrypt_data(master_key, MOSTRO_DB_PASSWORD.get()).map_err(MostroInternalErr)?;
+        // Check cache first, decrypt only if not found
+        let master_pubkey_decrypted = if let Some(decrypted) = decrypted_cache.get(&master_key) {
+            decrypted.clone()
+        } else {
+            let decrypted = decrypt_data(master_key.clone(), MOSTRO_DB_PASSWORD.get()).map_err(MostroInternalErr)?;
+            decrypted_cache.insert(master_key, decrypted.clone());
+            decrypted
+        };
         if master_pubkey_decrypted == pubkey {
             // Foundd another waiting-buyer-invoice order with the same pubkey
             return Ok(true);

---

795-798: Fix typo in comment

There's a typo in both seller_has_pending_order and buyer_has_pending_order functions: "Foundd" should be "Found".

 if master_pubkey_decrypted == pubkey {
-    // Foundd another waiting-buyer-invoice order with the same pubkey
+    // Found another waiting-buyer-invoice order with the same pubkey
     return Ok(true);
 }

Also applies to: 846-849

---

959-1057: Test function looks good, but consider adding more comprehensive tests

The test for encryption/decryption functionality is well-implemented. Consider adding tests for other scenarios:

1. Empty password handling
2. Password validation
3. Testing password attempt limits
4. Performance comparison with and without caching

---

960-998: Refactor test to use constants for predictable test cases

For more predictable test results and clearer assertions, refactor the test to use predefined test data instead of generating entries in a loop with varying conditions.

 #[tokio::test]
 async fn test_fetch_string_column_scalar() -> Result<(), Error> {
     // 1. Setup: Create in-memory DB and table
     let pool = setup_db().await?;
     println!("In-memory database and table created for test.");

-    // 2. Populate: Insert 100 entries
-    let total_entries = 10;
+    // 2. Populate: Insert test entries with known values
+    let test_entries = [
+        "Test Entry 1",
+        "Test Entry 2",
+        "Test Entry 3",
+        "Test Entry 4",
+        "Test Entry 5",
+    ];

     // Use a SecretString for the password
     let password = SecretString::from(SECRET_PASSWORD);

-    println!("Inserting {} entries...", total_entries);
-    for i in 0..total_entries {
-        let value_string = format!("Entry {}", i % 5);
-        println!("Inserting value {}", value_string);
-        let value_string = if i % 3 == 1 {
-            store_encrypted(&value_string, Some(&password))
-                .await
-                .unwrap() // Encrypt the string
-                          // Note: id is INTEGER PRIMARY KEY, so it will auto-increment if we don't specify it,
-                          // or we can specify it like this. Let's specify for clarity.
-        } else {
-            store_encrypted(&value_string, Some(&password))
-                .await
-                .unwrap() // Encrypt the string
-        };
+    println!("Inserting {} entries...", test_entries.len());
+    for (i, entry) in test_entries.iter().enumerate() {
+        println!("Inserting value {}", entry);
+        let encrypted_string = store_encrypted(entry, Some(&password))
+            .await
+            .unwrap(); // Encrypt the string

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

src/db.rs (2)

53-120: Password strength validation is well-implemented but has a small optimization opportunity.

The password requirements implementation is thorough with clear validation messages. The intentional handling of empty passwords (line 101-103) aligns with the project's approach of making encryption optional.

In the is_strong_password method, you're calling validate twice unnecessarily:

fn is_strong_password(&self, password: &str) -> bool {
-    match self.validate(password).is_empty() {
+    let failures = self.validate(password);
+    match failures.is_empty() {
        true => true,
        false => {
            println!("\nPassword is not strong enough:");
-            for failure in self.validate(password) {
+            for failure in failures {
                println!("- {}", failure);
            }
            false
        }
    }
}

---

779-813: Encryption-aware pending order check with proper error handling.

The implementation correctly handles both encrypted and non-encrypted database scenarios. For encrypted databases, it fetches all relevant orders and decrypts each key for comparison.

I notice a small typo in line 796: "Foundd" should be "Found", but this is just a comment and doesn't affect functionality.

-            // Foundd another waiting-buyer-invoice order with the same pubkey
+            // Found another waiting-buyer-invoice order with the same pubkey

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:122-145
Timestamp: 2025-04-28T13:21:36.613Z
Learning: Adding a small delay (e.g., 100ms) before password verification operations is a good security practice to mitigate brute force attacks by limiting the rate at which an attacker can attempt different passwords.

src/db.rs (2)

797-815: Fix typo in comment

There's a small typo in the comment.

-            // Foundd another waiting-buyer-invoice order with the same pubkey
+            // Found another waiting-buyer-invoice order with the same pubkey

---

863-864: Fix typo in comment

Same typo appears here.

-            // Foundd another waiting-buyer-invoice order with the same pubkey
+            // Found another waiting-buyer-invoice order with the same pubkey

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:122-145
Timestamp: 2025-04-28T13:21:36.613Z
Learning: Adding a small delay (e.g., 100ms) before password verification operations is a good security practice to mitigate brute force attacks by limiting the rate at which an attacker can attempt different passwords.

src/db.rs (2)

805-837: 🛠️ Refactor suggestion

Apply the same caching mechanism here for consistent implementation.

Similar to the seller pending order check, the buyer pending order check would benefit from the same caching mechanism to improve performance with large numbers of orders.

---

759-791: 🛠️ Refactor suggestion

Consider caching decrypted keys for better performance.

The approach of decrypting all master seller keys for every pending order check could become performance-intensive as the database grows. Consider implementing a caching mechanism for recently decrypted keys to reduce repeated computations.

+ // Add this at the module level with appropriate mutex protection
+ use std::collections::HashMap;
+ use std::sync::Mutex;
+ lazy_static! {
+     static ref DECRYPTED_KEYS_CACHE: Mutex<HashMap<String, String>> = Mutex::new(HashMap::new());
+ }
+
  // Inside the function where decryption happens
  if MOSTRO_DB_PASSWORD.get().is_some() {
      let orders_to_check: Vec<String> = sqlx::query_scalar(...)
          .fetch_all(pool)
          .await
          .map_err(...)?;
      
      for master_key in orders_to_check {
+         // Check cache first
+         let master_pubkey_decrypted = {
+             let mut cache = DECRYPTED_KEYS_CACHE.lock().unwrap();
+             if let Some(cached) = cache.get(&master_key) {
+                 cached.clone()
+             } else {
+                 let decrypted = decrypt_data(master_key.clone(), MOSTRO_DB_PASSWORD.get())
+                     .map_err(MostroInternalErr)?;
+                 // Cache with reasonable size limit
+                 if cache.len() > 1000 {
+                     // Simple eviction policy - clear half the cache when full
+                     let keys_to_remove: Vec<String> = cache.keys()
+                         .take(cache.len() / 2)
+                         .cloned()
+                         .collect();
+                     for key in keys_to_remove {
+                         cache.remove(&key);
+                     }
+                 }
+                 cache.insert(master_key.clone(), decrypted.clone());
+                 decrypted
+             }
+         };
-         // Decrypt master seller pubkey
-         let master_pubkey_decrypted =
-             decrypt_data(master_key, MOSTRO_DB_PASSWORD.get()).map_err(MostroInternalErr)?;
          if master_pubkey_decrypted == pubkey {
              return Ok(true);
          }
      }
  }

src/db.rs (3)

52-106: Clarify the intentional bypass of validation for empty passwords.

The password validation logic includes a special case that clears all validation failures if the password is empty. While this might be intentional to allow skipping encryption, it should be explicitly documented for clarity.

 // If password is empty, clear failures
 if password.is_empty() {
+    // Empty password is allowed to support optional encryption
     failures.clear();
 }

---

933-1030: Add more assertions to strengthen the test.

The encryption/decryption test is well-structured but could benefit from more explicit assertions to verify the expected behavior beyond just checking for success.

 // At line 1021
 if hash_set_values.contains("Entry 0") {
     println!("Entry 0 found in the hash set.");
 } else {
     println!("Entry 0 not found in the hash set.");
 }
+// Add explicit assertions to verify expected behavior
+assert!(hash_set_values.contains("Entry 0"), "Should contain Entry 0");
+assert!(hash_set_values.contains("Entry 1"), "Should contain Entry 1");
+assert!(hash_set_values.contains("Entry 2"), "Should contain Entry 2");
+assert!(hash_set_values.contains("Entry 3"), "Should contain Entry 3");
+assert!(hash_set_values.contains("Entry 4"), "Should contain Entry 4");
+assert_eq!(hash_set_values.len(), 5, "Should have exactly 5 unique entries");

---

994-998: Consider batching database insertions for better performance in tests.

For large test datasets, using a batch insert approach could significantly improve test execution time.

-// Individual inserts
-for i in 0..total_entries {
-    let value_string = format!("Entry {}", i % 5);
-    println!("Inserting value {}", value_string);
-    let value_string = if i % 3 == 1 {
-        store_encrypted(&value_string, Some(&password))
-            .await
-            .unwrap()
-    } else {
-        store_encrypted(&value_string, Some(&password))
-            .await
-            .unwrap()
-    };
-    sqlx::query("INSERT INTO items (id, value) VALUES (?, ?)")
-        .bind(i as i64)
-        .bind(&value_string)
-        .execute(&pool)
-        .await?;
-}
+// Prepare batch values
+let mut query_builder = String::from("INSERT INTO items (id, value) VALUES ");
+let mut params = Vec::new();
+
+for i in 0..total_entries {
+    let value_string = format!("Entry {}", i % 5);
+    println!("Inserting value {}", value_string);
+    let encrypted_value = store_encrypted(&value_string, Some(&password))
+        .await
+        .unwrap();
+    
+    if i > 0 {
+        query_builder.push_str(", ");
+    }
+    query_builder.push_str(&format!("({}, ?)", i));
+    params.push(encrypted_value);
+}
+
+// Execute batch insert
+let mut query = sqlx::query(&query_builder);
+for param in params {
+    query = query.bind(param);
+}
+query.execute(&pool).await?;

Note: This is a performance optimization suggestion for the test code, but may not be necessary for the small number of entries (10) currently used.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:122-145
Timestamp: 2025-04-28T13:21:36.613Z
Learning: Adding a small delay (e.g., 100ms) before password verification operations is a good security practice to mitigate brute force attacks by limiting the rate at which an attacker can attempt different passwords.

src/db.rs (2)

52-121: Robust password validation implementation with room for minor improvement.

The password requirements implementation is thorough and provides good security validation. The empty password handling with the comment on line 102 clarifies the intent.

Consider implementing a strength score calculation method rather than just binary validation, which would allow for more nuanced feedback to users based on password complexity.

---

759-793: Secure seller pending order check with encrypted key support.

The implementation properly handles both encrypted and unencrypted database states, providing a fallback mechanism for unencrypted databases.

There's a typo in the comment on line 775: "Foundd" should be "Found".

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/db.rs (4)

228-255: Consider handling database creation failure more gracefully

When setting up a new database, the code currently exits the application if password setup fails. Consider handling this more gracefully to prevent data loss.

  // Get user password
- get_user_password().await?;
+ match get_user_password().await {
+     Ok(_) => {}
+     Err(e) => {
+         tracing::error!("Failed to set up database password: {}", e);
+         println!("Failed to set up database password. Continuing without encryption.");
+     }
+ }
  
  // Save admin password hash securely
  if let Some(password) = MOSTRO_DB_PASSWORD.get() {
      store_password_hash(password, &pool).await.map_err(|e| {
          MostroInternalErr(ServiceError::DbAccessError(e.to_string()))
      })?;
  }

---

282-311: Three-attempt password verification with forced exit

The implementation of a three-attempt limit with application exit on failure is a strong security practice. This prevents brute force attacks by terminating the program after three failures.

Consider logging these failed attempts to help identify potential attacks.

  println!("Wrong password, attempts: {}", attempts);
+ tracing::warn!("Failed database password attempt {}/{}", attempts, max_attempts);
  if attempts >= max_attempts {
      println!("Maximum password attempts exceeded!!");
+     tracing::error!("Maximum password attempts exceeded, terminating application");
      std::process::exit(1);
  }

---

797-841: Duplicate implementation pattern in buyer and seller functions

The pattern for checking encrypted/unencrypted databases is identical in both seller_has_pending_order and buyer_has_pending_order, with only the field and status names changed.

Consider extracting this pattern into a helper function to reduce code duplication:

async fn has_pending_order_with_status(
    pool: &SqlitePool,
    pubkey: String,
    master_key_field: &str,
    status: &str
) -> Result<bool, MostroError> {
    // Validate public key format (32-bytes hex)
    if !pubkey.chars().all(|c| c.is_ascii_hexdigit()) || pubkey.len() != 64 {
        return Err(MostroCantDo(CantDoReason::InvalidPubkey));
    }

    // Check if database is encrypted
    if MOSTRO_DB_PASSWORD.get().is_some() {
        let orders_to_check: Vec<String> = sqlx::query_scalar(
            &format!("SELECT {} FROM orders WHERE status = ?", master_key_field)
        )
        .bind(status)
        .fetch_all(pool)
        .await
        .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
        
        // search for orders with the same pubkey
        for master_key in orders_to_check {
            // Decrypt master pubkey
            let master_pubkey_decrypted =
                decrypt_data(master_key, MOSTRO_DB_PASSWORD.get()).map_err(MostroInternalErr)?;
            if master_pubkey_decrypted == pubkey {
                return Ok(true);
            }
        }
        Ok(false)
    }
    // if not encrypted, use the default search
    else {
        let exists = sqlx::query_scalar::<_, bool>(
            &format!("SELECT EXISTS (SELECT 1 FROM orders WHERE {} = ? AND status = ?)", master_key_field)
        )
        .bind(pubkey)
        .bind(status)
        .fetch_one(pool)
        .await
        .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
        Ok(exists)
    }
}

Then call it with the appropriate parameters:

pub async fn buyer_has_pending_order(pool: &SqlitePool, pubkey: String) -> Result<bool, MostroError> {
    has_pending_order_with_status(pool, pubkey, "master_buyer_pubkey", "waiting-buyer-invoice").await
}

pub async fn seller_has_pending_order(pool: &SqlitePool, pubkey: String) -> Result<bool, MostroError> {
    has_pending_order_with_status(pool, pubkey, "master_seller_pubkey", "waiting-payment").await
}

---

936-1062: Comprehensive encryption test with performance tracking

The test module thoroughly verifies the encryption/decryption functionality:

• Creates in-memory test database
• Inserts multiple encrypted values
• Retrieves and decrypts them
• Verifies correctness with assertions
• Measures decryption performance

The performance tracking is valuable for understanding the real-world impact of encryption on the application.

One minor concern: The test uses a deterministic salt pattern similar to the main code. Consider using random salts in the test as well.

- let salt_base = b"1H/aaYsf8&asduA";
- for i in 0..total_entries {
-     let salt = format!("{}{}", String::from_utf8_lossy(salt_base), i % 5);
-     salt_vec.push(SaltString::encode_b64(salt.as_bytes()).unwrap());
- }
+ // Generate random salts for testing
+ for _ in 0..total_entries {
+     salt_vec.push(SaltString::generate(&mut rand_core::OsRng));
+ }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:122-145
Timestamp: 2025-04-28T13:21:36.613Z
Learning: Adding a small delay (e.g., 100ms) before password verification operations is a good security practice to mitigate brute force attacks by limiting the rate at which an attacker can attempt different passwords.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:0-0
Timestamp: 2025-05-08T13:07:29.455Z
Learning: For the database encryption in Mostro, the developer prefers caching only derived encryption keys rather than caching decrypted master keys in RAM, as a security measure to reduce the attack surface from potential memory dumps or runtime vulnerabilities.

src/db.rs (2)

322-338: Simplify salt generation since only the first salt is used

You're generating 50 salts but only using the first one. If deterministic salt generation is intentional for testing purposes, you can simplify this to just generate the one needed salt.

- // Generate a random salt
- let salt_base = b"1H/aaYsf8&asduA";
- let mut salt_vec: Vec<SaltString> = Vec::new();
- for i in 0..50 {
-     let salt = format!("{}{}", String::from_utf8_lossy(salt_base), i % 5);
-     salt_vec.push(SaltString::encode_b64(salt.as_bytes()).unwrap());
- }
- 
- // Configure Argon2 parameters
- let argon2 = Argon2::default();
- 
- // Derive the key
- let key = argon2
-     .hash_password(password.expose_secret().as_bytes(), &salt_vec[0])
-     .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?
-     .to_string();
+ // Generate a deterministic salt for testing
+ let salt_base = b"1H/aaYsf8&asduA";
+ let salt = format!("{}{}", String::from_utf8_lossy(salt_base), 0);
+ let salt_string = SaltString::encode_b64(salt.as_bytes())
+     .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
+ 
+ // Configure Argon2 parameters
+ let argon2 = Argon2::default();
+ 
+ // Derive the key
+ let key = argon2
+     .hash_password(password.expose_secret().as_bytes(), &salt_string)
+     .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?
+     .to_string();

---

768-815: Consider optimizing encrypted key searches with indexes

The current implementation decrypts all keys matching a status to find a match, which could become inefficient as the database grows. While your approach of caching derived keys helps with decryption speed, you could further optimize query performance.

Add indexes on the status column to improve query performance:

CREATE INDEX IF NOT EXISTS idx_orders_status ON orders(status);

Additionally, consider adding a comment explaining the performance characteristics and why this approach was chosen over alternatives, to help future maintainers understand the design decisions.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:122-145
Timestamp: 2025-04-28T13:21:36.613Z
Learning: Adding a small delay (e.g., 100ms) before password verification operations is a good security practice to mitigate brute force attacks by limiting the rate at which an attacker can attempt different passwords.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:0-0
Timestamp: 2025-05-08T13:07:29.484Z
Learning: For the database encryption in Mostro, the developer prefers caching only derived encryption keys rather than caching decrypted master keys in RAM, as a security measure to reduce the attack surface from potential memory dumps or runtime vulnerabilities.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:315-354
Timestamp: 2025-05-08T18:10:03.818Z
Learning: In the Mostro codebase, deterministic salt generation is used intentionally in testing functions to benchmark decryption timing and test caching mechanisms. This is a valid testing approach when consistent, reproducible results are needed for performance evaluation.

src/db.rs (5)

20-35: Add docstring to clarify platform-specific behavior

The restrict_file_permissions function lacks a docstring explaining its purpose and how it behaves differently across platforms. This is important since Windows users need to understand the security implications.

+/// Restricts file permissions to only allow access by the owner.
+/// 
+/// On Unix systems, this sets the permissions to 0o600 (owner read/write only).
+/// On Windows, this function only displays a warning as Windows permission model
+/// differs from Unix-like systems. Windows users should ensure proper ACLs are
+/// set manually or store the database in a secure location.
 fn restrict_file_permissions(path: &Path) -> Result<(), MostroError> {
     #[cfg(unix)]
     {
         let perms = Permissions::from_mode(0o600);
         set_permissions(path, perms)
             .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
     }

     #[cfg(windows)]
     {
         // Optional: could integrate with `winapi` or use a placeholder
-        println!("⚠️ Skipping permission change on Windows. Set it manually if needed.");
+        tracing::warn!("Database file permissions cannot be automatically restricted on Windows. Please ensure the file is stored in a secure location with appropriate access controls.");
     }

     Ok(())
 }

---

94-105: Avoid duplicate validation in is_strong_password

The function calls validate twice, which is inefficient. Store the validation result first, then check if it's empty.

 fn is_strong_password(&self, password: &str) -> bool {
-    match self.validate(password).is_empty() {
+    let failures = self.validate(password);
+    match failures.is_empty() {
         true => true,
         false => {
             println!("\nPassword is not strong enough:");
-            for failure in self.validate(password) {
+            for failure in failures {
                 println!("- {}", failure);
             }
             false
         }
     }
 }

---

768-815: Consider indexing for better encrypted lookup performance

The current approach of fetching all orders with a given status and then decrypting each key could become a performance bottleneck as the database grows.

Consider these performance optimizations:

1. Add database indices on the status field to speed up the initial query
2. Process batches of orders in parallel using Tokio's join operations
3. Consider a caching mechanism for frequently accessed decrypted keys

// Example of parallel processing with Tokio
let decryption_tasks = orders_to_check
    .into_iter()
    .map(|master_key| {
        let password = MOSTRO_DB_PASSWORD.get().clone();
        tokio::spawn(async move {
            CryptoUtils::decrypt_data(master_key, Some(password))
        })
    })
    .collect::<Vec<_>>();

// Wait for all tasks to complete
for task in decryption_tasks {
    if let Ok(Ok(decrypted)) = task.await {
        if decrypted == pubkey {
            return Ok(true);
        }
    }
}

---

791-799: Use early return for decryption error handling

The code currently unwraps the decryption result with map_err and ?, which will return early if there's an error. For better error handling and visibility, consider using a more explicit pattern that logs decryption failures.

 // Decrypt master pubkey
-let master_pubkey_decrypted =
-    CryptoUtils::decrypt_data(master_key, MOSTRO_DB_PASSWORD.get())
-        .map_err(MostroInternalErr)?;
-if master_pubkey_decrypted == pubkey {
-    return Ok(true);
+match CryptoUtils::decrypt_data(master_key, MOSTRO_DB_PASSWORD.get()) {
+    Ok(master_pubkey_decrypted) => {
+        if master_pubkey_decrypted == pubkey {
+            return Ok(true);
+        }
+    },
+    Err(e) => {
+        tracing::warn!("Failed to decrypt master key: {}", e);
+        // Continue with next key instead of failing the entire operation
+    }
 }

---

141-207: Consider extracting common password input logic

The password input code in get_user_password and check_password_hash could be extracted into a shared helper function to reduce duplication.

/// Reads a password from stdin with anti-timing attack measures
fn read_password_secure() -> Result<String, MostroError> {
    // Add a random delay to mitigate timing attacks
    let random_delay = rand::random::<u16>() % 1000;
    
    let password = read_password()
        .map_err(|e| MostroInternalErr(ServiceError::DbAccessError(e.to_string())))?;
    
    // Add delay after password input
    std::thread::sleep(std::time::Duration::from_millis(
        100_u64 + random_delay as u64,
    ));
    
    Ok(password)
}

This function could then be used in both places to reduce code duplication.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:122-145
Timestamp: 2025-04-28T13:21:36.613Z
Learning: Adding a small delay (e.g., 100ms) before password verification operations is a good security practice to mitigate brute force attacks by limiting the rate at which an attacker can attempt different passwords.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:0-0
Timestamp: 2025-05-08T13:07:29.484Z
Learning: For the database encryption in Mostro, the developer prefers caching only derived encryption keys rather than caching decrypted master keys in RAM, as a security measure to reduce the attack surface from potential memory dumps or runtime vulnerabilities.

Learnt from: arkanoider
PR: MostroP2P/mostro#475
File: src/db.rs:315-354
Timestamp: 2025-05-08T18:10:03.818Z
Learning: In the Mostro codebase, deterministic salt generation is used intentionally in testing functions to benchmark decryption timing and test caching mechanisms. This is a valid testing approach when consistent, reproducible results are needed for performance evaluation.