src/error.rs (1)

94-95: Consider adding specific error subtypes for better error handling.

The current implementation uses string messages for encryption/decryption errors, which makes programmatic error handling challenging.

Consider defining more specific error subtypes for common encryption failures:

pub enum EncryptionErrorKind {
    KeyDerivationFailed,
    EncryptionFailed,
    InvalidInput,
    Other(String),
}

// Then use it in ServiceError:
EncryptionError(EncryptionErrorKind),
DecryptionError(EncryptionErrorKind),

src/order.rs (3)

198-204: Misleading comment & redundant base-64 round-trip

Comment says “Enecode salt from base64 to bytes” but the code actually encodes raw bytes into base-64 again (encode_b64). Consider simplifying:

let salt = Salt::new(salt)?;   // consumes raw bytes directly

At minimum, fix the comment to avoid future confusion.

---

240-247: Missing Associated Data (AAD) reduces integrity guarantees

cipher.encrypt(&nonce, plaintext) signs only the ciphertext. If you have any contextual metadata (e.g., user id, column name, version), pass it as AAD so tampering with those fields is detectable.

cipher.encrypt(&nonce, aead::Payload {
    msg: idkey.as_bytes(),
    aad: b"user_id_column_v1",    // example
})

Even a constant string thwarts several classes of swapping/re-ordering attacks.

---

254-268: Asynchronous wrapper adds no value; remove async

store_encrypted is marked async but performs no .await except for its own signature, meaning every call spawns a needless future.

-pub async fn store_encrypted(
+pub fn store_encrypted(

This avoids surprising blocking points and simplifies call-sites.

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

src/order.rs (3)

187-196: ⚠️ Potential issue

Missing input validation before processing

The code splits encrypted_bytes without first checking if it's long enough, which could panic if the input is malformed (shorter than NONCE_SIZE + SALT_SIZE).

Add length validation before splitting:

// Decode the encrypted data from base64 to bytes
let encrypted_bytes = BASE64_STANDARD
    .decode(&data)
    .map_err(|_| ServiceError::DecryptionError("Error decoding encrypted data".to_string()))?;

+// Validate input length before processing
+if encrypted_bytes.len() < NONCE_SIZE + SALT_SIZE {
+    return Err(ServiceError::DecryptionError(
+        "Invalid encrypted data: too short for nonce and salt".to_string(),
+    ));
+}

// Split the encrypted data into nonce and data
let (nonce, data) = encrypted_bytes.split_at(NONCE_SIZE);

---

213-219: ⚠️ Potential issue

Argon2 parameters are below recommended security levels

Current parameters use only 16KiB of memory and minimum time cost, which are far below OWASP recommendations for password hashing (≥64MiB, ≥3 iterations).

Increase parameters to improve security against brute force attacks:

let params = Params::new(
-    16 * 1024,             // 16 KiB
-    Params::MIN_T_COST,    // 1
+    64 * 1024 * 1024,      // 64 MiB 
+    3,                     // time cost of 3
    Params::DEFAULT_P_COST * 2,
    Some(Params::DEFAULT_OUTPUT_LEN),
)

These stronger parameters significantly slow down offline attacks without noticeably impacting performance when the key is cached.

---

70-75: ⚠️ Potential issue

Hash collisions can corrupt data silently

The make_cache_key function uses a non-cryptographic DefaultHasher and returns a 64-bit hash, which has insufficient collision resistance for cryptographic applications. A collision would return the wrong encryption key silently, leading to corrupted data rather than a clear error.

Replace with a cryptographic hash function with a larger output space:

-fn make_cache_key(password: &str, salt: &[u8]) -> u64 {
-    let mut hasher = DefaultHasher::new();
-    password.hash(&mut hasher);
-    salt.hash(&mut hasher);
-    hasher.finish()
-}
+use blake3;
+
+fn make_cache_key(password: &str, salt: &[u8]) -> [u8; 32] {
+    let mut input = Vec::with_capacity(password.len() + salt.len());
+    input.extend_from_slice(password.as_bytes());
+    input.extend_from_slice(salt);
+    *blake3::hash(&input).as_bytes()
+}

This change requires updating the SimpleCache implementation to use [u8; 32] as the key type instead of u64.

src/order.rs (2)

176-255: Consider extracting shared key derivation logic

The key derivation logic is duplicated between decrypt_data and store_encrypted functions, which could lead to inconsistencies if one is updated but not the other.

Consider extracting the common logic into a separate function:

fn derive_key(password: &str, salt: &SaltString) -> Result<[u8; 32], ServiceError> {
    let cache_key = make_cache_key(password, salt.as_str().as_bytes());
    let mut cache = KEY_CACHE
        .write()
        .map_err(|e| ServiceError::DecryptionError(format!("Error in key cache: {}", e)))?;
    
    if let Some(cached_key) = cache.get(cache_key) {
        return Ok(cached_key);
    }
    
    let params = Params::new(
        64 * 1024 * 1024,  // 64 MiB
        3,                 // time cost
        Params::DEFAULT_P_COST,
        Some(Params::DEFAULT_OUTPUT_LEN),
    )
    .map_err(|e| ServiceError::DecryptionError(format!("Error creating Argon2 params: {}", e)))?;
    
    let argon2 = Argon2::new(Algorithm::Argon2id, Version::V0x13, params);
    let password_hash = argon2
        .hash_password(password.as_bytes(), salt)
        .map_err(|e| ServiceError::DecryptionError(format!("Error hashing password: {}", e)))?;
    
    let key = password_hash.hash
        .ok_or_else(|| ServiceError::DecryptionError("No hash generated".to_string()))?;
    
    let key_bytes = key.as_bytes();
    if key_bytes.len() != 32 {
        return Err(ServiceError::DecryptionError(
            format!("Key length is {} bytes, expected 32", key_bytes.len()),
        ));
    }
    
    let mut key_array = [0u8; 32];
    key_array.copy_from_slice(key_bytes);
    cache.put(cache_key, key_array);
    
    Ok(key_array)
}

This would simplify both functions and ensure consistent behavior.

Also applies to: 258-327

---

201-201: Error handling discards original error information

The error handling for SaltString::encode_b64 discards the original error message, which could make debugging more difficult.

Include the original error in the message:

let salt = SaltString::encode_b64(salt)
-    .map_err(|_| ServiceError::DecryptionError("Error decoding salt".to_string()))?;
+    .map_err(|e| ServiceError::DecryptionError(format!("Error decoding salt: {}", e)))?;

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.283Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.141Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

src/order.rs (5)

70-75: ⚠️ Potential issue

Hash collisions in make_cache_key can lead to security issues.

Using a 64-bit DefaultHasher for cryptographic purposes is risky as hash collisions could silently return the wrong key, leading to corrupted ciphertext rather than an error.

Consider implementing the safer approach from the previous review:

-fn make_cache_key(password: &str, salt: &[u8]) -> u64 {
-    let mut hasher = DefaultHasher::new();
-    password.hash(&mut hasher);
-    salt.hash(&mut hasher);
-    hasher.finish()
-}
+type CacheKey = blake3::Hash;        // 256-bit
+
+fn make_cache_key(password: &str, salt: &[u8]) -> CacheKey {
+    blake3::hash([password.as_bytes(), salt].concat().as_slice())
+}

---

214-219: ⚠️ Potential issue

Argon2 parameters are below recommended security levels.

The current settings use DEFAULT_M_COST (16 KiB by default) of RAM and MIN_T_COST (1) iteration, which falls below OWASP's recommendations for password hashing (≥64 MiB, ≥3 iterations).

Consider increasing the memory and time costs:

    let params = Params::new(
-       Params::DEFAULT_M_COST,
-       Params::MIN_T_COST,
+       64 * 1024,                          // 64 MiB (in KiB)
+       3,                                  // time-cost
        Params::DEFAULT_P_COST * 2,
        Some(Params::DEFAULT_OUTPUT_LEN),
    )

---

275-276: ⚠️ Potential issue

Unwrap on user input can cause panic.

Using unwrap() on the result of SaltString::encode_b64 can panic if the input salt is invalid, creating a stability risk in production.

Replace with proper error handling:

// Salt generation
let salt = if let Some(salt) = salt {
-    SaltString::encode_b64(salt.as_bytes()).unwrap()
+    SaltString::encode_b64(salt.as_bytes())
+        .map_err(|e| ServiceError::EncryptionError(format!("Error encoding salt: {}", e)))?
} else {
    SaltString::generate(&mut OsRng)
};

---

300-300: ⚠️ Potential issue

Unwrap on hash value can cause panic.

Using unwrap() on password_hash.hash can panic if hash generation fails, creating a stability risk in production.

Replace with proper error handling:

-let key = password_hash.hash.unwrap();
+let key = password_hash.hash
+    .ok_or_else(|| ServiceError::EncryptionError("Failed to generate hash".to_string()))?;

---

193-196: 💡 Verification agent

🧩 Analysis chain

Missing input validation before splitting encrypted data.

While you've added error handling for conversion to arrays, you should validate input length before processing to prevent panics with malformed input.

Add proper length validation before processing:

    let encrypted_bytes = BASE64_STANDARD
        .decode(&data)
        .map_err(|_| ServiceError::DecryptionError("Error decoding encrypted data".to_string()))?;

+   // Validate input length before processing
+   if encrypted_bytes.len() < NONCE_SIZE + SALT_SIZE {
+       return Err(ServiceError::DecryptionError(
+           "Invalid encrypted data: too short for nonce and salt".to_string(),
+       ));
+   }
    
    // Split the encrypted data into nonce and data
    let (nonce, data) = encrypted_bytes.split_at(NONCE_SIZE);

---

🏁 Script executed:

#!/bin/bash
# Check if there are any length validations before splitting data
rg -B 5 -A 10 'split_at' src/order.rs

Length of output: 959

---

Add length checks before splitting encrypted data

To prevent panics when decrypting malformed input, ensure you validate encrypted_bytes.len() before both split_at calls in src/order.rs.

Locations to update:

• Around the first split_at(NONCE_SIZE) (before line ~193)
• Around the subsequent split_at(SALT_SIZE) (before line ~196)

Suggested diff:

     let encrypted_bytes = BASE64_STANDARD
         .decode(&data)
         .map_err(|_| ServiceError::DecryptionError("Error decoding encrypted data".to_string()))?;

+    // Validate that we have at least nonce + salt
+    if encrypted_bytes.len() < NONCE_SIZE + SALT_SIZE {
+        return Err(ServiceError::DecryptionError(
+            "Invalid encrypted data: too short for nonce and salt".to_string(),
+        ));
+    }

     // Split out the nonce
     let (nonce_bytes, rest) = encrypted_bytes.split_at(NONCE_SIZE);
     let nonce: [u8; NONCE_SIZE] = nonce_bytes
         .try_into()
         .map_err(|_| ServiceError::DecryptionError("Error converting nonce to array".to_string()))?;

-    let (salt, ciphertext) = data.split_at(SALT_SIZE);
+    // Split out the salt
+    let (salt_bytes, ciphertext) = rest.split_at(SALT_SIZE);

     // Decode salt from base64 to bytes
     let salt = SaltString::encode_b64(salt_bytes)
         .map_err(|_| ServiceError::DecryptionError("Error decoding salt".to_string()))?;

This ensures neither split will panic even if the input is truncated.

src/order.rs (3)

176-328: Consider refactoring common code between crypto functions.

Both decrypt_data and store_encrypted share similar parameter handling, key derivation, and validation logic. Extracting these common operations into helper functions would improve maintainability and reduce duplication.

Consider creating helper functions for:

1. Key derivation from password and salt
2. Parameter validation
3. Consistent error handling

Example refactoring:

fn derive_key(password: &str, salt: &SaltString) -> Result<[u8; 32], ServiceError> {
    // Create Argon2 parameters
    let params = Params::new(
        64 * 1024,                          // 64 MiB (in KiB)
        3,                                  // time-cost
        Params::DEFAULT_P_COST,             // parallelism
        Some(Params::DEFAULT_OUTPUT_LEN),   // 32 bytes
    )
    .map_err(|e| ServiceError::EncryptionError(format!("Error creating params: {}", e)))?;
    
    let argon2 = Argon2::new(Algorithm::Argon2id, Version::V0x13, params);
    
    // Hash the password with the salt
    let password_hash = argon2
        .hash_password(password.as_bytes(), salt)
        .map_err(|e| ServiceError::EncryptionError(format!("Error hashing password: {}", e)))?;

    let key = password_hash.hash
        .ok_or_else(|| ServiceError::EncryptionError("Failed to generate hash".to_string()))?;
        
    let key_bytes = key.as_bytes();
    if key_bytes.len() != 32 {
        return Err(ServiceError::EncryptionError(
            format!("Key length is {} bytes, expected 32", key_bytes.len()),
        ));
    }
    
    let mut key_array = [0u8; 32];
    key_array.copy_from_slice(key_bytes);
    Ok(key_array)
}

---

176-255: Add documentation with security guarantees and limitations.

While the implementation includes good comments, it lacks formal documentation about the security guarantees provided, limitations, and appropriate usage patterns.

Add comprehensive documentation:

/// Decrypt data that was previously encrypted with `store_encrypted`.
///
/// # Security 
/// - Uses Argon2id for key derivation (memory-hard password hashing)
/// - Uses ChaCha20Poly1305 for authenticated encryption
/// - Provides confidentiality, integrity, and authentication
/// 
/// # Limitations
/// - Not resistant to side-channel attacks 
/// - Security depends on the strength of the provided password
///
/// # Parameters
/// - `data`: Base64-encoded encrypted data (containing nonce, salt, and ciphertext)
/// - `password`: Optional password for decryption
///
/// # Returns
/// - `Ok(String)`: Decrypted data as a UTF-8 string
/// - `Err(ServiceError)`: If decryption fails
pub fn decrypt_data(data: String, password: Option<&SecretString>) -> Result<String, ServiceError> {

Also applies to: 257-328

---

319-321: Consider using a structured format for encrypted data.

The current implementation concatenates binary data (nonce, salt, ciphertext) without clear separation, which could make future format changes difficult.

Consider either:

1. Adding version and length prefixes for forward compatibility
2. Using a structured format like PASETO for the encrypted payload

Example with simple versioning:

    // Combine nonce and ciphertext
-   let mut encrypted = Vec::with_capacity(NONCE_SIZE + SALT_SIZE + ciphertext.len());
-   encrypted.extend_from_slice(&nonce);
-   encrypted.extend_from_slice(salt_decoded);
-   encrypted.extend_from_slice(&ciphertext);
+   let mut encrypted = Vec::with_capacity(1 + NONCE_SIZE + SALT_SIZE + ciphertext.len());
+   encrypted.push(1); // Version byte for future format changes
+   encrypted.extend_from_slice(&nonce);
+   encrypted.extend_from_slice(salt_decoded);
+   encrypted.extend_from_slice(&ciphertext);

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.283Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.141Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

src/order.rs (3)

70-75: ⚠️ Potential issue

Hash collision risk in cache key generation

The current implementation of make_cache_key uses a 64-bit DefaultHasher which has a relatively high collision probability for cryptographic applications. A collision would cause a different derived key to be returned, potentially leading to corrupted data instead of an error.

Consider using a cryptographic hash function with a larger output:

-fn make_cache_key(password: &str, salt: &[u8]) -> u64 {
-    let mut hasher = DefaultHasher::new();
-    password.hash(&mut hasher);
-    salt.hash(&mut hasher);
-    hasher.finish()
-}
+type CacheKey = [u8; 32];  // 256-bit key
+
+fn make_cache_key(password: &str, salt: &[u8]) -> CacheKey {
+    // Use blake3 or similar for cryptographic security
+    let mut hasher = blake3::Hasher::new();
+    hasher.update(password.as_bytes());
+    hasher.update(salt);
+    let hash = hasher.finalize();
+    *hash.as_bytes()
+}

The SimpleCache implementation would also need to be updated to use this new key type.

---

193-197: ⚠️ Potential issue

Missing input validation before processing ciphertext

The code doesn't validate if the encrypted_bytes are long enough before trying to split them. If a malicious or corrupted input is shorter than NONCE_SIZE + SALT_SIZE, this could panic at runtime.

Add proper length validation before splitting:

+    // Validate input length before processing
+    if encrypted_bytes.len() < NONCE_SIZE + SALT_SIZE {
+        return Err(ServiceError::DecryptionError(
+            "Invalid encrypted data: too short for nonce and salt".to_string(),
+        ));
+    }
+
     // Split the encrypted data into nonce and data
     let (nonce, data) = encrypted_bytes.split_at(NONCE_SIZE);
     let nonce: [u8; NONCE_SIZE] = nonce.try_into().map_err(|_| {
         ServiceError::DecryptionError("Error converting nonce to array".to_string())
     })?;

---

214-219: ⚠️ Potential issue

Argon2 parameters are below recommended security standards

The current Argon2 parameters use minimal memory (DEFAULT_M_COST = 16 KiB) and iterations (MIN_T_COST = 1), which falls far below OWASP's recommendations for password hashing (≥64 MiB, ≥3 iterations).

Strengthen the parameters for better security against brute-force attacks:

     // Hash the password
     let params = Params::new(
-        Params::DEFAULT_M_COST,
-        Params::MIN_T_COST,
+        64 * 1024,                        // 64 MiB memory cost
+        3,                                // 3 iterations
         Params::DEFAULT_P_COST * 2,
         Some(Params::DEFAULT_OUTPUT_LEN),
     )

This makes offline attacks significantly more expensive while maintaining reasonable performance due to your cache.

src/order.rs (3)

273-276: Fixed salt in test mode could lead to unexpected behavior

Using a fixed salt in test mode is fine for deterministic testing, but it's worth adding a comment explaining this choice and its security implications.

Add an explanatory comment:

     // Salt generation
     #[cfg(test)]
+    // Fixed salt for deterministic test results - NEVER use this in production
     let salt = SaltString::encode_b64(b"1H/aaYsf8&asduA0").unwrap();
     #[cfg(not(test))]
     let salt = SaltString::generate(&mut OsRng);

---

188-190: Improve error message clarity for Base64 decoding failures

The error message for Base64 decoding failures doesn't include the specific error, making debugging more difficult.

Enhance the error message to include the original error:

     // Decode the encrypted data from base64 to bytes
     let encrypted_bytes = BASE64_STANDARD
         .decode(&data)
-        .map_err(|_| ServiceError::DecryptionError("Error decoding encrypted data".to_string()))?;
+        .map_err(|e| ServiceError::DecryptionError(format!("Error decoding encrypted data: {}", e)))?;

---

524-525: Generic error mapping masks specific decryption issues

The get_master_buyer_pubkey method maps any decryption error to ServiceError::InvalidPubkey. This makes it harder to distinguish between an actual invalid pubkey and decryption failures.

Consider preserving the original error information:

     pub fn get_master_buyer_pubkey(
         &self,
         password: Option<&SecretString>,
     ) -> Result<String, ServiceError> {
         if let Some(pk) = self.master_buyer_pubkey.as_ref() {
-            decrypt_data(pk.clone(), password).map_err(|_| ServiceError::InvalidPubkey)
+            decrypt_data(pk.clone(), password).map_err(|e| {
+                // For backward compatibility, log the error first
+                log::error!("Decryption error for master_buyer_pubkey: {}", e);
+                ServiceError::InvalidPubkey
+            })
         } else {
             Err(ServiceError::InvalidPubkey)
         }
     }

Apply the same pattern to get_master_seller_pubkey on line 535.

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.304Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.163Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

src/order.rs (1)

220-226: 🛠️ Refactor suggestion

Increase Argon2 memory and time costs for stronger password hashing

The current Argon2 parameters use the default memory cost and minimum time cost, which might not provide sufficient protection against offline brute-force attacks for sensitive encryption keys.

let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::MIN_T_COST,
-    Params::DEFAULT_P_COST * 2,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
+    Params::DEFAULT_P_COST,             // parallelism (1 thread)
    Some(Params::DEFAULT_OUTPUT_LEN),
)
.map_err(|_| ServiceError::DecryptionError("Error Argon2 creating params".to_string()))?;

src/order.rs (2)

280-285: Avoid fixed salt in test code

Using a fixed salt in test code is convenient but could lead to accidental security issues if mistakenly used in non-test code. Consider using a deterministic but still random approach for tests.

#[cfg(test)]
-let salt = SaltString::encode_b64(b"1H/aaYsf8&asduA0").unwrap();
+let salt = {
+    // Still deterministic but taken from a test-specific source
+    let mut test_rng = rand_chacha::ChaCha8Rng::seed_from_u64(12345);
+    SaltString::generate(&mut test_rng)
+};

---

175-176: Extract encryption parameters as constants

The salt and nonce sizes are defined separately in both encryption functions. Extract these to module-level constants to ensure consistency and make future updates easier.

+// Cryptographic constants
+const SALT_SIZE: usize = 16;
+const NONCE_SIZE: usize = 12;
+
 /// Decrypt an identity key from the database
 pub fn decrypt_data(data: String, password: Option<&SecretString>) -> Result<String, ServiceError> {
-    // Salt size and nonce size
-    const SALT_SIZE: usize = 16;
-    const NONCE_SIZE: usize = 12;

And similarly for the store_encrypted function.

Also applies to: 266-270

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.304Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.163Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

src/order.rs (2)

777-777: ⚠️ Potential issue

Replace unwrap with safe error handling in fiat_amount method

The fiat_amount method still uses unwrap() which could cause panics on invalid input. Consider replacing with safer alternatives.

pub fn fiat_amount(&self) -> String {
    if self.max_amount.is_some() {
-        format!("{}-{}", self.min_amount.unwrap(), self.max_amount.unwrap())
+        format!("{}-{}", 
+            self.min_amount.unwrap_or_default(), 
+            self.max_amount.unwrap_or_default())
    } else {
        self.fiat_amount.to_string()
    }
}

Or better yet, return a Result to propagate errors:

pub fn fiat_amount(&self) -> Result<String, ServiceError> {
    if self.max_amount.is_some() {
        let min = self.min_amount.ok_or_else(|| 
            ServiceError::InvalidOrderData("Missing min_amount".to_string()))?;
        let max = self.max_amount.ok_or_else(|| 
            ServiceError::InvalidOrderData("Missing max_amount".to_string()))?;
        Ok(format!("{}-{}", min, max))
    } else {
        Ok(self.fiat_amount.to_string())
    }
}

---

787-789: ⚠️ Potential issue

Replace unwrap in From implementation

The From<Order> implementation uses unwrap() which could cause panics on invalid input. Consider replacing with safe error handling.

impl From<Order> for SmallOrder {
    fn from(order: Order) -> Self {
        let id = Some(order.id);
-        let kind = Kind::from_str(&order.kind).unwrap();
-        let status = Status::from_str(&order.status).unwrap();
+        let kind = Kind::from_str(&order.kind).unwrap_or(Kind::Buy);
+        let status = Status::from_str(&order.status).unwrap_or(Status::Active);
        // ...
    }
}

Or define a TryFrom implementation that can return errors:

impl TryFrom<Order> for SmallOrder {
    type Error = ServiceError;
    
    fn try_from(order: Order) -> Result<Self, Self::Error> {
        let id = Some(order.id);
        let kind = Kind::from_str(&order.kind)
            .map_err(|_| ServiceError::InvalidOrderKind)?;
        let status = Status::from_str(&order.status)
            .map_err(|_| ServiceError::InvalidOrderStatus)?;
        // ...
        Ok(Self { /* ... */ })
    }
}

src/order.rs (2)

314-320: ⚠️ Potential issue

Inconsistent Argon2 parameters between functions

The Argon2 parameters in store_encrypted should match those in decrypt_data to ensure compatibility and consistent security. Apply the same security enhancement here as recommended above.

let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::MIN_T_COST,
-    Params::DEFAULT_P_COST * 2,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
+    Params::DEFAULT_P_COST,             // parallelism (1 thread)
    Some(Params::DEFAULT_OUTPUT_LEN),
)
.map_err(|_| ServiceError::EncryptionError("Error creating params".to_string()))?;

---

241-247: ⚠️ Potential issue

Security concern: Argon2 parameters are too weak

The current parameters use only 16 MiB of RAM (DEFAULT_M_COST) and a single iteration (MIN_T_COST), which is below OWASP recommendations for password hashing (≥ 64 MiB, ≥ 3 iterations).

let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::MIN_T_COST,
-    Params::DEFAULT_P_COST * 2,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
+    Params::DEFAULT_P_COST,             // parallelism (1 thread)
    Some(Params::DEFAULT_OUTPUT_LEN),
)
.map_err(|_| ServiceError::DecryptionError("Error Argon2 creating params".to_string()))?;

These stronger parameters significantly slow down offline attacks without noticeably impacting performance when the key is cached.

src/order.rs (1)

302-305: Consider adding a seed parameter for testing

Using a hardcoded salt in test mode is good, but consider making it configurable via a function parameter to support different test scenarios.

pub fn store_encrypted(
    idkey: &str,
    password: Option<&SecretString>,
+   test_salt: Option<&[u8]>,
) -> Result<String, ServiceError> {
    // ...
    
    // Salt generation
    #[cfg(test)]
-   let salt = SaltString::encode_b64(b"1H/aaYsf8&asduA0").unwrap();
+   let salt = if let Some(test_salt) = test_salt {
+       SaltString::encode_b64(test_salt)
+           .map_err(|_| ServiceError::EncryptionError("Error encoding test salt".to_string()))?
+   } else {
+       SaltString::encode_b64(b"1H/aaYsf8&asduA0")
+           .map_err(|_| ServiceError::EncryptionError("Error encoding default test salt".to_string()))?
+   };
    #[cfg(not(test))]
    let salt = SaltString::generate(&mut OsRng);

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.304Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.163Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:191-196
Timestamp: 2025-05-07T22:06:39.013Z
Learning: User is working on removing unwrap() calls in application code (especially in src/order.rs) but acknowledges that some unwrap() calls in test sections are acceptable.

src/order.rs (2)

314-320: 🛠️ Refactor suggestion

Ensure consistent Argon2 parameters between encryption and decryption

The Argon2 parameters should be identical in both encryption and decryption functions to ensure compatibility. Consider extracting these parameters to a constant or function to ensure they stay in sync.

let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::MIN_T_COST,
-    Params::DEFAULT_P_COST * 2,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
+    Params::DEFAULT_P_COST,             // parallelism (1 thread)
    Some(Params::DEFAULT_OUTPUT_LEN),
)

---

241-247: 💡 Verification agent

🧩 Analysis chain

Consider increasing Argon2 memory and time costs

The current Argon2 parameters (DEFAULT_M_COST, MIN_T_COST, DEFAULT_P_COST * 2) may not provide sufficient protection against dedicated attackers with hardware acceleration. Consider increasing the memory cost and time cost parameters to strengthen security, especially since the results are cached.

let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::MIN_T_COST,
-    Params::DEFAULT_P_COST * 2,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
+    Params::DEFAULT_P_COST,             // parallelism (1 thread)
    Some(Params::DEFAULT_OUTPUT_LEN),
)

---

🌐 Web query:

What are the recommended Argon2 parameters for password hashing in 2024?

💡 Result:

As of 2024, the recommended parameters for securely hashing passwords using Argon2 are as follows:

• Variant: Use Argon2id, which combines the benefits of both Argon2i and Argon2d, offering protection against side-channel and GPU-based attacks. (rfc-editor.org)

• Memory Cost (m): Allocate 2 GiB (2^21 KiB) of memory. This setting provides a robust defense against brute-force attacks by consuming significant memory resources. (rfc-editor.org)

• Iterations (t): Perform 1 iteration. A single iteration balances security and performance, ensuring the hashing process is time-consuming enough to deter attackers without introducing excessive delays for legitimate users. (rfc-editor.org)

• Parallelism (p): Utilize 4 lanes. This parameter defines the number of parallel computational chains, allowing the hashing process to leverage multiple CPU cores effectively. (rfc-editor.org)

• Salt Length: Use a 16-byte (128-bit) salt. A longer salt enhances security by making precomputed attacks, such as rainbow table attacks, more difficult. (rfc-editor.org)

• Tag Length: Generate a 32-byte (256-bit) tag. This provides a sufficient level of security for most applications. (rfc-editor.org)

These settings are recommended for general use cases, balancing security and performance. However, if your application operates in a memory-constrained environment, you might consider the following alternative:

• Memory Cost (m): Allocate 64 MiB (2^16 KiB) of memory.

• Iterations (t): Perform 3 iterations.

• Parallelism (p): Utilize 4 lanes.

This configuration offers a reasonable level of security while being more suitable for systems with limited memory resources. (rfc-editor.org)

It's important to note that the optimal settings may vary based on your specific application requirements and hardware capabilities. Regularly reviewing and adjusting these parameters in response to advancements in hardware and attack techniques is essential to maintain robust password security.

Citations:

• 1: https://www.rfc-editor.org/rfc/rfc9106.html?utm_source=openai
• 2: https://www.rfc-editor.org/rfc/rfc9106.html?utm_source=openai
• 3: https://www.rfc-editor.org/rfc/rfc9106.html?utm_source=openai
• 4: https://www.rfc-editor.org/rfc/rfc9106.html?utm_source=openai
• 5: https://www.rfc-editor.org/rfc/rfc9106.html?utm_source=openai
• 6: https://www.rfc-editor.org/rfc/rfc9106.html?utm_source=openai
• 7: https://www.rfc-editor.org/rfc/rfc9106.html?utm_source=openai

---

I recommend bumping the Argon2 parameters to use a more robust, 2024-recommended setup—especially if these hashes are cached long-term. For a reasonable trade-off in a memory-constrained service, switch to:

let params = Params::new(
-    Params::DEFAULT_M_COST,               // ~4 MiB
-    Params::MIN_T_COST,                   // 1 iteration
-    Params::DEFAULT_P_COST * 2,           // 2 lanes
+    64 * 1024,                            // 64 MiB (2^16 KiB)
+    3,                                     // 3 iterations
+    4,                                     // 4 lanes (parallelism)
    Some(Params::DEFAULT_OUTPUT_LEN),
)
.map_err(|_| ServiceError::DecryptionError("Error Argon2 creating params".to_string()))?;

• Consider also using the Argon2id variant if you aren’t already; it’s the current best practice per RFC 9106.
• If your environment can afford it, you may bump memory to 2 GiB (2^21 KiB) and drop iterations to 1 for maximum hardness against GPU attacks.

src/order.rs (1)

196-204: Consider documenting encryption method and parameters

The current function comments are useful but brief. Consider adding more detailed documentation about the encryption scheme (ChaCha20Poly1305 with Argon2id key derivation), the format of the encrypted data (nonce + salt + ciphertext), and any other relevant details that would help developers understand and use these functions correctly.

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.304Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.163Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:191-196
Timestamp: 2025-05-07T22:06:39.013Z
Learning: User is working on removing unwrap() calls in application code (especially in src/order.rs) but acknowledges that some unwrap() calls in test sections are acceptable.

src/order.rs (3)

195-216: Good implementation of input validation before processing

The decrypt_data function properly validates input length before processing to prevent potential panics from malformed input. This is a best practice in cryptographic code handling user input, and it directly addresses the concern raised in a previous review comment.

---

326-332: 🛠️ Refactor suggestion

Ensure consistent Argon2 parameters between encrypt and decrypt functions

The Argon2 parameters should be identical in both encryption and decryption functions to ensure compatibility. Please apply the same parameter improvements suggested for the decrypt_data function here as well.

 let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::DEFAULT_T_COST,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
     Params::DEFAULT_P_COST * 2,
     Some(Params::DEFAULT_OUTPUT_LEN),
 )

---

241-247: 💡 Verification agent

🧩 Analysis chain

Consider increasing Argon2 memory and time costs

The current Argon2 parameters are too weak for password-based encryption. The memory cost (DEFAULT_M_COST) and time cost (DEFAULT_T_COST) should be increased to provide better protection against brute-force attacks.

 let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::DEFAULT_T_COST,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
     Params::DEFAULT_P_COST * 2,
     Some(Params::DEFAULT_OUTPUT_LEN),
 )

This won't significantly impact performance since you're caching the derived keys.

---

🌐 Web query:

What are the recommended Argon2 parameters for password hashing?

💡 Result:

The recommended Argon2 parameters for password hashing depend on your specific security needs, including the available memory, desired level of protection, and acceptable performance impact. However, general guidelines for Argon2 (especially Argon2id, which is recommended for password hashing) are:

1. Memory Cost: Allocate a significant amount of memory to resist GPU and ASIC attacks. Typical recommendations range from 64 MB to 1 GB or more for interactive logins, with higher memory settings for more secure environments.

2. Time Cost (Iterations): This is the number of iterations the algorithm performs. A value between 2 to 4 iterations is common, balancing between security and performance.

3. Parallelism: The number of threads or parallel lanes used. Usually set to the number of CPU cores or 1 for single-threaded environments.

4. Hash Length: The length of the resulting hash. 16 to 32 bytes is typical.

A common starting point example would be:

• Memory: 64 MiB
• Time cost: 3
• Parallelism: 4 (if you have 4 CPU cores)
• Hash length: 32 bytes

It is recommended to benchmark these parameters on your target system to ensure the performance is acceptable while maximizing the resource usage to slow down any attackers.

If you want, I can provide more concrete examples for specific environments or use cases.

---

Increase Argon2 work factors for stronger password-based encryption

The current defaults (Params::DEFAULT_M_COST, Params::DEFAULT_T_COST) are on the low side for interactive logins. Benchmarks and community guidelines recommend:

• Memory cost: ≥ 64 MiB (64 × 1024 KiB)
• Time cost: 3 iterations
• Parallelism: match your CPU cores (e.g. num_cpus::get() as u32)
• Hash length: 32 bytes (the default)

Updating your code in src/order.rs (around lines 241–247) might look like:

 let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::DEFAULT_T_COST,
-    Params::DEFAULT_P_COST * 2,
-    Some(Params::DEFAULT_OUTPUT_LEN),
+    64 * 1024,                     // 64 MiB (in KiB)
+    3,                             // time-cost (iterations)
+    num_cpus::get() as u32,        // parallelism (CPU cores)
+    Some(Params::DEFAULT_OUTPUT_LEN), // output length (32 bytes)
 )
 .map_err(|_| ServiceError::DecryptionError("Error Argon2 creating params".to_string()))?;

These values will dramatically increase the work required for offline attacks without a significant performance hit—especially since you cache derived keys. Don’t forget to benchmark on your target environment and adjust if necessary.

src/order.rs (1)

38-71: Consider adding documentation to the SimpleCache implementation

The LRU cache implementation is well-structured, but it would benefit from documentation explaining its purpose and functionality, especially since it's handling sensitive cryptographic material.

+/// A simple LRU cache with fixed size for cryptographic keys
+/// 
+/// This cache stores derived cryptographic keys indexed by a hash of the password and salt.
+/// It has a fixed maximum size and evicts the least recently used entries when full.
 struct SimpleCache {
     map: HashMap<CacheKey, [u8; 32]>,
     order: VecDeque<CacheKey>,
 }

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.304Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.163Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

src/rating.rs (1)

35-35: Consider preserving original error details for debugging

While using specific error types is good, the current implementation discards original error information with map_err(|_| ServiceError::...). For debugging purposes, you might want to consider preserving this information, perhaps in logs or by including it in specific error variants where it doesn't risk exposing sensitive data.

Also applies to: 40-40, 97-97, 102-102, 107-107, 112-112, 117-117

README.md (2)

3-3: Fix missing article in description
Add “the” to improve clarity in:

- It serves as the foundation for building Mostro daemon.
+ It serves as the foundation for building the Mostro daemon.

🧰 Tools

🪛 LanguageTool

[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...t serves as the foundation for building Mostro daemon. ## Requirements - Rust 1.86.0...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

---

17-17: Improve heading grammar and casing
Consider updating the heading for consistency and clarity:

- ## Import prelude to use mostro core
+ ## Import the prelude to use Mostro Core

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

src/order.rs (2)

324-329: 🛠️ Refactor suggestion

Ensure consistent parameters between encrypt and decrypt functions

The Argon2 parameters should be identical between your encryption and decryption functions to ensure compatibility. The same security concerns apply here as with the decrypt_data function.

Apply the same improved parameters here:

let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::DEFAULT_T_COST,
-    Params::DEFAULT_P_COST * 2,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost
+    Params::DEFAULT_P_COST,             // parallelism
    Some(Params::DEFAULT_OUTPUT_LEN),
)

---

239-244: 🛠️ Refactor suggestion

Increase Argon2 memory and time costs for stronger password hashing

The current Argon2 parameters might not be sufficiently tuned for security. OWASP recommends higher memory usage and iterations for password hashing.

Increase the memory cost and time cost parameters for stronger security against offline attacks:

let params = Params::new(
-    Params::DEFAULT_M_COST,
-    Params::DEFAULT_T_COST,
-    Params::DEFAULT_P_COST * 2,
+    64 * 1024,                          // 64 MiB (in KiB)
+    3,                                  // time-cost (iterations)
+    Params::DEFAULT_P_COST,             // parallelism
    Some(Params::DEFAULT_OUTPUT_LEN),
)

These stronger parameters will significantly slow down offline attacks without noticeable impact when using the cache you've implemented.

src/order.rs (3)

217-221: Consider using constant-time comparisons for cryptographic operations

When working with cryptographic material, it's best practice to use constant-time operations to prevent timing attacks.

While the current implementation is functionally correct, consider using a library like subtle for constant-time comparisons when validating cryptographic data:

- let nonce: [u8; NONCE_SIZE] = nonce.try_into().map_err(|_| {
-     ServiceError::DecryptionError("Error converting nonce to array".to_string())
- })?;
+ let nonce: [u8; NONCE_SIZE] = nonce.try_into().map_err(|_| {
+     ServiceError::DecryptionError("Error converting nonce to array".to_string())
+ })?;
+ 
+ // When comparing MACs or other cryptographic values later
+ // use subtle::ConstantTimeEq::ct_eq instead of ==

---

194-282: Consider refactoring common cryptographic operations into a separate module

The decrypt_data and store_encrypted functions contain duplicated logic for key derivation and cryptographic operations.

Consider extracting the common cryptographic operations into a separate CryptoUtils module or struct to improve maintainability and reduce duplication:

// In a new file like src/crypto.rs or src/utils/crypto.rs
use crate::prelude::*;
// other imports...

pub struct CryptoUtils;

impl CryptoUtils {
    /// Derive a key from password and salt
    fn derive_key(password: &str, salt: &SaltString) -> Result<[u8; 32], ServiceError> {
        // Common key derivation logic
    }
    
    /// Encrypt data with the provided key
    pub fn encrypt(data: &[u8], key: &[u8; 32]) -> Result<(Vec<u8>, [u8; NONCE_SIZE]), ServiceError> {
        // Encryption logic
    }
    
    /// Decrypt data with the provided key
    pub fn decrypt(ciphertext: &[u8], nonce: &[u8; NONCE_SIZE], key: &[u8; 32]) -> Result<Vec<u8>, ServiceError> {
        // Decryption logic
    }
    
    // Then your public API functions that use these helpers:
    pub fn decrypt_data(data: String, password: Option<&SecretString>) -> Result<String, ServiceError> {
        // Use the helper functions
    }
    
    pub fn store_encrypted(
        idkey: &str,
        password: Option<&SecretString>,
        fixed_salt: Option<SaltString>,
    ) -> Result<String, ServiceError> {
        // Use the helper functions
    }
}

Also applies to: 296-367

---

1-24: Consider using external crates for common cryptographic patterns

While your implementation is solid, there are established Rust crates for common cryptographic patterns that could simplify your code.

Consider using established libraries like orion or ring that provide higher-level APIs for common cryptographic operations:

// Example using orion for password-based encryption
use orion::aead;
use orion::pwhash;

pub fn encrypt(data: &str, password: &str) -> Result<String, ServiceError> {
    let salt = pwhash::Salt::generate()?;
    let key = pwhash::Password::from_slice(password.as_bytes())?
        .derive_key(&salt, 3, 64 * 1024, 32)?;
    
    let sealed = aead::seal(&key, data.as_bytes())?;
    // Combine salt + sealed data and encode
}

These libraries often handle many of the security considerations like constant-time comparisons automatically.

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.304Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.163Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:191-196
Timestamp: 2025-05-07T22:06:39.041Z
Learning: User is working on removing unwrap() calls in application code (especially in src/order.rs) but acknowledges that some unwrap() calls in test sections are acceptable.

src/crypto.rs (1)

51-65: Early‐return validation duplicates error strings

Both key-length and salt-length checks return identical string literals from other sites. Consider extracting them into const error messages or using format! with the offending length to aid debugging.

Small nitpick—no functional change required.

src/order.rs (3)

2-10: Several heavy cryptography imports are unused

Algorithm, Argon2, Params, PasswordHasher, Version, and AeadCore are not referenced in this file.
Keeping them:

• emits warnings,
• slows compile times.

-use argon2::{
-    password_hash::{rand_core::OsRng, Salt, SaltString},
-    Algorithm, Argon2, Params, PasswordHasher, Version,
-};
-use chacha20poly1305::{
-    aead::{Aead, KeyInit},
-    AeadCore, ChaCha20Poly1305, Key,
-};
+use argon2::password_hash::{rand_core::OsRng, Salt, SaltString};
+use chacha20poly1305::{aead::{Aead, KeyInit}, ChaCha20Poly1305, Key};

Trimming the list keeps the module lean.

🧰 Tools

🪛 GitHub Actions: Rust

[warning] 4-4: Unused imports: Algorithm, Argon2, Params, PasswordHasher, and Version.

---

[warning] 9-9: Unused import: AeadCore.

---

49-57: Read–write contention inside get

Because get currently mutates self.order, every read requires &mut self.
If you adopt the redesign above, consider:

• splitting LRU maintenance into a separate method,
• or using parking_lot::Mutex + interior mutability,
• or replacing the whole cache with dashmap which gives lock-free reads.

This will materially improve throughput under high concurrency.

---

223-228: SaltString::encode_b64 performs an encode, not a decode

The comment says “decode salt from base64”, but the call encodes the raw
salt slice. The code is correct (derive_key expects SaltString), the
documentation is misleading.

-    // Enecode salt from base64 to bytes
+    // Convert raw 16-byte salt into a SaltString (base64 encoded)

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

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:30-69
Timestamp: 2025-04-26T09:46:57.304Z
Learning: The project uses RwLock instead of Mutex for the KEY_CACHE in src/order.rs to reduce contention in the encryption/decryption cache, as the cache is accessed during both encryption and decryption operations.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:189-196
Timestamp: 2025-04-26T09:41:59.163Z
Learning: When handling encrypted data or user input, always implement proper length validation before using unwrap() or other potentially panicking operations, especially in the decrypt_data() and store_encrypted() functions. The user acknowledged the importance of this practice and committed to implementing input validation in their cryptographic functions.

Learnt from: arkanoider
PR: MostroP2P/mostro-core#102
File: src/order.rs:191-196
Timestamp: 2025-05-07T22:06:39.041Z
Learning: User is working on removing unwrap() calls in application code (especially in src/order.rs) but acknowledges that some unwrap() calls in test sections are acceptable.

src/crypto.rs (2)

2-11: Unused imports causing warnings and potential confusion

Remove unused imports and consider being more specific with imports to improve maintainability:

 use crate::prelude::*;
 use argon2::{
-    password_hash::{rand_core::OsRng, Salt, SaltString},
+    password_hash::{rand_core::OsRng, SaltString},
     Algorithm, Argon2, Params, PasswordHasher, Version,
 };
 use base64::{engine::general_purpose::STANDARD as BASE64_STANDARD, Engine as _};
 use chacha20poly1305::{
     aead::{Aead, KeyInit},
-    AeadCore, ChaCha20Poly1305, Key,
+    ChaCha20Poly1305, Key,
 };
-use secrecy::*;
+use secrecy::SecretString;

These imports aren't used in the code and cause compiler warnings. A more specific import for secrecy would be better than the wildcard import.

---

97-102: Argon2 parameters are too weak for cryptographic security

The current Argon2 parameters use default memory and time costs that are too low for secure password hashing:

     pub fn derive_key(password: &str, salt: &SaltString) -> Result<Vec<u8>, ServiceError> {
         // Common key derivation logic
         let params = Params::new(
-            Params::DEFAULT_M_COST,
-            Params::DEFAULT_T_COST,
-            Params::DEFAULT_P_COST * 2,
-            Some(Params::DEFAULT_OUTPUT_LEN),
+            64 * 1024,   // 64 MiB in KiB
+            3,           // iterations
+            1,           // parallelism factor
+            Some(DERIVED_KEY_LENGTH as u32),
         )

OWASP and RFC 9106 recommend at least 64 MiB memory cost and 3 or more iterations for password hashing to resist offline attacks. This change will strengthen security against brute-force attacks.

src/crypto.rs (5)

56-65: Potential inefficiency in cache management logic

The cache implementation doesn't properly handle the case when a key already exists but the cache is full:

     fn put(&mut self, key: CacheKey, value: [u8; 32]) {
-        if !self.map.contains_key(&key) && self.map.len() >= MAX_CACHE_SIZE {
+        if !self.map.contains_key(&key) {
+            if self.map.len() >= MAX_CACHE_SIZE {
                 if let Some(oldest_key) = self.order.pop_front() {
                     self.map.remove(&oldest_key);
                 }
+            }
+            self.order.push_back(key);
+            self.map.insert(key, value);
+        } else {
+            // Update existing key's position in order queue
+            self.order.retain(|&k| k != key);
+            self.order.push_back(key);
+            self.map.insert(key, value);
         }
-        self.order.retain(|&k| k != key);
-        self.order.push_back(key);
-        self.map.insert(key, value);
     }

The current implementation always updates the order and map, even if the key already exists, and it only checks cache size for new keys. The refactored version handles updates more efficiently.

---

255-255: Remove unnecessary comment

     ) -> Result<String, ServiceError> {
-        // Salt size and nonce size
 
         // If password is not provided, return data as it is

This comment doesn't provide any useful information and appears to be a leftover. It should be removed for clarity.

---

17-19: Global cache implementation might benefit from additional considerations

The global cache implementation is a good optimization but has some potential issues:

1. It might grow unbounded if many different password/salt combinations are used
2. No eviction policy for sensitive keys that are no longer needed
3. Potential contention with RwLock if many threads are using it simultaneously

Consider:

1. Adding a configuration option to disable the cache completely for high-security environments
2. Implementing a periodic cleanup mechanism
3. Using a more sophisticated concurrency primitive if high contention is expected

// Example for a configurable cache:
static KEY_CACHE: LazyLock<Option<RwLock<SecretBox<SimpleCache>>>> = 
    LazyLock::new(|| {
        if std::env::var("DISABLE_KEY_CACHE").is_ok() {
            None
        } else {
            Some(RwLock::new(SecretBox::new(Box::new(SimpleCache::new()))))
        }
    });

---

68-77: Typo in comment and optimization opportunity in Zeroize implementation

-// Implemetation of zeroize required by secretbox
+// Implementation of zeroize required by secretbox
 impl Zeroize for SimpleCache {
     fn zeroize(&mut self) {
         for value in self.map.values_mut() {
             value.zeroize();
         }
         self.map.clear();
         self.order.clear();
     }
 }

There's a typo in the comment ("Implemetation" should be "Implementation"). Additionally, consider zeroizing the keys as well, as they contain sensitive password-derived data.

---

159-204: Improve error handling in decrypt method

The decrypt method has several places where errors are swallowed with generic messages:

     fn decrypt(data: Vec<u8>, password: &str) -> Result<Vec<u8>, ServiceError> {
         // Split the encrypted data into nonce and data
         let (nonce, data) = data.split_at(NONCE_SIZE);

         let nonce: [u8; NONCE_SIZE] = nonce
             .try_into()
-            .map_err(|_| ServiceError::DecryptionError("Error converting nonce".to_string()))?;
+            .map_err(|e| ServiceError::DecryptionError(format!("Error converting nonce: {}", e)))?;

         let (salt, ciphertext) = data.split_at(SALT_SIZE);

         // Enecode salt from base64 to bytes
         let salt = SaltString::encode_b64(salt)
-            .map_err(|_| ServiceError::DecryptionError("Error decoding salt".to_string()))?;
+            .map_err(|e| ServiceError::DecryptionError(format!("Error decoding salt: {}", e)))?;

Including the actual error in the message would make debugging much easier. This pattern should be applied throughout the method.

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