fix(security): symlinks and Grep bypassed the sensitive-path deny-list

src/tools/file_edit.rs

@@ -66,7 +66,7 @@ impl Tool for FileEditTool {
         if let Some(err) = super::check_protected_path(&path) {
             return Ok(err);
         }
-        if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Write) {
+        if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Write) {
             return Ok(err);
         }
 

src/tools/file_read.rs

@@ -61,7 +61,7 @@ impl Tool for FileReadTool {
             Err(e) => return Ok(ToolOutput::error(e.to_string())),
         };
 
-        if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Read) {
+        if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Read) {
             return Ok(err);
         }
 

src/tools/file_write.rs

@@ -53,7 +53,7 @@ impl Tool for FileWriteTool {
         if let Some(err) = super::check_protected_path(&path) {
             return Ok(err);
         }
-        if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Write) {
+        if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Write) {
             return Ok(err);
         }
 

src/tools/grep.rs

@@ -159,6 +159,13 @@ async fn run_with_rg(input: &GrepInput, ctx: &ToolContext) -> Result<ToolOutput>
         args.push(format!("!**/{excl}/**"));
     }
 
+    // Same read deny-list the fallback backend and FileRead enforce. rg opens
+    // files itself, so exclusions have to be declared rather than checked.
+    for g in super::denied_read_globs() {
+        args.push("--glob".into());
+        args.push(g);
+    }
+
     args.push("--".into());
     args.push(input.pattern.clone());
 
@@ -253,6 +260,15 @@ async fn run_with_regex(input: &GrepInput, ctx: &ToolContext) -> Result<ToolOutp
             continue;
         }
 
+        // Honour the same read deny-list the FileRead tool enforces. Grep
+        // returns matching lines verbatim, so without this it is a read
+        // primitive that bypasses the guard entirely — verified: a search for a
+        // string inside `server.pem` returned the key material, while FileRead
+        // on the same file was correctly refused.
+        if super::check_sensitive_path_resolved(path, super::SensitiveOp::Read).is_some() {
+            continue;
+        }
+
         let Ok(contents) = tokio::fs::read_to_string(path).await else {
             continue;
         };

src/tools/mod.rs

@@ -268,6 +268,64 @@ fn is_dotenv(file_name: &str) -> bool {
 /// Returns Some(error ToolOutput) if the path should be blocked for `op`.
 /// Read mode blocks private keys only. Write mode additionally blocks
 /// dotenv files, credential stores, and secrets directories.
+/// Deny-listed read paths expressed as ripgrep exclusion globs.
+///
+/// The Grep tool has two backends — a `ripgrep` subprocess and a pure-Rust
+/// fallback — and both must honour the same rules. The fallback can check each
+/// file as it opens it; `rg` opens files itself, so it has to be told up front.
+pub fn denied_read_globs() -> Vec<String> {
+    let mut g: Vec<String> = PRIVATE_KEY_NAMES.iter().map(|n| format!("!**/{n}")).collect();
+    g.extend(PRIVATE_KEY_SUFFIXES.iter().map(|s| format!("!**/*{s}")));
+    g
+}
+
+/// Resolve symlinks so [`check_sensitive_path`] sees the file that will actually
+/// be touched, not the name the caller supplied.
+///
+/// The deny-list matches on the *file name*, so without this every protection it
+/// offers is defeated by a symlink with an innocuous name. Verified: a link
+/// named `notes.md` pointing at `~/.ssh/id_rsa` returned the private key, and a
+/// link named `config.json` pointing at `~/.aws/credentials` overwrote them —
+/// while the same operations on the real names were correctly refused.
+///
+/// That needs no unusual privileges: a repository can simply *ship* a symlink
+/// called `README.md`, and asking the agent to read it exfiltrates the target.
+///
+/// Falls back to the parent directory for paths that do not exist yet (a fresh
+/// write), which also catches a symlinked parent, and to the input unchanged
+/// when nothing can be resolved — a check on the literal path is never worse
+/// than the old behaviour.
+pub fn resolve_for_sensitivity_check(path: &std::path::Path) -> std::path::PathBuf {
+    if let Ok(real) = std::fs::canonicalize(path) {
+        return real;
+    }
+    if let (Some(parent), Some(name)) = (path.parent(), path.file_name())
+        && let Ok(real_parent) = std::fs::canonicalize(parent)
+    {
+        return real_parent.join(name);
+    }
+    path.to_path_buf()
+}
+
+/// [`check_sensitive_path`] applied to both the supplied path and its symlink
+/// target. Either looking sensitive is a refusal.
+///
+/// Use this at every filesystem entry point; the raw `check_sensitive_path` only
+/// inspects the name it is given.
+pub fn check_sensitive_path_resolved(
+    path: &std::path::Path,
+    op: SensitiveOp,
+) -> Option<ToolOutput> {
+    if let Some(err) = check_sensitive_path(path, op) {
+        return Some(err);
+    }
+    let resolved = resolve_for_sensitivity_check(path);
+    if resolved != path {
+        return check_sensitive_path(&resolved, op);
+    }
+    None
+}
+
 pub fn check_sensitive_path(path: &std::path::Path, op: SensitiveOp) -> Option<ToolOutput> {
     let file_name = path.file_name().and_then(|n| n.to_str()).unwrap_or("");
 

src/tools/multi_edit.rs

@@ -117,7 +117,7 @@ impl Tool for MultiEditTool {
                 had_error = true;
                 continue;
             }
-            if let Some(err) = super::check_sensitive_path(&path, super::SensitiveOp::Write) {
+            if let Some(err) = super::check_sensitive_path_resolved(&path, super::SensitiveOp::Write) {
                 let msg = err
                     .content
                     .iter()

tests/fs_tool_sensitivity_tests.rs

@@ -0,0 +1,177 @@
+//! Phase 2: the filesystem tools must not become a way around the sensitive-path
+//! deny-list.
+//!
+//! Two bypasses were live before these tests existed:
+//!
+//!   1. `check_sensitive_path` matches on the *file name* of the path it is
+//!      handed, so a symlink with an innocuous name defeated every protection it
+//!      offers. A link `notes.md` -> `~/.ssh/id_rsa` returned the private key,
+//!      and `config.json` -> `~/.aws/credentials` overwrote them, while the same
+//!      operations on the real names were correctly refused. A repository can
+//!      simply ship such a link — "read the README" is enough to exfiltrate.
+//!
+//!   2. Grep never consulted the deny-list at all, so searching for a string
+//!      inside a private key returned the key material verbatim — a read
+//!      primitive that walked straight past the guard on FileRead.
+//!
+//! Unix-only: symlink semantics.
+
+#![cfg(unix)]
+
+use rustyclaw::tools::{
+    Tool, ToolContext, file_read::FileReadTool, file_write::FileWriteTool, grep::GrepTool,
+};
+use serde_json::json;
+use std::path::PathBuf;
+use tempfile::TempDir;
+
+fn text(o: &rustyclaw::tools::ToolOutput) -> String {
+    o.content
+        .iter()
+        .map(|c| match c {
+            rustyclaw::api::types::ToolResultContent::Text { text } => text.as_str(),
+        })
+        .collect::<Vec<_>>()
+        .join("")
+}
+
+/// Secrets outside the project, a project dir, and links pointing out of it.
+fn fixture() -> (TempDir, PathBuf) {
+    let td = TempDir::new().unwrap();
+    let ssh = td.path().join(".ssh");
+    std::fs::create_dir_all(&ssh).unwrap();
+    std::fs::write(ssh.join("id_rsa"), "PRIVATE-KEY-MATERIAL\n").unwrap();
+
+    let aws = td.path().join(".aws");
+    std::fs::create_dir_all(&aws).unwrap();
+    std::fs::write(aws.join("credentials"), "[default]\nreal=secret\n").unwrap();
+
+    let proj = td.path().join("proj");
+    std::fs::create_dir_all(&proj).unwrap();
+    std::os::unix::fs::symlink(ssh.join("id_rsa"), proj.join("notes.md")).unwrap();
+    std::os::unix::fs::symlink(aws.join("credentials"), proj.join("config.json")).unwrap();
+    (td, proj)
+}
+
+#[tokio::test]
+async fn symlink_cannot_be_used_to_read_a_private_key() {
+    let (_td, proj) = fixture();
+    let ctx = ToolContext::new(proj);
+    let out = FileReadTool
+        .execute(json!({"file_path": "notes.md"}), &ctx)
+        .await
+        .unwrap();
+    assert!(out.is_error, "reading a link to a private key must be refused");
+    assert!(
+        !text(&out).contains("PRIVATE-KEY-MATERIAL"),
+        "key material must not appear in the result"
+    );
+}
+
+#[tokio::test]
+async fn symlink_cannot_be_used_to_overwrite_credentials() {
+    let (td, proj) = fixture();
+    let creds = td.path().join(".aws").join("credentials");
+    let before = std::fs::read_to_string(&creds).unwrap();
+
+    let ctx = ToolContext::new(proj);
+    let out = FileWriteTool
+        .execute(json!({"file_path": "config.json", "content": "CLOBBERED\n"}), &ctx)
+        .await
+        .unwrap();
+
+    assert!(out.is_error, "writing through a link to credentials must be refused");
+    assert_eq!(
+        std::fs::read_to_string(&creds).unwrap(),
+        before,
+        "the target file must be byte-identical"
+    );
+}
+
+/// Ordinary files must keep working — a guard that blocks real work gets
+/// disabled, which is worse than no guard.
+#[tokio::test]
+async fn normal_files_are_unaffected() {
+    let (_td, proj) = fixture();
+    let ctx = ToolContext::new(proj.clone());
+
+    let w = FileWriteTool
+        .execute(json!({"file_path": "src.rs", "content": "fn main() {}\n"}), &ctx)
+        .await
+        .unwrap();
+    assert!(!w.is_error, "writing an ordinary file must work: {}", text(&w));
+
+    let r = FileReadTool
+        .execute(json!({"file_path": "src.rs"}), &ctx)
+        .await
+        .unwrap();
+    assert!(!r.is_error && text(&r).contains("fn main"), "{}", text(&r));
+
+    // A symlink to a harmless file is still fine.
+    std::os::unix::fs::symlink(proj.join("src.rs"), proj.join("alias.rs")).unwrap();
+    let a = FileReadTool
+        .execute(json!({"file_path": "alias.rs"}), &ctx)
+        .await
+        .unwrap();
+    assert!(!a.is_error, "benign symlinks must not be blocked: {}", text(&a));
+}
+
+/// Grep returns matching lines verbatim, so it must honour the same read
+/// deny-list as FileRead. Both backends are exercised: the `rg` subprocess when
+/// ripgrep is installed, and the pure-Rust fallback when it is not — a fix
+/// applied to only one of them leaves the other leaking.
+#[tokio::test]
+async fn grep_does_not_return_private_key_contents() {
+    let (_td, proj) = fixture();
+    std::fs::write(
+        proj.join("server.pem"),
+        "-----BEGIN PRIVATE KEY-----\nPEMSECRET\n",
+    )
+    .unwrap();
+    std::fs::write(proj.join("app.rs"), "// PEMSECRET appears here legitimately\n").unwrap();
+
+    let ctx = ToolContext::new(proj);
+    let out = GrepTool
+        .execute(json!({"pattern": "PEMSECRET", "output_mode": "content"}), &ctx)
+        .await
+        .unwrap();
+    let body = text(&out);
+
+    assert!(
+        !body.contains("server.pem"),
+        "key-material file must not be searched: {body}"
+    );
+    assert!(
+        body.contains("app.rs"),
+        "ordinary files must still be searched: {body}"
+    );
+}
+
+/// The same, forced down the non-ripgrep path.
+#[tokio::test]
+async fn grep_fallback_backend_also_honours_the_deny_list() {
+    let (_td, proj) = fixture();
+    std::fs::write(proj.join("key.pem"), "FALLBACKSECRET\n").unwrap();
+    std::fs::write(proj.join("ok.txt"), "FALLBACKSECRET\n").unwrap();
+
+    // An empty PATH removes `rg`, so the pure-Rust backend runs.
+    let ctx = ToolContext::new(proj);
+    let prev = std::env::var_os("PATH");
+    // SAFETY: restored below; this test does not run concurrently with other
+    // PATH users in this binary.
+    unsafe { std::env::set_var("PATH", "") };
+    let out = GrepTool
+        .execute(json!({"pattern": "FALLBACKSECRET", "output_mode": "content"}), &ctx)
+        .await
+        .unwrap();
+    unsafe {
+        match prev {
+            Some(v) => std::env::set_var("PATH", v),
+            None => std::env::remove_var("PATH"),
+        }
+    }
+
+    let body = text(&out);
+    assert!(!body.contains("key.pem"), "fallback leaked key material: {body}");
+    assert!(body.contains("ok.txt"), "fallback must still search normal files: {body}");
+}