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/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/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/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)", + )) + } +} 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/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)" + ); +} 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:]))