From bb81560dad5e5f6b8a951091e673af09d88a8698 Mon Sep 17 00:00:00 2001 From: Erben Mo Date: Tue, 9 Sep 2025 15:13:33 -0700 Subject: [PATCH 1/2] Reduce default fs_read trust permission to current working directory only Previously by default fs_read is trusted to read any file on user's file system. This PR reduces the fs_read permission to CWD only. This means user can still access any file under CWD without prompt. But if user needs to access file outside CWD, she will be prompted for explicit approval. User can still explicitly add fs_read to trusted tools in chat / agent definition so fs_read can read any file without prompt. This change essentially adds a layer of defense against prompt injection by following the least-privilege principle. --- crates/chat-cli/src/cli/agent/mod.rs | 8 +- crates/chat-cli/src/cli/chat/tools/fs_read.rs | 110 ++++++++++++++++-- crates/chat-cli/src/cli/chat/tools/mod.rs | 2 +- crates/chat-cli/src/os/env.rs | 7 ++ 4 files changed, 112 insertions(+), 15 deletions(-) diff --git a/crates/chat-cli/src/cli/agent/mod.rs b/crates/chat-cli/src/cli/agent/mod.rs index bf69dcba03..04e634f0c6 100644 --- a/crates/chat-cli/src/cli/agent/mod.rs +++ b/crates/chat-cli/src/cli/agent/mod.rs @@ -810,7 +810,7 @@ impl Agents { // This "static" way avoids needing to construct a tool instance. fn default_permission_label(&self, tool_name: &str) -> String { let label = match tool_name { - "fs_read" => "trusted".dark_green().bold(), + "fs_read" => "trust working directory".dark_grey(), "fs_write" => "not trusted".dark_grey(), #[cfg(not(windows))] "execute_bash" => "trust read-only commands".dark_grey(), @@ -1137,9 +1137,9 @@ mod tests { let label = agents.display_label("fs_read", &ToolOrigin::Native); // With no active agent, it should fall back to default permissions - // fs_read has a default of "trusted" + // fs_read has a default of "trust working directory" assert!( - label.contains("trusted"), + label.contains("trust working directory"), "fs_read should show default trusted permission, instead found: {}", label ); @@ -1168,7 +1168,7 @@ mod tests { // Test default permissions for known tools let fs_read_label = agents.display_label("fs_read", &ToolOrigin::Native); assert!( - fs_read_label.contains("trusted"), + fs_read_label.contains("trust working directory"), "fs_read should be trusted by default, instead found: {}", fs_read_label ); diff --git a/crates/chat-cli/src/cli/chat/tools/fs_read.rs b/crates/chat-cli/src/cli/chat/tools/fs_read.rs index a11924e9a2..0d1cdffc2f 100644 --- a/crates/chat-cli/src/cli/chat/tools/fs_read.rs +++ b/crates/chat-cli/src/cli/chat/tools/fs_read.rs @@ -114,23 +114,31 @@ impl FsRead { } fn default_allow_read_only() -> bool { - true + false } let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read"); - match agent.tools_settings.get("fs_read") { - Some(settings) => { + let settings = agent.tools_settings.get("fs_read").cloned() + .unwrap_or_else(|| serde_json::json!({})); + + { let Settings { - allowed_paths, + mut allowed_paths, denied_paths, allow_read_only, - } = match serde_json::from_value::(settings.clone()) { + } = match serde_json::from_value::(settings) { Ok(settings) => settings, Err(e) => { error!("Failed to deserialize tool settings for fs_read: {:?}", e); return PermissionEvalResult::Ask; }, }; + + // Always add current working directory to allowed paths + if let Ok(cwd) = os.env.current_dir() { + allowed_paths.push(cwd.to_string_lossy().to_string()); + } + let allow_set = { let mut builder = GlobSetBuilder::new(); for path in &allowed_paths { @@ -259,10 +267,7 @@ impl FsRead { PermissionEvalResult::Ask }, } - }, - None if is_in_allowlist => PermissionEvalResult::Allow, - _ => PermissionEvalResult::Ask, - } + } } pub async fn invoke(&self, os: &Os, updates: &mut impl Write) -> Result { @@ -862,6 +867,7 @@ fn format_mode(mode: u32) -> [char; 9] { #[cfg(test)] mod tests { use std::collections::HashMap; + use std::path::PathBuf; use super::*; use crate::cli::agent::ToolSettingTarget; @@ -1397,7 +1403,7 @@ mod tests { } #[tokio::test] - async fn test_eval_perm() { + async fn test_eval_perm_denied_path() { const DENIED_PATH_OR_FILE: &str = "/some/denied/path"; const DENIED_PATH_OR_FILE_GLOB: &str = "/denied/glob/**/path"; @@ -1447,4 +1453,88 @@ mod tests { && deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE).collect::>().len() == 2 )); } + + #[tokio::test] + async fn test_eval_perm_allowed_path_and_cwd() { + + // by default the fake env uses "/" as the CWD. + // change it to a sub folder so we can test fs_read reading files outside CWD + let os = Os::new().await.unwrap(); + os.env.set_current_dir_for_test(PathBuf::from("/home/user")); + + let agent = Agent { + name: "test_agent".to_string(), + tools_settings: { + let mut map = HashMap::new(); + map.insert( + ToolSettingTarget("fs_read".to_string()), + serde_json::json!({ + "allowedPaths": ["/explicitly/allowed/path"] + }), + ); + map + }, + ..Default::default() // Not in allowed_tools, allow_read_only = false + }; + + // Test 1: Explicitly allowed path should work + let allowed_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/explicitly/allowed/path", "mode": "Directory" }, + { "path": "/explicitly/allowed/path/file.txt", "mode": "Line" }, + ] + })).unwrap(); + let res = allowed_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Allow)); + + // Test 2: CWD should always be allowed + let cwd_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/home/user/", "mode": "Directory" }, + { "path": "/home/user/file.txt", "mode": "Line" }, + ] + })).unwrap(); + let res = cwd_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Allow)); + + // Test 3: Outside CWD and not explicitly allowed should ask + let outside_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/tmp/not/allowed/file.txt", "mode": "Line" } + ] + })).unwrap(); + let res = outside_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Ask)); + } + + #[tokio::test] + async fn test_eval_perm_no_settings_cwd_behavior() { + let os = Os::new().await.unwrap(); + os.env.set_current_dir_for_test(PathBuf::from("/home/user")); + + let agent = Agent { + name: "test_agent".to_string(), + tools_settings: HashMap::new(), // No fs_read settings + ..Default::default() + }; + + // Test 1: CWD should be allowed even with no settings + let cwd_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/home/user/", "mode": "Directory" }, + { "path": "/home/user/file.txt", "mode": "Line" }, + ] + })).unwrap(); + let res = cwd_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Allow)); + + // Test 2: Outside CWD should ask for permission + let outside_tool = serde_json::from_value::(serde_json::json!({ + "operations": [ + { "path": "/tmp/not/allowed/file.txt", "mode": "Line" } + ] + })).unwrap(); + let res = outside_tool.eval_perm(&os, &agent); + assert!(matches!(res, PermissionEvalResult::Ask)); + } } diff --git a/crates/chat-cli/src/cli/chat/tools/mod.rs b/crates/chat-cli/src/cli/chat/tools/mod.rs index 6b8baec18f..43ccb006cf 100644 --- a/crates/chat-cli/src/cli/chat/tools/mod.rs +++ b/crates/chat-cli/src/cli/chat/tools/mod.rs @@ -57,7 +57,7 @@ use crate::cli::agent::{ use crate::cli::chat::line_tracker::FileLineTracker; use crate::os::Os; -pub const DEFAULT_APPROVE: [&str; 1] = ["fs_read"]; +pub const DEFAULT_APPROVE: [&str; 0] = []; pub const NATIVE_TOOLS: [&str; 8] = [ "fs_read", "fs_write", diff --git a/crates/chat-cli/src/os/env.rs b/crates/chat-cli/src/os/env.rs index 40aac3b5fe..63e5449419 100644 --- a/crates/chat-cli/src/os/env.rs +++ b/crates/chat-cli/src/os/env.rs @@ -132,6 +132,13 @@ impl Env { } } + pub fn set_current_dir_for_test(&self, path: PathBuf) { + use inner::Inner; + if let Inner::Fake(fake) = &self.0 { + fake.lock().unwrap().cwd = path; + } + } + pub fn current_exe(&self) -> Result { use inner::Inner; match &self.0 { From d168f1c1fb72099d256e6e2d6745c4cac6850689 Mon Sep 17 00:00:00 2001 From: Erben Mo Date: Fri, 12 Sep 2025 13:07:31 -0700 Subject: [PATCH 2/2] remove allow_read_only since it is always false now --- crates/chat-cli/src/cli/chat/tools/fs_read.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/chat-cli/src/cli/chat/tools/fs_read.rs b/crates/chat-cli/src/cli/chat/tools/fs_read.rs index 0d1cdffc2f..c4e60ae6cc 100644 --- a/crates/chat-cli/src/cli/chat/tools/fs_read.rs +++ b/crates/chat-cli/src/cli/chat/tools/fs_read.rs @@ -109,14 +109,10 @@ impl FsRead { allowed_paths: Vec, #[serde(default)] denied_paths: Vec, - #[serde(default = "default_allow_read_only")] + #[serde(default)] allow_read_only: bool, } - fn default_allow_read_only() -> bool { - false - } - let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read"); let settings = agent.tools_settings.get("fs_read").cloned() .unwrap_or_else(|| serde_json::json!({}));