hardening(file_write): refuse a symlink target

.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

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"

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<PathBuf, AgentError> {
-    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),

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<PathBuf> {
-    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<PathBuf, AgentError> {
+/// 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<WorkspacePath, AgentError> {
     // 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<PathBuf, AgentError
             ));
         }
     }
-    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)
-    };
-
-    // Lexically normalize FIRST so any `..` chain is collapsed before we
-    // reason about containment. The old code returned the raw `candidate`
-    // with `..` intact, which the kernel later resolved through the
-    // filesystem — escaping the workspace (AR-B-006).
-    let candidate = lexical_normalize(&candidate).ok_or_else(|| {
-        AgentError::ExecutionFailed("path traversal outside workspace is not allowed".into())
-    })?;
-    if !candidate.starts_with(&workspace) {
-        return Err(AgentError::ExecutionFailed(
-            "path traversal outside workspace is not allowed".into(),
-        ));
-    }
-
-    // Defence in depth against a symlinked *existing* ancestor: walk up until
-    // we find an ancestor that exists, canonicalize it (following symlinks),
-    // and confirm it too is inside the workspace.
-    let parent = candidate
-        .parent()
-        .ok_or_else(|| AgentError::ExecutionFailed("invalid file path".into()))?;
-    let mut check = parent.to_path_buf();
-    loop {
-        if check.exists() {
-            let resolved_ancestor = check
-                .canonicalize()
-                .map_err(|e| AgentError::ExecutionFailed(format!("cannot resolve path: {e}")))?;
-            if !resolved_ancestor.starts_with(&workspace) {
-                return Err(AgentError::ExecutionFailed(
-                    "path traversal outside workspace is not allowed".into(),
-                ));
-            }
-            break;
-        }
-        if !check.pop() {
-            return Err(AgentError::ExecutionFailed(
-                "cannot find existing ancestor directory".into(),
-            ));
-        }
-    }
-
-    // Return the NORMALISED path (no surviving `..`), so the eventual write
-    // cannot be redirected by the kernel resolving traversal components.
-    Ok(candidate)
+    safe_write::resolve(workspace, path)
 }
 
 #[async_trait::async_trait]
@@ -162,35 +92,18 @@ impl AgentTool for FileWrite {
             .and_then(|c| c.as_str())
             .ok_or_else(|| AgentError::InvalidParams("'content' is required".into()))?;
 
-        let resolved = resolve_write_path(&ctx.workspace_dir, path)?;
-
-        // Create parent directories if needed
-        if let Some(parent) = resolved.parent() {
-            tokio::fs::create_dir_all(parent)
-                .await
-                .map_err(|e| AgentError::ExecutionFailed(format!("failed to create dirs: {e}")))?;
-
-            // Re-canonicalize the (now-materialised) parent and confirm it is
-            // still inside the workspace. This closes the symlink-in-the-final
-            // component variant: an allow-listed operation could have planted a
-            // symlink at `parent`, which only resolves once it exists on disk.
-            let workspace = Path::new(&ctx.workspace_dir).canonicalize().map_err(|e| {
-                AgentError::ExecutionFailed(format!("invalid workspace dir: {e}"))
-            })?;
-            let real_parent = parent.canonicalize().map_err(|e| {
-                AgentError::ExecutionFailed(format!("cannot resolve parent dir: {e}"))
-            })?;
-            if !real_parent.starts_with(&workspace) {
-                return Err(AgentError::ExecutionFailed(
-                    "path traversal outside workspace is not allowed".into(),
-                ));
-            }
-        }
+        let target = resolve_write_path(&ctx.workspace_dir, path)?;
 
         let bytes_written = content.len();
-        tokio::fs::write(&resolved, content)
-            .await
-            .map_err(|e| AgentError::ExecutionFailed(format!("failed to write '{}': {e}", path)))?;
+        let body = content.to_owned();
+        let shown = path.to_owned();
+        tokio::task::spawn_blocking(move || -> 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),

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),

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;