Skip to content

Commit 6e18c15

Browse files
dingfelidingfeli
authored
feat(agent): adds globbing support for tool settings deny (#2512)
* adds globbing support for deny list * docs update for tool settings globbing support * fixes tool name derivation for winwdows * adds description doc for PermissionEvalResult
1 parent 36e70d1 commit 6e18c15

File tree

8 files changed

+352
-54
lines changed

8 files changed

+352
-54
lines changed

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,11 +305,17 @@ impl Agent {
305305
}
306306
}
307307

308+
/// Result of evaluating tool permissions, indicating whether a tool should be allowed,
309+
/// require user confirmation, or be denied with specific reasons.
308310
#[derive(Debug, PartialEq)]
309311
pub enum PermissionEvalResult {
312+
/// Tool is allowed to execute without user confirmation
310313
Allow,
314+
/// Tool requires user confirmation before execution
311315
Ask,
312-
Deny,
316+
/// Denial with specific reasons explaining why the tool was denied
317+
/// Tools are free to overload what these reasons are
318+
Deny(Vec<String>),
313319
}
314320

315321
#[derive(Clone, Default, Debug)]

‎crates/chat-cli/src/cli/agent/wrapper_types.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ pub fn alias_schema(generator: &mut SchemaGenerator) -> Schema {
5353

5454
/// The name of the tool to be configured
5555
#[derive(Debug, Clone, Serialize, Deserialize, Eq, Hash, PartialEq, JsonSchema)]
56-
pub struct ToolSettingTarget(String);
56+
pub struct ToolSettingTarget(pub String);
5757

5858
impl Deref for ToolSettingTarget {
5959
type Target = String;

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

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1822,22 +1822,40 @@ impl ChatSession {
18221822
continue;
18231823
}
18241824

1825-
let mut denied = false;
1825+
let mut denied_match_set = None::<Vec<String>>;
18261826
let allowed =
18271827
self.conversation
18281828
.agents
18291829
.get_active()
18301830
.is_some_and(|a| match tool.tool.requires_acceptance(a) {
18311831
PermissionEvalResult::Allow => true,
18321832
PermissionEvalResult::Ask => false,
1833-
PermissionEvalResult::Deny => {
1834-
denied = true;
1833+
PermissionEvalResult::Deny(matches) => {
1834+
denied_match_set.replace(matches);
18351835
false
18361836
},
18371837
})
18381838
|| self.conversation.agents.trust_all_tools;
18391839

1840-
if denied {
1840+
if let Some(match_set) = denied_match_set {
1841+
let formatted_set = match_set.into_iter().fold(String::new(), |mut acc, rule| {
1842+
acc.push_str(&format!("\n - {rule}"));
1843+
acc
1844+
});
1845+
1846+
execute!(
1847+
self.stderr,
1848+
style::SetForegroundColor(Color::Red),
1849+
style::Print("Command "),
1850+
style::SetForegroundColor(Color::Yellow),
1851+
style::Print(&tool.name),
1852+
style::SetForegroundColor(Color::Red),
1853+
style::Print(" is rejected because it matches one or more rules on the denied list:"),
1854+
style::Print(formatted_set),
1855+
style::Print("\n"),
1856+
style::SetForegroundColor(Color::Reset),
1857+
)?;
1858+
18411859
return Ok(ChatState::HandleInput {
18421860
input: format!(
18431861
"Tool use with {} was rejected because the arguments supplied were forbidden",

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

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ impl ExecuteCommand {
193193

194194
let Self { command, .. } = self;
195195
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
196-
let is_in_allowlist = agent.allowed_tools.contains("execute_bash");
196+
let is_in_allowlist = agent.allowed_tools.contains(tool_name);
197197
match agent.tools_settings.get(tool_name) {
198198
Some(settings) if is_in_allowlist => {
199199
let Settings {
@@ -208,8 +208,15 @@ impl ExecuteCommand {
208208
},
209209
};
210210

211-
if denied_commands.iter().any(|dc| command.contains(dc)) {
212-
return PermissionEvalResult::Deny;
211+
let denied_match_set = denied_commands
212+
.iter()
213+
.filter_map(|dc| Regex::new(&format!(r"\A{dc}\z")).ok())
214+
.filter(|r| r.is_match(command))
215+
.map(|r| r.to_string())
216+
.collect::<Vec<_>>();
217+
218+
if !denied_match_set.is_empty() {
219+
return PermissionEvalResult::Deny(denied_match_set);
213220
}
214221

215222
if self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
@@ -249,7 +256,13 @@ pub fn format_output(output: &str, max_size: usize) -> String {
249256

250257
#[cfg(test)]
251258
mod tests {
259+
use std::collections::{
260+
HashMap,
261+
HashSet,
262+
};
263+
252264
use super::*;
265+
use crate::cli::agent::ToolSettingTarget;
253266

254267
#[test]
255268
fn test_requires_acceptance_for_readonly_commands() {
@@ -384,4 +397,44 @@ mod tests {
384397
);
385398
}
386399
}
400+
401+
#[test]
402+
fn test_eval_perm() {
403+
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
404+
let agent = Agent {
405+
name: "test_agent".to_string(),
406+
allowed_tools: {
407+
let mut allowed_tools = HashSet::<String>::new();
408+
allowed_tools.insert(tool_name.to_string());
409+
allowed_tools
410+
},
411+
tools_settings: {
412+
let mut map = HashMap::<ToolSettingTarget, serde_json::Value>::new();
413+
map.insert(
414+
ToolSettingTarget(tool_name.to_string()),
415+
serde_json::json!({
416+
"deniedCommands": ["git .*"]
417+
}),
418+
);
419+
map
420+
},
421+
..Default::default()
422+
};
423+
424+
let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
425+
"command": "git status",
426+
}))
427+
.unwrap();
428+
429+
let res = tool.eval_perm(&agent);
430+
assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
431+
432+
let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
433+
"command": "echo hello",
434+
}))
435+
.unwrap();
436+
437+
let res = tool.eval_perm(&agent);
438+
assert!(matches!(res, PermissionEvalResult::Allow));
439+
}
387440
}

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

Lines changed: 113 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,12 @@ impl FsRead {
144144
builder.build()
145145
};
146146

147+
let mut sanitized_deny_list = Vec::<&String>::new();
147148
let deny_set = {
148149
let mut builder = GlobSetBuilder::new();
149150
for path in &denied_paths {
150151
if let Ok(glob) = Glob::new(path) {
152+
sanitized_deny_list.push(path);
151153
builder.add(glob);
152154
} else {
153155
warn!("Failed to create glob from path given: {path}. Ignoring.");
@@ -158,43 +160,66 @@ impl FsRead {
158160

159161
match (allow_set, deny_set) {
160162
(Ok(allow_set), Ok(deny_set)) => {
161-
let eval_res = self
162-
.operations
163-
.iter()
164-
.map(|op| {
165-
match op {
166-
FsReadOperation::Line(FsLine { path, .. })
167-
| FsReadOperation::Directory(FsDirectory { path, .. })
168-
| FsReadOperation::Search(FsSearch { path, .. }) => {
169-
if deny_set.is_match(path) {
170-
return PermissionEvalResult::Deny;
171-
}
172-
if allow_set.is_match(path) {
173-
return PermissionEvalResult::Allow;
174-
}
175-
},
176-
FsReadOperation::Image(fs_image) => {
177-
let paths = &fs_image.image_paths;
178-
if paths.iter().any(|path| deny_set.is_match(path)) {
179-
return PermissionEvalResult::Deny;
180-
}
181-
if paths.iter().all(|path| allow_set.is_match(path)) {
182-
return PermissionEvalResult::Allow;
183-
}
184-
},
185-
}
186-
187-
if allow_read_only {
188-
PermissionEvalResult::Allow
189-
} else {
190-
PermissionEvalResult::Ask
191-
}
192-
})
193-
.collect::<Vec<_>>();
163+
let mut deny_list = Vec::<PermissionEvalResult>::new();
164+
let mut ask = false;
165+
166+
for op in &self.operations {
167+
match op {
168+
FsReadOperation::Line(FsLine { path, .. })
169+
| FsReadOperation::Directory(FsDirectory { path, .. })
170+
| FsReadOperation::Search(FsSearch { path, .. }) => {
171+
let denied_match_set = deny_set.matches(path);
172+
if !denied_match_set.is_empty() {
173+
let deny_res = PermissionEvalResult::Deny({
174+
denied_match_set
175+
.iter()
176+
.filter_map(|i| sanitized_deny_list.get(*i).map(|s| (*s).clone()))
177+
.collect::<Vec<_>>()
178+
});
179+
deny_list.push(deny_res);
180+
continue;
181+
}
182+
183+
// We only want to ask if we are not allowing read only
184+
// operation
185+
if !allow_read_only && !allow_set.is_match(path) {
186+
ask = true;
187+
}
188+
},
189+
FsReadOperation::Image(fs_image) => {
190+
let paths = &fs_image.image_paths;
191+
let denied_match_set =
192+
paths.iter().flat_map(|p| deny_set.matches(p)).collect::<Vec<_>>();
193+
if !denied_match_set.is_empty() {
194+
let deny_res = PermissionEvalResult::Deny({
195+
denied_match_set
196+
.iter()
197+
.filter_map(|i| sanitized_deny_list.get(*i).map(|s| (*s).clone()))
198+
.collect::<Vec<_>>()
199+
});
200+
deny_list.push(deny_res);
201+
continue;
202+
}
203+
204+
// We only want to ask if we are not allowing read only
205+
// operation
206+
if !allow_read_only && !paths.iter().any(|path| allow_set.is_match(path)) {
207+
ask = true;
208+
}
209+
},
210+
}
211+
}
194212

195-
if eval_res.contains(&PermissionEvalResult::Deny) {
196-
PermissionEvalResult::Deny
197-
} else if eval_res.contains(&PermissionEvalResult::Ask) {
213+
if !deny_list.is_empty() {
214+
PermissionEvalResult::Deny({
215+
deny_list.into_iter().fold(Vec::<String>::new(), |mut acc, res| {
216+
if let PermissionEvalResult::Deny(mut rules) = res {
217+
acc.append(&mut rules);
218+
}
219+
acc
220+
})
221+
})
222+
} else if ask {
198223
PermissionEvalResult::Ask
199224
} else {
200225
PermissionEvalResult::Allow
@@ -813,7 +838,13 @@ fn format_mode(mode: u32) -> [char; 9] {
813838

814839
#[cfg(test)]
815840
mod tests {
841+
use std::collections::{
842+
HashMap,
843+
HashSet,
844+
};
845+
816846
use super::*;
847+
use crate::cli::agent::ToolSettingTarget;
817848
use crate::cli::chat::util::test::{
818849
TEST_FILE_CONTENTS,
819850
TEST_FILE_PATH,
@@ -1323,6 +1354,7 @@ mod tests {
13231354
panic!("expected text output for batch operations");
13241355
}
13251356
}
1357+
13261358
#[tokio::test]
13271359
async fn test_fs_read_empty_operations() {
13281360
let os = Os::new().await.unwrap();
@@ -1343,4 +1375,49 @@ mod tests {
13431375
.contains("At least one operation must be provided")
13441376
);
13451377
}
1378+
1379+
#[test]
1380+
fn test_eval_perm() {
1381+
const DENIED_PATH_ONE: &str = "/some/denied/path";
1382+
const DENIED_PATH_GLOB: &str = "/denied/glob/**/path";
1383+
1384+
let agent = Agent {
1385+
name: "test_agent".to_string(),
1386+
allowed_tools: {
1387+
let mut allowed_tools = HashSet::<String>::new();
1388+
allowed_tools.insert("fs_read".to_string());
1389+
allowed_tools
1390+
},
1391+
tools_settings: {
1392+
let mut map = HashMap::<ToolSettingTarget, serde_json::Value>::new();
1393+
map.insert(
1394+
ToolSettingTarget("fs_read".to_string()),
1395+
serde_json::json!({
1396+
"deniedPaths": [DENIED_PATH_ONE, DENIED_PATH_GLOB]
1397+
}),
1398+
);
1399+
map
1400+
},
1401+
..Default::default()
1402+
};
1403+
1404+
let tool = serde_json::from_value::<FsRead>(serde_json::json!({
1405+
"operations": [
1406+
{ "path": DENIED_PATH_ONE, "mode": "Line", "start_line": 1, "end_line": 2 },
1407+
{ "path": "/denied/glob", "mode": "Directory" },
1408+
{ "path": "/denied/glob/child_one/path", "mode": "Directory" },
1409+
{ "path": "/denied/glob/child_one/grand_child_one/path", "mode": "Directory" },
1410+
{ "path": TEST_FILE_PATH, "mode": "Search", "pattern": "hello" }
1411+
],
1412+
}))
1413+
.unwrap();
1414+
1415+
let res = tool.eval_perm(&agent);
1416+
assert!(matches!(
1417+
res,
1418+
PermissionEvalResult::Deny(ref deny_list)
1419+
if deny_list.iter().filter(|p| *p == DENIED_PATH_GLOB).collect::<Vec<_>>().len() == 2
1420+
&& deny_list.iter().filter(|p| *p == DENIED_PATH_ONE).collect::<Vec<_>>().len() == 1
1421+
));
1422+
}
13461423
}

0 commit comments

Comments
 (0)