From 4b2362d18a51fccb8fff60b7dd08deee59c4039c Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Tue, 14 Jul 2026 02:43:33 +0530 Subject: [PATCH 1/2] fix(fs): crash-safe atomic writes for auth vault and shell profile files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit auth.rs's activate_with/activate_into write the agent's LIVE credential file (~/.claude/.credentials.json etc.) via plain fs::write, and alias.rs's write_managed_block writes directly into the user's real shell rc file. Neither is crash-atomic: a process killed mid-write (OOM kill, SIGKILL, power loss) leaves a truncated or zero-byte file in place of the original — for auth.rs that's a corrupted credential file breaking the agent's login; for alias.rs it's data loss in the user's own .bashrc/.zshrc. Adds src/atomic_fs.rs (ported from lean-ctx's core/atomic_fs.rs): same-directory temp file + rename for crash-atomicity, with an in-place-overwrite fallback for read-only-directory/writable-inode cases. Wires it into both write sites. libc added as a unix-only dependency for the O_NOFOLLOW/errno pieces of the fallback path (both already conditionally compiled per-platform; inert on Windows). --- Cargo.lock | 1 + Cargo.toml | 3 + src/alias.rs | 6 +- src/atomic_fs.rs | 228 +++++++++++++++++++++++++++++++++++++++++++++++ src/auth.rs | 11 +-- src/main.rs | 1 + 6 files changed, 243 insertions(+), 7 deletions(-) create mode 100644 src/atomic_fs.rs diff --git a/Cargo.lock b/Cargo.lock index 382a7b32..27d2b3b8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -84,6 +84,7 @@ dependencies = [ "flate2", "hex", "insta", + "libc", "pbkdf2", "ponytail", "pretty_assertions", diff --git a/Cargo.toml b/Cargo.toml index 5079b68d..c72bfc4a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,6 +71,9 @@ agentflare-backend = { package = "agentflare-backend", path = "crates/agentflare db_kit = { package = "agentflare-db-kit", path = "crates/agentflare-db-kit" } agent-detector = "0.2.1" +[target.'cfg(unix)'.dependencies] +libc = "0.2" + [features] default = [] process-tree = ["sysinfo"] diff --git a/src/alias.rs b/src/alias.rs index 6bb1a82f..bc9aae84 100644 --- a/src/alias.rs +++ b/src/alias.rs @@ -292,7 +292,8 @@ fn write_managed_block( end += 1; } let new_content = content[..start].to_string() + &new_block + &content[end..]; - std::fs::write(profile, new_content)?; + crate::atomic_fs::write_bytes_with_fallback(profile, new_content.as_bytes(), None) + .map_err(std::io::Error::other)?; return Ok(()); } @@ -301,7 +302,8 @@ fn write_managed_block( new_content.push('\n'); } new_content.push_str(&new_block); - std::fs::write(profile, new_content)?; + crate::atomic_fs::write_bytes_with_fallback(profile, new_content.as_bytes(), None) + .map_err(std::io::Error::other)?; Ok(()) } diff --git a/src/atomic_fs.rs b/src/atomic_fs.rs new file mode 100644 index 00000000..fdc0e7d6 --- /dev/null +++ b/src/atomic_fs.rs @@ -0,0 +1,228 @@ +//! Shared, policy-free atomic-write mechanics. +//! +//! agentflare writes files whose truncation on a crash mid-write would be a +//! real problem — auth vault credential files (`auth.rs`) and the user's own +//! shell profile (`alias.rs`'s managed alias block) — using plain +//! `fs::write`, which is not crash-atomic: a process killed between the +//! `open` and the `write` completing leaves a truncated (or zero-byte) file +//! in place of the original. +//! +//! This module provides one audited mechanism: a same-directory temp file + +//! `rename`, with an in-place-overwrite fallback when the directory is +//! read-only but the file inode itself is writable. Ported from lean-ctx's +//! `core/atomic_fs.rs`. + +use std::path::Path; +use std::time::{SystemTime, UNIX_EPOCH}; + +fn invalid_input(msg: &'static str) -> std::io::Error { + std::io::Error::new(std::io::ErrorKind::InvalidInput, msg) +} + +/// Durable, crash-atomic write: a temp file in the **same directory** as `path` +/// followed by `rename` over the target. Requires write permission on the parent +/// directory; the read-only-directory fallback is handled by +/// [`write_bytes_with_fallback`]. +pub fn try_atomic_write( + path: &Path, + bytes: &[u8], + permissions: Option<&std::fs::Permissions>, +) -> std::io::Result<()> { + use std::io::Write; + + let parent = path + .parent() + .ok_or_else(|| invalid_input("invalid path (no parent directory)"))?; + let filename = path + .file_name() + .ok_or_else(|| invalid_input("invalid path (no filename)"))? + .to_string_lossy(); + + let pid = std::process::id(); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map_or(0, |d| d.as_nanos()); + let tmp = parent.join(format!(".{filename}.agentflare.tmp.{pid}.{nanos}")); + + { + let mut f = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&tmp)?; + f.write_all(bytes)?; + let _ = f.flush(); + let _ = f.sync_all(); + } + + if let Some(perms) = permissions { + let _ = std::fs::set_permissions(&tmp, perms.clone()); + } + + #[cfg(windows)] + { + if path.exists() { + let _ = std::fs::remove_file(path); + } + } + + if let Err(e) = std::fs::rename(&tmp, path) { + // Don't leave a half-written temp behind before the caller decides + // whether to fall back. + let _ = std::fs::remove_file(&tmp); + return Err(e); + } + Ok(()) +} + +/// In-place overwrite of an existing file inode (`O_WRONLY|O_TRUNC`, plus +/// `O_NOFOLLOW` on Unix). Works when the parent directory is read-only but the +/// file itself is writable. Not crash-atomic — used only as a fallback when the +/// atomic path is impossible. +pub fn in_place_overwrite( + path: &Path, + bytes: &[u8], + permissions: Option<&std::fs::Permissions>, +) -> std::io::Result<()> { + use std::io::Write; + + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).truncate(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + // O_NOFOLLOW: a symlink swapped in after the caller's checks must never + // be followed here. + opts.custom_flags(libc::O_NOFOLLOW); + } + + let mut f = opts.open(path)?; + f.write_all(bytes)?; + let _ = f.flush(); + let _ = f.sync_all(); + + if let Some(perms) = permissions { + let _ = std::fs::set_permissions(path, perms.clone()); + } + Ok(()) +} + +/// True for errors that mean "this directory won't accept create/rename" even +/// though the target file may be writable: `EROFS` (read-only fs) plus +/// `EACCES`/`EPERM` (directory write denied). +pub fn is_readonly_dir_error(e: &std::io::Error) -> bool { + if e.kind() == std::io::ErrorKind::PermissionDenied { + return true; + } + #[cfg(unix)] + { + matches!( + e.raw_os_error(), + Some(libc::EROFS | libc::EACCES | libc::EPERM) + ) + } + #[cfg(not(unix))] + { + false + } +} + +/// Atomic write with the read-only-directory in-place fallback. Tries the +/// crash-atomic temp+rename first; if that fails because the *directory* is +/// read-only/permission-denied but an existing file inode is writable, overwrite +/// it in place. `permissions`, when given, is applied to the written file. +pub fn write_bytes_with_fallback( + path: &Path, + bytes: &[u8], + permissions: Option<&std::fs::Permissions>, +) -> Result<(), String> { + match try_atomic_write(path, bytes, permissions) { + Ok(()) => Ok(()), + Err(e) if is_readonly_dir_error(&e) && path.is_file() => { + in_place_overwrite(path, bytes, permissions).map_err(|fallback_err| { + format!( + "atomic write failed ({e}); in-place fallback also failed: {fallback_err} ({})", + path.display() + ) + }) + } + Err(e) => Err(format!("atomic write failed: {e} ({})", path.display())), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn readonly_dir_error_classification() { + assert!(is_readonly_dir_error(&std::io::Error::from( + std::io::ErrorKind::PermissionDenied + ))); + assert!(!is_readonly_dir_error(&std::io::Error::from( + std::io::ErrorKind::NotFound + ))); + #[cfg(unix)] + { + assert!(is_readonly_dir_error(&std::io::Error::from_raw_os_error( + libc::EROFS + ))); + assert!(is_readonly_dir_error(&std::io::Error::from_raw_os_error( + libc::EACCES + ))); + assert!(is_readonly_dir_error(&std::io::Error::from_raw_os_error( + libc::EPERM + ))); + } + } + + #[test] + fn try_atomic_write_creates_and_replaces() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cfg.toml"); + try_atomic_write(&path, b"first", None).unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), b"first"); + // No leftover temp files. + let strays: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .flatten() + .filter(|e| e.file_name().to_string_lossy().contains(".agentflare.tmp.")) + .collect(); + assert!(strays.is_empty(), "temp file must not linger"); + try_atomic_write(&path, b"second", None).unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), b"second"); + } + + #[cfg(unix)] + #[test] + fn in_place_overwrite_truncates_existing_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.jsonc"); + std::fs::write(&path, b"longer original content").unwrap(); + in_place_overwrite(&path, b"short", None).unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), b"short"); + } + + #[cfg(unix)] + #[test] + fn fallback_overwrites_when_parent_dir_is_readonly() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cfg.toml"); + std::fs::write(&path, b"original").unwrap(); + // Read-only parent dir: temp+rename is impossible, but the file inode + // stays writable, so the in-place fallback must succeed. + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o500)).unwrap(); + let res = write_bytes_with_fallback(&path, b"updated", None); + let _ = std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700)); + res.expect("read-only-dir fallback must succeed"); + assert_eq!(std::fs::read(&path).unwrap(), b"updated"); + } + + #[test] + fn write_bytes_with_fallback_creates_new_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("new.txt"); + write_bytes_with_fallback(&path, b"hello", None).unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), b"hello"); + } +} diff --git a/src/auth.rs b/src/auth.rs index 296c0aea..2ff8fbac 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -187,7 +187,8 @@ pub fn activate_with(agent: &str, profile: &str, reload_daemon: bool, json: bool if auth_crypt::is_encrypted(&data) { if let Some(ref pw) = passphrase { if let Some(decrypted) = auth_crypt::decrypt(&data, pw) { - fs::write(&dest, decrypted).expect("write"); + crate::atomic_fs::write_bytes_with_fallback(&dest, &decrypted, None) + .expect("write"); } else { eprintln!( "warning: cannot decrypt {} — wrong passphrase", @@ -203,7 +204,7 @@ pub fn activate_with(agent: &str, profile: &str, reload_daemon: bool, json: bool continue; } } else { - fs::write(&dest, data).expect("write"); + crate::atomic_fs::write_bytes_with_fallback(&dest, &data, None).expect("write"); } restored += 1; } @@ -1037,7 +1038,6 @@ fn read_isolate_mode(dir: &std::path::Path) -> Option { } None } - fn activate_into(agent: &str, profile: &str, target_dir: &std::path::Path) { let cat = match catalog_for(agent) { Some(c) => c, @@ -1058,7 +1058,8 @@ fn activate_into(agent: &str, profile: &str, target_dir: &std::path::Path) { if auth_crypt::is_encrypted(&data) { if let Some(ref pw) = passphrase { if let Some(decrypted) = auth_crypt::decrypt(&data, pw) { - fs::write(&dest, decrypted).expect("write"); + crate::atomic_fs::write_bytes_with_fallback(&dest, &decrypted, None) + .expect("write"); } else { eprintln!( "warning: cannot decrypt {} — wrong passphrase", @@ -1074,7 +1075,7 @@ fn activate_into(agent: &str, profile: &str, target_dir: &std::path::Path) { continue; } } else { - fs::write(&dest, data).expect("write"); + crate::atomic_fs::write_bytes_with_fallback(&dest, &data, None).expect("write"); } } } diff --git a/src/main.rs b/src/main.rs index 1387e37d..b19c6ff1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,6 +3,7 @@ mod agent_launch; mod agents; mod alias; mod artifacts; +mod atomic_fs; mod auth; mod auth_crypt; mod auth_db; From 6e246e92028fb9b88f0121edc9fed199d70f7205 Mon Sep 17 00:00:00 2001 From: Shivakumar Date: Tue, 14 Jul 2026 03:13:28 +0530 Subject: [PATCH 2/2] fix(fs): address atomic_fs review findings (permissions, symlinks, Windows rename) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues from review of the crash-safe write module: - try_atomic_write created the replacement inode with default (umask-masked) permissions when permissions=None, which every call site passes — a 0600 credential file being rewritten would silently become world-readable. Now preserves the existing file's permissions when none are explicitly given. - rename() replaces a symlink *path* with a regular file rather than writing through it, unlike the plain fs::write this module replaced. A symlinked .bashrc or credential file (common for dotfiles setups) would silently lose its symlink. Now resolves one level of symlink indirection first and performs the atomic write at the resolved target, matching the original write-through behavior while keeping the symlink itself intact. - Removed the Windows pre-removal-before-rename step: std::fs::rename already replaces an existing destination on Windows (MoveFileExW with MOVEFILE_REPLACE_EXISTING), so the explicit remove_file only opened a window where neither the old nor new file existed. New tests: permission preservation on replace, write-through-a-symlink. --- src/atomic_fs.rs | 95 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 85 insertions(+), 10 deletions(-) diff --git a/src/atomic_fs.rs b/src/atomic_fs.rs index fdc0e7d6..38aa289b 100644 --- a/src/atomic_fs.rs +++ b/src/atomic_fs.rs @@ -12,6 +12,7 @@ //! read-only but the file inode itself is writable. Ported from lean-ctx's //! `core/atomic_fs.rs`. +use std::borrow::Cow; use std::path::Path; use std::time::{SystemTime, UNIX_EPOCH}; @@ -19,10 +20,40 @@ fn invalid_input(msg: &'static str) -> std::io::Error { std::io::Error::new(std::io::ErrorKind::InvalidInput, msg) } +/// Resolves one level of symlink indirection so an atomic write lands on the +/// real file `path` points at, not on a new plain file replacing the symlink +/// itself — `rename()` does not follow a symlink destination, it unlinks it +/// and puts the new file in its place. A relative link target is resolved +/// against the symlink's own parent directory. Non-symlinks (and symlinks +/// this can't stat, e.g. a dangling one) pass through unchanged. +fn resolve_symlink_target(path: &Path) -> Cow<'_, Path> { + let Ok(meta) = std::fs::symlink_metadata(path) else { + return Cow::Borrowed(path); + }; + if !meta.file_type().is_symlink() { + return Cow::Borrowed(path); + } + let Ok(target) = std::fs::read_link(path) else { + return Cow::Borrowed(path); + }; + if target.is_absolute() { + Cow::Owned(target) + } else { + let parent = path.parent().unwrap_or_else(|| Path::new(".")); + Cow::Owned(parent.join(target)) + } +} + /// Durable, crash-atomic write: a temp file in the **same directory** as `path` -/// followed by `rename` over the target. Requires write permission on the parent -/// directory; the read-only-directory fallback is handled by -/// [`write_bytes_with_fallback`]. +/// (after resolving one level of symlink, so a symlinked target is written +/// through rather than replaced) followed by `rename` over the target. +/// Requires write permission on the parent directory; the read-only-directory +/// fallback is handled by [`write_bytes_with_fallback`]. +/// +/// When `permissions` is `None` and the target already exists, its current +/// permissions are carried over to the replacement — otherwise the new inode +/// would get the process's default (umask-masked) mode, silently loosening +/// e.g. a `0600` credential file to world-readable. pub fn try_atomic_write( path: &Path, bytes: &[u8], @@ -30,6 +61,9 @@ pub fn try_atomic_write( ) -> std::io::Result<()> { use std::io::Write; + let resolved = resolve_symlink_target(path); + let path: &Path = &resolved; + let parent = path .parent() .ok_or_else(|| invalid_input("invalid path (no parent directory)"))?; @@ -38,6 +72,15 @@ pub fn try_atomic_write( .ok_or_else(|| invalid_input("invalid path (no filename)"))? .to_string_lossy(); + let owned_perms; + let permissions = match permissions { + Some(p) => Some(p), + None => { + owned_perms = std::fs::metadata(path).ok().map(|m| m.permissions()); + owned_perms.as_ref() + } + }; + let pid = std::process::id(); let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -58,13 +101,10 @@ pub fn try_atomic_write( let _ = std::fs::set_permissions(&tmp, perms.clone()); } - #[cfg(windows)] - { - if path.exists() { - let _ = std::fs::remove_file(path); - } - } - + // std::fs::rename already replaces an existing destination on every + // platform we build for (including Windows, via MoveFileExW's + // MOVEFILE_REPLACE_EXISTING) — no separate pre-removal needed, and one + // would only open a window where neither the old nor new file exists. if let Err(e) = std::fs::rename(&tmp, path) { // Don't leave a half-written temp behind before the caller decides // whether to fall back. @@ -192,6 +232,41 @@ mod tests { assert_eq!(std::fs::read(&path).unwrap(), b"second"); } + #[cfg(unix)] + #[test] + fn try_atomic_write_preserves_existing_permissions_when_none_given() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secret.json"); + std::fs::write(&path, b"original").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + try_atomic_write(&path, b"updated", None).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o600, + "replacement must keep the original file's mode" + ); + } + + #[cfg(unix)] + #[test] + fn try_atomic_write_writes_through_a_symlink() { + use std::os::unix::fs::symlink; + let dir = tempfile::tempdir().unwrap(); + let real = dir.path().join("real.txt"); + let link = dir.path().join("link.txt"); + std::fs::write(&real, b"original").unwrap(); + symlink(&real, &link).unwrap(); + + try_atomic_write(&link, b"updated", None).unwrap(); + + assert!(link.is_symlink(), "the symlink itself must survive"); + assert_eq!(std::fs::read_link(&link).unwrap(), real); + assert_eq!(std::fs::read(&real).unwrap(), b"updated"); + } + #[cfg(unix)] #[test] fn in_place_overwrite_truncates_existing_file() {