diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index dbfa787496..c72aec0e27 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -459,20 +459,57 @@ impl Config { load_init_config_from_workspace() } + fn config_write_target_path(&self) -> Result { + let mut path = self.config_path.clone(); + + // Follow symlinks so we update the target file without replacing the link itself. + const MAX_SYMLINK_HOPS: usize = 1; + let mut hops = 0usize; + loop { + match std::fs::symlink_metadata(&path) { + Ok(meta) if meta.file_type().is_symlink() => { + if hops >= MAX_SYMLINK_HOPS { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "Too many symlink levels (or a cycle) while resolving config path: {:?}", + self.config_path + ), + ) + .into()); + } + hops += 1; + + let link = std::fs::read_link(&path)?; + path = if link.is_absolute() { + link + } else { + path.parent().unwrap_or_else(|| Path::new(".")).join(link) + }; + } + Ok(_) => return Ok(path), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(path), + Err(e) => return Err(e.into()), + } + } + } + fn save_values(&self, values: &Mapping) -> Result<(), ConfigError> { // Create backup before writing new config self.create_backup_if_needed()?; + let target_path = self.config_write_target_path()?; + // Convert to YAML for storage let yaml_value = serde_yaml::to_string(values)?; - if let Some(parent) = self.config_path.parent() { + if let Some(parent) = target_path.parent() { std::fs::create_dir_all(parent) .map_err(|e| ConfigError::DirectoryError(e.to_string()))?; } // Write to a temporary file first for atomic operation - let temp_path = self.config_path.with_extension("tmp"); + let temp_path = target_path.with_extension("tmp"); { let mut file = OpenOptions::new() @@ -493,7 +530,7 @@ impl Config { } // Atomically replace the original file - std::fs::rename(&temp_path, &self.config_path)?; + std::fs::rename(&temp_path, &target_path)?; Ok(()) } @@ -1029,7 +1066,7 @@ pub fn load_init_config_from_workspace() -> Result { mod tests { use super::*; use serial_test::serial; - use tempfile::NamedTempFile; + use tempfile::{NamedTempFile, TempDir}; #[test] fn test_basic_config() -> Result<(), ConfigError> { let config = new_test_config(); @@ -1250,6 +1287,78 @@ mod tests { Ok(()) } + #[test] + #[cfg(unix)] + fn test_write_follows_symlink() -> Result<(), ConfigError> { + use std::os::unix::fs as unix_fs; + + let dir = TempDir::new().unwrap(); + let target_path = dir.path().join("real_config.yaml"); + let symlink_path = dir.path().join("config.yaml"); + + std::fs::write(&target_path, "{}\n")?; + unix_fs::symlink(&target_path, &symlink_path)?; + + let secrets_file = NamedTempFile::new().unwrap(); + let config = Config::new_with_file_secrets(&symlink_path, secrets_file.path())?; + + config.set_param("key1", "value1")?; + + let meta = std::fs::symlink_metadata(&symlink_path)?; + assert!( + meta.file_type().is_symlink(), + "config path should remain a symlink" + ); + + let content = std::fs::read_to_string(&symlink_path)?; + assert!(content.contains("key1: value1")); + + let content = std::fs::read_to_string(&target_path)?; + assert!(content.contains("key1: value1")); + + Ok(()) + } + + #[test] + #[cfg(unix)] + fn test_write_fails_on_long_symlink_chain() -> Result<(), ConfigError> { + use std::os::unix::fs as unix_fs; + + let dir = TempDir::new().unwrap(); + let target_path = dir.path().join("real_config.yaml"); + std::fs::write(&target_path, "{}\n")?; + + // config.yaml -> link1.yaml -> real_config.yaml + // We only allow following one symlink hop. If there's another symlink, we should fail + // rather than overwrite the intermediate symlink. + let config_symlink = dir.path().join("config.yaml"); + let link1 = dir.path().join("link1.yaml"); + unix_fs::symlink(&target_path, &link1)?; + unix_fs::symlink(&link1, &config_symlink)?; + + let secrets_file = NamedTempFile::new().unwrap(); + let config = Config::new_with_file_secrets(&config_symlink, secrets_file.path())?; + + let err = config.set_param("key1", "value1").unwrap_err(); + assert!( + err.to_string().contains("Too many symlink levels"), + "unexpected error: {err}" + ); + + let meta = std::fs::symlink_metadata(&config_symlink)?; + assert!( + meta.file_type().is_symlink(), + "config path should remain a symlink" + ); + let meta = std::fs::symlink_metadata(&link1)?; + assert!( + meta.file_type().is_symlink(), + "intermediate link should remain a symlink" + ); + + Ok(()) + } + #[test] fn test_config_recovery_from_backup() -> Result<(), ConfigError> { let config_file = NamedTempFile::new().unwrap();