From 0ed7b02d5f5dc8fc9cd60ce9c8347cbabef7ec2d Mon Sep 17 00:00:00 2001 From: Jasper Date: Thu, 20 Aug 2026 14:03:53 +0000 Subject: [PATCH] fix(config): serialize secret mutations (#11388) Signed-off-by: Jasper Hugo --- crates/goose/src/config/base.rs | 208 ++++++++++++++++----- crates/goose/src/providers/mod.rs | 2 +- crates/goose/src/providers/private_file.rs | 111 ++++++++++- 3 files changed, 267 insertions(+), 54 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index a62eff31f..8e48d19b4 100644 --- a/crates/goose/src/config/base.rs +++ b/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 = 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, 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 { + 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 { + 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) -> Result<(), ConfigError> { match &self.secrets { #[cfg(feature = "system-keyring")] @@ -974,7 +992,8 @@ impl Config { mutate: impl FnOnce(&mut HashMap), ) -> 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::("revoked"), + Err(ConfigError::NotFound(_)) + )); + assert_eq!(current.get_secret::("retained")?, "retained-value"); + assert_eq!(current.get_secret::("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 = serde_yaml::from_str(&old_contents)?; + assert_eq!( + old_values.get("key"), + Some(&Value::String("old-value".into())) + ); + assert_eq!(config.get_secret::("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::("key")?, "initial"); + + second.set_secret("key", &"updated")?; + assert_eq!(first.get_secret::("key")?, "initial"); + + first.invalidate_secrets_cache(); + assert_eq!(first.get_secret::("key")?, "updated"); + + Ok(()) + } + #[test] fn test_concurrent_writes() -> Result<(), ConfigError> { use std::sync::{Arc, Barrier, Mutex}; diff --git a/crates/goose/src/providers/mod.rs b/crates/goose/src/providers/mod.rs index 75e6713b2..f7e5669e7 100644 --- a/crates/goose/src/providers/mod.rs +++ b/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; diff --git a/crates/goose/src/providers/private_file.rs b/crates/goose/src/providers/private_file.rs index ef188e426..8fd8aa183 100644 --- a/crates/goose/src/providers/private_file.rs +++ b/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> { @@ -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 { + 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))]