diff options
| author | akbash <akbash@herdr.dev> | 2026-09-14 23:13:52 +0300 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2026-09-14 22:13:52 +0200 |
| commit | 5e38e5fb5fc2cc5d30844df71432a60742bcb572 (patch) | |
| tree | 31896855b510cf03c57e19e4e3ef454bef028a59 | |
| parent | b0f21ed2cb85613050b794bf6c78a7c367a94e0d (diff) | |
fix: protect integration configs from failed writes (#3973)
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
* fix: atomically write integration configs
refs #3970
---------
Co-authored-by: akbash-bot <300245827+akbash-bot@users.noreply.github.com>
Co-authored-by: JJ Liebig <jonathan.liebig@gmail.com>
| -rw-r--r-- | docs/next/website/src/content/docs/integrations.mdx | 2 | ||||
| -rw-r--r-- | docs/next/website/src/content/docs/ja/integrations.mdx | 2 | ||||
| -rw-r--r-- | docs/next/website/src/content/docs/zh-cn/integrations.mdx | 2 | ||||
| -rw-r--r-- | src/integration/config_file.rs | 147 | ||||
| -rw-r--r-- | src/integration/config_file/tests.rs | 243 | ||||
| -rw-r--r-- | src/integration/mod.rs | 1 | ||||
| -rw-r--r-- | src/integration/opencode_config.rs | 51 | ||||
| -rw-r--r-- | src/integration/targets.rs | 98 | ||||
| -rw-r--r-- | src/integration/tests.rs | 83 | ||||
| -rw-r--r-- | src/platform/linux.rs | 131 | ||||
| -rw-r--r-- | src/platform/linux/config_file_tests.rs | 96 | ||||
| -rw-r--r-- | src/platform/macos.rs | 143 | ||||
| -rw-r--r-- | src/platform/macos/config_file_tests.rs | 44 | ||||
| -rw-r--r-- | src/platform/windows.rs | 154 | ||||
| -rw-r--r-- | src/platform/windows/config_backup.rs | 165 | ||||
| -rw-r--r-- | src/platform/windows/config_backup/tests.rs | 375 |
16 files changed, 1700 insertions, 37 deletions
diff --git a/docs/next/website/src/content/docs/integrations.mdx b/docs/next/website/src/content/docs/integrations.mdx index a3ddc968..4516061f 100644 --- a/docs/next/website/src/content/docs/integrations.mdx +++ b/docs/next/website/src/content/docs/integrations.mdx @@ -53,6 +53,8 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` +Herdr saves integration configs atomically on Linux/macOS and for new files; existing Windows files keep their permissions through a backup-first, in-place update. If Windows reports a backup, close the agent and inspect the config before retrying: discard—not restore—an unfinished `.herdr-backup.pending` only after verifying the config, or restore completed `.herdr-backup` contents into the existing file if needed (if missing, establish its intended permissions first), then verify the config and remove the backup. + ## How Herdr uses integrations Herdr uses integrations in two ways: diff --git a/docs/next/website/src/content/docs/ja/integrations.mdx b/docs/next/website/src/content/docs/ja/integrations.mdx index 69bda8c1..75cebbe4 100644 --- a/docs/next/website/src/content/docs/ja/integrations.mdx +++ b/docs/next/website/src/content/docs/ja/integrations.mdx @@ -55,6 +55,8 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` +Herdr は Linux/macOS の統合設定と全プラットフォームの新規ファイルをアトミックに保存し、Windows の既存ファイルは権限を維持するため、バックアップ後に同じファイルを更新します。Windows でバックアップが通知された場合は、再試行前にエージェントを終了して設定を確認し、未完了の `.herdr-backup.pending` は復旧元に使わず設定が正しいことを確認してから削除するか、必要なら完成済みの `.herdr-backup` の内容を既存ファイルに書き戻し(ファイルがない場合は先に意図した権限を設定)、設定が正しいことを確認してからバックアップを削除してください。 + ## Herdr がインテグレーションをどう使うか Herdr はインテグレーションを 2 つの異なる方法で使います: diff --git a/docs/next/website/src/content/docs/zh-cn/integrations.mdx b/docs/next/website/src/content/docs/zh-cn/integrations.mdx index df1a4d4b..f6d7c1f3 100644 --- a/docs/next/website/src/content/docs/zh-cn/integrations.mdx +++ b/docs/next/website/src/content/docs/zh-cn/integrations.mdx @@ -55,6 +55,8 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` +Herdr 对 Linux/macOS 上的集成配置和所有平台的新文件使用原子保存;Windows 上的现有文件先备份再原地更新,以保留权限。如果 Windows 提示存在备份,请在重试前关闭智能体并检查配置:未完成的 `.herdr-backup.pending` 只能在确认配置正确后删除,不能用于恢复;完整 `.herdr-backup` 的内容可在需要时写回现有文件(若文件已丢失,先设置其预期权限),确认配置正确后再删除备份。 + ## Herdr 如何使用集成 Herdr 以两种不同方式使用集成: diff --git a/src/integration/config_file.rs b/src/integration/config_file.rs new file mode 100644 index 00000000..11e34c3d --- /dev/null +++ b/src/integration/config_file.rs @@ -0,0 +1,147 @@ +//! Protected writes for user-owned integration configuration, not managed assets. + +use std::fs::{self, OpenOptions}; +use std::io; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +#[cfg(test)] +mod tests; + +static NEXT_TEMP: AtomicU64 = AtomicU64::new(0); + +/// Check before changing assets as well as immediately before replacing a config. +/// This is deliberately not config parsing or a transaction across multiple files. +pub(super) fn check_config_targets(dir: &Path, names: &[&str]) -> io::Result<()> { + for name in names { + check_config_target(&dir.join(name))?; + } + Ok(()) +} + +pub(super) fn check_config_target(path: &Path) -> io::Result<()> { + reject_hard_links(path)?; + crate::platform::check_config_write_target(&resolve_target(path)?) +} + +fn reject_hard_links(path: &Path) -> io::Result<()> { + let metadata = match fs::metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error), + }; + if metadata.is_file() && crate::platform::config_file_link_count(path)? > 1 { + return Err(io::Error::other(format!( + "cannot update {}: config has multiple hard links; use a separate file or a symlink before retrying", + path.display() + ))); + } + Ok(()) +} + +// Unlike canonicalize, this also follows dangling symlinks on a first install. +fn resolve_target(path: &Path) -> io::Result<PathBuf> { + let mut current = path.to_path_buf(); + for _ in 0..40 { + match fs::symlink_metadata(¤t) { + Ok(metadata) if metadata.file_type().is_symlink() => { + let link = fs::read_link(¤t)?; + current = if link.is_absolute() { + link + } else { + current.parent().unwrap_or(Path::new(".")).join(link) + }; + } + Ok(metadata) if !metadata.is_file() => { + return Err(io::Error::other(format!( + "cannot update {}: config is not a regular file", + path.display() + ))); + } + Ok(_) => return Ok(current), + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(current), + Err(error) => return Err(error), + } + } + Err(io::Error::other(format!( + "cannot update {}: too many symbolic links", + path.display() + ))) +} + +pub(super) fn write_config(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<()> { + check_config_target(path)?; + let target = resolve_target(path)?; + if crate::platform::write_existing_config(&target, contents.as_ref())? { + return Ok(()); + } + let replacement = Replacement::prepare(&target, contents.as_ref())?; + replacement.commit() +} + +struct Replacement { + target: PathBuf, + temporary: PathBuf, +} + +impl Replacement { + fn prepare(path: &Path, contents: &[u8]) -> io::Result<Self> { + reject_hard_links(path)?; + let target = resolve_target(path)?; + let existing = match fs::metadata(&target) { + Ok(_) => { + // A writable parent must not let rename bypass file write permissions. + OpenOptions::new().read(true).write(true).open(&target)?; + Some(target.as_path()) + } + Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Err(error) => return Err(error), + }; + let parent = target + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or(Path::new(".")); + for _ in 0..128 { + let sequence = NEXT_TEMP.fetch_add(1, Ordering::Relaxed); + let temporary = parent.join(format!( + ".herdr-config-{}-{sequence}.tmp", + std::process::id() + )); + // Existing configs can contain secrets. Start their staging file private; + // the platform writer preserves the original permissions before publication. + // New configs retain ordinary create/umask/inherited-ACL defaults. + let created = crate::platform::create_config_temporary(&temporary, existing.is_some()); + match created { + Ok(file) => drop(file), + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => continue, + Err(error) => return Err(error), + } + let replacement = Self { + target: target.clone(), + temporary, + }; + crate::platform::write_config_temporary(existing, &replacement.temporary, contents)?; + return Ok(replacement); + } + Err(io::Error::new( + io::ErrorKind::AlreadyExists, + "could not allocate a unique config temporary file", + )) + } + + fn commit(self) -> io::Result<()> { + reject_hard_links(&self.target)?; + crate::platform::replace_file(&self.temporary, &self.target) + } +} + +impl Drop for Replacement { + fn drop(&mut self) { + // After publication the temporary name is absent. Never remove the target. + if let Err(error) = fs::remove_file(&self.temporary) { + if error.kind() != io::ErrorKind::NotFound { + tracing::warn!(path = %self.temporary.display(), %error, "failed to remove integration config temporary file"); + } + } + } +} diff --git a/src/integration/config_file/tests.rs b/src/integration/config_file/tests.rs new file mode 100644 index 00000000..916edc67 --- /dev/null +++ b/src/integration/config_file/tests.rs @@ -0,0 +1,243 @@ +use super::*; + +struct Directory(PathBuf); + +impl Directory { + fn new() -> Self { + let path = std::env::temp_dir().join(format!( + "herdr-config-write-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + fs::create_dir(&path).unwrap(); + Self(path) + } +} + +impl Drop for Directory { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + +// Windows existing files have separate native backup/recovery coverage. +#[cfg(unix)] +const ATOMIC_CASES: &[bool] = &[false, true]; +#[cfg(windows)] +const ATOMIC_CASES: &[bool] = &[false]; + +#[test] +fn config_publication_keeps_old_content_until_commit() { + let dir = Directory::new(); + for &existing in ATOMIC_CASES { + let path = dir.0.join(if existing { "existing" } else { "new" }); + if existing { + fs::write(&path, b"old preferences").unwrap(); + } + let staged = Replacement::prepare(&path, b"complete new preferences").unwrap(); + if existing { + assert_eq!(fs::read(&path).unwrap(), b"old preferences"); + } else { + assert!(!path.exists()); + } + assert_eq!( + fs::read(&staged.temporary).unwrap(), + b"complete new preferences" + ); + staged.commit().unwrap(); + assert_eq!(fs::read(&path).unwrap(), b"complete new preferences"); + } + assert_eq!(fs::read_dir(&dir.0).unwrap().count(), ATOMIC_CASES.len()); +} + +#[test] +fn abandoned_and_failed_publication_leave_config_unchanged() { + for &existing in ATOMIC_CASES { + let dir = Directory::new(); + let path = dir.0.join("config"); + if existing { + fs::write(&path, b"original").unwrap(); + } + drop(Replacement::prepare(&path, b"new").unwrap()); + let staged = Replacement::prepare(&path, b"new").unwrap(); + fs::remove_file(&staged.temporary).unwrap(); + assert_eq!(staged.commit().unwrap_err().kind(), io::ErrorKind::NotFound); + if existing { + assert_eq!(fs::read(&path).unwrap(), b"original"); + } else { + assert!(!path.exists()); + } + assert_eq!(fs::read_dir(&dir.0).unwrap().count(), usize::from(existing)); + assert!(write_config(&dir.0, b"not a file").is_err()); + assert!(dir.0.is_dir()); + } +} + +#[test] +fn hard_links_are_rejected_before_staging_and_rechecked_before_commit() { + let dir = Directory::new(); + let path = dir.0.join("config"); + let alias = dir.0.join("alias"); + fs::write(&path, b"original").unwrap(); + #[cfg(unix)] + let staged = Replacement::prepare(&path, b"new").unwrap(); + fs::hard_link(&path, &alias).unwrap(); + #[cfg(unix)] + assert!(staged + .commit() + .unwrap_err() + .to_string() + .contains("multiple hard links")); + for candidate in [&path, &alias] { + let error = write_config(candidate, b"new").unwrap_err().to_string(); + assert!(error.contains(&candidate.display().to_string())); + assert_eq!(fs::read(candidate).unwrap(), b"original"); + assert_eq!( + crate::platform::config_file_link_count(candidate).unwrap(), + 2 + ); + } + assert_eq!(fs::read_dir(&dir.0).unwrap().count(), 2); +} + +#[cfg(unix)] +fn symlink(target: &Path, link: &Path) { + std::os::unix::fs::symlink(target, link).unwrap(); +} +#[cfg(windows)] +fn symlink(target: &Path, link: &Path) { + std::os::windows::fs::symlink_file(target, link).unwrap(); +} + +#[test] +fn symlink_chains_and_dangling_targets_preserve_links() { + let dir = Directory::new(); + let other = dir.0.join("other"); + fs::create_dir(&other).unwrap(); + let target = other.join("preferences"); + let intermediate = other.join("link"); + let entry = dir.0.join("config"); + symlink(&target, &intermediate); + // A Windows reparse target needs native separators, unlike ordinary Win32 paths. + let relative_target = Path::new("other").join("link"); + symlink(&relative_target, &entry); + let original_intermediate = fs::read_link(&intermediate).unwrap(); + assert_eq!( + fs::metadata(&entry).unwrap_err().kind(), + io::ErrorKind::NotFound + ); + write_config(&entry, b"first install").unwrap(); + write_config(&entry, b"second install").unwrap(); + assert_eq!(fs::read(&target).unwrap(), b"second install"); + assert_eq!(fs::read_link(&entry).unwrap(), relative_target); + assert_eq!(fs::read_link(&intermediate).unwrap(), original_intermediate); + assert_eq!(fs::read_dir(&other).unwrap().count(), 2); + let alias = other.join("hard-link"); + fs::hard_link(&target, &alias).unwrap(); + assert!(write_config(&entry, b"must not change").is_err()); + assert_eq!(fs::read(&alias).unwrap(), b"second install"); + assert_eq!(fs::read_link(&entry).unwrap(), relative_target); + + let cycle = dir.0.join("cycle"); + symlink(Path::new("cycle"), &cycle); + assert!(write_config(&cycle, b"must not replace the link").is_err()); + assert_eq!(fs::read_link(&cycle).unwrap(), Path::new("cycle")); +} + +#[test] +fn existing_permissions_and_new_file_defaults_are_preserved() { + let dir = Directory::new(); + let path = dir.0.join("existing"); + fs::write(&path, b"original").unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&path, fs::Permissions::from_mode(0o640)).unwrap(); + } + let permissions = fs::metadata(&path).unwrap().permissions(); + write_config(&path, b"new").unwrap(); + assert_eq!(fs::metadata(&path).unwrap().permissions(), permissions); + let ordinary = dir.0.join("ordinary"); + let new = dir.0.join("new"); + fs::write(&ordinary, b"ordinary creation").unwrap(); + write_config(&new, b"atomic creation").unwrap(); + assert_eq!( + fs::metadata(&ordinary).unwrap().permissions(), + fs::metadata(&new).unwrap().permissions() + ); +} + +#[cfg(unix)] +#[test] +fn writable_directory_does_not_bypass_read_only_config() { + use std::os::unix::{fs::PermissionsExt, process::CommandExt}; + const CHILD: &str = "HERDR_CONFIG_READ_ONLY_TEST"; + if let Some(path) = std::env::var_os(CHILD) { + let error = write_config(Path::new(&path), b"must not replace").unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + println!("read-only rejection executed"); + return; + } + let dir = Directory::new(); + let path = dir.0.join("read-only"); + fs::write(&path, b"original").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o444)).unwrap(); + fs::set_permissions(&dir.0, fs::Permissions::from_mode(0o777)).unwrap(); + let mut child = std::process::Command::new(std::env::current_exe().unwrap()); + child + .args([ + "--exact", + "integration::config_file::tests::writable_directory_does_not_bypass_read_only_config", + "--nocapture", + ]) + .env(CHILD, &path); + // Root bypasses Unix mode checks. Test the real user path in a child instead. + if unsafe { libc::geteuid() } == 0 { + child.gid(65534).uid(65534); + } + let output = child.output().unwrap(); + assert!(output.status.success(), "child failed: {output:?}"); + assert!(String::from_utf8_lossy(&output.stdout).contains("read-only rejection executed")); + assert_eq!(fs::read(&path).unwrap(), b"original"); + assert_eq!(fs::read_dir(&dir.0).unwrap().count(), 1); +} + +#[cfg(unix)] +#[test] +fn partial_write_errors_preserve_files_and_do_not_remove_collisions() { + const CHILD: &str = "HERDR_CONFIG_PARTIAL_WRITE_TEST"; + if let Some(path) = std::env::var_os(CHILD) { + let dir = PathBuf::from(path); + // This process runs only this test. A collision must neither be used nor removed. + NEXT_TEMP.store(0, Ordering::Relaxed); + let collision = dir.join(format!(".herdr-config-{}-0.tmp", std::process::id())); + fs::write(&collision, b"unrelated file").unwrap(); + for name in ["existing", "new"] { + let error = write_config(&dir.join(name), vec![b'x'; 8192]).unwrap_err(); + assert_eq!(error.raw_os_error(), Some(libc::EFBIG)); + } + assert_eq!(fs::read(collision).unwrap(), b"unrelated file"); + println!("partial-write paths executed"); + return; + } + let dir = Directory::new(); + fs::write(dir.0.join("existing"), b"original").unwrap(); + let output = std::process::Command::new("bash") + .args(["-c", "trap '' XFSZ; ulimit -f 1; exec \"$@\"", "herdr-test"]) + .arg(std::env::current_exe().unwrap()) + .args(["--exact", "integration::config_file::tests::partial_write_errors_preserve_files_and_do_not_remove_collisions", "--nocapture"]) + .env(CHILD, &dir.0) + .output().unwrap(); + assert!(output.status.success(), "child failed: {output:?}"); + assert!(String::from_utf8_lossy(&output.stdout).contains("partial-write paths executed")); + assert_eq!(fs::read(dir.0.join("existing")).unwrap(), b"original"); + assert!(!dir.0.join("new").exists()); + assert_eq!( + fs::read_dir(&dir.0).unwrap().count(), + 2, + "only original and collision may remain" + ); +} diff --git a/src/integration/mod.rs b/src/integration/mod.rs index 97163da5..daa714e0 100644 --- a/src/integration/mod.rs +++ b/src/integration/mod.rs @@ -2,6 +2,7 @@ mod actions; mod claude_settings; mod command; mod config_edit; +mod config_file; mod env; mod file_ops; mod opencode_config; diff --git a/src/integration/opencode_config.rs b/src/integration/opencode_config.rs index 2ab91e84..b062365b 100644 --- a/src/integration/opencode_config.rs +++ b/src/integration/opencode_config.rs @@ -6,6 +6,8 @@ use jsonc_parser::cst::{CstInputValue, CstRootNode}; use jsonc_parser::ParseOptions; use serde_json::Value; +use super::config_file::{check_config_target, write_config}; + const TUI_CONFIG_NAME: &str = "tui.jsonc"; pub(crate) fn tui_config_path(config_dir: &Path) -> PathBuf { @@ -44,6 +46,7 @@ pub(crate) fn add_cli_plugin( plugin_spec: &str, ) -> io::Result<Option<PathBuf>> { let path = config_dir.join("cli.json"); + check_config_target(&path)?; // OpenCode imports V1 TUI preferences (`tui.json`, `kv.json`) into cli.json on // its first V2 start, but only while cli.json is absent. Defer registration // while those sources still exist so we do not skip the migration; otherwise @@ -60,6 +63,7 @@ fn cli_migration_pending(config_dir: &Path, state_dir: &Path) -> bool { } fn add_plugin(config_path: PathBuf, key: &str, plugin_spec: &str) -> io::Result<PathBuf> { + check_config_target(&config_path)?; let content = if config_path.is_file() { fs::read_to_string(&config_path)? } else { @@ -90,7 +94,7 @@ fn add_plugin(config_path: PathBuf, key: &str, plugin_spec: &str) -> io::Result< } } - fs::write(&config_path, root.to_string())?; + write_config(&config_path, root.to_string())?; Ok(config_path) } @@ -103,6 +107,7 @@ pub(crate) fn remove_cli_plugin(config_dir: &Path, plugin_spec: &str) -> io::Res } fn remove_plugin(config_path: &Path, key: &str, plugin_spec: &str) -> io::Result<bool> { + check_config_target(config_path)?; if !config_path.is_file() { return Ok(false); } @@ -133,7 +138,7 @@ fn remove_plugin(config_path: &Path, key: &str, plugin_spec: &str) -> io::Result property.remove(); } - fs::write(config_path, root.to_string())?; + write_config(config_path, root.to_string())?; Ok(true) } @@ -245,6 +250,48 @@ mod tests { .unwrap() } + #[cfg(unix)] + #[test] + fn failed_cli_registration_preserves_existing_config() { + const CHILD_CONFIG: &str = "HERDR_TEST_3970_CONFIG_DIR"; + if let Some(dir) = std::env::var_os(CHILD_CONFIG) { + let dir = PathBuf::from(dir); + let result = add_cli_plugin(&dir, &dir.join("state"), "./herdr-opencode"); + assert_eq!(result.unwrap_err().raw_os_error(), Some(libc::EFBIG)); + println!("registration reached the file-size limit"); + return; + } + + let dir = unique_dir(); + let path = dir.join("cli.json"); + let original = r#"{"theme":{"name":"catppuccin"},"plugins":["example"]}"#; + fs::write(&path, original).unwrap(); + // Apply the limit only to a child, after seeding the existing preferences. + // Ignoring SIGXFSZ makes the kernel return EFBIG instead of killing it. + let output = std::process::Command::new("bash") + .args(["-c", "trap '' XFSZ; ulimit -f 0; exec \"$@\"", "herdr-test"]) + .arg(std::env::current_exe().unwrap()) + .args([ + "--exact", + "integration::opencode_config::tests::failed_cli_registration_preserves_existing_config", + "--nocapture", + ]) + .env(CHILD_CONFIG, &dir) + .output() + .unwrap(); + let actual = fs::read_to_string(&path).unwrap(); + let remaining_files = fs::read_dir(&dir).unwrap().count(); + fs::remove_dir_all(&dir).unwrap(); + assert!(output.status.success(), "child failed: {output:?}"); + assert!(String::from_utf8_lossy(&output.stdout) + .contains("registration reached the file-size limit")); + assert_eq!( + actual, original, + "failed registration must preserve preferences" + ); + assert_eq!(remaining_files, 1, "temporary files must be cleaned up"); + } + #[test] fn add_and_remove_tui_plugin_preserves_jsonc_config() { let dir = unique_dir(); diff --git a/src/integration/targets.rs b/src/integration/targets.rs index cab8f7ee..6cb90c18 100644 --- a/src/integration/targets.rs +++ b/src/integration/targets.rs @@ -19,6 +19,7 @@ use super::config_edit::{ remove_direct_hook_commands, remove_flat_command_hook, remove_hermes_plugin_enabled, remove_hook_commands, remove_kimi_config_block, remove_simple_command_hook, }; +use super::config_file::{check_config_targets, write_config}; use super::env::{ antigravity_cli_dir, claude_dir, codex_dir, copilot_dir, cursor_dir, devin_dir, droid_dir, grok_dir, hermes_dir, hermes_plugin_dir, kilo_dir, kimi_dir, letta_dir, mastracode_dir, @@ -122,6 +123,7 @@ pub(crate) fn remove_legacy_pi_extension_from_omp_dir(dir: &Path) -> io::Result< pub(crate) fn install_claude() -> io::Result<ClaudeInstallPaths> { let dir = claude_dir()?; + check_config_targets(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "claude directory not found at {}. install claude code first", @@ -146,7 +148,7 @@ pub(crate) fn install_claude() -> io::Result<ClaudeInstallPaths> { remove_legacy_bash_hook_file(&hook_path)?; if updated_settings != existing_settings { - fs::write(&settings_path, updated_settings)?; + write_config(&settings_path, updated_settings)?; } Ok(ClaudeInstallPaths { @@ -157,6 +159,7 @@ pub(crate) fn install_claude() -> io::Result<ClaudeInstallPaths> { pub(crate) fn install_codex() -> io::Result<CodexInstallPaths> { let dir = codex_dir()?; + check_config_targets(&dir, &["hooks.json", "config.toml"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "codex config directory not found at {}. install codex first", @@ -198,7 +201,7 @@ pub(crate) fn install_codex() -> io::Result<CodexInstallPaths> { )?; remove_legacy_bash_hook_file(&hook_path)?; - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; let config_path = dir.join("config.toml"); let existing_config = if config_path.is_file() { @@ -208,7 +211,7 @@ pub(crate) fn install_codex() -> io::Result<CodexInstallPaths> { }; let new_config = build_codex_config_with_hooks(&existing_config); if new_config != existing_config { - fs::write(&config_path, new_config)?; + write_config(&config_path, new_config)?; } Ok(CodexInstallPaths { @@ -220,6 +223,7 @@ pub(crate) fn install_codex() -> io::Result<CodexInstallPaths> { pub(crate) fn install_kimi() -> io::Result<KimiInstallPaths> { let dir = kimi_dir()?; + check_config_targets(&dir, &["config.toml"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "kimi code config directory not found at {}. install kimi code first", @@ -242,7 +246,7 @@ pub(crate) fn install_kimi() -> io::Result<KimiInstallPaths> { }; let new_config = build_kimi_config_with_hooks(&existing_config, &hook_path); if new_config != existing_config { - fs::write(&config_path, new_config)?; + write_config(&config_path, new_config)?; } remove_legacy_bash_hook_file(&hook_path)?; @@ -254,6 +258,7 @@ pub(crate) fn install_kimi() -> io::Result<KimiInstallPaths> { pub(crate) fn install_copilot() -> io::Result<CopilotInstallPaths> { let dir = copilot_dir()?; + check_config_targets(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "copilot config directory not found at {}. install github copilot cli first", @@ -298,7 +303,7 @@ pub(crate) fn install_copilot() -> io::Result<CopilotInstallPaths> { } remove_legacy_bash_hook_file(&hook_path)?; - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; Ok(CopilotInstallPaths { hook_path, @@ -308,6 +313,7 @@ pub(crate) fn install_copilot() -> io::Result<CopilotInstallPaths> { pub(crate) fn install_devin() -> io::Result<DevinInstallPaths> { let dir = devin_dir()?; + check_config_targets(&dir, &["config.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "devin config directory not found at {}. install devin cli first", @@ -354,7 +360,7 @@ pub(crate) fn install_devin() -> io::Result<DevinInstallPaths> { } remove_legacy_bash_hook_file(&hook_path)?; - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; Ok(DevinInstallPaths { hook_path, @@ -364,6 +370,7 @@ pub(crate) fn install_devin() -> io::Result<DevinInstallPaths> { pub(crate) fn install_droid() -> io::Result<DroidInstallPaths> { let dir = droid_dir()?; + check_config_targets(&dir, &["settings.json", "hooks.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "droid config directory not found at {}. install droid first", @@ -414,7 +421,7 @@ pub(crate) fn install_droid() -> io::Result<DroidInstallPaths> { } remove_legacy_bash_hook_file(&hook_path)?; - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; let hooks_path = dir.join("hooks.json"); let mut updated_legacy_hooks = false; @@ -440,7 +447,7 @@ pub(crate) fn install_droid() -> io::Result<DroidInstallPaths> { } } if updated_legacy_hooks { - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; } } @@ -454,6 +461,7 @@ pub(crate) fn install_droid() -> io::Result<DroidInstallPaths> { pub(crate) fn install_opencode() -> io::Result<OpenCodeInstallPaths> { let dir = opencode_dir()?; + check_config_targets(&dir, &["tui.jsonc", "cli.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "opencode config directory not found at {}. install opencode first", @@ -507,6 +515,7 @@ pub(crate) fn install_kilo() -> io::Result<KiloInstallPaths> { pub(crate) fn install_hermes() -> io::Result<HermesInstallPaths> { let dir = hermes_dir()?; + check_config_targets(&dir, &["config.yaml"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "hermes config directory not found at {}. install hermes agent first", @@ -533,7 +542,7 @@ pub(crate) fn install_hermes() -> io::Result<HermesInstallPaths> { }; let new_config = ensure_hermes_plugin_enabled(&existing_config); if new_config != existing_config { - fs::write(&config_path, new_config)?; + write_config(&config_path, new_config)?; } Ok(HermesInstallPaths { @@ -563,8 +572,10 @@ pub(crate) fn uninstall_omp() -> io::Result<OmpUninstallResult> { } pub(crate) fn uninstall_claude() -> io::Result<ClaudeUninstallResult> { - let hook_path = claude_dir()?.join("hooks").join(CLAUDE_HOOK_INSTALL_NAME); - let settings_path = claude_dir()?.join("settings.json"); + let dir = claude_dir()?; + check_config_targets(&dir, &["settings.json"])?; + let hook_path = dir.join("hooks").join(CLAUDE_HOOK_INSTALL_NAME); + let settings_path = dir.join("settings.json"); let mut updated_settings = false; if settings_path.is_file() { @@ -573,7 +584,7 @@ pub(crate) fn uninstall_claude() -> io::Result<ClaudeUninstallResult> { uninstall_claude_settings(&existing_settings, &settings_path, &hook_path)?; updated_settings = new_settings != existing_settings; if updated_settings { - fs::write(&settings_path, new_settings)?; + write_config(&settings_path, new_settings)?; } } @@ -590,6 +601,7 @@ pub(crate) fn uninstall_claude() -> io::Result<ClaudeUninstallResult> { pub(crate) fn uninstall_codex() -> io::Result<CodexUninstallResult> { let codex_dir = codex_dir()?; + check_config_targets(&codex_dir, &["hooks.json"])?; let hook_path = codex_dir.join(CODEX_HOOK_INSTALL_NAME); let hooks_path = codex_dir.join("hooks.json"); let config_path = codex_dir.join("config.toml"); @@ -620,7 +632,7 @@ pub(crate) fn uninstall_codex() -> io::Result<CodexUninstallResult> { } if updated_hooks { - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; } } @@ -638,6 +650,7 @@ pub(crate) fn uninstall_codex() -> io::Result<CodexUninstallResult> { pub(crate) fn uninstall_kimi() -> io::Result<KimiUninstallResult> { let kimi_dir = kimi_dir()?; + check_config_targets(&kimi_dir, &["config.toml"])?; let hook_path = kimi_dir.join("hooks").join(KIMI_HOOK_INSTALL_NAME); let config_path = kimi_dir.join("config.toml"); let mut updated_config = false; @@ -646,7 +659,7 @@ pub(crate) fn uninstall_kimi() -> io::Result<KimiUninstallResult> { let existing_config = fs::read_to_string(&config_path)?; let new_config = remove_kimi_config_block(&existing_config); if new_config != existing_config { - fs::write(&config_path, new_config)?; + write_config(&config_path, new_config)?; updated_config = true; } } @@ -664,6 +677,7 @@ pub(crate) fn uninstall_kimi() -> io::Result<KimiUninstallResult> { pub(crate) fn uninstall_copilot() -> io::Result<CopilotUninstallResult> { let copilot_dir = copilot_dir()?; + check_config_targets(&copilot_dir, &["settings.json"])?; let hook_path = copilot_dir.join("hooks").join(COPILOT_HOOK_INSTALL_NAME); let settings_path = copilot_dir.join("settings.json"); let mut updated_settings = false; @@ -692,7 +706,7 @@ pub(crate) fn uninstall_copilot() -> io::Result<CopilotUninstallResult> { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -709,6 +723,7 @@ pub(crate) fn uninstall_copilot() -> io::Result<CopilotUninstallResult> { pub(crate) fn uninstall_devin() -> io::Result<DevinUninstallResult> { let devin_dir = devin_dir()?; + check_config_targets(&devin_dir, &["config.json"])?; let hook_path = devin_dir.join(DEVIN_HOOK_INSTALL_NAME); let settings_path = devin_dir.join("config.json"); let mut updated_settings = false; @@ -737,7 +752,7 @@ pub(crate) fn uninstall_devin() -> io::Result<DevinUninstallResult> { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -754,6 +769,7 @@ pub(crate) fn uninstall_devin() -> io::Result<DevinUninstallResult> { pub(crate) fn uninstall_droid() -> io::Result<DroidUninstallResult> { let droid_dir = droid_dir()?; + check_config_targets(&droid_dir, &["settings.json", "hooks.json"])?; let hook_path = droid_dir.join("hooks").join(DROID_HOOK_INSTALL_NAME); let hooks_path = droid_dir.join("hooks.json"); let settings_path = droid_dir.join("settings.json"); @@ -781,7 +797,7 @@ pub(crate) fn uninstall_droid() -> io::Result<DroidUninstallResult> { } if updated_hooks { - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; } } @@ -809,7 +825,7 @@ pub(crate) fn uninstall_droid() -> io::Result<DroidUninstallResult> { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -828,6 +844,7 @@ pub(crate) fn uninstall_droid() -> io::Result<DroidUninstallResult> { pub(crate) fn uninstall_opencode() -> io::Result<OpenCodeUninstallResult> { let dir = opencode_dir()?; + check_config_targets(&dir, &["tui.jsonc", "cli.json"])?; let tui_config_path = tui_config_path(&dir); let plugin_path = dir.join("plugins").join(OPENCODE_PLUGIN_INSTALL_NAME); let tui_plugin_path = dir.join(OPENCODE_TUI_PLUGIN_INSTALL_NAME); @@ -883,6 +900,7 @@ pub(crate) fn uninstall_kilo() -> io::Result<KiloUninstallResult> { pub(crate) fn uninstall_hermes() -> io::Result<HermesUninstallResult> { let dir = hermes_dir()?; + check_config_targets(&dir, &["config.yaml"])?; let plugin_dir = hermes_plugin_dir()?; let config_path = dir.join("config.yaml"); @@ -892,7 +910,7 @@ pub(crate) fn uninstall_hermes() -> io::Result<HermesUninstallResult> { let existing_config = fs::read_to_string(&config_path)?; let new_config = remove_hermes_plugin_enabled(&existing_config); if new_config != existing_config { - fs::write(&config_path, new_config)?; + write_config(&config_path, new_config)?; updated_config = true; } } @@ -907,6 +925,7 @@ pub(crate) fn uninstall_hermes() -> io::Result<HermesUninstallResult> { pub(crate) fn install_qodercli() -> io::Result<QodercliInstallPaths> { let dir = qodercli_dir()?; + check_config_targets(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "qodercli config directory not found at {}. install qodercli first", @@ -961,7 +980,7 @@ pub(crate) fn install_qodercli() -> io::Result<QodercliInstallPaths> { } remove_legacy_bash_hook_file(&hook_path)?; - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; Ok(QodercliInstallPaths { hook_path, @@ -971,6 +990,7 @@ pub(crate) fn install_qodercli() -> io::Result<QodercliInstallPaths> { pub(crate) fn install_qwen() -> io::Result<QwenInstallPaths> { let dir = qwen_dir()?; + check_config_targets(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "qwen code config directory not found at {}. install qwen code first", @@ -1014,7 +1034,7 @@ pub(crate) fn install_qwen() -> io::Result<QwenInstallPaths> { )?; } - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; Ok(QwenInstallPaths { hook_path, @@ -1225,6 +1245,7 @@ pub(crate) fn install_letta() -> io::Result<LettaInstallPaths> { pub(crate) fn install_cursor() -> io::Result<CursorInstallPaths> { let dir = cursor_dir()?; + check_config_targets(&dir, &["hooks.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "cursor config directory not found at {}. install cursor agent cli first", @@ -1271,7 +1292,7 @@ pub(crate) fn install_cursor() -> io::Result<CursorInstallPaths> { remove_simple_command_hook(hooks, "sessionEnd", &session_command)?; ensure_simple_command_hook(hooks, "sessionStart", session_command)?; - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; Ok(CursorInstallPaths { hook_path, @@ -1280,10 +1301,10 @@ pub(crate) fn install_cursor() -> io::Result<CursorInstallPaths> { } pub(crate) fn uninstall_qodercli() -> io::Result<QodercliUninstallResult> { - let hook_path = qodercli_dir()? - .join("hooks") - .join(QODERCLI_HOOK_INSTALL_NAME); - let settings_path = qodercli_dir()?.join("settings.json"); + let dir = qodercli_dir()?; + check_config_targets(&dir, &["settings.json"])?; + let hook_path = dir.join("hooks").join(QODERCLI_HOOK_INSTALL_NAME); + let settings_path = dir.join("settings.json"); let mut updated_settings = false; if settings_path.is_file() { @@ -1310,7 +1331,7 @@ pub(crate) fn uninstall_qodercli() -> io::Result<QodercliUninstallResult> { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -1326,8 +1347,10 @@ pub(crate) fn uninstall_qodercli() -> io::Result<QodercliUninstallResult> { } pub(crate) fn uninstall_qwen() -> io::Result<QwenUninstallResult> { - let hook_path = qwen_dir()?.join("hooks").join(QWEN_HOOK_INSTALL_NAME); - let settings_path = qwen_dir()?.join("settings.json"); + let dir = qwen_dir()?; + check_config_targets(&dir, &["settings.json"])?; + let hook_path = dir.join("hooks").join(QWEN_HOOK_INSTALL_NAME); + let settings_path = dir.join("settings.json"); let mut updated_settings = false; if settings_path.is_file() { @@ -1351,7 +1374,7 @@ pub(crate) fn uninstall_qwen() -> io::Result<QwenUninstallResult> { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -1407,6 +1430,7 @@ pub(crate) fn uninstall_letta() -> io::Result<LettaUninstallResult> { pub(crate) fn uninstall_cursor() -> io::Result<CursorUninstallResult> { let cursor_home = cursor_dir()?; + check_config_targets(&cursor_home, &["hooks.json"])?; let hook_path = cursor_home.join(CURSOR_HOOK_INSTALL_NAME); let hooks_path = cursor_home.join("hooks.json"); let mut updated_hooks = false; @@ -1436,7 +1460,7 @@ pub(crate) fn uninstall_cursor() -> io::Result<CursorUninstallResult> { } if updated_hooks { - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; } } @@ -1463,6 +1487,7 @@ pub(crate) fn mastracode_hook_command(hook_path: &Path, action: &str) -> String pub(crate) fn install_mastracode() -> io::Result<MastracodeInstallPaths> { let mastracode_home = mastracode_dir()?; + check_config_targets(&mastracode_home, &["hooks.json"])?; let hook_dir = mastracode_home.join("hooks"); fs::create_dir_all(&hook_dir)?; @@ -1500,7 +1525,7 @@ pub(crate) fn install_mastracode() -> io::Result<MastracodeInstallPaths> { )?; } - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; Ok(MastracodeInstallPaths { hook_path, @@ -1510,6 +1535,7 @@ pub(crate) fn install_mastracode() -> io::Result<MastracodeInstallPaths> { pub(crate) fn uninstall_mastracode() -> io::Result<MastracodeUninstallResult> { let mastracode_home = mastracode_dir()?; + check_config_targets(&mastracode_home, &["hooks.json"])?; let hook_path = mastracode_home .join("hooks") .join(MASTRACODE_HOOK_INSTALL_NAME); @@ -1542,7 +1568,7 @@ pub(crate) fn uninstall_mastracode() -> io::Result<MastracodeUninstallResult> { } if updated_hooks { - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; } } @@ -1558,6 +1584,7 @@ pub(crate) fn uninstall_mastracode() -> io::Result<MastracodeUninstallResult> { pub(crate) fn install_antigravity_cli() -> io::Result<AntigravityCliInstallPaths> { let dir = antigravity_cli_dir()?; + check_config_targets(&dir, &["hooks.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "antigravity cli config directory not found at {}. install antigravity cli first", @@ -1595,7 +1622,7 @@ pub(crate) fn install_antigravity_cli() -> io::Result<AntigravityCliInstallPaths antigravity_cli_hook_block(&hook_path), ); - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; Ok(AntigravityCliInstallPaths { hook_path, @@ -1633,6 +1660,7 @@ fn antigravity_cli_hook_block(hook_path: &Path) -> Value { pub(crate) fn uninstall_antigravity_cli() -> io::Result<AntigravityCliUninstallResult> { let dir = antigravity_cli_dir()?; + check_config_targets(&dir, &["hooks.json"])?; let hook_path = dir.join("hooks").join(ANTIGRAVITY_CLI_HOOK_INSTALL_NAME); let hooks_path = dir.join("hooks.json"); let mut updated_hooks = false; @@ -1653,7 +1681,7 @@ pub(crate) fn uninstall_antigravity_cli() -> io::Result<AntigravityCliUninstallR updated_hooks = hooks.remove(ANTIGRAVITY_CLI_HOOK_BLOCK_NAME).is_some(); if updated_hooks { - fs::write(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; + write_config(&hooks_path, serde_json::to_string_pretty(&hooks_file)?)?; } } diff --git a/src/integration/tests.rs b/src/integration/tests.rs index eb310637..1a102992 100644 --- a/src/integration/tests.rs +++ b/src/integration/tests.rs @@ -2396,6 +2396,89 @@ fn opencode_v2_install_status_and_uninstall_preserve_cli_preferences() { } #[test] +fn opencode_hard_link_rejection_precedes_install_and_uninstall_asset_changes() { + let _lock = integration_env_lock(); + let base = unique_base(); + let home = base.join("home"); + let dir = home.join(".config/opencode"); + fs::create_dir_all(dir.join("plugins")).unwrap(); + std::env::set_var("HOME", &home); + let plugin = dir.join("plugins").join(OPENCODE_PLUGIN_INSTALL_NAME); + fs::write(&plugin, "previous integration").unwrap(); + let config = dir.join("cli.json"); + let original = r#"{"plugins":["./herdr-opencode"],"theme":"system"}"#; + fs::write(&config, original).unwrap(); + let alias = base.join("linked-config"); + fs::hard_link(&config, &alias).unwrap(); + let target = crate::api::schema::IntegrationTarget::Opencode; + for error in [ + install_target(target).unwrap_err(), + uninstall_target(target).unwrap_err(), + ] { + assert!(error.to_string().contains("multiple hard links")); + assert!(error.to_string().contains("cli.json")); + } + assert_eq!(fs::read_to_string(&plugin).unwrap(), "previous integration"); + assert_eq!(fs::read_to_string(&alias).unwrap(), original); + assert_eq!(crate::platform::config_file_link_count(&config).unwrap(), 2); + assert!(!dir.join("tui.jsonc").exists()); + assert!(!dir.join(OPENCODE_V2_TUI_PLUGIN_DIR).exists()); + std::env::remove_var("HOME"); + fs::remove_dir_all(base).unwrap(); +} + +#[cfg(windows)] +#[test] +fn opencode_recovery_copy_blocks_retry_before_parsing_or_asset_changes() { + let _lock = integration_env_lock(); + let base = unique_base(); + let home = base.join("home"); + let dir = home.join(".config/opencode"); + fs::create_dir_all(dir.join("plugins")).unwrap(); + std::env::set_var("HOME", &home); + let plugin = dir.join("plugins").join(OPENCODE_PLUGIN_INSTALL_NAME); + fs::write(&plugin, "previous integration").unwrap(); + let config = dir.join("cli.json"); + let backup = dir.join("cli.json.herdr-backup"); + let original = r#"{"plugins":["./herdr-opencode"],"theme":"system"}"#; + fs::write(&backup, original).unwrap(); + let target = crate::api::schema::IntegrationTarget::Opencode; + for contents in [Some("{"), Some(original), None] { + if let Some(contents) = contents { + fs::write(&config, contents).unwrap(); + } else { + fs::remove_file(&config).unwrap(); + } + for error in [ + install_target(target).unwrap_err(), + uninstall_target(target).unwrap_err(), + ] { + assert!(error.to_string().contains("recovery copy"), "{error}"); + assert!(error.to_string().contains("cli.json.herdr-backup")); + } + assert_eq!(fs::read_to_string(&backup).unwrap(), original); + if contents.is_none() { + assert!(!config.exists()); + } + } + // A symlink invocation must discover the referent's recovery copy too. + let referent = base.join("preferences.json"); + fs::write(&referent, "{").unwrap(); + let linked_backup = base.join("preferences.json.herdr-backup"); + fs::rename(&backup, &linked_backup).unwrap(); + std::os::windows::fs::symlink_file(&referent, &config).unwrap(); + let link_before = fs::read_link(&config).unwrap(); + let error = install_target(target).unwrap_err(); + assert!(error.to_string().contains("preferences.json.herdr-backup")); + assert_eq!(fs::read_link(&config).unwrap(), link_before); + assert_eq!(fs::read_to_string(&plugin).unwrap(), "previous integration"); + assert!(!dir.join("tui.jsonc").exists()); + assert!(!dir.join(OPENCODE_V2_TUI_PLUGIN_DIR).exists()); + std::env::remove_var("HOME"); + fs::remove_dir_all(base).unwrap(); +} + +#[test] fn opencode_invalid_cli_config_does_not_overwrite_existing_plugins() { let _lock = integration_env_lock(); let base = unique_base(); diff --git a/src/platform/linux.rs b/src/platform/linux.rs index 7ea0a2c6..7dd2879e 100644 --- a/src/platform/linux.rs +++ b/src/platform/linux.rs @@ -23,6 +23,9 @@ pub(crate) use super::unix_common::{ ClientStreamReader, StatusCommandGuard, }; +#[cfg(test)] +mod config_file_tests; + const WSL_MARKER_ENV_VARS: &[&str] = &["WSL_DISTRO_NAME", "WSL_INTEROP"]; const PROCESS_DETECTION_ENV_VAR: &str = "HERDR_PROCESS_DETECTION"; const CHILD_GROUPS_SCAN_LIMIT: usize = 64; @@ -104,6 +107,134 @@ pub(crate) fn launch_executable() -> std::io::Result<PathBuf> { Ok(executable) } +pub(crate) fn config_file_link_count(path: &std::path::Path) -> std::io::Result<u64> { + use std::os::unix::fs::MetadataExt; + Ok(std::fs::metadata(path)?.nlink()) +} + +pub(crate) fn check_config_write_target(_target: &std::path::Path) -> std::io::Result<()> { + Ok(()) +} + +pub(crate) fn write_existing_config( + _target: &std::path::Path, + _contents: &[u8], +) -> std::io::Result<bool> { + // Unix keeps atomic replacement for existing files too. + Ok(false) +} + +pub(crate) fn create_config_temporary( + path: &std::path::Path, + private: bool, +) -> std::io::Result<std::fs::File> { + use std::os::unix::fs::OpenOptionsExt; + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(if private { 0o600 } else { 0o666 }) + .open(path) +} + +pub(crate) fn write_config_temporary( + source: Option<&std::path::Path>, + temporary: &std::path::Path, + contents: &[u8], +) -> std::io::Result<()> { + use std::os::{fd::AsRawFd, unix::fs::MetadataExt}; + let mut output = std::fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(temporary)?; + if let Some(source) = source { + let input = std::fs::File::open(source)?; + let metadata = input.metadata()?; + let current = output.metadata()?; + if (metadata.uid(), metadata.gid()) != (current.uid(), current.gid()) { + // Keep ownership before restoring mode/ACLs; chown can clear mode bits. + if unsafe { libc::fchown(output.as_raw_fd(), metadata.uid(), metadata.gid()) } != 0 { + return Err(std::io::Error::last_os_error()); + } + } + // Replace inherited ACLs before enabling the original mode. Prepare all + // access controls while the temporary is empty, before writing secrets. + copy_config_xattrs(input.as_raw_fd(), output.as_raw_fd())?; + output.set_permissions(metadata.permissions())?; + } + output.write_all(contents)?; + output.sync_all() +} + +// Access ACLs and security labels live in xattrs on Linux. Mode bits alone can +// silently broaden access, especially with a default ACL on the parent directory. +fn copy_config_xattrs(source: RawFd, destination: RawFd) -> std::io::Result<()> { + use std::ffi::CStr; + fn names(fd: RawFd) -> std::io::Result<Vec<u8>> { + let size = unsafe { libc::flistxattr(fd, std::ptr::null_mut(), 0) }; + if size < 0 { + let error = std::io::Error::last_os_error(); + if error.raw_os_error() == Some(libc::ENOTSUP) { + return Ok(Vec::new()); + } + return Err(error); + } + let mut buffer = vec![0; size as usize]; + let read = unsafe { libc::flistxattr(fd, buffer.as_mut_ptr().cast(), buffer.len()) }; + if read < 0 { + return Err(std::io::Error::last_os_error()); + } + buffer.truncate(read as usize); + Ok(buffer) + } + fn value(fd: RawFd, name: &CStr) -> std::io::Result<Vec<u8>> { + let size = unsafe { libc::fgetxattr(fd, name.as_ptr(), std::ptr::null_mut(), 0) }; + if size < 0 { + return Err(std::io::Error::last_os_error()); + } + let mut buffer = vec![0; size as usize]; + let read = + unsafe { libc::fgetxattr(fd, name.as_ptr(), buffer.as_mut_ptr().cast(), buffer.len()) }; + if read < 0 { + return Err(std::io::Error::last_os_error()); + } + buffer.truncate(read as usize); + Ok(buffer) + } + let source_names = names(source)?; + for bytes in names(destination)?.split_inclusive(|byte| *byte == 0) { + if !source_names + .split_inclusive(|byte| *byte == 0) + .any(|name| name == bytes) + { + let name = CStr::from_bytes_with_nul(bytes).map_err(std::io::Error::other)?; + if unsafe { libc::fremovexattr(destination, name.as_ptr()) } != 0 { + return Err(std::io::Error::last_os_error()); + } + } + } + for bytes in source_names.split_inclusive(|byte| *byte == 0) { + let name = CStr::from_bytes_with_nul(bytes).map_err(std::io::Error::other)?; + let original = value(source, name)?; + // Avoid requiring relabel privileges when the inherited label already matches. + if value(destination, name).is_ok_and(|current| current == original) { + continue; + } + if unsafe { + libc::fsetxattr( + destination, + name.as_ptr(), + original.as_ptr().cast(), + original.len(), + 0, + ) + } != 0 + { + return Err(std::io::Error::last_os_error()); + } + } + Ok(()) +} + pub fn raise_server_nofile_limit() {} pub(crate) fn should_draw_host_cursor_by_default() -> bool { diff --git a/src/platform/linux/config_file_tests.rs b/src/platform/linux/config_file_tests.rs new file mode 100644 index 00000000..b11c117d --- /dev/null +++ b/src/platform/linux/config_file_tests.rs @@ -0,0 +1,96 @@ +use super::*; +use std::ffi::CStr; +use std::os::{fd::AsRawFd, unix::fs::MetadataExt}; + +fn set_attribute(file: &std::fs::File, name: &CStr, value: &[u8]) { + assert_eq!( + unsafe { + libc::fsetxattr( + file.as_raw_fd(), + name.as_ptr(), + value.as_ptr().cast(), + value.len(), + 0, + ) + }, + 0, + "{}", + std::io::Error::last_os_error() + ); +} + +fn attribute(file: &std::fs::File, name: &CStr) -> Option<Vec<u8>> { + let mut value = vec![0; 1024]; + let read = unsafe { + libc::fgetxattr( + file.as_raw_fd(), + name.as_ptr(), + value.as_mut_ptr().cast(), + value.len(), + ) + }; + if read < 0 { + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::ENODATA) + ); + return None; + } + value.truncate(read as usize); + Some(value) +} + +#[test] +fn config_metadata_preserves_ownership_and_acl_without_inheriting_extra_access() { + let dir = std::env::temp_dir().join(format!("herdr-config-acl-{}", std::process::id())); + std::fs::create_dir(&dir).unwrap(); + // Linux UAPI posix_acl_xattr_header/entry, version 2, little-endian fields. + // Owner rw, named user 65534 read, group none, mask read, other none. + let mut acl = 2_u32.to_le_bytes().to_vec(); + for (tag, permissions, id) in [ + (1_u16, 6_u16, u32::MAX), + (2, 4, 65534), + (4, 0, u32::MAX), + (16, 4, u32::MAX), + (32, 0, u32::MAX), + ] { + acl.extend(tag.to_le_bytes()); + acl.extend(permissions.to_le_bytes()); + acl.extend(id.to_le_bytes()); + } + for has_acl in [false, true] { + let source = dir.join(format!("source-{has_acl}")); + let target = dir.join(format!("target-{has_acl}")); + std::fs::write(&source, b"old").unwrap(); + let input = std::fs::File::open(&source).unwrap(); + if unsafe { libc::geteuid() } == 0 { + assert_eq!(unsafe { libc::fchown(input.as_raw_fd(), 1001, 1002) }, 0); + } + if has_acl { + set_attribute(&input, c"system.posix_acl_access", &acl); + } + set_attribute(&input, c"user.herdr-test", b"preserve this attribute"); + let original = input.metadata().unwrap(); + drop(create_config_temporary(&target, true).unwrap()); + let output = std::fs::File::open(&target).unwrap(); + // Model a default ACL inherited from the destination's parent directory. + set_attribute(&output, c"system.posix_acl_access", &acl); + write_config_temporary(Some(&source), &target, b"new").unwrap(); + let actual = output.metadata().unwrap(); + assert_eq!( + (actual.uid(), actual.gid(), actual.mode()), + (original.uid(), original.gid(), original.mode()) + ); + assert_eq!( + attribute(&output, c"system.posix_acl_access"), + attribute(&input, c"system.posix_acl_access") + ); + assert_eq!( + attribute(&output, c"user.herdr-test"), + Some(b"preserve this attribute".to_vec()) + ); + assert_eq!(std::fs::read(source).unwrap(), b"old"); + assert_eq!(std::fs::read(target).unwrap(), b"new"); + } + std::fs::remove_dir_all(dir).unwrap(); +} diff --git a/src/platform/macos.rs b/src/platform/macos.rs index 4e86deab..4b11a618 100644 --- a/src/platform/macos.rs +++ b/src/platform/macos.rs @@ -23,6 +23,149 @@ pub(crate) use super::unix_common::{ ClientStreamReader, StatusCommandGuard, }; +#[cfg(test)] +mod config_file_tests; + +pub(crate) fn config_file_link_count(path: &Path) -> std::io::Result<u64> { + use std::os::unix::fs::MetadataExt; + Ok(std::fs::metadata(path)?.nlink()) +} + +pub(crate) fn check_config_write_target(_target: &std::path::Path) -> std::io::Result<()> { + Ok(()) +} + +pub(crate) fn write_existing_config( + _target: &std::path::Path, + _contents: &[u8], +) -> std::io::Result<bool> { + // Unix keeps atomic replacement for existing files too. + Ok(false) +} + +pub(crate) fn create_config_temporary( + path: &Path, + private: bool, +) -> std::io::Result<std::fs::File> { + if !private { + return std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path); + } + use std::os::fd::FromRawFd; + // Darwin's opaque ACL/filesec APIs (<sys/acl.h>, <sys/fcntl.h>) are not + // exposed by libc. Supply a non-inheriting empty ACL at creation: clearing + // inherited ACEs later cannot revoke descriptors opened in the meantime. + unsafe extern "C" { + fn acl_init(count: libc::c_int) -> *mut libc::c_void; + fn acl_free(acl: *mut libc::c_void) -> libc::c_int; + fn acl_get_flagset_np(acl: *mut libc::c_void, flags: *mut *mut libc::c_void) + -> libc::c_int; + fn acl_add_flag_np(flags: *mut libc::c_void, flag: libc::c_uint) -> libc::c_int; + fn filesec_init() -> *mut libc::c_void; + fn filesec_free(security: *mut libc::c_void); + fn filesec_set_property( + security: *mut libc::c_void, + property: libc::c_int, + value: *const libc::c_void, + ) -> libc::c_int; + fn openx_np( + path: *const libc::c_char, + flags: libc::c_int, + security: *mut libc::c_void, + ) -> libc::c_int; + } + const FILESEC_MODE: libc::c_int = 4; + const FILESEC_ACL: libc::c_int = 5; + const ACL_FLAG_NO_INHERIT: libc::c_uint = 1 << 17; + let path = + std::ffi::CString::new(path.as_os_str().as_bytes()).map_err(std::io::Error::other)?; + let acl = unsafe { acl_init(0) }; + if acl.is_null() { + return Err(std::io::Error::last_os_error()); + } + let security = unsafe { filesec_init() }; + if security.is_null() { + let error = std::io::Error::last_os_error(); + unsafe { + acl_free(acl); + } + return Err(error); + } + let result = (|| { + let mut flags = std::ptr::null_mut(); + let mode: libc::mode_t = 0o600; + if unsafe { acl_get_flagset_np(acl, &mut flags) } != 0 + || unsafe { acl_add_flag_np(flags, ACL_FLAG_NO_INHERIT) } != 0 + || unsafe { + filesec_set_property(security, FILESEC_MODE, std::ptr::from_ref(&mode).cast()) + } != 0 + || unsafe { + filesec_set_property(security, FILESEC_ACL, std::ptr::from_ref(&acl).cast()) + } != 0 + { + return Err(std::io::Error::last_os_error()); + } + let fd = unsafe { + openx_np( + path.as_ptr(), + libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL | libc::O_CLOEXEC, + security, + ) + }; + if fd < 0 { + return Err(std::io::Error::last_os_error()); + } + // The successful exclusive create returns one owned descriptor. + Ok(unsafe { std::fs::File::from_raw_fd(fd) }) + })(); + unsafe { + filesec_free(security); + acl_free(acl); + } + result +} + +pub(crate) fn write_config_temporary( + source: Option<&Path>, + temporary: &Path, + contents: &[u8], +) -> std::io::Result<()> { + use std::os::{fd::AsRawFd, unix::fs::MetadataExt}; + let mut output = std::fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(temporary)?; + if let Some(source) = source { + let input = std::fs::File::open(source)?; + let metadata = input.metadata()?; + let current = output.metadata()?; + if (metadata.uid(), metadata.gid()) != (current.uid(), current.gid()) + && unsafe { libc::fchown(output.as_raw_fd(), metadata.uid(), metadata.gid()) } != 0 + { + return Err(std::io::Error::last_os_error()); + } + // Prepare access controls while the temporary is still empty. Copy ACLs + // before mode bits so no inherited/default grant can expose the content. + // Do not copy data or old timestamps. + if unsafe { + libc::fcopyfile( + input.as_raw_fd(), + output.as_raw_fd(), + std::ptr::null_mut(), + libc::COPYFILE_ACL | libc::COPYFILE_XATTR, + ) + } != 0 + { + return Err(std::io::Error::last_os_error()); + } + output.set_permissions(metadata.permissions())?; + } + output.write_all(contents)?; + output.sync_all() +} + const PROC_PGRP_ONLY: u32 = 2; const SERVER_NOFILE_LIMIT_TARGET: libc::rlim_t = 8192; diff --git a/src/platform/macos/config_file_tests.rs b/src/platform/macos/config_file_tests.rs new file mode 100644 index 00000000..f08d6c2a --- /dev/null +++ b/src/platform/macos/config_file_tests.rs @@ -0,0 +1,44 @@ +use super::*; + +#[test] +fn config_replacement_preserves_macos_acl() { + let dir = std::env::temp_dir().join(format!("herdr-config-acl-{}", std::process::id())); + std::fs::create_dir(&dir).unwrap(); + let source = dir.join("source"); + let temporary = dir.join("temporary"); + std::fs::write(&source, b"original").unwrap(); + assert!(Command::new("chmod") + .args(["+a", "everyone deny execute"]) + .arg(&source) + .status() + .unwrap() + .success()); + let acl = |path: &Path| { + let output = Command::new("ls").arg("-le").arg(path).output().unwrap(); + assert!(output.status.success()); + String::from_utf8(output.stdout) + .unwrap() + .lines() + .skip(1) + .collect::<Vec<_>>() + .join("\n") + }; + let original = acl(&source); + assert!(original.contains("everyone deny execute")); + assert!(Command::new("chmod") + .args(["+a", "everyone allow read,file_inherit"]) + .arg(&dir) + .status() + .unwrap() + .success()); + drop(create_config_temporary(&temporary, true).unwrap()); + assert!( + acl(&temporary).is_empty(), + "staging must not inherit allow ACEs" + ); + write_config_temporary(Some(&source), &temporary, b"new").unwrap(); + assert_eq!(acl(&temporary), original); + assert_eq!(std::fs::read(&source).unwrap(), b"original"); + assert_eq!(std::fs::read(&temporary).unwrap(), b"new"); + std::fs::remove_dir_all(dir).unwrap(); +} diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 879778cc..080bbf34 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -14,6 +14,7 @@ use std::{ }; mod clipboard_image; +mod config_backup; pub(crate) fn classify_child_exit(status: &portable_pty::ExitStatus) -> super::ChildExitReason { // STATUS_CONTROL_C_EXIT is reported without a Unix signal by portable-pty. @@ -140,6 +141,159 @@ pub(crate) fn replace_file( } } +pub(crate) fn config_file_link_count(path: &std::path::Path) -> std::io::Result<u64> { + use windows_sys::Win32::Storage::FileSystem::{ + GetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION, + }; + let file = std::fs::File::open(path)?; + let mut info = BY_HANDLE_FILE_INFORMATION::default(); + if unsafe { GetFileInformationByHandle(file.as_raw_handle(), &mut info) } == 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(u64::from(info.nNumberOfLinks)) +} + +pub(crate) fn create_config_temporary( + path: &std::path::Path, + private: bool, +) -> std::io::Result<std::fs::File> { + if !private { + return std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path); + } + use interprocess::os::windows::security_descriptor::{ + AsSecurityDescriptorExt as _, SecurityDescriptor, + }; + use widestring::U16CString; + use windows_sys::Win32::{ + Foundation::GENERIC_WRITE, + Storage::FileSystem::{ + CreateFileW, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_DELETE, FILE_SHARE_READ, + FILE_SHARE_WRITE, + }, + }; + let sddl = + U16CString::from_str("D:P(A;;GA;;;SY)(A;;GA;;;OW)").map_err(std::io::Error::other)?; + let descriptor = SecurityDescriptor::deserialize(&sddl)?; + let mut attributes = SECURITY_ATTRIBUTES { + nLength: size_of::<SECURITY_ATTRIBUTES>() as u32, + lpSecurityDescriptor: null_mut(), + bInheritHandle: 0, + }; + descriptor.write_to_security_attributes(&mut attributes); + let path = extended_length_path(path)?; + let handle = unsafe { + CreateFileW( + path.as_ptr(), + GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + &attributes, + CREATE_NEW, + FILE_ATTRIBUTE_NORMAL, + null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(std::io::Error::last_os_error()); + } + // CreateFileW returned an owned handle; File closes it exactly once. + Ok(unsafe { std::fs::File::from_raw_handle(handle) }) +} + +pub(crate) fn write_config_temporary( + source: Option<&std::path::Path>, + temporary: &std::path::Path, + contents: &[u8], +) -> std::io::Result<()> { + use std::io::Write; + if source.is_some() { + // If preparation finds an existing file, leave it to the recovery-backed + // path instead of applying replacement-file permissions. + return Err(std::io::Error::new( + std::io::ErrorKind::AlreadyExists, + "config appeared while preparing a new file; retry the update", + )); + } + let mut output = std::fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(temporary)?; + output.write_all(contents)?; + output.sync_all() +} + +pub(crate) fn check_config_write_target(target: &std::path::Path) -> std::io::Result<()> { + config_backup::check_recovery(target) +} + +pub(crate) fn write_existing_config( + target: &std::path::Path, + contents: &[u8], +) -> std::io::Result<bool> { + config_backup::write_existing(target, contents) +} + +#[cfg(test)] +fn config_security_descriptor( + path: &std::path::Path, + information: windows_sys::Win32::Security::OBJECT_SECURITY_INFORMATION, +) -> std::io::Result<Vec<u8>> { + use windows_sys::Win32::Security::GetFileSecurityW; + let path = extended_length_path(path)?; + let mut needed = 0; + unsafe { GetFileSecurityW(path.as_ptr(), information, null_mut(), 0, &mut needed) }; + if needed == 0 { + return Err(std::io::Error::last_os_error()); + } + let mut descriptor = vec![0_u8; needed as usize]; + if unsafe { + GetFileSecurityW( + path.as_ptr(), + information, + descriptor.as_mut_ptr().cast(), + needed, + &mut needed, + ) + } == 0 + { + return Err(std::io::Error::last_os_error()); + } + Ok(descriptor) +} + +#[cfg(test)] +fn config_security_sddl( + descriptor: &mut [u8], + information: windows_sys::Win32::Security::OBJECT_SECURITY_INFORMATION, +) -> std::io::Result<Vec<u16>> { + use windows_sys::Win32::Security::{ + Authorization::{ConvertSecurityDescriptorToStringSecurityDescriptorW, SDDL_REVISION_1}, + SACL_SECURITY_INFORMATION, + }; + let mut text = null_mut(); + if unsafe { + ConvertSecurityDescriptorToStringSecurityDescriptorW( + descriptor.as_mut_ptr().cast(), + SDDL_REVISION_1, + // Only labels were queried from the SACL. Serialize that returned + // SACL too; this does not request audit access to either file. + information | SACL_SECURITY_INFORMATION, + &mut text, + null_mut(), + ) + } == 0 + { + return Err(std::io::Error::last_os_error()); + } + let result = unsafe { widestring::U16CStr::from_ptr_str(text) } + .as_slice() + .to_vec(); + unsafe { LocalFree(text.cast()) }; + Ok(result) +} + pub(crate) fn set_default_plugin_pane_pwd( _env: &mut Vec<(String, String)>, _cwd: &std::path::Path, diff --git a/src/platform/windows/config_backup.rs b/src/platform/windows/config_backup.rs new file mode 100644 index 00000000..8e7390ab --- /dev/null +++ b/src/platform/windows/config_backup.rs @@ -0,0 +1,165 @@ +//! Recovery-backed writes for existing Windows integration configs. + +use std::fs::{self, File, OpenOptions}; +use std::io::{self, Read, Seek, Write}; +use std::os::windows::{fs::MetadataExt, io::AsRawHandle}; +use std::path::{Path, PathBuf}; +use windows_sys::Win32::Storage::FileSystem::{ + GetFileInformationByHandle, MoveFileExW, BY_HANDLE_FILE_INFORMATION, FILE_ATTRIBUTE_ENCRYPTED, +}; + +#[cfg(test)] +mod tests; + +fn backup_paths(target: &Path) -> (PathBuf, PathBuf) { + let mut name = target.as_os_str().to_os_string(); + name.push(".herdr-backup"); + let complete = PathBuf::from(&name); + name.push(".pending"); + (complete, PathBuf::from(name)) +} + +pub(super) fn check_recovery(target: &Path) -> io::Result<()> { + let (complete, pending) = backup_paths(target); + for (path, description) in [ + (&complete, "recovery copy"), + (&pending, "unfinished backup"), + ] { + match fs::symlink_metadata(path) { + Ok(_) => { + return Err(io::Error::other(format!( + "cannot update {}: {description} exists at {}; inspect the config and resolve this backup before retrying", + target.display(), path.display() + ))); + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => return Err(error), + } + } + Ok(()) +} + +struct PendingBackup(PathBuf); +impl Drop for PendingBackup { + fn drop(&mut self) { + // Only the incomplete name is owned by this cleanup. The completed + // recovery copy must survive errors and must never be removed here. + if let Err(error) = fs::remove_file(&self.0) { + if error.kind() != io::ErrorKind::NotFound { + tracing::warn!(path = %self.0.display(), %error, "failed to remove incomplete config backup"); + } + } + } +} + +// Internal observation points let native tests terminate a child at actual I/O +// boundaries. The production callback is a no-op; there is no runtime failpoint. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum Phase { + BackupChunk, + BackupReady, + Truncated, + Committed, +} + +pub(super) fn write_existing(target: &Path, contents: &[u8]) -> io::Result<bool> { + write_observed(target, contents, |_| Ok(())) +} + +fn check_source(file: &File, target: &Path) -> io::Result<()> { + if file.metadata()?.file_attributes() & FILE_ATTRIBUTE_ENCRYPTED != 0 { + return Err(io::Error::other(format!( + "cannot update {}: encrypted configs are not supported by recovery-backed writes; the original was not changed", + target.display() + ))); + } + let mut info = BY_HANDLE_FILE_INFORMATION::default(); + if unsafe { GetFileInformationByHandle(file.as_raw_handle(), &mut info) } == 0 { + return Err(io::Error::last_os_error()); + } + if info.nNumberOfLinks > 1 { + return Err(io::Error::other(format!( + "cannot update {}: config has multiple hard links; use a separate file or a symlink before retrying", + target.display() + ))); + } + Ok(()) +} + +fn write_observed( + target: &Path, + contents: &[u8], + mut observe: impl FnMut(Phase) -> io::Result<()>, +) -> io::Result<bool> { + check_recovery(target)?; + // Keep this handle for both backup reads and the destructive write. Do not + // reopen the pathname after saving the original contents. + let mut source = match OpenOptions::new().read(true).write(true).open(target) { + Ok(source) => source, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(false), + Err(error) => return Err(error), + }; + check_source(&source, target)?; + let (complete, pending) = backup_paths(target); + let mut backup = super::create_config_temporary(&pending, true)?; + let pending = PendingBackup(pending); + let preparation = (|| { + let mut remaining = source.metadata()?.len(); + let mut buffer = [0_u8; 8192]; + while remaining != 0 { + let requested = remaining.min(buffer.len() as u64) as usize; + let count = source.read(&mut buffer[..requested])?; + if count == 0 { + return Err(io::Error::new( + io::ErrorKind::UnexpectedEof, + "config shortened during backup", + )); + } + backup.write_all(&buffer[..count])?; + remaining -= count as u64; + observe(Phase::BackupChunk)?; + } + backup.sync_all()?; + drop(backup); + let from = super::extended_length_path(&pending.0)?; + let to = super::extended_length_path(&complete)?; + // No REPLACE_EXISTING: never overwrite a previous recovery copy. + if unsafe { MoveFileExW(from.as_ptr(), to.as_ptr(), 0) } == 0 { + return Err(io::Error::last_os_error()); + } + Ok::<_, io::Error>(()) + })(); + preparation.map_err(|error| { + io::Error::other(format!( + "cannot back up {}; original not changed (unfinished backup: {}): {error}", + target.display(), + pending.0.display() + )) + })?; + + let update = (|| { + observe(Phase::BackupReady)?; + check_source(&source, target)?; + source.rewind()?; + source.set_len(0)?; + observe(Phase::Truncated)?; + source.write_all(contents)?; + source.sync_all()?; + Ok::<_, io::Error>(()) + })(); + update.map_err(|error| { + io::Error::other(format!( + "could not update {}; config may be incomplete; original contents retained at {}: {error}", + target.display(), complete.display() + )) + })?; + // Success here is after the new contents were synced. A cleanup failure is + // not an unchanged-original failure, and must not trigger automatic rollback. + observe(Phase::Committed).and_then(|()| fs::remove_file(&complete)).map_err(|error| { + io::Error::other(format!( + "updated {}, but recovery copy remains at {}; verify the config and remove that copy before retrying: {error}", + target.display(), complete.display() + )) + })?; + Ok(true) +} diff --git a/src/platform/windows/config_backup/tests.rs b/src/platform/windows/config_backup/tests.rs new file mode 100644 index 00000000..966ba2a9 --- /dev/null +++ b/src/platform/windows/config_backup/tests.rs @@ -0,0 +1,375 @@ +use super::super::{config_security_descriptor, config_security_sddl}; +use super::*; +use std::process::Command; +use windows_sys::Win32::{ + Security::{ + DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, + OWNER_SECURITY_INFORMATION, + }, + Storage::FileSystem::{ + EncryptFileW, LockFileEx, LOCKFILE_EXCLUSIVE_LOCK, LOCKFILE_FAIL_IMMEDIATELY, + }, + System::IO::OVERLAPPED, +}; + +struct Directory(PathBuf); +impl Directory { + fn new(case: &str) -> Self { + let path = + std::env::temp_dir().join(format!("herdr-config-backup-{case}-{}", std::process::id())); + fs::create_dir(&path).unwrap(); + Self(path) + } +} +impl Drop for Directory { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} +fn powershell(script: &str, path: &Path) { + let output = Command::new("powershell.exe") + .args(["-NoProfile", "-NonInteractive", "-Command", script]) + .env("HERDR_TEST_CONFIG_SOURCE", path) + .output() + .unwrap(); + assert!( + output.status.success() && output.stderr.is_empty(), + "{output:?}" + ); +} +fn security(path: &Path) -> Vec<u16> { + let flags = OWNER_SECURITY_INFORMATION + | GROUP_SECURITY_INFORMATION + | DACL_SECURITY_INFORMATION + | LABEL_SECURITY_INFORMATION; + let mut descriptor = config_security_descriptor(path, flags).unwrap(); + config_security_sddl(&mut descriptor, flags).unwrap() +} +fn identity(path: &Path) -> (u32, u32, u32) { + let file = File::open(path).unwrap(); + let mut info = BY_HANDLE_FILE_INFORMATION::default(); + assert_ne!( + unsafe { GetFileInformationByHandle(file.as_raw_handle(), &mut info) }, + 0 + ); + ( + info.dwVolumeSerialNumber, + info.nFileIndexHigh, + info.nFileIndexLow, + ) +} +fn lock_range(path: &Path, offset: u32) -> File { + let file = OpenOptions::new() + .read(true) + .write(true) + .open(path) + .unwrap(); + let mut overlapped = OVERLAPPED::default(); + overlapped.Anonymous.Anonymous.Offset = offset; + assert_ne!( + unsafe { + LockFileEx( + file.as_raw_handle(), + LOCKFILE_EXCLUSIVE_LOCK | LOCKFILE_FAIL_IMMEDIATELY, + 0, + 1, + 0, + &mut overlapped, + ) + }, + 0, + "{}", + io::Error::last_os_error() + ); + file +} +fn successful_update(case: &str) { + let dir = Directory::new(case); + let source = dir.0.join("config"); + fs::write(&source, b"original preferences").unwrap(); + fs::write(dir.0.join("config:private"), b"original stream").unwrap(); + if case == "protected" || case == "unprotected" || case == "moved" { + powershell( + &format!( + r#" +$ErrorActionPreference = 'Stop' +$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) +$acl.SetAccessRuleProtection(${}, $false) +$sid = [System.Security.Principal.WindowsIdentity]::GetCurrent().User +$rule = [System.Security.AccessControl.FileSystemAccessRule]::new($sid, [System.Security.AccessControl.FileSystemRights]::FullControl, [System.Security.AccessControl.AccessControlType]::Allow) +$acl.AddAccessRule($rule) +[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) +"#, + case == "protected" + ), + &source, + ); + } + if case == "metadata" { + powershell( + r#" +$ErrorActionPreference = 'Stop' +$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) +$acl.SetOwner([System.Security.Principal.WindowsIdentity]::GetCurrent().User) +$acl.SetGroup([System.Security.Principal.SecurityIdentifier]::new('S-1-5-32-545')) +[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) +$null = & icacls.exe $env:HERDR_TEST_CONFIG_SOURCE /setintegritylevel L +if ($LASTEXITCODE -ne 0) { throw 'could not install low integrity label' } +"#, + &source, + ); + } + let source = if case == "moved" { + let original = security(&source); + let parent = dir.0.join("broader-parent"); + fs::create_dir(&parent).unwrap(); + powershell( + r#" +$ErrorActionPreference = 'Stop' +$acl = [System.IO.Directory]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) +$sid = [System.Security.Principal.SecurityIdentifier]::new('S-1-5-32-546') +$rule = [System.Security.AccessControl.FileSystemAccessRule]::new($sid, [System.Security.AccessControl.FileSystemRights]::Read, [System.Security.AccessControl.InheritanceFlags]::ObjectInherit, [System.Security.AccessControl.PropagationFlags]::None, [System.Security.AccessControl.AccessControlType]::Allow) +$acl.AddAccessRule($rule) +[System.IO.Directory]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) +"#, + &parent, + ); + let moved = parent.join("config"); + fs::rename(&source, &moved).unwrap(); + assert_eq!(security(&moved), original); + assert!(!String::from_utf16_lossy(&original).contains(";;;BG)")); + let inherited = parent.join("inherited"); + fs::write(&inherited, b"").unwrap(); + assert!(String::from_utf16_lossy(&security(&inherited)).contains(";;;BG)")); + moved + } else { + source + }; + let before = security(&source); + let before_id = identity(&source); + let text = String::from_utf16_lossy(&before); + match case { + "legacy" => assert!(text.contains("D:("), "{text}"), + "protected" => assert!(text.contains("D:PAI"), "{text}"), + "unprotected" | "moved" => assert!(text.contains("D:AI"), "{text}"), + "metadata" => assert!(text.contains("G:BU") && text.contains(";;;LW)"), "{text}"), + _ => unreachable!(), + } + let (complete, pending) = backup_paths(&source); + let mut phases = Vec::new(); + assert!(write_observed(&source, b"new", |phase| { + phases.push(phase); + if phase == Phase::BackupReady { + assert_eq!(fs::read(&complete).unwrap(), b"original preferences"); + assert_eq!(fs::read(&source).unwrap(), b"original preferences"); + assert!(!pending.exists()); + let private = String::from_utf16_lossy(&security(&complete)); + assert!( + private.contains("D:P") && !private.contains(";;;BG)"), + "{private}" + ); + } + if phase == Phase::Truncated { + assert!(fs::read(&source).unwrap().is_empty()); + } + Ok(()) + }) + .unwrap()); + assert_eq!( + phases, + [ + Phase::BackupChunk, + Phase::BackupReady, + Phase::Truncated, + Phase::Committed + ] + ); + assert_eq!(fs::read(&source).unwrap(), b"new"); + assert_eq!( + fs::read(source.with_file_name("config:private")).unwrap(), + b"original stream" + ); + assert_eq!(security(&source), before); + assert_eq!(identity(&source), before_id); + assert!(!complete.exists() && !pending.exists()); +} +#[test] +fn backup_preserves_legacy() { + successful_update("legacy"); +} +#[test] +fn backup_preserves_protected() { + successful_update("protected"); +} +#[test] +fn backup_preserves_unprotected() { + successful_update("unprotected"); +} +#[test] +fn backup_preserves_moved() { + successful_update("moved"); +} +#[test] +fn backup_preserves_metadata() { + successful_update("metadata"); +} + +#[test] +fn native_io_failures_preserve_recovery_ordering() { + let dir = Directory::new("io-failures"); + for during_backup in [true, false] { + let path = dir.0.join(if during_backup { "backup" } else { "write" }); + let original = vec![b'a'; if during_backup { 32768 } else { 16 }]; + fs::write(&path, &original).unwrap(); + let before = security(&path); + let before_id = identity(&path); + // First case fails a later backup read after one real chunk. In the + // second, lock after truncation so the actual new WriteFile must fail. + let mut locked = during_backup.then(|| lock_range(&path, 8192)); + let mut phases = Vec::new(); + let error = write_observed(&path, &vec![b'x'; 16384], |phase| { + phases.push(phase); + if !during_backup && phase == Phase::Truncated { + locked = Some(lock_range(&path, 4096)); + } + Ok(()) + }) + .unwrap_err(); + eprintln!("during_backup={during_backup}: {error}; phases={phases:?}"); + assert!(error.to_string().contains("os error 33")); + drop(locked); + let (complete, pending) = backup_paths(&path); + assert!(!pending.exists()); + assert_eq!(security(&path), before); + assert_eq!(identity(&path), before_id); + if during_backup { + assert_eq!(phases, [Phase::BackupChunk]); + assert_eq!(fs::read(&path).unwrap(), original); + assert!(!complete.exists()); + } else { + assert!(phases.contains(&Phase::Truncated)); + assert_eq!(fs::read(&complete).unwrap(), original); + assert!(error.to_string().contains(&complete.display().to_string())); + assert!(write_existing(&path, b"retry") + .unwrap_err() + .to_string() + .contains("recovery copy")); + // Recovery is contents-only, never rename over the original file. + fs::write(&path, fs::read(&complete).unwrap()).unwrap(); + assert_eq!(identity(&path), before_id); + assert_eq!(security(&path), before); + fs::remove_file(&complete).unwrap(); + assert!(write_existing(&path, b"resolved").unwrap()); + } + } +} + +#[test] +fn interrupted_backups_and_writes_are_distinguishable() { + const CHILD: &str = "HERDR_BACKUP_CRASH_TEST"; + if let Some(path) = std::env::var_os(CHILD) { + let path = PathBuf::from(path); + let crash_phase = if path.file_name().unwrap() == "copy" { + Phase::BackupChunk + } else { + Phase::Truncated + }; + let _ = write_observed(&path, b"new", |phase| { + if phase == crash_phase { + std::process::exit(23); + } + Ok(()) + }); + panic!("crash phase not reached"); + } + let dir = Directory::new("crash"); + for name in ["copy", "write"] { + let path = dir.0.join(name); + let original = vec![b'a'; 32768]; + fs::write(&path, &original).unwrap(); + let output = Command::new(std::env::current_exe().unwrap()).args(["--exact", "platform::windows::config_backup::tests::interrupted_backups_and_writes_are_distinguishable", "--nocapture"]) + .env(CHILD, &path).output().unwrap(); + assert_eq!(output.status.code(), Some(23), "{output:?}"); + let (complete, pending) = backup_paths(&path); + if name == "copy" { + assert_eq!(fs::read(&pending).unwrap().len(), 8192); + assert!(!complete.exists()); + assert_eq!(fs::read(&path).unwrap(), original); + assert!(check_recovery(&path) + .unwrap_err() + .to_string() + .contains("unfinished backup")); + } else { + assert!(!pending.exists()); + assert_eq!(fs::read(&complete).unwrap(), original); + assert!(fs::read(&path).unwrap().is_empty()); + fs::remove_file(&path).unwrap(); + assert!(write_existing(&path, b"must not recreate").is_err()); + assert!(!path.exists()); + assert_eq!(fs::read(&complete).unwrap(), original); + } + } +} + +#[test] +fn completed_write_cleanup_failure_and_foreign_backups_are_reported() { + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::Storage::FileSystem::{FILE_SHARE_READ, FILE_SHARE_WRITE}; + let dir = Directory::new("cleanup"); + let path = dir.0.join("config"); + fs::write(&path, b"original").unwrap(); + let (complete, _) = backup_paths(&path); + fs::write(&complete, b"foreign contents").unwrap(); + assert!(write_existing(&path, b"rejected").is_err()); + assert_eq!(fs::read(&complete).unwrap(), b"foreign contents"); + assert_eq!(fs::read(&path).unwrap(), b"original"); + fs::remove_file(&complete).unwrap(); + let mut held = None; + let error = write_observed(&path, b"saved", |phase| { + if phase == Phase::Committed { + held = Some( + OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .open(&complete)?, + ); + } + Ok(()) + }) + .unwrap_err(); + assert!( + error.to_string().contains("updated ") + && error.to_string().contains("recovery copy remains") + ); + assert_eq!(fs::read(&path).unwrap(), b"saved"); + assert_eq!(fs::read(&complete).unwrap(), b"original"); + assert!(write_existing(&path, b"retry").is_err()); + drop(held); +} + +#[test] +fn encrypted_config_is_rejected_without_plaintext_backup() { + let dir = Directory::new("encrypted"); + let path = dir.0.join("config"); + fs::write(&path, b"encrypted original").unwrap(); + let wide = super::super::extended_length_path(&path).unwrap(); + assert_ne!( + unsafe { EncryptFileW(wide.as_ptr()) }, + 0, + "cannot establish EFS fixture: {}", + io::Error::last_os_error() + ); + assert_ne!( + fs::metadata(&path).unwrap().file_attributes() & FILE_ATTRIBUTE_ENCRYPTED, + 0 + ); + let before = security(&path); + let before_id = identity(&path); + let error = write_existing(&path, b"new").unwrap_err(); + assert!(error.to_string().contains("encrypted configs")); + assert_eq!(fs::read(&path).unwrap(), b"encrypted original"); + assert_eq!(security(&path), before); + assert_eq!(identity(&path), before_id); + let (complete, pending) = backup_paths(&path); + assert!(!complete.exists() && !pending.exists()); +} |
