-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: write to real file if config.yaml is symlink #7669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -459,20 +459,57 @@ impl Config { | |
| load_init_config_from_workspace() | ||
| } | ||
|
|
||
| fn config_write_target_path(&self) -> Result<PathBuf, ConfigError> { | ||
| 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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Limiting symlink resolution to a single hop causes Useful? React with 👍 / 👎. |
||
| 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<Mapping, ConfigError> { | |
| 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(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal makes sense to me, but why not use
fs::canonicalize?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the config_path may not exist, we should return it as the target_path in this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. It just seems like symlink resolution is something we should let the OS do, not on our own in a loop. But I'm not sure there's a better way if we also want to preserve the creating of parent directories.