From 5b76c95d25ec2ec9329c19ab0d6286cf6fc40ee9 Mon Sep 17 00:00:00 2001 From: Larry Klosowski Date: Thu, 22 Oct 2026 18:47:41 -0700 Subject: [PATCH 1/3] test(agent-code): file_write/file_edit refuse symlink and multiply-linked targets (red) Drives the real FileWrite/FileEdit entry points against a private dgx-fw- temp tree: final and intermediate symlinks (outside file, outside dir, dangling, inside the workspace), hard links, and bounded swap races on the final and an intermediate component. 11 of 16 fail on an assertion against main. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KGMeatV8UjtCB4CTbWuAv4 --- agent-code/tests/file_write_symlink.rs | 509 +++++++++++++++++++++++++ 1 file changed, 509 insertions(+) create mode 100644 agent-code/tests/file_write_symlink.rs diff --git a/agent-code/tests/file_write_symlink.rs b/agent-code/tests/file_write_symlink.rs new file mode 100644 index 0000000..08e1326 --- /dev/null +++ b/agent-code/tests/file_write_symlink.rs @@ -0,0 +1,509 @@ +//! `file_write` / `file_edit` must not write through a symbolic link, and must +//! not write to a file that is also reachable from outside the workspace. +//! +//! Every test drives the real tool entry point (`AgentTool::execute`) against a +//! private temporary tree created with a unique `dgx-fw-` prefix. The "outside" +//! file each test protects lives in that same tree (a sibling of the +//! workspace), never at a real host path, and is removed with the tree when the +//! `TempDir` drops. +//! +//! Policy pinned here (documented in `agent-code/src/tools/safe_write.rs`): +//! * a symlink as the final path component is refused, wherever it points +//! (outside file, outside directory, dangling, or another file inside the +//! workspace); +//! * a symlink as an intermediate directory component is refused, wherever it +//! points; +//! * a regular file with more than one hard link is refused, since the other +//! name may be outside the workspace; +//! * swapping the target between a regular file and a symlink while writes are +//! running never lets a write land outside. +#![cfg(unix)] + +use citrate_agent_code::tools::{FileEdit, FileWrite}; +use citrate_agent_core::error::AgentError; +use citrate_agent_core::tool::{AgentTool, ToolContext, ToolResult}; +use std::os::unix::fs::symlink; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::Arc; + +const ORIGINAL: &str = "ORIGINAL-CONTENT"; +const PAYLOAD: &str = "WRITTEN-BY-TOOL"; + +/// A private tree: `/ws` is the workspace, `/outside` is a sibling +/// directory holding `victim.txt` (the file no write may change). +struct Tree { + _root: tempfile::TempDir, + ws: PathBuf, + outside: PathBuf, + victim: PathBuf, +} + +fn tree() -> Tree { + let root = tempfile::Builder::new() + .prefix("dgx-fw-") + .tempdir() + .expect("create temp tree"); + let ws = root.path().join("ws"); + let outside = root.path().join("outside"); + std::fs::create_dir(&ws).expect("mkdir ws"); + std::fs::create_dir(&outside).expect("mkdir outside"); + let victim = outside.join("victim.txt"); + std::fs::write(&victim, ORIGINAL).expect("write victim"); + Tree { + _root: root, + ws, + outside, + victim, + } +} + +fn ctx(ws: &Path) -> ToolContext { + ToolContext { + session_id: "file-write-symlink-test".to_string(), + wallet_address: None, + chain_id: 40204, + workspace_dir: ws.to_str().expect("utf-8 temp path").to_string(), + } +} + +fn refused(r: &Result) -> bool { + match r { + Err(_) => true, + Ok(res) => !res.success, + } +} + +async fn write(ws: &Path, path: &str) -> Result { + FileWrite + .execute( + serde_json::json!({ "path": path, "content": PAYLOAD }), + &ctx(ws), + ) + .await +} + +fn read(p: &Path) -> String { + std::fs::read_to_string(p).expect("read file") +} + +fn outside_entries(t: &Tree) -> Vec { + let mut v: Vec = std::fs::read_dir(&t.outside) + .expect("read outside dir") + .map(|e| { + e.expect("dir entry") + .file_name() + .to_string_lossy() + .into_owned() + }) + .collect(); + v.sort(); + v +} + +// ── final component ──────────────────────────────────────────────────────── + +#[tokio::test] +async fn refuses_final_symlink_to_outside_file() { + let t = tree(); + symlink(&t.victim, t.ws.join("target.txt")).expect("plant symlink"); + + let r = write(&t.ws, "target.txt").await; + + assert!( + refused(&r), + "write through a final symlink must be refused, got {r:?}" + ); + assert_eq!(read(&t.victim), ORIGINAL, "the outside file changed"); +} + +#[tokio::test] +async fn refuses_final_symlink_to_outside_file_by_absolute_path() { + let t = tree(); + let link = t.ws.join("target.txt"); + symlink(&t.victim, &link).expect("plant symlink"); + let abs = t.ws.canonicalize().expect("canon ws").join("target.txt"); + + let r = write(&t.ws, abs.to_str().expect("utf-8")).await; + + assert!( + refused(&r), + "absolute-path write through a symlink must be refused, got {r:?}" + ); + assert_eq!(read(&t.victim), ORIGINAL, "the outside file changed"); +} + +#[tokio::test] +async fn refuses_final_symlink_to_outside_directory() { + let t = tree(); + symlink(&t.outside, t.ws.join("out")).expect("plant dir symlink"); + let before = outside_entries(&t); + + let r_link = write(&t.ws, "out").await; + let r_into = write(&t.ws, "out/new.txt").await; + + assert!( + refused(&r_link), + "write to a directory symlink must be refused, got {r_link:?}" + ); + assert!( + refused(&r_into), + "write through a directory symlink must be refused, got {r_into:?}" + ); + assert_eq!( + outside_entries(&t), + before, + "a file appeared outside the workspace" + ); + assert_eq!(read(&t.victim), ORIGINAL); +} + +#[tokio::test] +async fn refuses_dangling_final_symlink() { + let t = tree(); + let would_be = t.outside.join("created.txt"); + symlink(&would_be, t.ws.join("dangling.txt")).expect("plant dangling symlink"); + + let r = write(&t.ws, "dangling.txt").await; + + assert!( + refused(&r), + "write through a dangling symlink must be refused, got {r:?}" + ); + assert!( + !would_be.exists(), + "the dangling symlink's target was created outside" + ); +} + +#[tokio::test] +async fn refuses_final_symlink_pointing_inside_workspace() { + // Policy: refused. A link inside the workspace can be retargeted at any + // time, so the tool writes only to the name it was given. + let t = tree(); + let real = t.ws.join("real.txt"); + std::fs::write(&real, ORIGINAL).expect("write real"); + symlink(&real, t.ws.join("alias.txt")).expect("plant inside symlink"); + + let r = write(&t.ws, "alias.txt").await; + + assert!( + refused(&r), + "write through an inside symlink must be refused, got {r:?}" + ); + assert_eq!(read(&real), ORIGINAL, "the link target changed"); +} + +#[tokio::test] +async fn refuses_hardlinked_target() { + // Policy: a regular file with more than one link is refused, because the + // other name can be outside the workspace and the write would change it. + let t = tree(); + let hl = t.ws.join("hl.txt"); + std::fs::hard_link(&t.victim, &hl).expect("plant hard link"); + + let r = write(&t.ws, "hl.txt").await; + + assert!( + refused(&r), + "write to a multiply-linked file must be refused, got {r:?}" + ); + assert_eq!( + read(&t.victim), + ORIGINAL, + "the outside file changed via its hard link" + ); +} + +// ── intermediate components ──────────────────────────────────────────────── + +#[tokio::test] +async fn refuses_intermediate_symlink_inside_workspace() { + // Policy: refused, same reasoning as the final component. + let t = tree(); + let real_dir = t.ws.join("real_dir"); + std::fs::create_dir(&real_dir).expect("mkdir real_dir"); + symlink(&real_dir, t.ws.join("alias_dir")).expect("plant dir symlink"); + + let r = write(&t.ws, "alias_dir/f.txt").await; + + assert!( + refused(&r), + "write through an intermediate symlink must be refused, got {r:?}" + ); + assert!( + !real_dir.join("f.txt").exists(), + "the write went through the link" + ); +} + +#[tokio::test] +async fn refuses_deep_intermediate_symlink_to_outside() { + let t = tree(); + std::fs::create_dir_all(t.ws.join("a/b")).expect("mkdir a/b"); + symlink(&t.outside, t.ws.join("a/b/c")).expect("plant deep dir symlink"); + let before = outside_entries(&t); + + let r = write(&t.ws, "a/b/c/new_dir/f.txt").await; + + assert!( + refused(&r), + "write through a deep intermediate symlink must be refused, got {r:?}" + ); + assert_eq!( + outside_entries(&t), + before, + "a directory or file appeared outside" + ); +} + +// ── ordinary behaviour is kept ───────────────────────────────────────────── + +#[tokio::test] +async fn ordinary_writes_still_work() { + let t = tree(); + let r_new = write(&t.ws, "deep/nested/new.txt").await; + assert!( + !refused(&r_new), + "creating a nested file must work, got {r_new:?}" + ); + assert_eq!(read(&t.ws.join("deep/nested/new.txt")), PAYLOAD); + + std::fs::write(t.ws.join("exists.txt"), "a much longer original body").expect("seed"); + let r_over = write(&t.ws, "exists.txt").await; + assert!( + !refused(&r_over), + "overwriting a regular file must work, got {r_over:?}" + ); + assert_eq!( + read(&t.ws.join("exists.txt")), + PAYLOAD, + "overwrite must truncate" + ); + + let abs = t.ws.canonicalize().expect("canon ws").join("abs.txt"); + let r_abs = write(&t.ws, abs.to_str().expect("utf-8")).await; + assert!( + !refused(&r_abs), + "absolute path inside the workspace must work, got {r_abs:?}" + ); + assert_eq!(read(&abs), PAYLOAD); +} + +#[tokio::test] +async fn workspace_reached_through_a_symlink_still_works() { + // The workspace root itself may be a symlinked path (e.g. /tmp -> /private/tmp + // on macOS). That is resolved once, up front, and is not a refusal. + let t = tree(); + let ws_link = t.outside.join("ws_link"); + symlink(&t.ws, &ws_link).expect("symlink to workspace"); + + let r = write(&ws_link, "via_link.txt").await; + + assert!( + !refused(&r), + "a symlinked workspace root must be usable, got {r:?}" + ); + assert_eq!(read(&t.ws.join("via_link.txt")), PAYLOAD); +} + +// ── races ────────────────────────────────────────────────────────────────── + +/// Repeatedly swap `ws/` between a regular file (or directory) and a +/// symlink to `link_to`, using atomic renames, until `stop` is set. +fn spawn_swapper( + ws: PathBuf, + name: &'static str, + link_to: PathBuf, + as_dir: bool, + stop: Arc, +) -> std::thread::JoinHandle { + std::thread::spawn(move || { + let target = ws.join(name); + let tmp_link = ws.join(format!(".swap-link-{name}")); + let tmp_real = ws.join(format!(".swap-real-{name}")); + let mut swaps = 0u64; + while !stop.load(Ordering::Relaxed) { + let _ = std::fs::remove_file(&tmp_link); + if symlink(&link_to, &tmp_link).is_ok() && std::fs::rename(&tmp_link, &target).is_ok() { + swaps += 1; + } + if as_dir { + // rename(dir, path) cannot replace a symlink with a directory on + // every platform; remove the link first, then move a dir in. + let _ = std::fs::create_dir(&tmp_real); + let _ = std::fs::remove_file(&target); + if std::fs::rename(&tmp_real, &target).is_ok() { + swaps += 1; + } + // Clear out anything the tool wrote so the next rename works. + let _ = std::fs::remove_file(target.join("f.txt")); + let _ = std::fs::remove_dir(&target); + } else { + let _ = std::fs::write(&tmp_real, "regular"); + if std::fs::rename(&tmp_real, &target).is_ok() { + swaps += 1; + } + } + } + swaps + }) +} + +const RACE_WRITES: usize = 3_000; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn race_final_component_swap_never_writes_outside() { + let t = tree(); + std::fs::write(t.ws.join("target.txt"), "regular").expect("seed target"); + let stop = Arc::new(AtomicBool::new(false)); + let h = spawn_swapper( + t.ws.clone(), + "target.txt", + t.victim.clone(), + false, + stop.clone(), + ); + + let mut escaped_at = None; + for i in 0..RACE_WRITES { + let _ = write(&t.ws, "target.txt").await; + if read(&t.victim) != ORIGINAL { + escaped_at = Some(i); + break; + } + } + stop.store(true, Ordering::Relaxed); + let swaps = h.join().expect("swapper thread"); + + assert!( + swaps > 0, + "the swapper never ran; the race was not exercised" + ); + assert_eq!( + escaped_at, None, + "a write landed outside the workspace (after {swaps} swaps)" + ); + assert_eq!(read(&t.victim), ORIGINAL); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn race_intermediate_component_swap_never_writes_outside() { + let t = tree(); + let stop = Arc::new(AtomicBool::new(false)); + let h = spawn_swapper(t.ws.clone(), "d", t.outside.clone(), true, stop.clone()); + let escaped = t.outside.join("f.txt"); + + let mut escaped_at = None; + for i in 0..RACE_WRITES { + let _ = write(&t.ws, "d/f.txt").await; + if escaped.exists() { + escaped_at = Some(i); + break; + } + } + stop.store(true, Ordering::Relaxed); + let swaps = h.join().expect("swapper thread"); + + assert!( + swaps > 0, + "the swapper never ran; the race was not exercised" + ); + assert_eq!( + escaped_at, None, + "a write landed outside the workspace (after {swaps} swaps)" + ); +} + +// ── file_edit (same write path) ──────────────────────────────────────────── + +async fn edit(ws: &Path, path: &str) -> Result { + FileEdit + .execute( + serde_json::json!({ "path": path, "old_string": "CONTENT", "new_string": "EDITED" }), + &ctx(ws), + ) + .await +} + +#[tokio::test] +async fn file_edit_refuses_symlink_pointing_inside_workspace() { + let t = tree(); + let real = t.ws.join("real.txt"); + std::fs::write(&real, ORIGINAL).expect("write real"); + symlink(&real, t.ws.join("alias.txt")).expect("plant inside symlink"); + + let r = edit(&t.ws, "alias.txt").await; + + assert!( + refused(&r), + "edit through a symlink must be refused, got {r:?}" + ); + assert_eq!(read(&real), ORIGINAL, "the link target changed"); +} + +#[tokio::test] +async fn file_edit_refuses_hardlinked_target() { + let t = tree(); + std::fs::hard_link(&t.victim, t.ws.join("hl.txt")).expect("plant hard link"); + + let r = edit(&t.ws, "hl.txt").await; + + assert!( + refused(&r), + "edit of a multiply-linked file must be refused, got {r:?}" + ); + assert_eq!( + read(&t.victim), + ORIGINAL, + "the outside file changed via its hard link" + ); +} + +#[tokio::test] +async fn file_edit_still_edits_regular_files() { + let t = tree(); + let f = t.ws.join("plain.txt"); + std::fs::write(&f, ORIGINAL).expect("seed"); + + let r = edit(&t.ws, "plain.txt").await; + + assert!(!refused(&r), "editing a regular file must work, got {r:?}"); + assert_eq!(read(&f), "ORIGINAL-EDITED"); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn race_file_edit_swap_never_writes_outside() { + let t = tree(); + std::fs::write(t.ws.join("target.txt"), "regular CONTENT").expect("seed target"); + let stop = Arc::new(AtomicBool::new(false)); + // Swap to a symlink pointing at the victim; the swapper's regular file has + // no "CONTENT" match, so edits succeed only when they open the victim. + let h = spawn_swapper( + t.ws.clone(), + "target.txt", + t.victim.clone(), + false, + stop.clone(), + ); + + let mut escaped_at = None; + for i in 0..RACE_WRITES { + let _ = edit(&t.ws, "target.txt").await; + if read(&t.victim) != ORIGINAL { + escaped_at = Some(i); + break; + } + } + stop.store(true, Ordering::Relaxed); + let swaps = h.join().expect("swapper thread"); + + assert!( + swaps > 0, + "the swapper never ran; the race was not exercised" + ); + assert_eq!( + escaped_at, None, + "an edit landed outside the workspace (after {swaps} swaps)" + ); +} From dd6a70894a4ed7ed6eec31f2a0303b8e7eb808d7 Mon Sep 17 00:00:00 2001 From: Larry Klosowski Date: Thu, 22 Oct 2026 18:52:12 -0700 Subject: [PATCH 2/3] hardening(file_write): refuse a symlink target file_write and file_edit now open through tools/safe_write.rs: the workspace root is canonicalized once, then every component below it is opened with openat + O_NOFOLLOW relative to the previous directory descriptor (missing directories via mkdirat), so no component may be a symbolic link and nothing can be swapped between a check and the open. The target must be a regular file with one hard link; on Linux the opened descriptor's /proc path is re-checked against the workspace before any byte is written (fails closed without /proc). Non-Unix platforms refuse every write. file_edit reads and rewrites through the same descriptor. Removes the old canonicalize-the-ancestor check and the post create_dir_all re-check, which the descriptor walk replaces. rustix (already in Cargo.lock) gains a direct unix-only dependency. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KGMeatV8UjtCB4CTbWuAv4 --- Cargo.lock | 1 + agent-code/Cargo.toml | 4 + agent-code/src/tools/file_edit.rs | 83 ++++------ agent-code/src/tools/file_write.rs | 133 +++------------- agent-code/src/tools/mod.rs | 1 + agent-code/src/tools/safe_write.rs | 241 +++++++++++++++++++++++++++++ 6 files changed, 303 insertions(+), 160 deletions(-) create mode 100644 agent-code/src/tools/safe_write.rs diff --git a/Cargo.lock b/Cargo.lock index ff8a0ac..ce06979 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1117,6 +1117,7 @@ version = "0.4.0" dependencies = [ "async-trait", "citrate-agent-legacy", + "rustix", "serde", "serde_json", "tempfile", diff --git a/agent-code/Cargo.toml b/agent-code/Cargo.toml index 29cfba3..79aa883 100644 --- a/agent-code/Cargo.toml +++ b/agent-code/Cargo.toml @@ -13,6 +13,10 @@ serde_json = "1" tracing = "0.1" tokio = { version = "1", features = ["full"] } +# openat + O_NOFOLLOW for workspace-confined writes (tools/safe_write.rs). +[target.'cfg(unix)'.dependencies] +rustix = { version = "1.1", features = ["fs"] } + [dev-dependencies] tokio = { version = "1", features = ["full", "test-util"] } tempfile = "3" diff --git a/agent-code/src/tools/file_edit.rs b/agent-code/src/tools/file_edit.rs index 78b9498..4e0ad96 100644 --- a/agent-code/src/tools/file_edit.rs +++ b/agent-code/src/tools/file_edit.rs @@ -1,39 +1,18 @@ //! FileEdit tool — replace an exact string occurrence in a file. //! -//! Data source: local filesystem via `tokio::fs::read_to_string` + `tokio::fs::write`. +//! Data source: local filesystem; the file is opened once through +//! [`super::safe_write`] (`openat` + `O_NOFOLLOW` from the workspace root), read +//! and rewritten through that one descriptor. Symbolic links in any component +//! and files with more than one hard link are refused. //! The old_string must appear exactly once in the file to avoid ambiguous edits. use citrate_agent_core::error::AgentError; use citrate_agent_core::tool::{AgentTool, RiskLevel, ToolContext, ToolResult}; -use std::path::{Path, PathBuf}; +use super::safe_write; +use std::io::{Read, Seek, SeekFrom, Write}; pub struct FileEdit; -/// Resolve a path relative to the workspace, requiring it to exist. -fn resolve_existing_path(workspace: &str, path: &str) -> Result { - let workspace = Path::new(workspace) - .canonicalize() - .map_err(|e| AgentError::ExecutionFailed(format!("invalid workspace dir: {e}")))?; - - let candidate = if Path::new(path).is_absolute() { - PathBuf::from(path) - } else { - workspace.join(path) - }; - - let resolved = candidate - .canonicalize() - .map_err(|e| AgentError::ExecutionFailed(format!("cannot resolve path '{}': {e}", path)))?; - - if !resolved.starts_with(&workspace) { - return Err(AgentError::ExecutionFailed( - "path traversal outside workspace is not allowed".into(), - )); - } - - Ok(resolved) -} - #[async_trait::async_trait] impl AgentTool for FileEdit { fn name(&self) -> &str { @@ -93,29 +72,33 @@ impl AgentTool for FileEdit { )); } - let resolved = resolve_existing_path(&ctx.workspace_dir, path)?; - let content = tokio::fs::read_to_string(&resolved) - .await - .map_err(|e| AgentError::ExecutionFailed(format!("failed to read '{}': {e}", path)))?; - - let match_count = content.matches(old_string).count(); - if match_count == 0 { - return Err(AgentError::ExecutionFailed(format!( - "old_string not found in '{}'", - path - ))); - } - if match_count > 1 { - return Err(AgentError::ExecutionFailed(format!( - "old_string found {} times in '{}' — must be unique", - match_count, path - ))); - } - - let new_content = content.replacen(old_string, new_string, 1); - tokio::fs::write(&resolved, &new_content) - .await - .map_err(|e| AgentError::ExecutionFailed(format!("failed to write '{}': {e}", path)))?; + let target = safe_write::resolve(&ctx.workspace_dir, path)?; + let (shown, old, new) = (path.to_owned(), old_string.to_owned(), new_string.to_owned()); + tokio::task::spawn_blocking(move || -> Result<(), AgentError> { + let fail = AgentError::ExecutionFailed; + let mut file = safe_write::open_for_edit(&target)?; + let mut content = String::new(); + file.read_to_string(&mut content) + .map_err(|e| fail(format!("failed to read '{shown}': {e}")))?; + + let match_count = content.matches(old.as_str()).count(); + if match_count == 0 { + return Err(fail(format!("old_string not found in '{shown}'"))); + } + if match_count > 1 { + return Err(fail(format!( + "old_string found {match_count} times in '{shown}' — must be unique" + ))); + } + + let new_content = content.replacen(old.as_str(), new.as_str(), 1); + file.set_len(0) + .and_then(|()| file.seek(SeekFrom::Start(0)).map(|_| ())) + .and_then(|()| file.write_all(new_content.as_bytes())) + .map_err(|e| fail(format!("failed to write '{shown}': {e}"))) + }) + .await + .map_err(|e| AgentError::ExecutionFailed(format!("edit task failed: {e}")))??; Ok(ToolResult::ok_with_data( format!("Edited {}: replaced 1 occurrence", path), diff --git a/agent-code/src/tools/file_write.rs b/agent-code/src/tools/file_write.rs index 9873251..7a9c09f 100644 --- a/agent-code/src/tools/file_write.rs +++ b/agent-code/src/tools/file_write.rs @@ -1,12 +1,16 @@ //! FileWrite tool — write content to a file in the workspace. //! -//! Data source: local filesystem via `tokio::fs::write`. -//! Restricted to the workspace directory to prevent path traversal. +//! Data source: local filesystem, opened through [`super::safe_write`] +//! (`openat` + `O_NOFOLLOW` from the workspace root). +//! Restricted to the workspace directory to prevent path traversal; refuses +//! symbolic links in any component and files with more than one hard link. //! Creates parent directories if they do not exist. use citrate_agent_core::error::AgentError; use citrate_agent_core::tool::{AgentTool, RiskLevel, ToolContext, ToolResult}; -use std::path::{Path, PathBuf}; +use super::safe_write::{self, WorkspacePath}; +use std::io::Write; +use std::path::Path; pub struct FileWrite; @@ -18,35 +22,12 @@ const DENIED_PATTERNS: &[&str] = &[ /// File extensions that are always denied for write operations. const DENIED_EXTENSIONS: &[&str] = &["pem", "key", "p12", "pfx", "jks"]; -/// Lexically normalize a path — resolve `.` and `..` components without -/// touching the filesystem. Returns `None` if the path escapes above its -/// root via `..` (a traversal attempt). This is what closes AR-B-006: the -/// previous ancestor-walk stripped `..` with `PathBuf::pop()` for the -/// existence check but returned the *un-normalised* candidate, so the kernel -/// resolved the surviving `..` chain at write time and the write landed -/// outside the workspace. -fn lexical_normalize(path: &Path) -> Option { - use std::path::Component; - let mut out = PathBuf::new(); - for comp in path.components() { - match comp { - Component::ParentDir => { - // Refuse to pop past the root / an empty accumulator. - if !out.pop() { - return None; - } - } - Component::CurDir => {} - other => out.push(other.as_os_str()), - } - } - Some(out) -} - /// Resolve a path relative to the workspace for write operations. -/// Unlike reads, the file need not exist yet — we validate the parent directory. -/// Also checks denied patterns and extensions per SandboxPolicy rules. -fn resolve_write_path(workspace: &str, path: &str) -> Result { +/// Checks denied patterns and extensions per SandboxPolicy rules, then +/// resolves the path lexically ([`safe_write::resolve`]): no `..` survives and +/// nothing outside the workspace is accepted. Symlinks and hard links are +/// handled when the file is opened ([`safe_write::open_for_write`]). +fn resolve_write_path(workspace: &str, path: &str) -> Result { // Check denied patterns for pattern in DENIED_PATTERNS { if path.contains(pattern) { @@ -63,58 +44,7 @@ fn resolve_write_path(workspace: &str, path: &str) -> Result Result<(), AgentError> { + let mut file = safe_write::open_for_write(&target)?; + file.write_all(body.as_bytes()) + .map_err(|e| AgentError::ExecutionFailed(format!("failed to write '{shown}': {e}"))) + }) + .await + .map_err(|e| AgentError::ExecutionFailed(format!("write task failed: {e}")))??; Ok(ToolResult::ok_with_data( format!("Wrote {} bytes to {}", bytes_written, path), diff --git a/agent-code/src/tools/mod.rs b/agent-code/src/tools/mod.rs index 8cba2e8..e624f6e 100644 --- a/agent-code/src/tools/mod.rs +++ b/agent-code/src/tools/mod.rs @@ -1,5 +1,6 @@ mod file_read; mod file_write; +mod safe_write; mod file_edit; pub mod shell_exec; mod git_operations; diff --git a/agent-code/src/tools/safe_write.rs b/agent-code/src/tools/safe_write.rs new file mode 100644 index 0000000..fe280cf --- /dev/null +++ b/agent-code/src/tools/safe_write.rs @@ -0,0 +1,241 @@ +//! Workspace-confined file opening for the writing tools (`file_write`, `file_edit`). +//! +//! Data source: the local filesystem, opened component by component with +//! `openat(2)` relative to a directory descriptor for the canonical workspace root. +//! +//! Policy (pinned by `agent-code/tests/file_write_symlink.rs`): +//! * The workspace root is canonicalized once (it may itself be reached through a +//! symlink, e.g. `/tmp` on macOS). Everything below it is opened with +//! `O_NOFOLLOW`, so no component of the requested path may be a symbolic link, +//! wherever the link points (outside, inside, dangling). Because each component +//! is opened relative to the descriptor of the one before it, there is no +//! window between a check and the open in which a component can be swapped. +//! * Missing intermediate directories are created with `mkdirat(2)` relative to the +//! parent descriptor (never `create_dir_all`, which follows symlinks). +//! * The final component must be a regular file with exactly one hard link. A +//! file with more links is refused, since another name for it may be outside the +//! workspace. +//! * On Linux the opened descriptor's path (`/proc/self/fd/N`) is re-checked to be +//! inside the workspace before any byte is written; a file this call created is +//! removed again if that check fails. The check fails closed if `/proc` is absent. +//! * On platforms without `O_NOFOLLOW` / `openat` (non-Unix) every write is refused. + +use citrate_agent_core::error::AgentError; +use std::fs::File; +use std::path::{Component, Path, PathBuf}; + +fn fail(msg: impl Into) -> AgentError { + AgentError::ExecutionFailed(msg.into()) +} + +const OUTSIDE: &str = "path traversal outside workspace is not allowed"; + +/// Lexically normalize a path — resolve `.` and `..` components without +/// touching the filesystem. Returns `None` if the path escapes above its +/// root via `..`. +fn lexical_normalize(path: &Path) -> Option { + let mut out = PathBuf::new(); + for comp in path.components() { + match comp { + Component::ParentDir => { + // Refuse to pop past the root / an empty accumulator. + if !out.pop() { + return None; + } + } + Component::CurDir => {} + other => out.push(other.as_os_str()), + } + } + Some(out) +} + +/// A request resolved against the workspace: the canonical root plus the path +/// below it, made only of normal components. +pub(crate) struct WorkspacePath { + pub(crate) root: PathBuf, + pub(crate) rel: PathBuf, +} + +/// Resolve `path` (relative to the workspace, or absolute inside it) lexically. +/// No `..` survives and nothing outside the workspace is accepted. The +/// filesystem is consulted only to canonicalize the workspace root itself. +pub(crate) fn resolve(workspace: &str, path: &str) -> Result { + let root = Path::new(workspace) + .canonicalize() + .map_err(|e| fail(format!("invalid workspace dir: {e}")))?; + let candidate = if Path::new(path).is_absolute() { + PathBuf::from(path) + } else { + root.join(path) + }; + let candidate = lexical_normalize(&candidate).ok_or_else(|| fail(OUTSIDE))?; + let rel = candidate + .strip_prefix(&root) + .map_err(|_| fail(OUTSIDE))? + .to_path_buf(); + if rel.as_os_str().is_empty() || !rel.components().all(|c| matches!(c, Component::Normal(_))) { + return Err(fail("invalid file path")); + } + Ok(WorkspacePath { root, rel }) +} + +/// Open `wp` for writing: missing directories and the file are created, and the +/// file is truncated only after every check has passed. +pub(crate) fn open_for_write(wp: &WorkspacePath) -> Result { + imp::open(wp, true) +} + +/// Open the existing regular file `wp` for reading and writing (no creation, no +/// truncation). +pub(crate) fn open_for_edit(wp: &WorkspacePath) -> Result { + imp::open(wp, false) +} + +#[cfg(unix)] +mod imp { + use super::{fail, WorkspacePath, OUTSIDE}; + use citrate_agent_core::error::AgentError; + use rustix::fs::{fstat, mkdirat, openat, unlinkat, AtFlags, FileType, Mode, OFlags}; + use rustix::io::Errno; + use std::ffi::OsStr; + use std::fs::File; + use std::os::fd::{AsFd, OwnedFd}; + + const SYMLINK: &str = "refusing to write through a symbolic link"; + + /// `O_NOFOLLOW` on a final symlink reports `ELOOP` on Linux/macOS and `EMLINK` + /// on FreeBSD; an `O_DIRECTORY` open of a symlink or a file reports `ENOTDIR`. + fn is_link_refusal(e: Errno) -> bool { + e == Errno::LOOP || e == Errno::MLINK || e == Errno::NOTDIR + } + + fn open_dir(parent: &OwnedFd, name: &OsStr, create: bool) -> Result { + let flags = OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC; + match openat(parent, name, flags, Mode::empty()) { + Ok(fd) => Ok(fd), + Err(e) if e == Errno::NOENT && create => { + match mkdirat(parent, name, Mode::from_raw_mode(0o755)) { + Ok(()) => {} + Err(e) if e == Errno::EXIST => {} + Err(e) => return Err(fail(format!("failed to create dirs: {e}"))), + } + openat(parent, name, flags, Mode::empty()).map_err(|e| { + if is_link_refusal(e) { + fail(SYMLINK) + } else { + fail(format!("failed to open directory: {e}")) + } + }) + } + Err(e) if is_link_refusal(e) => Err(fail(SYMLINK)), + Err(e) => Err(fail(format!("failed to open directory: {e}"))), + } + } + + /// Returns the opened file and whether this call created it. + fn open_leaf(dir: &OwnedFd, name: &OsStr, create: bool) -> Result<(OwnedFd, bool), AgentError> { + // NONBLOCK so a FIFO planted at the target cannot hang the call; it has no + // effect on regular files. + let access = if create { OFlags::WRONLY } else { OFlags::RDWR }; + let base = access | OFlags::NOFOLLOW | OFlags::CLOEXEC | OFlags::NONBLOCK; + if create { + // O_EXCL never follows a symlink, dangling or not: it reports EEXIST. + match openat( + dir, + name, + base | OFlags::CREATE | OFlags::EXCL, + Mode::from_raw_mode(0o644), + ) { + Ok(fd) => return Ok((fd, true)), + Err(e) if e == Errno::EXIST => {} + Err(e) => return Err(fail(format!("failed to open file: {e}"))), + } + } + match openat(dir, name, base, Mode::empty()) { + Ok(fd) => Ok((fd, false)), + Err(e) if e == Errno::LOOP || e == Errno::MLINK => Err(fail(SYMLINK)), + Err(e) if e == Errno::NOENT && create => Err(fail( + "the target changed while it was being opened; refusing", + )), + Err(e) => Err(fail(format!("failed to open file: {e}"))), + } + } + + /// Linux: confirm the opened descriptor really lives inside the workspace. + #[cfg(any(target_os = "linux", target_os = "android"))] + fn fd_inside(fd: &OwnedFd, root: &std::path::Path) -> Result { + use std::os::fd::AsRawFd; + let real = std::fs::read_link(format!("/proc/self/fd/{}", fd.as_fd().as_raw_fd())) + .map_err(|e| fail(format!("cannot confirm the opened file's location: {e}")))?; + Ok(real.starts_with(root)) + } + + /// Elsewhere the descriptor chain from the workspace root is the guarantee. + #[cfg(not(any(target_os = "linux", target_os = "android")))] + fn fd_inside(_fd: &OwnedFd, _root: &std::path::Path) -> Result { + Ok(true) + } + + pub(super) fn open(wp: &WorkspacePath, create: bool) -> Result { + let root_flags = OFlags::RDONLY | OFlags::DIRECTORY | OFlags::CLOEXEC; + let mut dir = openat( + rustix::fs::CWD, + wp.root.as_path(), + root_flags, + Mode::empty(), + ) + .map_err(|e| fail(format!("invalid workspace dir: {e}")))?; + + let mut names: Vec<&OsStr> = wp.rel.iter().collect(); + let leaf = names.pop().ok_or_else(|| fail("invalid file path"))?; + for name in names { + dir = open_dir(&dir, name, create)?; + } + + let (fd, created) = open_leaf(&dir, leaf, create)?; + let undo = |msg: String| -> AgentError { + if created { + let _ = unlinkat(&dir, leaf, AtFlags::empty()); + } + fail(msg) + }; + + let st = fstat(&fd).map_err(|e| undo(format!("cannot stat the opened file: {e}")))?; + if FileType::from_raw_mode(st.st_mode) != FileType::RegularFile { + return Err(undo( + "refusing to write: the target is not a regular file".into(), + )); + } + if st.st_nlink != 1 { + return Err(undo( + "refusing to write: the target has more than one hard link".into(), + )); + } + if !fd_inside(&fd, &wp.root).map_err(|e| undo(e.to_string()))? { + return Err(undo(OUTSIDE.into())); + } + + let file = File::from(fd); + if create && !created { + file.set_len(0) + .map_err(|e| fail(format!("failed to truncate: {e}")))?; + } + Ok(file) + } +} + +#[cfg(not(unix))] +mod imp { + use super::{fail, WorkspacePath}; + use citrate_agent_core::error::AgentError; + use std::fs::File; + + /// Fail closed: without `openat` + `O_NOFOLLOW` the symlink and swap + /// guarantees above cannot be given, so nothing is written. + pub(super) fn open(_wp: &WorkspacePath, _create: bool) -> Result { + Err(fail( + "file writes are not supported on this platform (no O_NOFOLLOW/openat)", + )) + } +} From d49f9f740297ddb66cccbaf2fccd4c1907f3b0d8 Mon Sep 17 00:00:00 2001 From: Larry Klosowski Date: Thu, 22 Oct 2026 18:52:12 -0700 Subject: [PATCH 3/3] ci: tripwire for path-based writes in agent-code tools scripts/check_tool_write_paths.py flags fs::write / File::create / OpenOptions / create_dir_all / fs::copy / fs::rename / symlink / hard_link in agent-code/src/tools outside safe_write.rs unless the line (or the one above) carries '// write-path-ok: '. --self-test holds the negative controls; against main it flags the old file_write and file_edit write sites. The two tool-owned scratch-HOME dirs in git_operations and shell_exec are marked. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KGMeatV8UjtCB4CTbWuAv4 --- .github/workflows/ci.yml | 14 ++++ agent-code/src/tools/git_operations.rs | 1 + agent-code/src/tools/shell_exec.rs | 1 + scripts/check_tool_write_paths.py | 93 ++++++++++++++++++++++++++ 4 files changed, 109 insertions(+) create mode 100644 scripts/check_tool_write_paths.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3b95204..866163a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -123,3 +123,17 @@ jobs: run: | python3 scripts/check_hic_terminology.py --self-test python3 scripts/check_hic_terminology.py + + tool-write-paths: + name: Tool-layer write paths go through safe_write (no symlink follow) + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 + with: + persist-credentials: false + - name: path-based writes in agent-code/src/tools + run: | + python3 scripts/check_tool_write_paths.py --self-test + python3 scripts/check_tool_write_paths.py diff --git a/agent-code/src/tools/git_operations.rs b/agent-code/src/tools/git_operations.rs index 21c613a..7574cc4 100644 --- a/agent-code/src/tools/git_operations.rs +++ b/agent-code/src/tools/git_operations.rs @@ -34,6 +34,7 @@ async fn run_git( std::process::id(), GITOPS_HOME_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed) )); + // write-path-ok: tool-owned scratch HOME under the temp dir, not a model-chosen path let _ = std::fs::create_dir_all(&scratch_home); let output = tokio::time::timeout( Duration::from_millis(GIT_TIMEOUT_MS), diff --git a/agent-code/src/tools/shell_exec.rs b/agent-code/src/tools/shell_exec.rs index 4f1b7b4..6e6809f 100644 --- a/agent-code/src/tools/shell_exec.rs +++ b/agent-code/src/tools/shell_exec.rs @@ -289,6 +289,7 @@ impl AgentTool for ShellExec { std::process::id(), SCRATCH_HOME_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed) )); + // write-path-ok: tool-owned scratch HOME under the temp dir, not a model-chosen path let _ = std::fs::create_dir_all(&scratch_home); let mut cmd = tokio::process::Command::new(&binary); cmd.args(&args) diff --git a/scripts/check_tool_write_paths.py b/scripts/check_tool_write_paths.py new file mode 100644 index 0000000..64fa503 --- /dev/null +++ b/scripts/check_tool_write_paths.py @@ -0,0 +1,93 @@ +#!/usr/bin/env python3 +"""Tool-layer write-path guard. + +The coding tools in `agent-code/src/tools/` write files at paths a model +chooses. Those writes must go through `safe_write.rs`, which opens every +component with `openat` + `O_NOFOLLOW` from the workspace root and refuses +multiply-linked files. A path-based write (`fs::write`, `File::create`, +`OpenOptions::new`, `create_dir_all`, `fs::copy`, `fs::rename`, +`symlink`, `hard_link`) anywhere else in that directory is flagged. + +Exceptions are explicit and local: the flagged line, or the line directly +above it, must carry `// write-path-ok: ` with a non-empty reason. +`safe_write.rs` itself is the one file not scanned. + +Usage: check_tool_write_paths.py [--self-test] [repo_root] +Exit codes: 0 clean, 1 violations (listed as file:line). +""" +import os +import re +import sys + +TOOLS_DIR = os.path.join("agent-code", "src", "tools") +EXEMPT_FILE = "safe_write.rs" +PATTERN = re.compile( + r"\b(?:fs::write|File::create(?:_new)?|OpenOptions::new|create_dir_all|" + r"fs::copy|fs::rename|symlink|hard_link)\s*\(" +) +MARKER = re.compile(r"//\s*write-path-ok:\s*\S") + + +def scan_text(text): + """Return 1-based line numbers of unmarked path-based writes.""" + lines = text.splitlines() + hits = [] + for i, line in enumerate(lines): + code = line.split("//", 1)[0] + if not PATTERN.search(code): + continue + if MARKER.search(line) or (i > 0 and MARKER.search(lines[i - 1])): + continue + hits.append(i + 1) + return hits + + +def self_test(): + bad = "fn f(p: &Path) {\n std::fs::write(p, b\"x\").unwrap();\n}\n" + assert scan_text(bad) == [2], "negative control: an unmarked fs::write must be flagged" + bad2 = "let f = std::fs::OpenOptions::new().write(true).open(p)?;\n" + assert scan_text(bad2) == [1], "negative control: OpenOptions must be flagged" + bad3 = "tokio::fs::create_dir_all(parent).await?;\n" + assert scan_text(bad3) == [1], "negative control: create_dir_all must be flagged" + empty_reason = "// write-path-ok:\nstd::fs::write(p, b\"x\")?;\n" + assert scan_text(empty_reason) == [2], "an empty reason must not exempt" + ok_above = "// write-path-ok: tool-owned scratch dir\nstd::fs::create_dir_all(&d);\n" + assert scan_text(ok_above) == [], "marker on the line above exempts" + ok_same = "std::fs::create_dir_all(&d); // write-path-ok: tool-owned scratch dir\n" + assert scan_text(ok_same) == [], "marker on the same line exempts" + comment = "// we used to call fs::write(p) here\n" + assert scan_text(comment) == [], "comments are not code" + print("check_tool_write_paths: self-test ok") + + +def main(argv): + args = [a for a in argv if a != "--self-test"] + if "--self-test" in argv: + self_test() + if not args: + return 0 + root = args[0] if args else os.getcwd() + tools = os.path.join(root, TOOLS_DIR) + if not os.path.isdir(tools): + print(f"check_tool_write_paths: {tools} not found", file=sys.stderr) + return 1 + violations = [] + for name in sorted(os.listdir(tools)): + if not name.endswith(".rs") or name == EXEMPT_FILE: + continue + path = os.path.join(tools, name) + with open(path, encoding="utf-8") as fh: + for line in scan_text(fh.read()): + violations.append(f"{os.path.join(TOOLS_DIR, name)}:{line}") + if violations: + print("path-based writes in the tool layer must go through safe_write.rs") + print("(or carry `// write-path-ok: `):") + for v in violations: + print(f" {v}") + return 1 + print("check_tool_write_paths: clean") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:]))