From 392b7e8e705d66f947b37a8cca42bab7a4712f40 Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 21:52:27 +0000 Subject: [PATCH 01/13] fix: atomically write integration configs refs #3970 --- .../website/src/content/docs/integrations.mdx | 2 + .../src/content/docs/ja/integrations.mdx | 2 + .../src/content/docs/zh-cn/integrations.mdx | 2 + src/integration/config_file.rs | 137 +++++++++++ src/integration/config_file/tests.rs | 220 ++++++++++++++++++ src/integration/mod.rs | 1 + src/integration/opencode_config.rs | 48 +++- src/integration/targets.rs | 98 +++++--- src/integration/tests.rs | 32 +++ src/platform/linux.rs | 119 ++++++++++ src/platform/linux/config_file_tests.rs | 96 ++++++++ src/platform/macos.rs | 131 +++++++++++ src/platform/macos/config_file_tests.rs | 44 ++++ src/platform/windows.rs | 148 ++++++++++++ src/platform/windows/config_file_tests.rs | 41 ++++ 15 files changed, 1084 insertions(+), 37 deletions(-) create mode 100644 src/integration/config_file.rs create mode 100644 src/integration/config_file/tests.rs create mode 100644 src/platform/linux/config_file_tests.rs create mode 100644 src/platform/macos/config_file_tests.rs create mode 100644 src/platform/windows/config_file_tests.rs diff --git a/docs/next/website/src/content/docs/integrations.mdx b/docs/next/website/src/content/docs/integrations.mdx index 4cbd0a631b..d7154f015b 100644 --- a/docs/next/website/src/content/docs/integrations.mdx +++ b/docs/next/website/src/content/docs/integrations.mdx @@ -51,6 +51,8 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` +Shared agent configuration is written to a temporary file and replaced only after the complete write succeeds. Herdr preserves file permissions and follows symlinks. Config files with multiple hard links are rejected before installation or removal changes hook files; use a separate file or a symlink before retrying. This protects each config file, not an entire multi-file installation from partial completion. + ## 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 124010e177..7649633b58 100644 --- a/docs/next/website/src/content/docs/ja/integrations.mdx +++ b/docs/next/website/src/content/docs/ja/integrations.mdx @@ -53,6 +53,8 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` +共有されるエージェント設定は一時ファイルに書き込まれ、書き込みがすべて成功した後に置き換えられます。Herdr はファイルのアクセス権を保持し、シンボリックリンクをたどります。複数のハードリンクを持つ設定ファイルは、インストールやアンインストールでフックファイルを変更する前に拒否されます。独立したファイルまたはシンボリックリンクに変更してから再試行してください。この保護は設定ファイル単位であり、複数ファイルにまたがる操作全体のロールバックを保証するものではありません。 + ## 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 e5535808a1..95b65c0f8a 100644 --- a/docs/next/website/src/content/docs/zh-cn/integrations.mdx +++ b/docs/next/website/src/content/docs/zh-cn/integrations.mdx @@ -53,6 +53,8 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` +共享的智能体配置会先写入临时文件，完整写入成功后才替换原文件。Herdr 会保留文件权限并跟随符号链接。如果配置文件有多个硬链接，安装或卸载会在修改钩子文件之前拒绝操作；请改用独立文件或符号链接后重试。这项保护针对单个配置文件，不保证跨多个文件的整个安装操作能够回滚。 + ## Herdr 如何使用集成 Herdr 以两种不同方式使用集成: diff --git a/src/integration/config_file.rs b/src/integration/config_file.rs new file mode 100644 index 0000000000..22924fdfe4 --- /dev/null +++ b/src/integration/config_file.rs @@ -0,0 +1,137 @@ +//! Atomic 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 reject_hard_linked_configs(dir: &Path, names: &[&str]) -> io::Result<()> { + for name in names { + reject_hard_links(&dir.join(name))?; + } + Ok(()) +} + +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 { + 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<()> { + let replacement = Replacement::prepare(path, contents.as_ref())?; + replacement.commit() +} + +struct Replacement { + target: PathBuf, + temporary: PathBuf, +} + +impl Replacement { + fn prepare(path: &Path, contents: &[u8]) -> io::Result { + 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 0000000000..a716ec91ce --- /dev/null +++ b/src/integration/config_file/tests.rs @@ -0,0 +1,220 @@ +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); + } +} + +#[test] +fn config_publication_keeps_old_content_until_commit() { + let dir = Directory::new(); + for existing in [false, true] { + 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(), 2); +} + +#[test] +fn abandoned_and_failed_publication_leave_config_unchanged() { + let dir = Directory::new(); + let path = dir.0.join("config"); + 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); + assert_eq!(fs::read(&path).unwrap(), b"original"); + assert_eq!(fs::read_dir(&dir.0).unwrap().count(), 1); + 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(); + let staged = Replacement::prepare(&path, b"new").unwrap(); + fs::hard_link(&path, &alias).unwrap(); + 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); + symlink(Path::new("other/link"), &entry); + 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(), Path::new("other/link")); + assert_eq!(fs::read_link(&intermediate).unwrap(), target); + 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(), Path::new("other/link")); + + 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 d956c7bcd7..1e053b8155 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 2ab91e844c..b4a969889d 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::write_config; + const TUI_CONFIG_NAME: &str = "tui.jsonc"; pub(crate) fn tui_config_path(config_dir: &Path) -> PathBuf { @@ -90,7 +92,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) } @@ -133,7 +135,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 +247,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 d204c55293..7c500733f3 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::{reject_hard_linked_configs, 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, mastracode_dir, omp_extension_dir, @@ -121,6 +122,7 @@ pub(crate) fn remove_legacy_pi_extension_from_omp_dir(dir: &Path) -> io::Result< pub(crate) fn install_claude() -> io::Result { let dir = claude_dir()?; + reject_hard_linked_configs(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "claude directory not found at {}. install claude code first", @@ -145,7 +147,7 @@ pub(crate) fn install_claude() -> io::Result { 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 { @@ -156,6 +158,7 @@ pub(crate) fn install_claude() -> io::Result { pub(crate) fn install_codex() -> io::Result { let dir = codex_dir()?; + reject_hard_linked_configs(&dir, &["hooks.json", "config.toml"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "codex config directory not found at {}. install codex first", @@ -197,7 +200,7 @@ pub(crate) fn install_codex() -> io::Result { )?; 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() { @@ -207,7 +210,7 @@ pub(crate) fn install_codex() -> io::Result { }; 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 { @@ -219,6 +222,7 @@ pub(crate) fn install_codex() -> io::Result { pub(crate) fn install_kimi() -> io::Result { let dir = kimi_dir()?; + reject_hard_linked_configs(&dir, &["config.toml"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "kimi code config directory not found at {}. install kimi code first", @@ -241,7 +245,7 @@ pub(crate) fn install_kimi() -> io::Result { }; 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)?; @@ -253,6 +257,7 @@ pub(crate) fn install_kimi() -> io::Result { pub(crate) fn install_copilot() -> io::Result { let dir = copilot_dir()?; + reject_hard_linked_configs(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "copilot config directory not found at {}. install github copilot cli first", @@ -297,7 +302,7 @@ pub(crate) fn install_copilot() -> io::Result { } 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, @@ -307,6 +312,7 @@ pub(crate) fn install_copilot() -> io::Result { pub(crate) fn install_devin() -> io::Result { let dir = devin_dir()?; + reject_hard_linked_configs(&dir, &["config.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "devin config directory not found at {}. install devin cli first", @@ -353,7 +359,7 @@ pub(crate) fn install_devin() -> io::Result { } 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, @@ -363,6 +369,7 @@ pub(crate) fn install_devin() -> io::Result { pub(crate) fn install_droid() -> io::Result { let dir = droid_dir()?; + reject_hard_linked_configs(&dir, &["settings.json", "hooks.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "droid config directory not found at {}. install droid first", @@ -413,7 +420,7 @@ pub(crate) fn install_droid() -> io::Result { } 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; @@ -439,7 +446,7 @@ pub(crate) fn install_droid() -> io::Result { } } 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)?)?; } } @@ -453,6 +460,7 @@ pub(crate) fn install_droid() -> io::Result { pub(crate) fn install_opencode() -> io::Result { let dir = opencode_dir()?; + reject_hard_linked_configs(&dir, &["tui.jsonc", "cli.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "opencode config directory not found at {}. install opencode first", @@ -506,6 +514,7 @@ pub(crate) fn install_kilo() -> io::Result { pub(crate) fn install_hermes() -> io::Result { let dir = hermes_dir()?; + reject_hard_linked_configs(&dir, &["config.yaml"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "hermes config directory not found at {}. install hermes agent first", @@ -532,7 +541,7 @@ pub(crate) fn install_hermes() -> io::Result { }; 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 { @@ -562,8 +571,10 @@ pub(crate) fn uninstall_omp() -> io::Result { } pub(crate) fn uninstall_claude() -> io::Result { - let hook_path = claude_dir()?.join("hooks").join(CLAUDE_HOOK_INSTALL_NAME); - let settings_path = claude_dir()?.join("settings.json"); + let dir = claude_dir()?; + reject_hard_linked_configs(&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() { @@ -572,7 +583,7 @@ pub(crate) fn uninstall_claude() -> io::Result { 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)?; } } @@ -589,6 +600,7 @@ pub(crate) fn uninstall_claude() -> io::Result { pub(crate) fn uninstall_codex() -> io::Result { let codex_dir = codex_dir()?; + reject_hard_linked_configs(&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"); @@ -619,7 +631,7 @@ pub(crate) fn uninstall_codex() -> io::Result { } 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)?)?; } } @@ -637,6 +649,7 @@ pub(crate) fn uninstall_codex() -> io::Result { pub(crate) fn uninstall_kimi() -> io::Result { let kimi_dir = kimi_dir()?; + reject_hard_linked_configs(&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; @@ -645,7 +658,7 @@ pub(crate) fn uninstall_kimi() -> io::Result { 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; } } @@ -663,6 +676,7 @@ pub(crate) fn uninstall_kimi() -> io::Result { pub(crate) fn uninstall_copilot() -> io::Result { let copilot_dir = copilot_dir()?; + reject_hard_linked_configs(&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; @@ -691,7 +705,7 @@ pub(crate) fn uninstall_copilot() -> io::Result { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -708,6 +722,7 @@ pub(crate) fn uninstall_copilot() -> io::Result { pub(crate) fn uninstall_devin() -> io::Result { let devin_dir = devin_dir()?; + reject_hard_linked_configs(&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; @@ -736,7 +751,7 @@ pub(crate) fn uninstall_devin() -> io::Result { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -753,6 +768,7 @@ pub(crate) fn uninstall_devin() -> io::Result { pub(crate) fn uninstall_droid() -> io::Result { let droid_dir = droid_dir()?; + reject_hard_linked_configs(&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"); @@ -780,7 +796,7 @@ pub(crate) fn uninstall_droid() -> io::Result { } 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)?)?; } } @@ -808,7 +824,7 @@ pub(crate) fn uninstall_droid() -> io::Result { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -827,6 +843,7 @@ pub(crate) fn uninstall_droid() -> io::Result { pub(crate) fn uninstall_opencode() -> io::Result { let dir = opencode_dir()?; + reject_hard_linked_configs(&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); @@ -882,6 +899,7 @@ pub(crate) fn uninstall_kilo() -> io::Result { pub(crate) fn uninstall_hermes() -> io::Result { let dir = hermes_dir()?; + reject_hard_linked_configs(&dir, &["config.yaml"])?; let plugin_dir = hermes_plugin_dir()?; let config_path = dir.join("config.yaml"); @@ -891,7 +909,7 @@ pub(crate) fn uninstall_hermes() -> io::Result { 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; } } @@ -906,6 +924,7 @@ pub(crate) fn uninstall_hermes() -> io::Result { pub(crate) fn install_qodercli() -> io::Result { let dir = qodercli_dir()?; + reject_hard_linked_configs(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "qodercli config directory not found at {}. install qodercli first", @@ -960,7 +979,7 @@ pub(crate) fn install_qodercli() -> io::Result { } 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, @@ -970,6 +989,7 @@ pub(crate) fn install_qodercli() -> io::Result { pub(crate) fn install_qwen() -> io::Result { let dir = qwen_dir()?; + reject_hard_linked_configs(&dir, &["settings.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "qwen code config directory not found at {}. install qwen code first", @@ -1013,7 +1033,7 @@ pub(crate) fn install_qwen() -> io::Result { )?; } - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; Ok(QwenInstallPaths { hook_path, @@ -1023,6 +1043,7 @@ pub(crate) fn install_qwen() -> io::Result { pub(crate) fn install_cursor() -> io::Result { let dir = cursor_dir()?; + reject_hard_linked_configs(&dir, &["hooks.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "cursor config directory not found at {}. install cursor agent cli first", @@ -1069,7 +1090,7 @@ pub(crate) fn install_cursor() -> io::Result { 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, @@ -1078,10 +1099,10 @@ pub(crate) fn install_cursor() -> io::Result { } pub(crate) fn uninstall_qodercli() -> io::Result { - let hook_path = qodercli_dir()? - .join("hooks") - .join(QODERCLI_HOOK_INSTALL_NAME); - let settings_path = qodercli_dir()?.join("settings.json"); + let dir = qodercli_dir()?; + reject_hard_linked_configs(&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() { @@ -1108,7 +1129,7 @@ pub(crate) fn uninstall_qodercli() -> io::Result { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -1124,8 +1145,10 @@ pub(crate) fn uninstall_qodercli() -> io::Result { } pub(crate) fn uninstall_qwen() -> io::Result { - let hook_path = qwen_dir()?.join("hooks").join(QWEN_HOOK_INSTALL_NAME); - let settings_path = qwen_dir()?.join("settings.json"); + let dir = qwen_dir()?; + reject_hard_linked_configs(&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() { @@ -1149,7 +1172,7 @@ pub(crate) fn uninstall_qwen() -> io::Result { } if updated_settings { - fs::write(&settings_path, serde_json::to_string_pretty(&settings)?)?; + write_config(&settings_path, serde_json::to_string_pretty(&settings)?)?; } } @@ -1165,6 +1188,7 @@ pub(crate) fn uninstall_qwen() -> io::Result { pub(crate) fn uninstall_cursor() -> io::Result { let cursor_home = cursor_dir()?; + reject_hard_linked_configs(&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; @@ -1194,7 +1218,7 @@ pub(crate) fn uninstall_cursor() -> io::Result { } 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)?)?; } } @@ -1221,6 +1245,7 @@ pub(crate) fn mastracode_hook_command(hook_path: &Path, action: &str) -> String pub(crate) fn install_mastracode() -> io::Result { let mastracode_home = mastracode_dir()?; + reject_hard_linked_configs(&mastracode_home, &["hooks.json"])?; let hook_dir = mastracode_home.join("hooks"); fs::create_dir_all(&hook_dir)?; @@ -1258,7 +1283,7 @@ pub(crate) fn install_mastracode() -> io::Result { )?; } - 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, @@ -1268,6 +1293,7 @@ pub(crate) fn install_mastracode() -> io::Result { pub(crate) fn uninstall_mastracode() -> io::Result { let mastracode_home = mastracode_dir()?; + reject_hard_linked_configs(&mastracode_home, &["hooks.json"])?; let hook_path = mastracode_home .join("hooks") .join(MASTRACODE_HOOK_INSTALL_NAME); @@ -1300,7 +1326,7 @@ pub(crate) fn uninstall_mastracode() -> io::Result { } 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)?)?; } } @@ -1316,6 +1342,7 @@ pub(crate) fn uninstall_mastracode() -> io::Result { pub(crate) fn install_antigravity_cli() -> io::Result { let dir = antigravity_cli_dir()?; + reject_hard_linked_configs(&dir, &["hooks.json"])?; if !dir.is_dir() { return Err(io::Error::other(format!( "antigravity cli config directory not found at {}. install antigravity cli first", @@ -1353,7 +1380,7 @@ pub(crate) fn install_antigravity_cli() -> io::Result Value { pub(crate) fn uninstall_antigravity_cli() -> io::Result { let dir = antigravity_cli_dir()?; + reject_hard_linked_configs(&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; @@ -1411,7 +1439,7 @@ pub(crate) fn uninstall_antigravity_cli() -> io::Result std::io::Result { + use std::os::unix::fs::MetadataExt; + Ok(std::fs::metadata(path)?.nlink()) +} + +pub(crate) fn create_config_temporary( + path: &std::path::Path, + private: bool, +) -> std::io::Result { + 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> { + 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> { + 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 0000000000..b11c117daa --- /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> { + 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 8840c7ed9b..ede316a8c1 100644 --- a/src/platform/macos.rs +++ b/src/platform/macos.rs @@ -20,6 +20,137 @@ pub(crate) use super::unix_common::{ wait_client_stream_readable, StatusCommandGuard, }; +#[cfg(test)] +mod config_file_tests; + +pub(crate) fn config_file_link_count(path: &Path) -> std::io::Result { + use std::os::unix::fs::MetadataExt; + Ok(std::fs::metadata(path)?.nlink()) +} + +pub(crate) fn create_config_temporary( + path: &Path, + private: bool, +) -> std::io::Result { + 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 (, ) 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()) { + if 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 0000000000..f08d6c2aa4 --- /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::>() + .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 8344be2dc2..91640c089c 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -14,6 +14,8 @@ use std::{ }; mod clipboard_image; +#[cfg(test)] +mod config_file_tests; 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. @@ -137,6 +139,152 @@ pub(crate) fn replace_file( } } +pub(crate) fn config_file_link_count(path: &std::path::Path) -> std::io::Result { + 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 { + 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::() 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; + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::{ + Security::{ + GetFileSecurityW, GetSecurityDescriptorControl, SetKernelObjectSecurity, + DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, + OWNER_SECURITY_INFORMATION, PROTECTED_DACL_SECURITY_INFORMATION, SE_DACL_PROTECTED, + UNPROTECTED_DACL_SECURITY_INFORMATION, + }, + Storage::FileSystem::{FILE_GENERIC_WRITE, WRITE_DAC, WRITE_OWNER}, + }; + if let Some(source) = source { + // CopyFile preserves file attributes, encryption and alternate streams. + // It does not preserve the DACL; that is copied explicitly below. + std::fs::copy(source, temporary)?; + } + let mut options = std::fs::OpenOptions::new(); + options.write(true).truncate(true); + if source.is_some() { + options.access_mode(FILE_GENERIC_WRITE | WRITE_DAC | WRITE_OWNER); + } + let mut output = options.open(temporary)?; + output.write_all(contents)?; + if let Some(source) = source { + let source = extended_length_path(source)?; + let information = OWNER_SECURITY_INFORMATION + | GROUP_SECURITY_INFORMATION + | DACL_SECURITY_INFORMATION + | LABEL_SECURITY_INFORMATION; + let mut needed = 0; + unsafe { + GetFileSecurityW(source.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( + source.as_ptr(), + information, + descriptor.as_mut_ptr().cast(), + needed, + &mut needed, + ) + } == 0 + { + return Err(std::io::Error::last_os_error()); + } + let mut control = 0; + let mut revision = 0; + if unsafe { + GetSecurityDescriptorControl( + descriptor.as_mut_ptr().cast(), + &mut control, + &mut revision, + ) + } == 0 + { + return Err(std::io::Error::last_os_error()); + } + let protection = if control & SE_DACL_PROTECTED != 0 { + PROTECTED_DACL_SECURITY_INFORMATION + } else { + UNPROTECTED_DACL_SECURITY_INFORMATION + }; + if unsafe { + SetKernelObjectSecurity( + output.as_raw_handle(), + information | protection, + descriptor.as_mut_ptr().cast(), + ) + } == 0 + { + return Err(std::io::Error::last_os_error()); + } + } + output.sync_all() +} + pub(crate) fn set_default_plugin_pane_pwd( _env: &mut Vec<(String, String)>, _cwd: &std::path::Path, diff --git a/src/platform/windows/config_file_tests.rs b/src/platform/windows/config_file_tests.rs new file mode 100644 index 0000000000..d27b3bd002 --- /dev/null +++ b/src/platform/windows/config_file_tests.rs @@ -0,0 +1,41 @@ +use super::*; + +fn powershell(script: &str, source: &std::path::Path) -> String { + let output = std::process::Command::new("powershell.exe") + .args(["-NoProfile", "-NonInteractive", "-Command", script]) + .env("HERDR_TEST_CONFIG_SOURCE", source) + .output() + .unwrap(); + assert!(output.status.success(), "{output:?}"); + assert!(output.stderr.is_empty(), "{output:?}"); + String::from_utf8(output.stdout).unwrap() +} + +#[test] +fn config_replacement_preserves_windows_access_control() { + 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 target = dir.join("temporary"); + std::fs::write(&source, b"original").unwrap(); + powershell( + r#" +$ErrorActionPreference = 'Stop' +$acl = Get-Acl -LiteralPath $env:HERDR_TEST_CONFIG_SOURCE +$acl.SetAccessRuleProtection($true, $false) +$sid = [System.Security.Principal.WindowsIdentity]::GetCurrent().User +$rule = New-Object System.Security.AccessControl.FileSystemAccessRule($sid, 'FullControl', 'Allow') +$acl.AddAccessRule($rule) +Set-Acl -LiteralPath $env:HERDR_TEST_CONFIG_SOURCE -AclObject $acl +"#, + &source, + ); + let snapshot = r#"$ErrorActionPreference = 'Stop'; (Get-Acl -LiteralPath $env:HERDR_TEST_CONFIG_SOURCE).Sddl"#; + let before = powershell(snapshot, &source); + drop(create_config_temporary(&target, true).unwrap()); + write_config_temporary(Some(&source), &target, b"new").unwrap(); + replace_file(&target, &source).unwrap(); + assert_eq!(powershell(snapshot, &source), before); + assert_eq!(std::fs::read(&source).unwrap(), b"new"); + std::fs::remove_dir_all(dir).unwrap(); +} From 1ff21e6ab512b29942c81b43c8650d7c554c8fca Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 22:15:11 +0000 Subject: [PATCH 02/13] fix: atomically write integration configs refs #3970 --- docs/next/website/src/content/docs/integrations.mdx | 2 +- docs/next/website/src/content/docs/ja/integrations.mdx | 2 +- docs/next/website/src/content/docs/zh-cn/integrations.mdx | 2 +- src/platform/macos.rs | 8 ++++---- src/platform/windows.rs | 7 +++++-- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/docs/next/website/src/content/docs/integrations.mdx b/docs/next/website/src/content/docs/integrations.mdx index d7154f015b..1c92a2e87f 100644 --- a/docs/next/website/src/content/docs/integrations.mdx +++ b/docs/next/website/src/content/docs/integrations.mdx @@ -51,7 +51,7 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` -Shared agent configuration is written to a temporary file and replaced only after the complete write succeeds. Herdr preserves file permissions and follows symlinks. Config files with multiple hard links are rejected before installation or removal changes hook files; use a separate file or a symlink before retrying. This protects each config file, not an entire multi-file installation from partial completion. +Shared agent configuration is written to a temporary file and replaced only after the complete write succeeds. Herdr preserves file permissions and follows symlinks. Before changing hook files, Herdr checks for config files with multiple hard links and rejects the operation if it finds any; use a separate file or a symlink before retrying. A later failure, including a concurrent config change, can still leave earlier installation changes in place. This is per-file protection against incomplete writes, not an installation transaction or a power-loss durability guarantee. ## How Herdr uses integrations diff --git a/docs/next/website/src/content/docs/ja/integrations.mdx b/docs/next/website/src/content/docs/ja/integrations.mdx index 7649633b58..83a642f093 100644 --- a/docs/next/website/src/content/docs/ja/integrations.mdx +++ b/docs/next/website/src/content/docs/ja/integrations.mdx @@ -53,7 +53,7 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` -共有されるエージェント設定は一時ファイルに書き込まれ、書き込みがすべて成功した後に置き換えられます。Herdr はファイルのアクセス権を保持し、シンボリックリンクをたどります。複数のハードリンクを持つ設定ファイルは、インストールやアンインストールでフックファイルを変更する前に拒否されます。独立したファイルまたはシンボリックリンクに変更してから再試行してください。この保護は設定ファイル単位であり、複数ファイルにまたがる操作全体のロールバックを保証するものではありません。 +共有されるエージェント設定は一時ファイルに書き込まれ、書き込みがすべて成功した後に置き換えられます。Herdr はファイルのアクセス権を保持し、シンボリックリンクをたどります。フックファイルを変更する前に、複数のハードリンクを持つ設定ファイルがないかを確認し、見つかった場合は操作を拒否します。独立したファイルまたはシンボリックリンクに変更してから再試行してください。その後の失敗や同時に行われた設定変更によって、それまでのインストール変更が残る場合があります。この保護はファイル単位の不完全な書き込みを防ぐものであり、インストール全体のトランザクションや停電時の永続性を保証するものではありません。 ## Herdr がインテグレーションをどう使うか 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 95b65c0f8a..1a56863ec4 100644 --- a/docs/next/website/src/content/docs/zh-cn/integrations.mdx +++ b/docs/next/website/src/content/docs/zh-cn/integrations.mdx @@ -53,7 +53,7 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` -共享的智能体配置会先写入临时文件，完整写入成功后才替换原文件。Herdr 会保留文件权限并跟随符号链接。如果配置文件有多个硬链接，安装或卸载会在修改钩子文件之前拒绝操作；请改用独立文件或符号链接后重试。这项保护针对单个配置文件，不保证跨多个文件的整个安装操作能够回滚。 +共享的智能体配置会先写入临时文件，完整写入成功后才替换原文件。Herdr 会保留文件权限并跟随符号链接。修改钩子文件之前，Herdr 会检查配置文件是否有多个硬链接，如有则拒绝操作；请改用独立文件或符号链接后重试。后续失败（包括并发修改配置）仍可能留下之前已完成的安装变更。这是针对单个文件的不完整写入保护，不是整个安装的事务，也不保证断电后的持久性。 ## Herdr 如何使用集成 diff --git a/src/platform/macos.rs b/src/platform/macos.rs index ede316a8c1..80cd48af87 100644 --- a/src/platform/macos.rs +++ b/src/platform/macos.rs @@ -126,10 +126,10 @@ pub(crate) fn write_config_temporary( let input = std::fs::File::open(source)?; let metadata = input.metadata()?; let current = output.metadata()?; - if (metadata.uid(), metadata.gid()) != (current.uid(), current.gid()) { - if unsafe { libc::fchown(output.as_raw_fd(), metadata.uid(), metadata.gid()) } != 0 { - return Err(std::io::Error::last_os_error()); - } + 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. diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 91640c089c..2ce575aa1c 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -208,13 +208,14 @@ pub(crate) fn write_config_temporary( use std::io::Write; use std::os::windows::fs::OpenOptionsExt; use windows_sys::Win32::{ + Foundation::GENERIC_WRITE, Security::{ GetFileSecurityW, GetSecurityDescriptorControl, SetKernelObjectSecurity, DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, PROTECTED_DACL_SECURITY_INFORMATION, SE_DACL_PROTECTED, UNPROTECTED_DACL_SECURITY_INFORMATION, }, - Storage::FileSystem::{FILE_GENERIC_WRITE, WRITE_DAC, WRITE_OWNER}, + Storage::FileSystem::{WRITE_DAC, WRITE_OWNER}, }; if let Some(source) = source { // CopyFile preserves file attributes, encryption and alternate streams. @@ -224,7 +225,9 @@ pub(crate) fn write_config_temporary( let mut options = std::fs::OpenOptions::new(); options.write(true).truncate(true); if source.is_some() { - options.access_mode(FILE_GENERIC_WRITE | WRITE_DAC | WRITE_OWNER); + // TRUNCATE_EXISTING requires the GENERIC_WRITE bit, not its mapped + // FILE_GENERIC_WRITE rights, even though those grant equivalent access. + options.access_mode(GENERIC_WRITE | WRITE_DAC | WRITE_OWNER); } let mut output = options.open(temporary)?; output.write_all(contents)?; From 5f76f3526dbfd89ff518bbfa6416895dd30d1ed5 Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 22:27:20 +0000 Subject: [PATCH 03/13] fix: atomically write integration configs refs #3970 --- src/integration/config_file/tests.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/integration/config_file/tests.rs b/src/integration/config_file/tests.rs index a716ec91ce..896434b350 100644 --- a/src/integration/config_file/tests.rs +++ b/src/integration/config_file/tests.rs @@ -105,18 +105,25 @@ fn symlink_chains_and_dangling_targets_preserve_links() { let intermediate = other.join("link"); let entry = dir.0.join("config"); symlink(&target, &intermediate); - symlink(Path::new("other/link"), &entry); + // 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(), Path::new("other/link")); - assert_eq!(fs::read_link(&intermediate).unwrap(), target); + 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(), Path::new("other/link")); + assert_eq!(fs::read_link(&entry).unwrap(), relative_target); let cycle = dir.0.join("cycle"); symlink(Path::new("cycle"), &cycle); From 563e21a6e7f4738adedc4cae902b8c522c18df0f Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 22:37:35 +0000 Subject: [PATCH 04/13] fix: atomically write integration configs refs #3970 --- src/platform/windows/config_file_tests.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/platform/windows/config_file_tests.rs b/src/platform/windows/config_file_tests.rs index d27b3bd002..7336698fa0 100644 --- a/src/platform/windows/config_file_tests.rs +++ b/src/platform/windows/config_file_tests.rs @@ -21,16 +21,18 @@ fn config_replacement_preserves_windows_access_control() { powershell( r#" $ErrorActionPreference = 'Stop' -$acl = Get-Acl -LiteralPath $env:HERDR_TEST_CONFIG_SOURCE +# Use the .NET Framework API directly: a parent pwsh process can pass a +# PSModulePath containing incompatible PowerShell 7 versions of Get-Acl/Set-Acl. +$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) $acl.SetAccessRuleProtection($true, $false) $sid = [System.Security.Principal.WindowsIdentity]::GetCurrent().User -$rule = New-Object System.Security.AccessControl.FileSystemAccessRule($sid, 'FullControl', 'Allow') +$rule = [System.Security.AccessControl.FileSystemAccessRule]::new($sid, [System.Security.AccessControl.FileSystemRights]::FullControl, [System.Security.AccessControl.AccessControlType]::Allow) $acl.AddAccessRule($rule) -Set-Acl -LiteralPath $env:HERDR_TEST_CONFIG_SOURCE -AclObject $acl +[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) "#, &source, ); - let snapshot = r#"$ErrorActionPreference = 'Stop'; (Get-Acl -LiteralPath $env:HERDR_TEST_CONFIG_SOURCE).Sddl"#; + let snapshot = r#"$ErrorActionPreference = 'Stop'; [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE).GetSecurityDescriptorSddlForm([System.Security.AccessControl.AccessControlSections]::All)"#; let before = powershell(snapshot, &source); drop(create_config_temporary(&target, true).unwrap()); write_config_temporary(Some(&source), &target, b"new").unwrap(); From 597931e5221793c634b42a50be241291d5c1a762 Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 22:56:38 +0000 Subject: [PATCH 05/13] fix: atomically write integration configs refs #3970 --- src/platform/windows.rs | 144 ++++++++++++++++------ src/platform/windows/config_file_tests.rs | 75 ++++++++++- 2 files changed, 179 insertions(+), 40 deletions(-) diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 2ce575aa1c..7df1506e61 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -210,18 +210,15 @@ pub(crate) fn write_config_temporary( use windows_sys::Win32::{ Foundation::GENERIC_WRITE, Security::{ - GetFileSecurityW, GetSecurityDescriptorControl, SetKernelObjectSecurity, - DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, - OWNER_SECURITY_INFORMATION, PROTECTED_DACL_SECURITY_INFORMATION, SE_DACL_PROTECTED, + Authorization::{SetSecurityInfo, SE_FILE_OBJECT}, + GetSecurityDescriptorControl, GetSecurityDescriptorDacl, GetSecurityDescriptorGroup, + GetSecurityDescriptorOwner, GetSecurityDescriptorSacl, DACL_SECURITY_INFORMATION, + GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, + PROTECTED_DACL_SECURITY_INFORMATION, SE_DACL_PROTECTED, UNPROTECTED_DACL_SECURITY_INFORMATION, }, Storage::FileSystem::{WRITE_DAC, WRITE_OWNER}, }; - if let Some(source) = source { - // CopyFile preserves file attributes, encryption and alternate streams. - // It does not preserve the DACL; that is copied explicitly below. - std::fs::copy(source, temporary)?; - } let mut options = std::fs::OpenOptions::new(); options.write(true).truncate(true); if source.is_some() { @@ -230,33 +227,13 @@ pub(crate) fn write_config_temporary( options.access_mode(GENERIC_WRITE | WRITE_DAC | WRITE_OWNER); } let mut output = options.open(temporary)?; - output.write_all(contents)?; if let Some(source) = source { - let source = extended_length_path(source)?; let information = OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION | LABEL_SECURITY_INFORMATION; - let mut needed = 0; - unsafe { - GetFileSecurityW(source.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( - source.as_ptr(), - information, - descriptor.as_mut_ptr().cast(), - needed, - &mut needed, - ) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } + let mut descriptor = config_security_descriptor(source, information)?; + let expected = config_security_sddl(&mut descriptor, information)?; let mut control = 0; let mut revision = 0; if unsafe { @@ -274,20 +251,115 @@ pub(crate) fn write_config_temporary( } else { UNPROTECTED_DACL_SECURITY_INFORMATION }; - if unsafe { - SetKernelObjectSecurity( + let mut owner = null_mut(); + let mut group = null_mut(); + let mut dacl = null_mut(); + let mut sacl = null_mut(); + let mut defaulted = 0; + let mut present = 0; + let descriptor = descriptor.as_mut_ptr().cast(); + if unsafe { GetSecurityDescriptorOwner(descriptor, &mut owner, &mut defaulted) } == 0 + || unsafe { GetSecurityDescriptorGroup(descriptor, &mut group, &mut defaulted) } == 0 + || unsafe { + GetSecurityDescriptorDacl(descriptor, &mut present, &mut dacl, &mut defaulted) + } == 0 + || unsafe { + GetSecurityDescriptorSacl(descriptor, &mut present, &mut sacl, &mut defaulted) + } == 0 + { + return Err(std::io::Error::last_os_error()); + } + // Files require SetSecurityInfo, not SetKernelObjectSecurity: the latter + // drops the filesystem ACL's automatic-inheritance metadata. + let error = unsafe { + SetSecurityInfo( output.as_raw_handle(), + SE_FILE_OBJECT, information | protection, - descriptor.as_mut_ptr().cast(), + owner, + group, + dacl, + sacl, ) - } == 0 - { - return Err(std::io::Error::last_os_error()); + }; + if error != 0 { + return Err(std::io::Error::from_raw_os_error(error as i32)); + } + let mut installed = config_security_descriptor(temporary, information)?; + if config_security_sddl(&mut installed, information)? != expected { + // An unprotected file moved from another directory can retain old + // inherited permissions. Never put secrets into a temporary whose + // new parent added access, even if publication would be rejected. + return Err(std::io::Error::other( + "cannot preserve config access controls during atomic replacement", + )); } + // Only copy sensitive contents/streams after access controls match. + // CopyFile preserves attributes, encryption and alternate streams. + std::fs::copy(source, temporary)?; + output.set_len(0)?; } + output.write_all(contents)?; output.sync_all() } +fn config_security_descriptor( + path: &std::path::Path, + information: windows_sys::Win32::Security::OBJECT_SECURITY_INFORMATION, +) -> std::io::Result> { + 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) +} + +fn config_security_sddl( + descriptor: &mut [u8], + information: windows_sys::Win32::Security::OBJECT_SECURITY_INFORMATION, +) -> std::io::Result> { + 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_file_tests.rs b/src/platform/windows/config_file_tests.rs index 7336698fa0..24ca325daf 100644 --- a/src/platform/windows/config_file_tests.rs +++ b/src/platform/windows/config_file_tests.rs @@ -33,11 +33,78 @@ $acl.AddAccessRule($rule) &source, ); let snapshot = r#"$ErrorActionPreference = 'Stop'; [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE).GetSecurityDescriptorSddlForm([System.Security.AccessControl.AccessControlSections]::All)"#; + for protected in [true, false] { + if !protected { + powershell( + r#" +$ErrorActionPreference = 'Stop' +$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) +$acl.SetAccessRuleProtection($false, $false) +[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) +"#, + &source, + ); + } + let before = powershell(snapshot, &source); + drop(create_config_temporary(&target, true).unwrap()); + write_config_temporary(Some(&source), &target, b"new").unwrap(); + replace_file(&target, &source).unwrap(); + assert_eq!( + powershell(snapshot, &source), + before, + "protected={protected}" + ); + assert_eq!(std::fs::read(&source).unwrap(), b"new"); + } + std::fs::remove_dir_all(dir).unwrap(); +} + +#[test] +fn config_replacement_rejects_changed_inherited_access_before_copying_contents() { + let dir = std::env::temp_dir().join(format!("herdr-config-moved-acl-{}", std::process::id())); + std::fs::create_dir(&dir).unwrap(); + let parent = dir.join("different-parent"); + std::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 source = dir.join("source"); + std::fs::write(&source, b"private preferences").unwrap(); + std::fs::write(source.with_file_name("source:private"), b"private stream").unwrap(); + let snapshot = r#"$ErrorActionPreference = 'Stop'; [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE).GetSecurityDescriptorSddlForm([System.Security.AccessControl.AccessControlSections]::All)"#; let before = powershell(snapshot, &source); + assert!(!before.contains(";;;BG)"), "{before}"); + assert!( + !before.contains("D:P"), + "source must be unprotected: {before}" + ); + let moved = parent.join("source"); + std::fs::rename(&source, &moved).unwrap(); + assert_eq!(powershell(snapshot, &moved), before, "move retains old ACL"); + let probe = parent.join("inherited"); + std::fs::write(&probe, b"").unwrap(); + assert!(powershell(snapshot, &probe).contains(";;;BG)")); + let target = parent.join("temporary"); drop(create_config_temporary(&target, true).unwrap()); - write_config_temporary(Some(&source), &target, b"new").unwrap(); - replace_file(&target, &source).unwrap(); - assert_eq!(powershell(snapshot, &source), before); - assert_eq!(std::fs::read(&source).unwrap(), b"new"); + let error = write_config_temporary(Some(&moved), &target, b"new").unwrap_err(); + assert!(error + .to_string() + .contains("cannot preserve config access controls")); + assert!(std::fs::read(&target).unwrap().is_empty()); + assert!(!target.with_file_name("temporary:private").exists()); + assert_eq!(std::fs::read(&moved).unwrap(), b"private preferences"); + assert_eq!( + std::fs::read(moved.with_file_name("source:private")).unwrap(), + b"private stream" + ); + assert_eq!(powershell(snapshot, &moved), before); std::fs::remove_dir_all(dir).unwrap(); } From c3a47480d4ddc241b8aba8a600fe76576d03b127 Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:04:10 +0000 Subject: [PATCH 06/13] fix: atomically write integration configs refs #3970 --- src/platform/windows.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 7df1506e61..41e65e7d30 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -286,7 +286,14 @@ pub(crate) fn write_config_temporary( return Err(std::io::Error::from_raw_os_error(error as i32)); } let mut installed = config_security_descriptor(temporary, information)?; - if config_security_sddl(&mut installed, information)? != expected { + let installed = config_security_sddl(&mut installed, information)?; + if installed != expected { + #[cfg(test)] + eprintln!( + "config ACL mismatch: expected {}, installed {}", + String::from_utf16_lossy(&expected), + String::from_utf16_lossy(&installed), + ); // An unprotected file moved from another directory can retain old // inherited permissions. Never put secrets into a temporary whose // new parent added access, even if publication would be rejected. From 8aa5b796ad58a4e7fe1f1838cbf2d062cf6e05c5 Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:57:44 +0000 Subject: [PATCH 07/13] fix: atomically write integration configs refs #3970 --- .github/workflows/ci.yml | 8 + src/platform/windows.rs | 2 + src/platform/windows/config_file_tests.rs | 2 +- src/platform/windows/native_replace_probe.rs | 325 +++++++++++++++++++ 4 files changed, 336 insertions(+), 1 deletion(-) create mode 100644 src/platform/windows/native_replace_probe.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a481594e90..a5cdbc9731 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -167,6 +167,14 @@ jobs: $env:CARGO_INCREMENTAL = "1" just check + # Temporary native feasibility evidence for #3970; does not replace checks. + - name: Probe native Windows config replacement + if: matrix.kind == 'windows' && !cancelled() + shell: pwsh + run: | + cargo nextest run --locked -E 'test(native_replace_probe)' --no-fail-fast --status-level all --final-status-level all --success-output immediate --failure-output immediate + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + - name: Smoke ConPTY pane if: matrix.kind == 'windows' shell: pwsh diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 41e65e7d30..7b09ff4f41 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -16,6 +16,8 @@ use std::{ mod clipboard_image; #[cfg(test)] mod config_file_tests; +#[cfg(test)] +mod native_replace_probe; 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. diff --git a/src/platform/windows/config_file_tests.rs b/src/platform/windows/config_file_tests.rs index 24ca325daf..d13ff101ae 100644 --- a/src/platform/windows/config_file_tests.rs +++ b/src/platform/windows/config_file_tests.rs @@ -1,6 +1,6 @@ use super::*; -fn powershell(script: &str, source: &std::path::Path) -> String { +pub(super) fn powershell(script: &str, source: &std::path::Path) -> String { let output = std::process::Command::new("powershell.exe") .args(["-NoProfile", "-NonInteractive", "-Command", script]) .env("HERDR_TEST_CONFIG_SOURCE", source) diff --git a/src/platform/windows/native_replace_probe.rs b/src/platform/windows/native_replace_probe.rs new file mode 100644 index 0000000000..5ddd7f5e68 --- /dev/null +++ b/src/platform/windows/native_replace_probe.rs @@ -0,0 +1,325 @@ +//! Native feasibility evidence for #3970; not used by the config writer. + +use super::*; +use std::io::Write; +use std::path::{Path, PathBuf}; +use windows_sys::Win32::{ + Security::{ + DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, + OWNER_SECURITY_INFORMATION, + }, + Storage::FileSystem::{MoveFileExW, ReplaceFileW}, +}; + +use super::config_file_tests::powershell; + +struct Probe(PathBuf); +impl Probe { + fn new(case: &str) -> Self { + let path = std::env::temp_dir().join(format!( + "herdr-native-replace-{case}-{}", + std::process::id() + )); + std::fs::create_dir(&path).unwrap(); + Self(path) + } +} +impl Drop for Probe { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn snapshot(path: &Path) -> Vec { + let information = OWNER_SECURITY_INFORMATION + | GROUP_SECURITY_INFORMATION + | DACL_SECURITY_INFORMATION + | LABEL_SECURITY_INFORMATION; + let mut descriptor = config_security_descriptor(path, information).unwrap(); + config_security_sddl(&mut descriptor, information).unwrap() +} + +fn replace(source: &Path, temporary: &Path, backup: &Path) -> std::io::Result<()> { + let source = extended_length_path(source)?; + let temporary = extended_length_path(temporary)?; + let backup = extended_length_path(backup)?; + // Neither ACL-ignore flag is allowed. WRITE_THROUGH is unsupported here. + if unsafe { + ReplaceFileW( + source.as_ptr(), + temporary.as_ptr(), + backup.as_ptr(), + 0, + null_mut(), + null_mut(), + ) + } == 0 + { + return Err(std::io::Error::last_os_error()); + } + Ok(()) +} + +fn restore_without_overwrite(backup: &Path, target: &Path) -> std::io::Result<()> { + let backup = extended_length_path(backup)?; + let target = extended_length_path(target)?; + // Deliberately no REPLACE_EXISTING: recovery must not clobber an intervening file. + if unsafe { MoveFileExW(backup.as_ptr(), target.as_ptr(), 0) } == 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) +} + +fn seed_non_dacl_metadata(source: &Path, temporary: &Path) { + use windows_sys::Win32::Security::Authorization::{SetNamedSecurityInfoW, SE_FILE_OBJECT}; + use windows_sys::Win32::Security::{ + GetSecurityDescriptorGroup, GetSecurityDescriptorOwner, GetSecurityDescriptorSacl, + }; + let information = + OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | LABEL_SECURITY_INFORMATION; + let mut descriptor = config_security_descriptor(source, information).unwrap(); + let descriptor = descriptor.as_mut_ptr().cast(); + let mut owner = null_mut(); + let mut group = null_mut(); + let mut sacl = null_mut(); + let mut defaulted = 0; + let mut present = 0; + assert_ne!( + unsafe { GetSecurityDescriptorOwner(descriptor, &mut owner, &mut defaulted) }, + 0 + ); + assert_ne!( + unsafe { GetSecurityDescriptorGroup(descriptor, &mut group, &mut defaulted) }, + 0 + ); + assert_ne!( + unsafe { GetSecurityDescriptorSacl(descriptor, &mut present, &mut sacl, &mut defaulted) }, + 0 + ); + let mut information = OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION; + if present != 0 && !sacl.is_null() { + information |= LABEL_SECURITY_INFORMATION; + } + let temporary = extended_length_path(temporary).unwrap(); + let error = unsafe { + SetNamedSecurityInfoW( + temporary.as_ptr(), + SE_FILE_OBJECT, + information, + owner, + group, + null_mut(), + sacl, + ) + }; + assert_eq!( + error, + 0, + "non-DACL metadata setup: {}", + std::io::Error::from_raw_os_error(error as i32) + ); +} + +fn probe_success(case: &str, seed_metadata: bool) { + let dir = Probe::new(case); + let source = dir.0.join("source"); + std::fs::write(&source, b"original preferences").unwrap(); + std::fs::write(dir.0.join("source:private"), b"original stream").unwrap(); + match case { + "protected" | "unprotected" => { + let script = format!( + r#" +$ErrorActionPreference = 'Stop' +$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) +$acl.SetAccessRuleProtection(${}, $false) +[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) +"#, + case == "protected" + ); + powershell(&script, &source); + } + "metadata-bare" | "metadata-seeded" => { + 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 before = snapshot(&source); + if case == "legacy" { + assert!( + String::from_utf16_lossy(&before).contains("D:("), + "legacy fixture: {}", + String::from_utf16_lossy(&before) + ); + } + let source = if case == "moved" { + powershell( + r#" +$ErrorActionPreference = 'Stop' +$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) +$acl.SetAccessRuleProtection($false, $false) +[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) +"#, + &source, + ); + let retained = snapshot(&source); + let parent = dir.0.join("broader-parent"); + std::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("source"); + std::fs::rename(&source, &moved).unwrap(); + assert_eq!(snapshot(&moved), retained); + assert!(!String::from_utf16_lossy(&retained).contains(";;;BG)")); + let inherited = parent.join("inherited"); + std::fs::write(&inherited, b"").unwrap(); + assert!(String::from_utf16_lossy(&snapshot(&inherited)).contains(";;;BG)")); + moved + } else { + source + }; + let expected = snapshot(&source); + let expected_text = String::from_utf16_lossy(&expected); + match case { + "protected" => assert!(expected_text.contains("D:PAI"), "{expected_text}"), + "unprotected" | "moved" => { + assert!(expected_text.contains("D:AI"), "{expected_text}"); + assert!(!expected_text.contains("D:P"), "{expected_text}"); + } + _ => {} + } + let parent = source.parent().unwrap(); + let temporary = parent.join("replacement"); + let backup = parent.join("backup"); + let mut stage = create_config_temporary(&temporary, true).unwrap(); + if case.starts_with("metadata-") { + let initial = snapshot(&temporary); + let initial_text = String::from_utf16_lossy(&initial); + let expected_identity = expected_text.split("D:").next().unwrap(); + let initial_identity = initial_text.split("D:").next().unwrap(); + assert_ne!( + expected_identity.split("G:").next(), + initial_identity.split("G:").next(), + "source owner must differ from staging defaults" + ); + assert_ne!( + expected_identity.split("G:").nth(1), + initial_identity.split("G:").nth(1), + "source group must differ from staging defaults" + ); + assert!(String::from_utf16_lossy(&expected).contains(";;;LW)")); + if seed_metadata { + seed_non_dacl_metadata(&source, &temporary); + } + } + stage.write_all(b"complete new preferences").unwrap(); + stage.sync_all().unwrap(); + drop(stage); + replace(&source, &temporary, &backup).unwrap(); + let actual = snapshot(&source); + let backup_security = snapshot(&backup); + eprintln!( + "{case}: expected {}; actual {}; backup {}", + String::from_utf16_lossy(&expected), + String::from_utf16_lossy(&actual), + String::from_utf16_lossy(&backup_security) + ); + assert_eq!(std::fs::read(&source).unwrap(), b"complete new preferences"); + assert_eq!(std::fs::read(&backup).unwrap(), b"original preferences"); + assert_eq!( + std::fs::read(parent.join("source:private")).unwrap(), + b"original stream" + ); + assert_eq!( + std::fs::read(parent.join("backup:private")).unwrap(), + b"original stream" + ); + assert!(!temporary.exists()); + assert_eq!(backup_security, expected, "backup security"); + assert_eq!(actual, expected, "replacement security"); +} + +#[test] +fn native_replace_probe_legacy() { + probe_success("legacy", false); +} +#[test] +fn native_replace_probe_protected() { + probe_success("protected", false); +} +#[test] +fn native_replace_probe_unprotected() { + probe_success("unprotected", false); +} +#[test] +fn native_replace_probe_moved() { + probe_success("moved", false); +} +#[test] +fn native_replace_probe_metadata_bare() { + probe_success("metadata-bare", false); +} +#[test] +fn native_replace_probe_metadata_seeded() { + probe_success("metadata-seeded", true); +} + +#[test] +fn native_replace_probe_failure_and_recovery() { + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::Storage::FileSystem::{FILE_SHARE_READ, FILE_SHARE_WRITE}; + let dir = Probe::new("recovery"); + let source = dir.0.join("source"); + let temporary = dir.0.join("replacement"); + let backup = dir.0.join("backup"); + std::fs::write(&source, b"original").unwrap(); + let expected = snapshot(&source); + let mut stage = create_config_temporary(&temporary, true).unwrap(); + stage.write_all(b"new").unwrap(); + stage.sync_all().unwrap(); + drop(stage); + let held = std::fs::OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .open(&source) + .unwrap(); + let error = replace(&source, &temporary, &backup).unwrap_err(); + eprintln!("native sharing failure: {error}"); + assert_eq!(std::fs::read(&source).unwrap(), b"original"); + assert_eq!(snapshot(&source), expected); + assert_eq!(std::fs::read(&temporary).unwrap(), b"new"); + assert!(!backup.exists()); + drop(held); + replace(&source, &temporary, &backup).unwrap(); + // Construct the documented 1177 recovery layout; this is not fault injection. + std::fs::remove_file(&source).unwrap(); + std::fs::write(&source, b"intervening update").unwrap(); + assert!(restore_without_overwrite(&backup, &source).is_err()); + assert_eq!(std::fs::read(&source).unwrap(), b"intervening update"); + assert_eq!(std::fs::read(&backup).unwrap(), b"original"); + assert_eq!(snapshot(&backup), expected); + std::fs::remove_file(&source).unwrap(); + restore_without_overwrite(&backup, &source).unwrap(); + assert_eq!(std::fs::read(&source).unwrap(), b"original"); + assert_eq!(snapshot(&source), expected); + assert!(!backup.exists()); +} From 633d466e094e23e9832553f63fd61934e1ede95c Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:07:35 +0000 Subject: [PATCH 08/13] fix: atomically write integration configs refs #3970 --- .github/workflows/ci.yml | 3 +- src/platform/windows/native_replace_probe.rs | 36 ++++++++++---------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a5cdbc9731..c3ef7edca4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,7 +172,8 @@ jobs: if: matrix.kind == 'windows' && !cancelled() shell: pwsh run: | - cargo nextest run --locked -E 'test(native_replace_probe)' --no-fail-fast --status-level all --final-status-level all --success-output immediate --failure-output immediate + $env:CARGO_INCREMENTAL = "1" + cargo nextest run --locked -E 'test(native_replace_probe)' --no-fail-fast --status-level pass --final-status-level pass --success-output immediate --failure-output immediate if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - name: Smoke ConPTY pane diff --git a/src/platform/windows/native_replace_probe.rs b/src/platform/windows/native_replace_probe.rs index 5ddd7f5e68..4dc144245e 100644 --- a/src/platform/windows/native_replace_probe.rs +++ b/src/platform/windows/native_replace_probe.rs @@ -70,20 +70,15 @@ fn restore_without_overwrite(backup: &Path, target: &Path) -> std::io::Result<() Ok(()) } -fn seed_non_dacl_metadata(source: &Path, temporary: &Path) { +fn seed_owner_and_group(source: &Path, temporary: &Path) { use windows_sys::Win32::Security::Authorization::{SetNamedSecurityInfoW, SE_FILE_OBJECT}; - use windows_sys::Win32::Security::{ - GetSecurityDescriptorGroup, GetSecurityDescriptorOwner, GetSecurityDescriptorSacl, - }; - let information = - OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | LABEL_SECURITY_INFORMATION; + use windows_sys::Win32::Security::{GetSecurityDescriptorGroup, GetSecurityDescriptorOwner}; + let information = OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION; let mut descriptor = config_security_descriptor(source, information).unwrap(); let descriptor = descriptor.as_mut_ptr().cast(); let mut owner = null_mut(); let mut group = null_mut(); - let mut sacl = null_mut(); let mut defaulted = 0; - let mut present = 0; assert_ne!( unsafe { GetSecurityDescriptorOwner(descriptor, &mut owner, &mut defaulted) }, 0 @@ -92,14 +87,6 @@ fn seed_non_dacl_metadata(source: &Path, temporary: &Path) { unsafe { GetSecurityDescriptorGroup(descriptor, &mut group, &mut defaulted) }, 0 ); - assert_ne!( - unsafe { GetSecurityDescriptorSacl(descriptor, &mut present, &mut sacl, &mut defaulted) }, - 0 - ); - let mut information = OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION; - if present != 0 && !sacl.is_null() { - information |= LABEL_SECURITY_INFORMATION; - } let temporary = extended_length_path(temporary).unwrap(); let error = unsafe { SetNamedSecurityInfoW( @@ -109,7 +96,7 @@ fn seed_non_dacl_metadata(source: &Path, temporary: &Path) { owner, group, null_mut(), - sacl, + null_mut(), ) }; assert_eq!( @@ -132,6 +119,9 @@ fn probe_success(case: &str, seed_metadata: bool) { $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" @@ -228,12 +218,22 @@ $acl.AddAccessRule($rule) ); assert!(String::from_utf16_lossy(&expected).contains(";;;LW)")); if seed_metadata { - seed_non_dacl_metadata(&source, &temporary); + // The bare probe retained the low label but not owner/group. + // Test only that missing supplement; do not reapply DACLs or labels. + seed_owner_and_group(&source, &temporary); } } stage.write_all(b"complete new preferences").unwrap(); stage.sync_all().unwrap(); drop(stage); + // Permission setup must not make the source itself unwritable. + drop( + std::fs::OpenOptions::new() + .read(true) + .write(true) + .open(&source) + .unwrap(), + ); replace(&source, &temporary, &backup).unwrap(); let actual = snapshot(&source); let backup_security = snapshot(&backup); From f6d44bca4e32ee12b851a6a66d253dd487594751 Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:15:50 +0000 Subject: [PATCH 09/13] fix: atomically write integration configs refs #3970 --- .github/workflows/ci.yml | 9 - src/platform/windows.rs | 2 - src/platform/windows/config_file_tests.rs | 2 +- src/platform/windows/native_replace_probe.rs | 325 ------------------- 4 files changed, 1 insertion(+), 337 deletions(-) delete mode 100644 src/platform/windows/native_replace_probe.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c3ef7edca4..a481594e90 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -167,15 +167,6 @@ jobs: $env:CARGO_INCREMENTAL = "1" just check - # Temporary native feasibility evidence for #3970; does not replace checks. - - name: Probe native Windows config replacement - if: matrix.kind == 'windows' && !cancelled() - shell: pwsh - run: | - $env:CARGO_INCREMENTAL = "1" - cargo nextest run --locked -E 'test(native_replace_probe)' --no-fail-fast --status-level pass --final-status-level pass --success-output immediate --failure-output immediate - if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - - name: Smoke ConPTY pane if: matrix.kind == 'windows' shell: pwsh diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 7b09ff4f41..41e65e7d30 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -16,8 +16,6 @@ use std::{ mod clipboard_image; #[cfg(test)] mod config_file_tests; -#[cfg(test)] -mod native_replace_probe; 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. diff --git a/src/platform/windows/config_file_tests.rs b/src/platform/windows/config_file_tests.rs index d13ff101ae..24ca325daf 100644 --- a/src/platform/windows/config_file_tests.rs +++ b/src/platform/windows/config_file_tests.rs @@ -1,6 +1,6 @@ use super::*; -pub(super) fn powershell(script: &str, source: &std::path::Path) -> String { +fn powershell(script: &str, source: &std::path::Path) -> String { let output = std::process::Command::new("powershell.exe") .args(["-NoProfile", "-NonInteractive", "-Command", script]) .env("HERDR_TEST_CONFIG_SOURCE", source) diff --git a/src/platform/windows/native_replace_probe.rs b/src/platform/windows/native_replace_probe.rs deleted file mode 100644 index 4dc144245e..0000000000 --- a/src/platform/windows/native_replace_probe.rs +++ /dev/null @@ -1,325 +0,0 @@ -//! Native feasibility evidence for #3970; not used by the config writer. - -use super::*; -use std::io::Write; -use std::path::{Path, PathBuf}; -use windows_sys::Win32::{ - Security::{ - DACL_SECURITY_INFORMATION, GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, - OWNER_SECURITY_INFORMATION, - }, - Storage::FileSystem::{MoveFileExW, ReplaceFileW}, -}; - -use super::config_file_tests::powershell; - -struct Probe(PathBuf); -impl Probe { - fn new(case: &str) -> Self { - let path = std::env::temp_dir().join(format!( - "herdr-native-replace-{case}-{}", - std::process::id() - )); - std::fs::create_dir(&path).unwrap(); - Self(path) - } -} -impl Drop for Probe { - fn drop(&mut self) { - let _ = std::fs::remove_dir_all(&self.0); - } -} - -fn snapshot(path: &Path) -> Vec { - let information = OWNER_SECURITY_INFORMATION - | GROUP_SECURITY_INFORMATION - | DACL_SECURITY_INFORMATION - | LABEL_SECURITY_INFORMATION; - let mut descriptor = config_security_descriptor(path, information).unwrap(); - config_security_sddl(&mut descriptor, information).unwrap() -} - -fn replace(source: &Path, temporary: &Path, backup: &Path) -> std::io::Result<()> { - let source = extended_length_path(source)?; - let temporary = extended_length_path(temporary)?; - let backup = extended_length_path(backup)?; - // Neither ACL-ignore flag is allowed. WRITE_THROUGH is unsupported here. - if unsafe { - ReplaceFileW( - source.as_ptr(), - temporary.as_ptr(), - backup.as_ptr(), - 0, - null_mut(), - null_mut(), - ) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } - Ok(()) -} - -fn restore_without_overwrite(backup: &Path, target: &Path) -> std::io::Result<()> { - let backup = extended_length_path(backup)?; - let target = extended_length_path(target)?; - // Deliberately no REPLACE_EXISTING: recovery must not clobber an intervening file. - if unsafe { MoveFileExW(backup.as_ptr(), target.as_ptr(), 0) } == 0 { - return Err(std::io::Error::last_os_error()); - } - Ok(()) -} - -fn seed_owner_and_group(source: &Path, temporary: &Path) { - use windows_sys::Win32::Security::Authorization::{SetNamedSecurityInfoW, SE_FILE_OBJECT}; - use windows_sys::Win32::Security::{GetSecurityDescriptorGroup, GetSecurityDescriptorOwner}; - let information = OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION; - let mut descriptor = config_security_descriptor(source, information).unwrap(); - let descriptor = descriptor.as_mut_ptr().cast(); - let mut owner = null_mut(); - let mut group = null_mut(); - let mut defaulted = 0; - assert_ne!( - unsafe { GetSecurityDescriptorOwner(descriptor, &mut owner, &mut defaulted) }, - 0 - ); - assert_ne!( - unsafe { GetSecurityDescriptorGroup(descriptor, &mut group, &mut defaulted) }, - 0 - ); - let temporary = extended_length_path(temporary).unwrap(); - let error = unsafe { - SetNamedSecurityInfoW( - temporary.as_ptr(), - SE_FILE_OBJECT, - information, - owner, - group, - null_mut(), - null_mut(), - ) - }; - assert_eq!( - error, - 0, - "non-DACL metadata setup: {}", - std::io::Error::from_raw_os_error(error as i32) - ); -} - -fn probe_success(case: &str, seed_metadata: bool) { - let dir = Probe::new(case); - let source = dir.0.join("source"); - std::fs::write(&source, b"original preferences").unwrap(); - std::fs::write(dir.0.join("source:private"), b"original stream").unwrap(); - match case { - "protected" | "unprotected" => { - let script = 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" - ); - powershell(&script, &source); - } - "metadata-bare" | "metadata-seeded" => { - 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 before = snapshot(&source); - if case == "legacy" { - assert!( - String::from_utf16_lossy(&before).contains("D:("), - "legacy fixture: {}", - String::from_utf16_lossy(&before) - ); - } - let source = if case == "moved" { - powershell( - r#" -$ErrorActionPreference = 'Stop' -$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) -$acl.SetAccessRuleProtection($false, $false) -[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) -"#, - &source, - ); - let retained = snapshot(&source); - let parent = dir.0.join("broader-parent"); - std::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("source"); - std::fs::rename(&source, &moved).unwrap(); - assert_eq!(snapshot(&moved), retained); - assert!(!String::from_utf16_lossy(&retained).contains(";;;BG)")); - let inherited = parent.join("inherited"); - std::fs::write(&inherited, b"").unwrap(); - assert!(String::from_utf16_lossy(&snapshot(&inherited)).contains(";;;BG)")); - moved - } else { - source - }; - let expected = snapshot(&source); - let expected_text = String::from_utf16_lossy(&expected); - match case { - "protected" => assert!(expected_text.contains("D:PAI"), "{expected_text}"), - "unprotected" | "moved" => { - assert!(expected_text.contains("D:AI"), "{expected_text}"); - assert!(!expected_text.contains("D:P"), "{expected_text}"); - } - _ => {} - } - let parent = source.parent().unwrap(); - let temporary = parent.join("replacement"); - let backup = parent.join("backup"); - let mut stage = create_config_temporary(&temporary, true).unwrap(); - if case.starts_with("metadata-") { - let initial = snapshot(&temporary); - let initial_text = String::from_utf16_lossy(&initial); - let expected_identity = expected_text.split("D:").next().unwrap(); - let initial_identity = initial_text.split("D:").next().unwrap(); - assert_ne!( - expected_identity.split("G:").next(), - initial_identity.split("G:").next(), - "source owner must differ from staging defaults" - ); - assert_ne!( - expected_identity.split("G:").nth(1), - initial_identity.split("G:").nth(1), - "source group must differ from staging defaults" - ); - assert!(String::from_utf16_lossy(&expected).contains(";;;LW)")); - if seed_metadata { - // The bare probe retained the low label but not owner/group. - // Test only that missing supplement; do not reapply DACLs or labels. - seed_owner_and_group(&source, &temporary); - } - } - stage.write_all(b"complete new preferences").unwrap(); - stage.sync_all().unwrap(); - drop(stage); - // Permission setup must not make the source itself unwritable. - drop( - std::fs::OpenOptions::new() - .read(true) - .write(true) - .open(&source) - .unwrap(), - ); - replace(&source, &temporary, &backup).unwrap(); - let actual = snapshot(&source); - let backup_security = snapshot(&backup); - eprintln!( - "{case}: expected {}; actual {}; backup {}", - String::from_utf16_lossy(&expected), - String::from_utf16_lossy(&actual), - String::from_utf16_lossy(&backup_security) - ); - assert_eq!(std::fs::read(&source).unwrap(), b"complete new preferences"); - assert_eq!(std::fs::read(&backup).unwrap(), b"original preferences"); - assert_eq!( - std::fs::read(parent.join("source:private")).unwrap(), - b"original stream" - ); - assert_eq!( - std::fs::read(parent.join("backup:private")).unwrap(), - b"original stream" - ); - assert!(!temporary.exists()); - assert_eq!(backup_security, expected, "backup security"); - assert_eq!(actual, expected, "replacement security"); -} - -#[test] -fn native_replace_probe_legacy() { - probe_success("legacy", false); -} -#[test] -fn native_replace_probe_protected() { - probe_success("protected", false); -} -#[test] -fn native_replace_probe_unprotected() { - probe_success("unprotected", false); -} -#[test] -fn native_replace_probe_moved() { - probe_success("moved", false); -} -#[test] -fn native_replace_probe_metadata_bare() { - probe_success("metadata-bare", false); -} -#[test] -fn native_replace_probe_metadata_seeded() { - probe_success("metadata-seeded", true); -} - -#[test] -fn native_replace_probe_failure_and_recovery() { - use std::os::windows::fs::OpenOptionsExt; - use windows_sys::Win32::Storage::FileSystem::{FILE_SHARE_READ, FILE_SHARE_WRITE}; - let dir = Probe::new("recovery"); - let source = dir.0.join("source"); - let temporary = dir.0.join("replacement"); - let backup = dir.0.join("backup"); - std::fs::write(&source, b"original").unwrap(); - let expected = snapshot(&source); - let mut stage = create_config_temporary(&temporary, true).unwrap(); - stage.write_all(b"new").unwrap(); - stage.sync_all().unwrap(); - drop(stage); - let held = std::fs::OpenOptions::new() - .read(true) - .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) - .open(&source) - .unwrap(); - let error = replace(&source, &temporary, &backup).unwrap_err(); - eprintln!("native sharing failure: {error}"); - assert_eq!(std::fs::read(&source).unwrap(), b"original"); - assert_eq!(snapshot(&source), expected); - assert_eq!(std::fs::read(&temporary).unwrap(), b"new"); - assert!(!backup.exists()); - drop(held); - replace(&source, &temporary, &backup).unwrap(); - // Construct the documented 1177 recovery layout; this is not fault injection. - std::fs::remove_file(&source).unwrap(); - std::fs::write(&source, b"intervening update").unwrap(); - assert!(restore_without_overwrite(&backup, &source).is_err()); - assert_eq!(std::fs::read(&source).unwrap(), b"intervening update"); - assert_eq!(std::fs::read(&backup).unwrap(), b"original"); - assert_eq!(snapshot(&backup), expected); - std::fs::remove_file(&source).unwrap(); - restore_without_overwrite(&backup, &source).unwrap(); - assert_eq!(std::fs::read(&source).unwrap(), b"original"); - assert_eq!(snapshot(&source), expected); - assert!(!backup.exists()); -} From c9c0a24d94d3607fa4c55145c8d7a82fb6c33e4a Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:50:21 +0000 Subject: [PATCH 10/13] fix: atomically write integration configs refs #3970 --- .github/workflows/ci.yml | 9 + src/platform/windows.rs | 3 + src/platform/windows/config_backup.rs | 165 +++++++++ src/platform/windows/config_backup/tests.rs | 371 ++++++++++++++++++++ 4 files changed, 548 insertions(+) create mode 100644 src/platform/windows/config_backup.rs create mode 100644 src/platform/windows/config_backup/tests.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a481594e90..2d0edea1af 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -167,6 +167,15 @@ jobs: $env:CARGO_INCREMENTAL = "1" just check + # Candidate validation is additional evidence, not a replacement for checks. + - name: Validate Windows recovery-backed config writes + if: matrix.kind == 'windows' && !cancelled() + shell: pwsh + run: | + $env:CARGO_INCREMENTAL = "1" + cargo nextest run --locked -E 'test(config_backup::tests)' --no-fail-fast --status-level pass --final-status-level pass --success-output immediate --failure-output immediate + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + - name: Smoke ConPTY pane if: matrix.kind == 'windows' shell: pwsh diff --git a/src/platform/windows.rs b/src/platform/windows.rs index 41e65e7d30..d00a957862 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -14,6 +14,9 @@ use std::{ }; mod clipboard_image; +// Validate the recovery-backed candidate natively before routing config writes to it. +#[cfg(test)] +mod config_backup; #[cfg(test)] mod config_file_tests; diff --git a/src/platform/windows/config_backup.rs b/src/platform/windows/config_backup.rs new file mode 100644 index 0000000000..8e7390abf8 --- /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 { + 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 { + 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 0000000000..98217da549 --- /dev/null +++ b/src/platform/windows/config_backup/tests.rs @@ -0,0 +1,371 @@ +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 { + 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)).into_owned(); + 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() { + 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 error = write_observed(&path, b"saved", |phase| { + if phase == Phase::Committed { + let mut permissions = fs::metadata(&complete)?.permissions(); + permissions.set_readonly(true); + fs::set_permissions(&complete, permissions)?; + } + 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()); + let mut permissions = fs::metadata(&complete).unwrap().permissions(); + permissions.set_readonly(false); + fs::set_permissions(&complete, permissions).unwrap(); +} + +#[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()); +} From 4012705aaa3cb6674b77439b17ee6812a046939d Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Sat, 12 Sep 2026 00:59:41 +0000 Subject: [PATCH 11/13] fix: atomically write integration configs refs #3970 --- src/platform/windows/config_backup/tests.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/platform/windows/config_backup/tests.rs b/src/platform/windows/config_backup/tests.rs index 98217da549..4b4574621a 100644 --- a/src/platform/windows/config_backup/tests.rs +++ b/src/platform/windows/config_backup/tests.rs @@ -163,7 +163,7 @@ $acl.AddAccessRule($rule) 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)).into_owned(); + let private = String::from_utf16_lossy(&security(&complete)); assert!( private.contains("D:P") && !private.contains(";;;BG)"), "{private}" @@ -339,6 +339,8 @@ fn completed_write_cleanup_failure_and_foreign_backups_are_reported() { assert_eq!(fs::read(&complete).unwrap(), b"original"); assert!(write_existing(&path, b"retry").is_err()); let mut permissions = fs::metadata(&complete).unwrap().permissions(); + // This Windows-only fixture clears FILE_ATTRIBUTE_READONLY, not Unix mode bits. + #[allow(clippy::permissions_set_readonly_false)] permissions.set_readonly(false); fs::set_permissions(&complete, permissions).unwrap(); } From aa73dbecd4bb270715f1a003bcdf6f04aff8b36a Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Sat, 12 Sep 2026 01:06:59 +0000 Subject: [PATCH 12/13] fix: atomically write integration configs refs #3970 --- src/platform/windows/config_backup/tests.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/platform/windows/config_backup/tests.rs b/src/platform/windows/config_backup/tests.rs index 4b4574621a..966ba2a9d0 100644 --- a/src/platform/windows/config_backup/tests.rs +++ b/src/platform/windows/config_backup/tests.rs @@ -313,6 +313,8 @@ fn interrupted_backups_and_writes_are_distinguishable() { #[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(); @@ -322,11 +324,15 @@ fn completed_write_cleanup_failure_and_foreign_backups_are_reported() { 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 { - let mut permissions = fs::metadata(&complete)?.permissions(); - permissions.set_readonly(true); - fs::set_permissions(&complete, permissions)?; + held = Some( + OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .open(&complete)?, + ); } Ok(()) }) @@ -338,11 +344,7 @@ fn completed_write_cleanup_failure_and_foreign_backups_are_reported() { assert_eq!(fs::read(&path).unwrap(), b"saved"); assert_eq!(fs::read(&complete).unwrap(), b"original"); assert!(write_existing(&path, b"retry").is_err()); - let mut permissions = fs::metadata(&complete).unwrap().permissions(); - // This Windows-only fixture clears FILE_ATTRIBUTE_READONLY, not Unix mode bits. - #[allow(clippy::permissions_set_readonly_false)] - permissions.set_readonly(false); - fs::set_permissions(&complete, permissions).unwrap(); + drop(held); } #[test] From 2d567b9fc207aca0d41e0416a0393441b7c69ff6 Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Sat, 12 Sep 2026 01:26:05 +0000 Subject: [PATCH 13/13] fix: atomically write integration configs refs #3970 --- .github/workflows/ci.yml | 9 -- .../website/src/content/docs/integrations.mdx | 29 +++- .../src/content/docs/ja/integrations.mdx | 29 +++- .../src/content/docs/zh-cn/integrations.mdx | 29 +++- src/integration/config_file.rs | 18 ++- src/integration/config_file/tests.rs | 42 ++++-- src/integration/opencode_config.rs | 5 +- src/integration/targets.rs | 54 ++++---- src/integration/tests.rs | 51 +++++++ src/platform/linux.rs | 12 ++ src/platform/macos.rs | 12 ++ src/platform/windows.rs | 125 ++++-------------- src/platform/windows/config_file_tests.rs | 110 --------------- 13 files changed, 256 insertions(+), 269 deletions(-) delete mode 100644 src/platform/windows/config_file_tests.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d0edea1af..a481594e90 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -167,15 +167,6 @@ jobs: $env:CARGO_INCREMENTAL = "1" just check - # Candidate validation is additional evidence, not a replacement for checks. - - name: Validate Windows recovery-backed config writes - if: matrix.kind == 'windows' && !cancelled() - shell: pwsh - run: | - $env:CARGO_INCREMENTAL = "1" - cargo nextest run --locked -E 'test(config_backup::tests)' --no-fail-fast --status-level pass --final-status-level pass --success-output immediate --failure-output immediate - if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } - - name: Smoke ConPTY pane if: matrix.kind == 'windows' shell: pwsh diff --git a/docs/next/website/src/content/docs/integrations.mdx b/docs/next/website/src/content/docs/integrations.mdx index 1c92a2e87f..71baa66ce0 100644 --- a/docs/next/website/src/content/docs/integrations.mdx +++ b/docs/next/website/src/content/docs/integrations.mdx @@ -51,7 +51,34 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` -Shared agent configuration is written to a temporary file and replaced only after the complete write succeeds. Herdr preserves file permissions and follows symlinks. Before changing hook files, Herdr checks for config files with multiple hard links and rejects the operation if it finds any; use a separate file or a symlink before retrying. A later failure, including a concurrent config change, can still leave earlier installation changes in place. This is per-file protection against incomplete writes, not an installation transaction or a power-loss durability guarantee. +On Linux and macOS, shared agent configuration is staged in a temporary file and atomically replaced after a complete write. Existing Windows configs are instead backed up before being updated in place, preserving their ownership and access rules; interrupted updates may require recovery. New files use staged replacement on every platform. Herdr follows symlinks and rejects configurations with multiple hard links before changing hook files; use a separate file or a symlink before retrying. Protection is per file, not an installation transaction, concurrency control, or a power-loss durability guarantee. Later failures can leave earlier installation changes in place. + +
+Windows config recovery + +Before changing an existing config, Herdr saves and syncs its original contents in a private `.herdr-backup` file. A failed write retains this recovery copy and reports both paths. Further updates refuse to overwrite it, even if the config is missing or invalid. Existing EFS-encrypted configs are rejected unchanged rather than creating plaintext backups. + +A `.herdr-backup.pending` file is an unfinished backup, **not a recovery source**. The in-place write has not started for that backup. Inspect the config before removing the unfinished file and retrying. A completed backup can also remain after a successful update if cleanup failed or was interrupted; its presence alone does not mean the current config is damaged. + +If recovery is needed, close the agent and restore the backup's **contents into the existing config**, not by moving the backup over it. In PowerShell, set `$config` to the config path reported by Herdr: + +```powershell +$ErrorActionPreference = 'Stop' +$config = 'C:\path\to\cli.json' +$backup = "$config.herdr-backup" +$bytes = [System.IO.File]::ReadAllBytes($backup) +$file = [System.IO.File]::Open($config, [System.IO.FileMode]::Open, [System.IO.FileAccess]::Write) +try { + $file.SetLength(0) + $file.Write($bytes, 0, $bytes.Length) + $file.Flush($true) +} finally { + $file.Dispose() +} +``` + +Keep the backup if restoration fails. After verifying the config is correct, remove the completed backup with `Remove-Item -LiteralPath $backup` before retrying the integration update. If the original file is missing, restore its intended permissions as well; creating a replacement file does not preserve them automatically. +
## How Herdr uses integrations diff --git a/docs/next/website/src/content/docs/ja/integrations.mdx b/docs/next/website/src/content/docs/ja/integrations.mdx index 83a642f093..136d74a1e7 100644 --- a/docs/next/website/src/content/docs/ja/integrations.mdx +++ b/docs/next/website/src/content/docs/ja/integrations.mdx @@ -53,7 +53,34 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` -共有されるエージェント設定は一時ファイルに書き込まれ、書き込みがすべて成功した後に置き換えられます。Herdr はファイルのアクセス権を保持し、シンボリックリンクをたどります。フックファイルを変更する前に、複数のハードリンクを持つ設定ファイルがないかを確認し、見つかった場合は操作を拒否します。独立したファイルまたはシンボリックリンクに変更してから再試行してください。その後の失敗や同時に行われた設定変更によって、それまでのインストール変更が残る場合があります。この保護はファイル単位の不完全な書き込みを防ぐものであり、インストール全体のトランザクションや停電時の永続性を保証するものではありません。 +Linux と macOS では、共有エージェント設定を一時ファイルに完全に書き込んでからアトミックに置き換えます。Windows の既存設定は、所有者とアクセス規則を維持するため、バックアップ後に同じファイルを更新します。中断された更新には復旧が必要な場合があります。新規ファイルはすべてのプラットフォームで一時ファイルを使って作成します。Herdr はシンボリックリンクをたどり、複数のハードリンクがある設定はフックファイルの変更前に拒否します。独立したファイルまたはシンボリックリンクに変更して再試行してください。これはファイル単位の保護であり、インストール全体のトランザクション、同時編集の制御、停電時の永続性は保証しません。後から失敗すると、それまでのインストール変更が残る場合があります。 + +
+Windows の設定の復旧 + +既存設定を変更する前に、Herdr は元の内容を非公開の `.herdr-backup` に保存して同期します。書き込みが失敗すると、この復旧用コピーを保持し、両方のパスを通知します。設定が存在しない場合や無効な場合も、コピーが残っている間は再更新を拒否します。EFS で暗号化された既存設定は、平文バックアップを作らず、変更せずに拒否します。 + +`.herdr-backup.pending` は未完了のバックアップであり、**復旧元として使わないでください**。そのバックアップに対応する設定への書き込みはまだ始まっていません。設定を確認してから未完了ファイルを削除し、再試行してください。更新成功後でも、後処理の失敗や中断により完成済みバックアップが残ることがあります。バックアップの存在だけでは、現在の設定が壊れているとは判断できません。 + +復旧が必要ならエージェントを終了し、バックアップを移動して置き換えるのではなく、**既存設定ファイルに内容を書き戻してください**。PowerShell で `$config` を Herdr が通知した設定パスに変更します。 + +```powershell +$ErrorActionPreference = 'Stop' +$config = 'C:\path\to\cli.json' +$backup = "$config.herdr-backup" +$bytes = [System.IO.File]::ReadAllBytes($backup) +$file = [System.IO.File]::Open($config, [System.IO.FileMode]::Open, [System.IO.FileAccess]::Write) +try { + $file.SetLength(0) + $file.Write($bytes, 0, $bytes.Length) + $file.Flush($true) +} finally { + $file.Dispose() +} +``` + +復旧に失敗したらバックアップを保持してください。設定が正しいことを確認した後、`Remove-Item -LiteralPath $backup` で完成済みバックアップを削除してから統合の更新を再試行します。元のファイルが存在しない場合は、意図したアクセス権も復元してください。新しいファイルを作るだけでは元の権限は保持されません。 +
## Herdr がインテグレーションをどう使うか 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 1a56863ec4..1cad193201 100644 --- a/docs/next/website/src/content/docs/zh-cn/integrations.mdx +++ b/docs/next/website/src/content/docs/zh-cn/integrations.mdx @@ -53,7 +53,34 @@ herdr integration uninstall antigravity-cli herdr integration uninstall grok ``` -共享的智能体配置会先写入临时文件，完整写入成功后才替换原文件。Herdr 会保留文件权限并跟随符号链接。修改钩子文件之前，Herdr 会检查配置文件是否有多个硬链接，如有则拒绝操作；请改用独立文件或符号链接后重试。后续失败（包括并发修改配置）仍可能留下之前已完成的安装变更。这是针对单个文件的不完整写入保护，不是整个安装的事务，也不保证断电后的持久性。 +在 Linux 和 macOS 上，共享的智能体配置会完整写入临时文件，再进行原子替换。Windows 上的现有配置则先备份，再原地更新，以保留文件所有者和访问规则；更新中断后可能需要恢复。所有平台的新文件都使用临时文件方式创建。Herdr 会跟随符号链接，并在修改钩子文件之前拒绝具有多个硬链接的配置；请改用独立文件或符号链接后重试。这是单文件保护，不是整个安装的事务、并发编辑控制或断电持久性保证。后续失败仍可能留下之前已完成的安装变更。 + +
+Windows 配置恢复 + +修改现有配置前，Herdr 会将原始内容保存并同步到私有的 `.herdr-backup` 文件。写入失败时会保留此恢复副本并报告两个路径。即使配置已丢失或无效，后续更新也不会覆盖尚未处理的恢复副本。对于现有的 EFS 加密配置，Herdr 会拒绝更新并保持原样，而不是创建明文备份。 + +`.herdr-backup.pending` 是未完成的备份，**不能作为恢复来源**。对应的原地写入尚未开始。请检查配置后再删除未完成文件并重试。更新成功后，如果清理失败或中断，完整备份也可能留下；仅凭备份存在不能判断当前配置已损坏。 + +需要恢复时，请先关闭智能体，将备份的**内容写回现有配置文件**，不要移动备份来替换它。在 PowerShell 中，将 `$config` 设置为 Herdr 报告的配置路径： + +```powershell +$ErrorActionPreference = 'Stop' +$config = 'C:\path\to\cli.json' +$backup = "$config.herdr-backup" +$bytes = [System.IO.File]::ReadAllBytes($backup) +$file = [System.IO.File]::Open($config, [System.IO.FileMode]::Open, [System.IO.FileAccess]::Write) +try { + $file.SetLength(0) + $file.Write($bytes, 0, $bytes.Length) + $file.Flush($true) +} finally { + $file.Dispose() +} +``` + +恢复失败时请保留备份。确认配置正确后，运行 `Remove-Item -LiteralPath $backup` 删除完整备份，再重试集成更新。如果原文件已丢失，还需恢复其预期权限；创建替代文件不会自动保留原权限。 +
## Herdr 如何使用集成 diff --git a/src/integration/config_file.rs b/src/integration/config_file.rs index 22924fdfe4..11e34c3d25 100644 --- a/src/integration/config_file.rs +++ b/src/integration/config_file.rs @@ -1,4 +1,4 @@ -//! Atomic writes for user-owned integration configuration, not managed assets. +//! Protected writes for user-owned integration configuration, not managed assets. use std::fs::{self, OpenOptions}; use std::io; @@ -12,13 +12,18 @@ 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 reject_hard_linked_configs(dir: &Path, names: &[&str]) -> io::Result<()> { +pub(super) fn check_config_targets(dir: &Path, names: &[&str]) -> io::Result<()> { for name in names { - reject_hard_links(&dir.join(name))?; + 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, @@ -65,7 +70,12 @@ fn resolve_target(path: &Path) -> io::Result { } pub(super) fn write_config(path: &Path, contents: impl AsRef<[u8]>) -> io::Result<()> { - let replacement = Replacement::prepare(path, contents.as_ref())?; + 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() } diff --git a/src/integration/config_file/tests.rs b/src/integration/config_file/tests.rs index 896434b350..916edc67b5 100644 --- a/src/integration/config_file/tests.rs +++ b/src/integration/config_file/tests.rs @@ -23,10 +23,16 @@ impl Drop for Directory { } } +// 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 [false, true] { + for &existing in ATOMIC_CASES { let path = dir.0.join(if existing { "existing" } else { "new" }); if existing { fs::write(&path, b"old preferences").unwrap(); @@ -44,22 +50,30 @@ fn config_publication_keeps_old_content_until_commit() { staged.commit().unwrap(); assert_eq!(fs::read(&path).unwrap(), b"complete new preferences"); } - assert_eq!(fs::read_dir(&dir.0).unwrap().count(), 2); + assert_eq!(fs::read_dir(&dir.0).unwrap().count(), ATOMIC_CASES.len()); } #[test] fn abandoned_and_failed_publication_leave_config_unchanged() { - let dir = Directory::new(); - let path = dir.0.join("config"); - 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); - assert_eq!(fs::read(&path).unwrap(), b"original"); - assert_eq!(fs::read_dir(&dir.0).unwrap().count(), 1); - assert!(write_config(&dir.0, b"not a file").is_err()); - assert!(dir.0.is_dir()); + 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] @@ -68,8 +82,10 @@ fn hard_links_are_rejected_before_staging_and_rechecked_before_commit() { 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() diff --git a/src/integration/opencode_config.rs b/src/integration/opencode_config.rs index b4a969889d..b062365b19 100644 --- a/src/integration/opencode_config.rs +++ b/src/integration/opencode_config.rs @@ -6,7 +6,7 @@ use jsonc_parser::cst::{CstInputValue, CstRootNode}; use jsonc_parser::ParseOptions; use serde_json::Value; -use super::config_file::write_config; +use super::config_file::{check_config_target, write_config}; const TUI_CONFIG_NAME: &str = "tui.jsonc"; @@ -46,6 +46,7 @@ pub(crate) fn add_cli_plugin( plugin_spec: &str, ) -> io::Result> { 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 @@ -62,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 { + check_config_target(&config_path)?; let content = if config_path.is_file() { fs::read_to_string(&config_path)? } else { @@ -105,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 { + check_config_target(config_path)?; if !config_path.is_file() { return Ok(false); } diff --git a/src/integration/targets.rs b/src/integration/targets.rs index 7c500733f3..cdd31317c4 100644 --- a/src/integration/targets.rs +++ b/src/integration/targets.rs @@ -19,7 +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::{reject_hard_linked_configs, write_config}; +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, mastracode_dir, omp_extension_dir, @@ -122,7 +122,7 @@ pub(crate) fn remove_legacy_pi_extension_from_omp_dir(dir: &Path) -> io::Result< pub(crate) fn install_claude() -> io::Result { let dir = claude_dir()?; - reject_hard_linked_configs(&dir, &["settings.json"])?; + 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", @@ -158,7 +158,7 @@ pub(crate) fn install_claude() -> io::Result { pub(crate) fn install_codex() -> io::Result { let dir = codex_dir()?; - reject_hard_linked_configs(&dir, &["hooks.json", "config.toml"])?; + 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", @@ -222,7 +222,7 @@ pub(crate) fn install_codex() -> io::Result { pub(crate) fn install_kimi() -> io::Result { let dir = kimi_dir()?; - reject_hard_linked_configs(&dir, &["config.toml"])?; + 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", @@ -257,7 +257,7 @@ pub(crate) fn install_kimi() -> io::Result { pub(crate) fn install_copilot() -> io::Result { let dir = copilot_dir()?; - reject_hard_linked_configs(&dir, &["settings.json"])?; + 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", @@ -312,7 +312,7 @@ pub(crate) fn install_copilot() -> io::Result { pub(crate) fn install_devin() -> io::Result { let dir = devin_dir()?; - reject_hard_linked_configs(&dir, &["config.json"])?; + 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", @@ -369,7 +369,7 @@ pub(crate) fn install_devin() -> io::Result { pub(crate) fn install_droid() -> io::Result { let dir = droid_dir()?; - reject_hard_linked_configs(&dir, &["settings.json", "hooks.json"])?; + 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", @@ -460,7 +460,7 @@ pub(crate) fn install_droid() -> io::Result { pub(crate) fn install_opencode() -> io::Result { let dir = opencode_dir()?; - reject_hard_linked_configs(&dir, &["tui.jsonc", "cli.json"])?; + 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", @@ -514,7 +514,7 @@ pub(crate) fn install_kilo() -> io::Result { pub(crate) fn install_hermes() -> io::Result { let dir = hermes_dir()?; - reject_hard_linked_configs(&dir, &["config.yaml"])?; + 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", @@ -572,7 +572,7 @@ pub(crate) fn uninstall_omp() -> io::Result { pub(crate) fn uninstall_claude() -> io::Result { let dir = claude_dir()?; - reject_hard_linked_configs(&dir, &["settings.json"])?; + 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; @@ -600,7 +600,7 @@ pub(crate) fn uninstall_claude() -> io::Result { pub(crate) fn uninstall_codex() -> io::Result { let codex_dir = codex_dir()?; - reject_hard_linked_configs(&codex_dir, &["hooks.json"])?; + 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"); @@ -649,7 +649,7 @@ pub(crate) fn uninstall_codex() -> io::Result { pub(crate) fn uninstall_kimi() -> io::Result { let kimi_dir = kimi_dir()?; - reject_hard_linked_configs(&kimi_dir, &["config.toml"])?; + 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; @@ -676,7 +676,7 @@ pub(crate) fn uninstall_kimi() -> io::Result { pub(crate) fn uninstall_copilot() -> io::Result { let copilot_dir = copilot_dir()?; - reject_hard_linked_configs(&copilot_dir, &["settings.json"])?; + 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; @@ -722,7 +722,7 @@ pub(crate) fn uninstall_copilot() -> io::Result { pub(crate) fn uninstall_devin() -> io::Result { let devin_dir = devin_dir()?; - reject_hard_linked_configs(&devin_dir, &["config.json"])?; + 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; @@ -768,7 +768,7 @@ pub(crate) fn uninstall_devin() -> io::Result { pub(crate) fn uninstall_droid() -> io::Result { let droid_dir = droid_dir()?; - reject_hard_linked_configs(&droid_dir, &["settings.json", "hooks.json"])?; + 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"); @@ -843,7 +843,7 @@ pub(crate) fn uninstall_droid() -> io::Result { pub(crate) fn uninstall_opencode() -> io::Result { let dir = opencode_dir()?; - reject_hard_linked_configs(&dir, &["tui.jsonc", "cli.json"])?; + 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); @@ -899,7 +899,7 @@ pub(crate) fn uninstall_kilo() -> io::Result { pub(crate) fn uninstall_hermes() -> io::Result { let dir = hermes_dir()?; - reject_hard_linked_configs(&dir, &["config.yaml"])?; + check_config_targets(&dir, &["config.yaml"])?; let plugin_dir = hermes_plugin_dir()?; let config_path = dir.join("config.yaml"); @@ -924,7 +924,7 @@ pub(crate) fn uninstall_hermes() -> io::Result { pub(crate) fn install_qodercli() -> io::Result { let dir = qodercli_dir()?; - reject_hard_linked_configs(&dir, &["settings.json"])?; + 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", @@ -989,7 +989,7 @@ pub(crate) fn install_qodercli() -> io::Result { pub(crate) fn install_qwen() -> io::Result { let dir = qwen_dir()?; - reject_hard_linked_configs(&dir, &["settings.json"])?; + 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", @@ -1043,7 +1043,7 @@ pub(crate) fn install_qwen() -> io::Result { pub(crate) fn install_cursor() -> io::Result { let dir = cursor_dir()?; - reject_hard_linked_configs(&dir, &["hooks.json"])?; + 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", @@ -1100,7 +1100,7 @@ pub(crate) fn install_cursor() -> io::Result { pub(crate) fn uninstall_qodercli() -> io::Result { let dir = qodercli_dir()?; - reject_hard_linked_configs(&dir, &["settings.json"])?; + 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; @@ -1146,7 +1146,7 @@ pub(crate) fn uninstall_qodercli() -> io::Result { pub(crate) fn uninstall_qwen() -> io::Result { let dir = qwen_dir()?; - reject_hard_linked_configs(&dir, &["settings.json"])?; + 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; @@ -1188,7 +1188,7 @@ pub(crate) fn uninstall_qwen() -> io::Result { pub(crate) fn uninstall_cursor() -> io::Result { let cursor_home = cursor_dir()?; - reject_hard_linked_configs(&cursor_home, &["hooks.json"])?; + 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; @@ -1245,7 +1245,7 @@ pub(crate) fn mastracode_hook_command(hook_path: &Path, action: &str) -> String pub(crate) fn install_mastracode() -> io::Result { let mastracode_home = mastracode_dir()?; - reject_hard_linked_configs(&mastracode_home, &["hooks.json"])?; + check_config_targets(&mastracode_home, &["hooks.json"])?; let hook_dir = mastracode_home.join("hooks"); fs::create_dir_all(&hook_dir)?; @@ -1293,7 +1293,7 @@ pub(crate) fn install_mastracode() -> io::Result { pub(crate) fn uninstall_mastracode() -> io::Result { let mastracode_home = mastracode_dir()?; - reject_hard_linked_configs(&mastracode_home, &["hooks.json"])?; + check_config_targets(&mastracode_home, &["hooks.json"])?; let hook_path = mastracode_home .join("hooks") .join(MASTRACODE_HOOK_INSTALL_NAME); @@ -1342,7 +1342,7 @@ pub(crate) fn uninstall_mastracode() -> io::Result { pub(crate) fn install_antigravity_cli() -> io::Result { let dir = antigravity_cli_dir()?; - reject_hard_linked_configs(&dir, &["hooks.json"])?; + 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", @@ -1418,7 +1418,7 @@ fn antigravity_cli_hook_block(hook_path: &Path) -> Value { pub(crate) fn uninstall_antigravity_cli() -> io::Result { let dir = antigravity_cli_dir()?; - reject_hard_linked_configs(&dir, &["hooks.json"])?; + 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; diff --git a/src/integration/tests.rs b/src/integration/tests.rs index e72202573c..7b223d6312 100644 --- a/src/integration/tests.rs +++ b/src/integration/tests.rs @@ -2415,6 +2415,57 @@ fn opencode_hard_link_rejection_precedes_install_and_uninstall_asset_changes() { 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(); diff --git a/src/platform/linux.rs b/src/platform/linux.rs index 016c21597e..16cd60e49b 100644 --- a/src/platform/linux.rs +++ b/src/platform/linux.rs @@ -44,6 +44,18 @@ pub(crate) fn config_file_link_count(path: &std::path::Path) -> std::io::Result< 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 { + // Unix keeps atomic replacement for existing files too. + Ok(false) +} + pub(crate) fn create_config_temporary( path: &std::path::Path, private: bool, diff --git a/src/platform/macos.rs b/src/platform/macos.rs index 80cd48af87..83c493d889 100644 --- a/src/platform/macos.rs +++ b/src/platform/macos.rs @@ -28,6 +28,18 @@ pub(crate) fn config_file_link_count(path: &Path) -> std::io::Result { 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 { + // Unix keeps atomic replacement for existing files too. + Ok(false) +} + pub(crate) fn create_config_temporary( path: &Path, private: bool, diff --git a/src/platform/windows.rs b/src/platform/windows.rs index d00a957862..4f96ec7709 100644 --- a/src/platform/windows.rs +++ b/src/platform/windows.rs @@ -14,11 +14,7 @@ use std::{ }; mod clipboard_image; -// Validate the recovery-backed candidate natively before routing config writes to it. -#[cfg(test)] mod config_backup; -#[cfg(test)] -mod config_file_tests; 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. @@ -209,110 +205,34 @@ pub(crate) fn write_config_temporary( contents: &[u8], ) -> std::io::Result<()> { use std::io::Write; - use std::os::windows::fs::OpenOptionsExt; - use windows_sys::Win32::{ - Foundation::GENERIC_WRITE, - Security::{ - Authorization::{SetSecurityInfo, SE_FILE_OBJECT}, - GetSecurityDescriptorControl, GetSecurityDescriptorDacl, GetSecurityDescriptorGroup, - GetSecurityDescriptorOwner, GetSecurityDescriptorSacl, DACL_SECURITY_INFORMATION, - GROUP_SECURITY_INFORMATION, LABEL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, - PROTECTED_DACL_SECURITY_INFORMATION, SE_DACL_PROTECTED, - UNPROTECTED_DACL_SECURITY_INFORMATION, - }, - Storage::FileSystem::{WRITE_DAC, WRITE_OWNER}, - }; - let mut options = std::fs::OpenOptions::new(); - options.write(true).truncate(true); if source.is_some() { - // TRUNCATE_EXISTING requires the GENERIC_WRITE bit, not its mapped - // FILE_GENERIC_WRITE rights, even though those grant equivalent access. - options.access_mode(GENERIC_WRITE | WRITE_DAC | WRITE_OWNER); - } - let mut output = options.open(temporary)?; - if let Some(source) = source { - let information = OWNER_SECURITY_INFORMATION - | GROUP_SECURITY_INFORMATION - | DACL_SECURITY_INFORMATION - | LABEL_SECURITY_INFORMATION; - let mut descriptor = config_security_descriptor(source, information)?; - let expected = config_security_sddl(&mut descriptor, information)?; - let mut control = 0; - let mut revision = 0; - if unsafe { - GetSecurityDescriptorControl( - descriptor.as_mut_ptr().cast(), - &mut control, - &mut revision, - ) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } - let protection = if control & SE_DACL_PROTECTED != 0 { - PROTECTED_DACL_SECURITY_INFORMATION - } else { - UNPROTECTED_DACL_SECURITY_INFORMATION - }; - let mut owner = null_mut(); - let mut group = null_mut(); - let mut dacl = null_mut(); - let mut sacl = null_mut(); - let mut defaulted = 0; - let mut present = 0; - let descriptor = descriptor.as_mut_ptr().cast(); - if unsafe { GetSecurityDescriptorOwner(descriptor, &mut owner, &mut defaulted) } == 0 - || unsafe { GetSecurityDescriptorGroup(descriptor, &mut group, &mut defaulted) } == 0 - || unsafe { - GetSecurityDescriptorDacl(descriptor, &mut present, &mut dacl, &mut defaulted) - } == 0 - || unsafe { - GetSecurityDescriptorSacl(descriptor, &mut present, &mut sacl, &mut defaulted) - } == 0 - { - return Err(std::io::Error::last_os_error()); - } - // Files require SetSecurityInfo, not SetKernelObjectSecurity: the latter - // drops the filesystem ACL's automatic-inheritance metadata. - let error = unsafe { - SetSecurityInfo( - output.as_raw_handle(), - SE_FILE_OBJECT, - information | protection, - owner, - group, - dacl, - sacl, - ) - }; - if error != 0 { - return Err(std::io::Error::from_raw_os_error(error as i32)); - } - let mut installed = config_security_descriptor(temporary, information)?; - let installed = config_security_sddl(&mut installed, information)?; - if installed != expected { - #[cfg(test)] - eprintln!( - "config ACL mismatch: expected {}, installed {}", - String::from_utf16_lossy(&expected), - String::from_utf16_lossy(&installed), - ); - // An unprotected file moved from another directory can retain old - // inherited permissions. Never put secrets into a temporary whose - // new parent added access, even if publication would be rejected. - return Err(std::io::Error::other( - "cannot preserve config access controls during atomic replacement", - )); - } - // Only copy sensitive contents/streams after access controls match. - // CopyFile preserves attributes, encryption and alternate streams. - std::fs::copy(source, temporary)?; - output.set_len(0)?; + // 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 { + config_backup::write_existing(target, contents) +} + +#[cfg(test)] fn config_security_descriptor( path: &std::path::Path, information: windows_sys::Win32::Security::OBJECT_SECURITY_INFORMATION, @@ -340,6 +260,7 @@ fn config_security_descriptor( Ok(descriptor) } +#[cfg(test)] fn config_security_sddl( descriptor: &mut [u8], information: windows_sys::Win32::Security::OBJECT_SECURITY_INFORMATION, diff --git a/src/platform/windows/config_file_tests.rs b/src/platform/windows/config_file_tests.rs deleted file mode 100644 index 24ca325daf..0000000000 --- a/src/platform/windows/config_file_tests.rs +++ /dev/null @@ -1,110 +0,0 @@ -use super::*; - -fn powershell(script: &str, source: &std::path::Path) -> String { - let output = std::process::Command::new("powershell.exe") - .args(["-NoProfile", "-NonInteractive", "-Command", script]) - .env("HERDR_TEST_CONFIG_SOURCE", source) - .output() - .unwrap(); - assert!(output.status.success(), "{output:?}"); - assert!(output.stderr.is_empty(), "{output:?}"); - String::from_utf8(output.stdout).unwrap() -} - -#[test] -fn config_replacement_preserves_windows_access_control() { - 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 target = dir.join("temporary"); - std::fs::write(&source, b"original").unwrap(); - powershell( - r#" -$ErrorActionPreference = 'Stop' -# Use the .NET Framework API directly: a parent pwsh process can pass a -# PSModulePath containing incompatible PowerShell 7 versions of Get-Acl/Set-Acl. -$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) -$acl.SetAccessRuleProtection($true, $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) -"#, - &source, - ); - let snapshot = r#"$ErrorActionPreference = 'Stop'; [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE).GetSecurityDescriptorSddlForm([System.Security.AccessControl.AccessControlSections]::All)"#; - for protected in [true, false] { - if !protected { - powershell( - r#" -$ErrorActionPreference = 'Stop' -$acl = [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE) -$acl.SetAccessRuleProtection($false, $false) -[System.IO.File]::SetAccessControl($env:HERDR_TEST_CONFIG_SOURCE, $acl) -"#, - &source, - ); - } - let before = powershell(snapshot, &source); - drop(create_config_temporary(&target, true).unwrap()); - write_config_temporary(Some(&source), &target, b"new").unwrap(); - replace_file(&target, &source).unwrap(); - assert_eq!( - powershell(snapshot, &source), - before, - "protected={protected}" - ); - assert_eq!(std::fs::read(&source).unwrap(), b"new"); - } - std::fs::remove_dir_all(dir).unwrap(); -} - -#[test] -fn config_replacement_rejects_changed_inherited_access_before_copying_contents() { - let dir = std::env::temp_dir().join(format!("herdr-config-moved-acl-{}", std::process::id())); - std::fs::create_dir(&dir).unwrap(); - let parent = dir.join("different-parent"); - std::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 source = dir.join("source"); - std::fs::write(&source, b"private preferences").unwrap(); - std::fs::write(source.with_file_name("source:private"), b"private stream").unwrap(); - let snapshot = r#"$ErrorActionPreference = 'Stop'; [System.IO.File]::GetAccessControl($env:HERDR_TEST_CONFIG_SOURCE).GetSecurityDescriptorSddlForm([System.Security.AccessControl.AccessControlSections]::All)"#; - let before = powershell(snapshot, &source); - assert!(!before.contains(";;;BG)"), "{before}"); - assert!( - !before.contains("D:P"), - "source must be unprotected: {before}" - ); - let moved = parent.join("source"); - std::fs::rename(&source, &moved).unwrap(); - assert_eq!(powershell(snapshot, &moved), before, "move retains old ACL"); - let probe = parent.join("inherited"); - std::fs::write(&probe, b"").unwrap(); - assert!(powershell(snapshot, &probe).contains(";;;BG)")); - let target = parent.join("temporary"); - drop(create_config_temporary(&target, true).unwrap()); - let error = write_config_temporary(Some(&moved), &target, b"new").unwrap_err(); - assert!(error - .to_string() - .contains("cannot preserve config access controls")); - assert!(std::fs::read(&target).unwrap().is_empty()); - assert!(!target.with_file_name("temporary:private").exists()); - assert_eq!(std::fs::read(&moved).unwrap(), b"private preferences"); - assert_eq!( - std::fs::read(moved.with_file_name("source:private")).unwrap(), - b"private stream" - ); - assert_eq!(powershell(snapshot, &moved), before); - std::fs::remove_dir_all(dir).unwrap(); -}