Skip to content

Commit f0b1bb3

Browse files
committed
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.
1 parent 5d94464 commit f0b1bb3

File tree

5 files changed

+113
-16
lines changed

5 files changed

+113
-16
lines changed

crates/chat-cli/src/cli/agent/mod.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -806,7 +806,7 @@ impl Agents {
806806
// This "static" way avoids needing to construct a tool instance.
807807
fn default_permission_label(&self, tool_name: &str) -> String {
808808
let label = match tool_name {
809-
"fs_read" => "trusted".dark_green().bold(),
809+
"fs_read" => "trust working directory".dark_grey(),
810810
"fs_write" => "not trusted".dark_grey(),
811811
#[cfg(not(windows))]
812812
"execute_bash" => "trust read-only commands".dark_grey(),
@@ -1133,9 +1133,9 @@ mod tests {
11331133

11341134
let label = agents.display_label("fs_read", &ToolOrigin::Native);
11351135
// With no active agent, it should fall back to default permissions
1136-
// fs_read has a default of "trusted"
1136+
// fs_read has a default of "trust working directory"
11371137
assert!(
1138-
label.contains("trusted"),
1138+
label.contains("trust working directory"),
11391139
"fs_read should show default trusted permission, instead found: {}",
11401140
label
11411141
);
@@ -1164,7 +1164,7 @@ mod tests {
11641164
// Test default permissions for known tools
11651165
let fs_read_label = agents.display_label("fs_read", &ToolOrigin::Native);
11661166
assert!(
1167-
fs_read_label.contains("trusted"),
1167+
fs_read_label.contains("trust working directory"),
11681168
"fs_read should be trusted by default, instead found: {}",
11691169
fs_read_label
11701170
);

crates/chat-cli/src/cli/chat/tools/fs_read.rs

Lines changed: 100 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -114,23 +114,31 @@ impl FsRead {
114114
}
115115

116116
fn default_allow_read_only() -> bool {
117-
true
117+
false
118118
}
119119

120120
let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read");
121-
match agent.tools_settings.get("fs_read") {
122-
Some(settings) => {
121+
let settings = agent.tools_settings.get("fs_read").cloned()
122+
.unwrap_or_else(|| serde_json::json!({}));
123+
124+
{
123125
let Settings {
124-
allowed_paths,
126+
mut allowed_paths,
125127
denied_paths,
126128
allow_read_only,
127-
} = match serde_json::from_value::<Settings>(settings.clone()) {
129+
} = match serde_json::from_value::<Settings>(settings) {
128130
Ok(settings) => settings,
129131
Err(e) => {
130132
error!("Failed to deserialize tool settings for fs_read: {:?}", e);
131133
return PermissionEvalResult::Ask;
132134
},
133135
};
136+
137+
// Always add current working directory to allowed paths
138+
if let Ok(cwd) = os.env.current_dir() {
139+
allowed_paths.push(cwd.to_string_lossy().to_string());
140+
}
141+
134142
let allow_set = {
135143
let mut builder = GlobSetBuilder::new();
136144
for path in &allowed_paths {
@@ -259,10 +267,7 @@ impl FsRead {
259267
PermissionEvalResult::Ask
260268
},
261269
}
262-
},
263-
None if is_in_allowlist => PermissionEvalResult::Allow,
264-
_ => PermissionEvalResult::Ask,
265-
}
270+
}
266271
}
267272

268273
pub async fn invoke(&self, os: &Os, updates: &mut impl Write) -> Result<InvokeOutput> {
@@ -862,6 +867,7 @@ fn format_mode(mode: u32) -> [char; 9] {
862867
#[cfg(test)]
863868
mod tests {
864869
use std::collections::HashMap;
870+
use std::path::PathBuf;
865871

866872
use super::*;
867873
use crate::cli::agent::ToolSettingTarget;
@@ -1397,7 +1403,7 @@ mod tests {
13971403
}
13981404

13991405
#[tokio::test]
1400-
async fn test_eval_perm() {
1406+
async fn test_eval_perm_denied_path() {
14011407
const DENIED_PATH_OR_FILE: &str = "/some/denied/path";
14021408
const DENIED_PATH_OR_FILE_GLOB: &str = "/denied/glob/**/path";
14031409

@@ -1447,4 +1453,88 @@ mod tests {
14471453
&& deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE).collect::<Vec<_>>().len() == 2
14481454
));
14491455
}
1456+
1457+
#[tokio::test]
1458+
async fn test_eval_perm_allowed_path_and_cwd() {
1459+
1460+
// by default the fake env uses "/" as the CWD.
1461+
// change it to a sub folder so we can test fs_read reading files outside CWD
1462+
let os = Os::new().await.unwrap();
1463+
os.env.set_current_dir_for_test(PathBuf::from("/home/user"));
1464+
1465+
let agent = Agent {
1466+
name: "test_agent".to_string(),
1467+
tools_settings: {
1468+
let mut map = HashMap::new();
1469+
map.insert(
1470+
ToolSettingTarget("fs_read".to_string()),
1471+
serde_json::json!({
1472+
"allowedPaths": ["/explicitly/allowed/path"]
1473+
}),
1474+
);
1475+
map
1476+
},
1477+
..Default::default() // Not in allowed_tools, allow_read_only = false
1478+
};
1479+
1480+
// Test 1: Explicitly allowed path should work
1481+
let allowed_tool = serde_json::from_value::<FsRead>(serde_json::json!({
1482+
"operations": [
1483+
{ "path": "/explicitly/allowed/path", "mode": "Directory" },
1484+
{ "path": "/explicitly/allowed/path/file.txt", "mode": "Line" },
1485+
]
1486+
})).unwrap();
1487+
let res = allowed_tool.eval_perm(&os, &agent);
1488+
assert!(matches!(res, PermissionEvalResult::Allow));
1489+
1490+
// Test 2: CWD should always be allowed
1491+
let cwd_tool = serde_json::from_value::<FsRead>(serde_json::json!({
1492+
"operations": [
1493+
{ "path": "/home/user/", "mode": "Directory" },
1494+
{ "path": "/home/user/file.txt", "mode": "Line" },
1495+
]
1496+
})).unwrap();
1497+
let res = cwd_tool.eval_perm(&os, &agent);
1498+
assert!(matches!(res, PermissionEvalResult::Allow));
1499+
1500+
// Test 3: Outside CWD and not explicitly allowed should ask
1501+
let outside_tool = serde_json::from_value::<FsRead>(serde_json::json!({
1502+
"operations": [
1503+
{ "path": "/tmp/not/allowed/file.txt", "mode": "Line" }
1504+
]
1505+
})).unwrap();
1506+
let res = outside_tool.eval_perm(&os, &agent);
1507+
assert!(matches!(res, PermissionEvalResult::Ask));
1508+
}
1509+
1510+
#[tokio::test]
1511+
async fn test_eval_perm_no_settings_cwd_behavior() {
1512+
let os = Os::new().await.unwrap();
1513+
os.env.set_current_dir_for_test(PathBuf::from("/home/user"));
1514+
1515+
let agent = Agent {
1516+
name: "test_agent".to_string(),
1517+
tools_settings: HashMap::new(), // No fs_read settings
1518+
..Default::default()
1519+
};
1520+
1521+
// Test 1: CWD should be allowed even with no settings
1522+
let cwd_tool = serde_json::from_value::<FsRead>(serde_json::json!({
1523+
"operations": [
1524+
{ "path": "/home/user/", "mode": "Directory" },
1525+
{ "path": "/home/user/file.txt", "mode": "Line" },
1526+
]
1527+
})).unwrap();
1528+
let res = cwd_tool.eval_perm(&os, &agent);
1529+
assert!(matches!(res, PermissionEvalResult::Allow));
1530+
1531+
// Test 2: Outside CWD should ask for permission
1532+
let outside_tool = serde_json::from_value::<FsRead>(serde_json::json!({
1533+
"operations": [
1534+
{ "path": "/tmp/not/allowed/file.txt", "mode": "Line" }
1535+
]
1536+
})).unwrap();
1537+
let res = outside_tool.eval_perm(&os, &agent);
1538+
assert!(matches!(res, PermissionEvalResult::Ask));
1539+
}
14501540
}

crates/chat-cli/src/cli/chat/tools/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ use crate::cli::agent::{
5757
use crate::cli::chat::line_tracker::FileLineTracker;
5858
use crate::os::Os;
5959

60-
pub const DEFAULT_APPROVE: [&str; 1] = ["fs_read"];
60+
pub const DEFAULT_APPROVE: [&str; 0] = [];
6161
pub const NATIVE_TOOLS: [&str; 8] = [
6262
"fs_read",
6363
"fs_write",

crates/chat-cli/src/os/env.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,13 @@ impl Env {
132132
}
133133
}
134134

135+
pub fn set_current_dir_for_test(&self, path: PathBuf) {
136+
use inner::Inner;
137+
if let Inner::Fake(fake) = &self.0 {
138+
fake.lock().unwrap().cwd = path;
139+
}
140+
}
141+
135142
pub fn current_exe(&self) -> Result<PathBuf, io::Error> {
136143
use inner::Inner;
137144
match &self.0 {

crates/chat-cli/src/util/directories.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,6 @@ pub fn add_gitignore_globs(builder: &mut GlobSetBuilder, path: &str) -> Result<(
198198
// Glob doesn't normalize the path so it doesn't work with double slash
199199
let dir_pattern: String = format!("{}/**", path.trim_end_matches('/'));
200200
let glob_for_dir = Glob::new(&dir_pattern)?;
201-
202201
builder.add(glob_for_file);
203202
builder.add(glob_for_dir);
204203

@@ -276,6 +275,7 @@ pub fn database_path() -> Result<PathBuf> {
276275
#[cfg(test)]
277276
mod linux_tests {
278277
use super::*;
278+
use globset::GlobSetBuilder;
279279

280280
#[test]
281281
fn all_paths() {

0 commit comments

Comments
 (0)