fix: prevent race conditions in encrypted backend with write locks, track keychain keys, and propagate environment override status in operations

Hakanbaban53 · Hakanbaban53 · commit 5a4164f0a601 · 2026-04-13T19:51:16.000+03:00
diff --git a/src/credentials/encrypted.rs b/src/credentials/encrypted.rs
@@ -19,7 +19,7 @@ use std::collections::HashMap;
 use std::fs;
 use std::io::Write;
 use std::path::{Path, PathBuf};
-use std::sync::RwLock;
+use std::sync::{Mutex, RwLock};
 
 /// Encrypted credential entry
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -46,7 +46,19 @@ pub struct EncryptedFileBackend {
     cipher: Aes256Gcm,
     /// Salt used for key derivation (stored in file for decryption on restart)
     salt: [u8; 16],
+    /// Plaintext read-cache.
+    ///
+    /// Populated lazily on `get()` and kept in sync on `store()`/`remove()`.
     cache: RwLock<HashMap<String, String>>,
+    /// Serializes all mutations to the encrypted file.
+    ///
+    /// BUG FIX: without this lock, two concurrent `store()` / `remove()` calls
+    /// both read the file into separate in-memory copies, modify their own copy,
+    /// and write back.  The second write silently drops the first writer's change.
+    /// Holding this mutex across the entire read-modify-write cycle prevents the
+    /// TOCTOU race.  The read cache (`self.cache`) is updated while the mutex is
+    /// still held, so cache and disk are always consistent.
+    write_lock: Mutex<()>,
 }
 
 impl EncryptedFileBackend {
@@ -66,6 +78,7 @@ impl EncryptedFileBackend {
                 .map_err(|_| Error::Credential("Invalid encryption key length".into()))?,
             salt,
             cache: RwLock::new(HashMap::new()),
+            write_lock: Mutex::new(()),
         })
     }
 
@@ -311,20 +324,27 @@ impl EncryptedFileBackend {
 
 impl CredentialBackend for EncryptedFileBackend {
     fn store(&self, key: &str, value: &str) -> Result<()> {
+        let _guard = self
+            .write_lock
+            .lock()
+            .map_err(|_| Error::Credential("Encrypted file write lock poisoned".into()))?;
+
         let mut store = self.load_store()?;
         let encrypted = self.encrypt(value)?;
         store.entries.insert(key.to_string(), encrypted);
         self.save_store(store.entries)?;
 
-        let mut cache = self.cache.write_recovered()?;
-        cache.insert(key.to_string(), value.to_string());
+        // Update cache while write_lock is held so cache and disk are always consistent.
+        self.cache
+            .write_recovered()?
+            .insert(key.to_string(), value.to_string());
 
         debug!("Credential stored in encrypted file: {key}");
         Ok(())
     }
 
     fn get(&self, key: &str) -> Result<Option<String>> {
-        // Check cache first
+        // Check cache first (no write_lock needed for reads)
         {
             let cache = self.cache.read_recovered()?;
             if let Some(value) = cache.get(key) {
@@ -349,6 +369,11 @@ impl CredentialBackend for EncryptedFileBackend {
     }
 
     fn remove(&self, key: &str) -> Result<()> {
+        let _guard = self
+            .write_lock
+            .lock()
+            .map_err(|_| Error::Credential("Encrypted file write lock poisoned".into()))?;
+
         let mut store = self.load_store()?;
         store.entries.remove(key);
         self.save_store(store.entries)?;
@@ -459,4 +484,28 @@ mod tests {
         let key4 = EncryptedFileBackend::derive_key("password123", &salt2).unwrap();
         assert_ne!(key1, key4);
     }
+
+    /// Regression test for the concurrent write TOCTOU bug.
+    /// Two threads writing different keys must both survive.
+    #[test]
+    fn test_concurrent_store_no_lost_writes() {
+        use std::sync::Arc;
+        use std::thread;
+
+        let temp = tempdir().unwrap();
+        let path = temp.path().join("credentials.enc.json");
+        let backend = Arc::new(EncryptedFileBackend::with_password(path, "password").unwrap());
+
+        let b1 = Arc::clone(&backend);
+        let b2 = Arc::clone(&backend);
+
+        let t1 = thread::spawn(move || b1.store("key_a", "value_a").unwrap());
+        let t2 = thread::spawn(move || b2.store("key_b", "value_b").unwrap());
+
+        t1.join().unwrap();
+        t2.join().unwrap();
+
+        assert_eq!(backend.get("key_a").unwrap(), Some("value_a".to_string()));
+        assert_eq!(backend.get("key_b").unwrap(), Some("value_b".to_string()));
+    }
 }
diff --git a/src/credentials/keychain.rs b/src/credentials/keychain.rs
@@ -2,7 +2,6 @@
 
 use super::CredentialBackend;
 use crate::error::{Error, Result};
-// Removed: use keyring::use_native_store;
 use keyring_core::{Entry, Error as KeyringError};
 use log::{debug, warn};
 use std::collections::HashSet;
@@ -13,7 +12,7 @@ static NATIVE_STORE_INIT: OnceLock<()> = OnceLock::new();
 /// OS Keychain backend for secure credential storage
 pub struct KeychainBackend {
     service_name: String,
-    /// Cache of known keys (keychain doesn't support listing)
+    /// Cache of known keys (keychain doesn't support listing).
     known_keys: RwLock<HashSet<String>>,
 }
 
@@ -88,6 +87,7 @@ impl CredentialBackend for KeychainBackend {
     fn get(&self, key: &str) -> Result<Option<String>> {
         match self.get_entry(key)?.get_password() {
             Ok(password) => {
+                self.track_key(key);
                 debug!("Credential retrieved from keychain: {key}");
                 Ok(Some(password))
             }
diff --git a/src/manager/operations.rs b/src/manager/operations.rs
@@ -35,35 +35,38 @@ impl<S: StorageBackend + 'static, Schema: SettingsSchema> SettingsManager<S, Sch
         &self,
         key: &str,
         metadata: &SettingMetadata,
-    ) -> Result<Option<Value>> {
+    ) -> Result<Option<(Value, bool)>> {
         // For secrets, check keyring first
         if metadata.is_secret() {
             // Check env var override for secrets if enabled
             if self.config.env_overrides_secrets
                 && let Some(env_value) = self.get_env_override(key)
             {
-                return Ok(Some(env_value));
+                return Ok(Some((env_value, true)));
             }
 
             // Try retrieving from keyring
             if let Ok(Some(secret_value)) = self.get_credential_with_profile(key) {
-                return Ok(Some(Value::String(secret_value)));
+                return Ok(Some((Value::String(secret_value), false)));
             }
 
             // Secret not found, use default
-            return Ok(Some(metadata.default.clone()));
+            return Ok(Some((metadata.default.clone(), false)));
         }
 
         // Not a secret - check cache (with env override support)
         if let Some(env_value) = self.get_env_override(key) {
-            return Ok(Some(env_value));
+            return Ok(Some((env_value, true)));
         }
 
         let Some((category, setting_name)) = Self::parse_setting_key(key) else {
             return Ok(None);
         };
 
-        self.settings_cache.get_value(category, setting_name, key)
+        Ok(self
+            .settings_cache
+            .get_value(category, setting_name, key)?
+            .map(|v| (v, false)))
     }
 
     /// Helper to get a setting value (non-credential version)
@@ -72,18 +75,22 @@ impl<S: StorageBackend + 'static, Schema: SettingsSchema> SettingsManager<S, Sch
         &self,
         key: &str,
         _metadata: &SettingMetadata,
-    ) -> Result<Option<Value>> {
+    ) -> Result<Option<(Value, bool)>> {
         // Check env override first
         if let Some(env_value) = self.get_env_override(key) {
-            return Ok(Some(env_value));
+            return Ok(Some((env_value, true)));
         }
 
         let Some((category, setting_name)) = Self::parse_setting_key(key) else {
             return Ok(None);
         };
 
-        self.settings_cache.get_value(category, setting_name, key)
+        Ok(self
+            .settings_cache
+            .get_value(category, setting_name, key)?
+            .map(|v| (v, false)))
     }
+
     /// Check if a setting value is overridden by an environment variable
     ///
     /// Returns the parsed value if env var is set and successfully parsed.
@@ -113,20 +120,24 @@ impl<S: StorageBackend + 'static, Schema: SettingsSchema> SettingsManager<S, Sch
 
         for (key, option) in &mut metadata {
             if Self::parse_setting_key(key).is_some() {
-                // Use the helper method to get value (handles secrets and env overrides)
-                if let Ok(Some(value)) = self.get_value_with_secret_support(key, option) {
-                    option.value = Some(value);
-
-                    // Mark as env-overridden if applicable
-                    if self.get_env_override(key).is_some() {
-                        option
-                            .metadata
-                            .insert("env_override".to_string(), Value::Bool(true));
-                        debug!("Setting {key} overridden by env var");
+                match self.get_value_with_secret_support(key, option) {
+                    Ok(Some((value, env_overridden))) => {
+                        option.value = Some(value);
+                        if env_overridden {
+                            option
+                                .metadata
+                                .insert("env_override".to_string(), Value::Bool(true));
+                            debug!("Setting {key} overridden by env var");
+                        }
+                    }
+                    Ok(None) => {
+                        // Fallback to default if helper returns None
+                        option.value = Some(option.default.clone());
+                    }
+                    Err(e) => {
+                        debug!("Failed to read value for {key}: {e}");
+                        option.value = Some(option.default.clone());
                     }
-                } else {
-                    // Fallback to default if helper returns None
-                    option.value = Some(option.default.clone());
                 }
             }
         }
@@ -189,8 +200,9 @@ impl<S: StorageBackend + 'static, Schema: SettingsSchema> SettingsManager<S, Sch
             .get(key)
             .ok_or_else(|| Error::SettingNotFound(format!("{category}.{setting_name}")))?;
 
-        // Use the helper that handles both secrets and regular settings
+        // Use the helper that handles both secrets and regular settings.
         self.get_value_with_secret_support(key, setting_metadata)?
+            .map(|(v, _)| v)
             .ok_or_else(|| Error::SettingNotFound(format!("{category}.{setting_name}")))
     }
 
@@ -374,7 +386,7 @@ impl<S: StorageBackend + 'static, Schema: SettingsSchema> SettingsManager<S, Sch
         for sub_type in sub_types {
             categories.push(ExportCategory {
                 id: sub_type.clone(),
-                name: sub_type.clone(), // Could be enhanced with display names
+                name: sub_type.clone(),
                 category_type: ExportCategoryType::SubSettings,
                 optional: false,
                 description: None,
