Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions crates/chat-cli/src/cli/agent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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
);
Expand Down Expand Up @@ -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
);
Expand Down
114 changes: 100 additions & 14 deletions crates/chat-cli/src/cli/chat/tools/fs_read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,28 +109,32 @@ impl FsRead {
allowed_paths: Vec<String>,
#[serde(default)]
denied_paths: Vec<String>,
#[serde(default = "default_allow_read_only")]
#[serde(default)]
allow_read_only: bool,
}

fn default_allow_read_only() -> bool {
true
}

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>(settings.clone()) {
} = match serde_json::from_value::<Settings>(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 {
Expand Down Expand Up @@ -259,10 +263,7 @@ impl FsRead {
PermissionEvalResult::Ask
},
}
},
None if is_in_allowlist => PermissionEvalResult::Allow,
_ => PermissionEvalResult::Ask,
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I am also changing the behavior for the case that "fs_read" is not explicitly defined in agent.tools_settings to re-use the big chunk of code above as if the fs_read is an empty json (no allowed path, no denied path, etc)


pub async fn invoke(&self, os: &Os, updates: &mut impl Write) -> Result<InvokeOutput> {
Expand Down Expand Up @@ -862,6 +863,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;
Expand Down Expand Up @@ -1397,7 +1399,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";

Expand Down Expand Up @@ -1447,4 +1449,88 @@ mod tests {
&& deny_list.iter().filter(|p| *p == DENIED_PATH_OR_FILE).collect::<Vec<_>>().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::<FsRead>(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::<FsRead>(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::<FsRead>(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::<FsRead>(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::<FsRead>(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));
}
}
2 changes: 1 addition & 1 deletion crates/chat-cli/src/cli/chat/tools/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
7 changes: 7 additions & 0 deletions crates/chat-cli/src/os/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PathBuf, io::Error> {
use inner::Inner;
match &self.0 {
Expand Down