fix(config): serialize secret mutations (#11388)

Signed-off-by: Jasper Hugo <jasper@spiral.xyz>

crates/goose/src/config/base.rs

@@ -1,5 +1,6 @@
 use crate::config::paths::Paths;
 use crate::config::GooseMode;
+use crate::providers::private_file::{private_file_target_path, write_private_file};
 use fs2::FileExt;
 use goose_providers::thinking::ThinkingEffort;
 #[cfg(feature = "system-keyring")]
@@ -17,25 +18,13 @@ use std::sync::{Arc, Mutex};
 use thiserror::Error;
 
 fn write_secrets_file(path: &Path, content: &str) -> std::io::Result<()> {
-    #[cfg(unix)]
-    {
-        use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
-        let mut file = OpenOptions::new()
-            .write(true)
-            .create(true)
-            .truncate(false)
-            .mode(0o600)
-            .open(path)?;
+    write_private_file(path, content)
+}
 
-        file.set_permissions(std::fs::Permissions::from_mode(0o600))?;
-        file.set_len(0)?;
-        file.write_all(content.as_bytes())
-    }
-
-    #[cfg(not(unix))]
-    {
-        std::fs::write(path, content)
-    }
+fn secrets_lock_path(path: &Path) -> PathBuf {
+    let mut lock_path = path.as_os_str().to_os_string();
+    lock_path.push(".lock");
+    PathBuf::from(lock_path)
 }
 
 #[cfg(feature = "system-keyring")]
@@ -684,32 +673,7 @@ impl Config {
             cached_secrets.clone()
         } else {
             tracing::debug!("secrets cache miss, fetching from storage");
-
-            let loaded = match &self.secrets {
-                #[cfg(feature = "system-keyring")]
-                SecretStorage::Keyring { service } => {
-                    let result =
-                        self.handle_keyring_operation(|entry| entry.get_password(), service, None);
-
-                    match result {
-                        Ok(content) => {
-                            let values: HashMap<String, Value> = serde_json::from_str(&content)?;
-                            values
-                        }
-                        Err(ConfigError::FallbackToFileStorage) => {
-                            self.fallback_to_file_storage()?
-                        }
-                        Err(ConfigError::KeyringError(msg))
-                            if msg.contains("No entry found")
-                                || msg.contains("No matching entry found") =>
-                        {
-                            self.fallback_to_file_storage()?
-                        }
-                        Err(e) => return Err(e),
-                    }
-                }
-                SecretStorage::File { path } => self.read_secrets_from_file(path)?,
-            };
+            let loaded = self.load_secrets_from_storage()?;
 
             *cache = Some(loaded.clone());
             loaded
@@ -944,6 +908,60 @@ impl Config {
         Ok(result)
     }
 
+    fn load_secrets_from_storage(&self) -> Result<HashMap<String, Value>, ConfigError> {
+        match &self.secrets {
+            #[cfg(feature = "system-keyring")]
+            SecretStorage::Keyring { service } => {
+                let result =
+                    self.handle_keyring_operation(|entry| entry.get_password(), service, None);
+
+                match result {
+                    Ok(content) => Ok(serde_json::from_str(&content)?),
+                    Err(ConfigError::FallbackToFileStorage) => self.fallback_to_file_storage(),
+                    Err(ConfigError::KeyringError(msg))
+                        if msg.contains("No entry found")
+                            || msg.contains("No matching entry found") =>
+                    {
+                        self.fallback_to_file_storage()
+                    }
+                    Err(e) => Err(e),
+                }
+            }
+            SecretStorage::File { path } => self.read_secrets_from_file(path),
+        }
+    }
+
+    fn secrets_mutation_lock_path(&self) -> Result<PathBuf, ConfigError> {
+        let storage_path = match &self.secrets {
+            #[cfg(feature = "system-keyring")]
+            SecretStorage::Keyring { .. } => Self::secrets_file_path(),
+            SecretStorage::File { path } => path.clone(),
+        };
+        Ok(secrets_lock_path(&private_file_target_path(&storage_path)?))
+    }
+
+    fn lock_secrets_for_mutation(&self) -> Result<std::fs::File, ConfigError> {
+        let lock_path = self.secrets_mutation_lock_path()?;
+        if let Some(parent) = lock_path
+            .parent()
+            .filter(|parent| !parent.as_os_str().is_empty())
+        {
+            std::fs::create_dir_all(parent)
+                .map_err(|e| ConfigError::DirectoryError(e.to_string()))?;
+        }
+
+        let lock_file = OpenOptions::new()
+            .read(true)
+            .write(true)
+            .create(true)
+            .truncate(false)
+            .open(lock_path)?;
+        lock_file
+            .lock_exclusive()
+            .map_err(|e| ConfigError::LockError(e.to_string()))?;
+        Ok(lock_file)
+    }
+
     fn write_all_secrets(&self, values: &HashMap<String, Value>) -> Result<(), ConfigError> {
         match &self.secrets {
             #[cfg(feature = "system-keyring")]
@@ -974,7 +992,8 @@ impl Config {
         mutate: impl FnOnce(&mut HashMap<String, Value>),
     ) -> Result<(), ConfigError> {
         let _guard = self.guard.lock().unwrap();
-        let mut values = self.all_secrets()?;
+        let _storage_lock = self.lock_secrets_for_mutation()?;
+        let mut values = self.load_secrets_from_storage()?;
         mutate(&mut values);
         self.write_all_secrets(&values)
     }
@@ -1547,6 +1566,105 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn test_secret_mutation_does_not_restore_deleted_secret() -> Result<(), ConfigError> {
+        let directory = TempDir::new().unwrap();
+        let config_path = directory.path().join("config.yaml");
+        let secrets_path = directory.path().join("secrets.yaml");
+        let first = Config::new_with_file_secrets(&config_path, &secrets_path)?;
+        let second = Config::new_with_file_secrets(&config_path, &secrets_path)?;
+
+        first.set_secret("revoked", &"old-token")?;
+        first.set_secret("retained", &"retained-value")?;
+        let _: String = first.get_secret("revoked")?;
+
+        second.delete_secret("revoked")?;
+        first.set_secret("new", &"new-value")?;
+
+        let current = Config::new_with_file_secrets(&config_path, &secrets_path)?;
+        assert!(matches!(
+            current.get_secret::<String>("revoked"),
+            Err(ConfigError::NotFound(_))
+        ));
+        assert_eq!(current.get_secret::<String>("retained")?, "retained-value");
+        assert_eq!(current.get_secret::<String>("new")?, "new-value");
+
+        Ok(())
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_secret_mutation_atomically_replaces_storage_file() -> Result<(), ConfigError> {
+        use std::io::Read;
+        use std::os::unix::fs::MetadataExt;
+
+        let directory = TempDir::new().unwrap();
+        let config_path = directory.path().join("config.yaml");
+        let secrets_path = directory.path().join("secrets.yaml");
+        let config = Config::new_with_file_secrets(&config_path, &secrets_path)?;
+
+        config.set_secret("key", &"old-value")?;
+        let mut old_file = std::fs::File::open(&secrets_path)?;
+        let old_inode = old_file.metadata()?.ino();
+
+        config.set_secret("key", &"new-value")?;
+
+        assert_ne!(std::fs::metadata(&secrets_path)?.ino(), old_inode);
+        let mut old_contents = String::new();
+        old_file.read_to_string(&mut old_contents)?;
+        let old_values: HashMap<String, Value> = serde_yaml::from_str(&old_contents)?;
+        assert_eq!(
+            old_values.get("key"),
+            Some(&Value::String("old-value".into()))
+        );
+        assert_eq!(config.get_secret::<String>("key")?, "new-value");
+
+        Ok(())
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_secret_mutation_lock_uses_resolved_storage_target() -> Result<(), ConfigError> {
+        use std::os::unix::fs::symlink;
+
+        let directory = TempDir::new().unwrap();
+        let config_path = directory.path().join("config.yaml");
+        let secrets_path = directory.path().join("secrets.yaml");
+        let secrets_alias = directory.path().join("secrets-alias.yaml");
+        std::fs::write(&secrets_path, "{}\n")?;
+        symlink("secrets.yaml", &secrets_alias)?;
+
+        let direct = Config::new_with_file_secrets(&config_path, &secrets_path)?;
+        let aliased = Config::new_with_file_secrets(&config_path, &secrets_alias)?;
+
+        assert_eq!(
+            direct.secrets_mutation_lock_path()?,
+            aliased.secrets_mutation_lock_path()?
+        );
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_secret_reads_remain_cached_across_instances() -> Result<(), ConfigError> {
+        let directory = TempDir::new().unwrap();
+        let config_path = directory.path().join("config.yaml");
+        let secrets_path = directory.path().join("secrets.yaml");
+        let first = Config::new_with_file_secrets(&config_path, &secrets_path)?;
+        let second = Config::new_with_file_secrets(&config_path, &secrets_path)?;
+
+        first.set_secret("key", &"initial")?;
+        assert_eq!(first.get_secret::<String>("key")?, "initial");
+
+        second.set_secret("key", &"updated")?;
+        assert_eq!(first.get_secret::<String>("key")?, "initial");
+
+        first.invalidate_secrets_cache();
+        assert_eq!(first.get_secret::<String>("key")?, "updated");
+
+        Ok(())
+    }
+
     #[test]
     fn test_concurrent_writes() -> Result<(), ConfigError> {
         use std::sync::{Arc, Barrier, Mutex};

crates/goose/src/providers/mod.rs

@@ -70,7 +70,7 @@ pub mod openai_compatible {
 }
 pub mod openrouter;
 pub mod pi_acp;
-mod private_file;
+pub(crate) mod private_file;
 pub mod provider_registry;
 pub mod provider_secrets;
 pub mod provider_test;

crates/goose/src/providers/private_file.rs

@@ -1,5 +1,5 @@
 use std::io::{self, Write};
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 #[cfg(windows)]
 fn to_windows_api_path(path: &Path) -> io::Result<Vec<u16>> {
@@ -186,13 +186,50 @@ fn persist_private_temporary_file(
         .map_err(|error| error.error)
 }
 
+pub(crate) fn private_file_target_path(path: &Path) -> io::Result<PathBuf> {
+    const MAX_SYMLINK_HOPS: usize = 1;
+
+    let mut resolved = PathBuf::from(path);
+    let mut hops = 0;
+    loop {
+        match std::fs::symlink_metadata(&resolved) {
+            Ok(metadata) if metadata.file_type().is_symlink() => {
+                if hops >= MAX_SYMLINK_HOPS {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidInput,
+                        format!(
+                            "Too many symlink levels (or a cycle) while resolving private file path: {path:?}"
+                        ),
+                    ));
+                }
+                hops += 1;
+
+                let target = std::fs::read_link(&resolved)?;
+                resolved = if target.is_absolute() {
+                    target
+                } else {
+                    resolved
+                        .parent()
+                        .unwrap_or_else(|| Path::new("."))
+                        .join(target)
+                };
+            }
+            Ok(_) => return Ok(resolved),
+            Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(resolved),
+            Err(error) => return Err(error),
+        }
+    }
+}
+
+fn private_file_parent(path: &Path) -> &Path {
+    path.parent()
+        .filter(|parent| !parent.as_os_str().is_empty())
+        .unwrap_or_else(|| Path::new("."))
+}
+
 pub(crate) fn write_private_file(path: &Path, contents: &str) -> io::Result<()> {
-    let parent = path.parent().ok_or_else(|| {
-        io::Error::new(
-            io::ErrorKind::InvalidInput,
-            "private file path must have a parent directory",
-        )
-    })?;
+    let write_path = private_file_target_path(path)?;
+    let parent = private_file_parent(&write_path);
     std::fs::create_dir_all(parent)?;
 
     let mut temporary = create_private_temporary_file(parent)?;
@@ -205,7 +242,7 @@ pub(crate) fn write_private_file(path: &Path, contents: &str) -> io::Result<()>
     }
     temporary.write_all(contents.as_bytes())?;
     temporary.as_file().sync_all()?;
-    persist_private_temporary_file(temporary, path)?;
+    persist_private_temporary_file(temporary, &write_path)?;
     Ok(())
 }
 
@@ -231,6 +268,64 @@ mod tests {
         assert_eq!(metadata.permissions().mode() & 0o777, 0o600);
         assert_eq!(std::fs::read_to_string(path).unwrap(), "new-secret");
     }
+
+    #[test]
+    fn preserves_symlink_and_replaces_target() {
+        use std::os::unix::fs::symlink;
+
+        let directory = tempfile::tempdir().unwrap();
+        let path = directory.path().join("token.json");
+        let target = directory.path().join("managed-token.json");
+        std::fs::write(&target, "old").unwrap();
+        std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)).unwrap();
+        let old_inode = std::fs::metadata(&target).unwrap().ino();
+        symlink("managed-token.json", &path).unwrap();
+
+        write_private_file(&path, "new-secret").unwrap();
+
+        assert!(std::fs::symlink_metadata(&path)
+            .unwrap()
+            .file_type()
+            .is_symlink());
+        let metadata = std::fs::metadata(&target).unwrap();
+        assert_ne!(metadata.ino(), old_inode);
+        assert_eq!(metadata.permissions().mode() & 0o777, 0o600);
+        assert_eq!(std::fs::read_to_string(target).unwrap(), "new-secret");
+    }
+
+    #[test]
+    fn parentless_relative_path_uses_current_directory() {
+        let path = Path::new("token.json");
+        let resolved = private_file_target_path(path).unwrap();
+
+        assert_eq!(private_file_parent(&resolved), Path::new("."));
+    }
+
+    #[test]
+    fn rejects_chained_symlinks() {
+        use std::os::unix::fs::symlink;
+
+        let directory = tempfile::tempdir().unwrap();
+        let path = directory.path().join("token.json");
+        let intermediate = directory.path().join("managed-token.json");
+        let target = directory.path().join("actual-token.json");
+        std::fs::write(&target, "old").unwrap();
+        symlink("actual-token.json", &intermediate).unwrap();
+        symlink("managed-token.json", &path).unwrap();
+
+        let error = write_private_file(&path, "new-secret").unwrap_err();
+
+        assert_eq!(error.kind(), io::ErrorKind::InvalidInput);
+        assert_eq!(std::fs::read_to_string(target).unwrap(), "old");
+        assert!(std::fs::symlink_metadata(path)
+            .unwrap()
+            .file_type()
+            .is_symlink());
+        assert!(std::fs::symlink_metadata(intermediate)
+            .unwrap()
+            .file_type()
+            .is_symlink());
+    }
 }
 
 #[cfg(all(test, windows))]