Skip to content

Commit 5acd7e3

Browse files
nirajchowdharyNiraj Chowdhary
andauthored
fix: consolidate tool permission logic for consistent display and exe… (#2975)
* fix: consolidate tool permission logic for consistent display and execution * fix: centralize tool permission checking logic --------- Co-authored-by: Niraj Chowdhary <[email protected]>
1 parent bf183f0 commit 5acd7e3

File tree

9 files changed

+104
-46
lines changed

9 files changed

+104
-46
lines changed

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

Lines changed: 6 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -776,32 +776,14 @@ impl Agents {
776776

777777
/// Returns a label to describe the permission status for a given tool.
778778
pub fn display_label(&self, tool_name: &str, origin: &ToolOrigin) -> String {
779-
use crate::util::pattern_matching::matches_any_pattern;
779+
use crate::util::tool_permission_checker::is_tool_in_allowlist;
780780

781781
let tool_trusted = self.get_active().is_some_and(|a| {
782-
if matches!(origin, &ToolOrigin::Native) {
783-
return matches_any_pattern(&a.allowed_tools, tool_name);
784-
}
785-
786-
a.allowed_tools.iter().any(|name| {
787-
name.strip_prefix("@").is_some_and(|remainder| {
788-
remainder
789-
.split_once(MCP_SERVER_TOOL_DELIMITER)
790-
.is_some_and(|(_left, right)| right == tool_name)
791-
|| remainder == <ToolOrigin as Borrow<str>>::borrow(origin)
792-
}) || {
793-
if let Some(server_name) = name.strip_prefix("@").and_then(|s| s.split('/').next()) {
794-
if server_name == <ToolOrigin as Borrow<str>>::borrow(origin) {
795-
let tool_pattern = format!("@{}/{}", server_name, tool_name);
796-
matches_any_pattern(&a.allowed_tools, &tool_pattern)
797-
} else {
798-
false
799-
}
800-
} else {
801-
false
802-
}
803-
}
804-
})
782+
let server_name = match origin {
783+
ToolOrigin::Native => None,
784+
_ => Some(<ToolOrigin as Borrow<str>>::borrow(origin)),
785+
};
786+
is_tool_in_allowlist(&a.allowed_tools, tool_name, server_name)
805787
});
806788

807789
if tool_trusted || self.trust_all_tools {

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

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ use crate::mcp_client::{
2828
};
2929
use crate::os::Os;
3030
use crate::util::MCP_SERVER_TOOL_DELIMITER;
31-
use crate::util::pattern_matching::matches_any_pattern;
3231

3332
#[derive(Clone, Serialize, Deserialize, Debug, Eq, PartialEq, JsonSchema)]
3433
#[serde(rename_all = "camelCase")]
@@ -175,18 +174,12 @@ impl CustomTool {
175174
}
176175

177176
pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult {
178-
let server_name = &self.server_name;
177+
use crate::util::tool_permission_checker::is_tool_in_allowlist;
179178

180-
let server_pattern = format!("@{server_name}");
181-
if agent.allowed_tools.contains(&server_pattern) {
182-
return PermissionEvalResult::Allow;
183-
}
184-
185-
let tool_pattern = self.namespaced_tool_name();
186-
if matches_any_pattern(&agent.allowed_tools, &tool_pattern) {
187-
return PermissionEvalResult::Allow;
179+
if is_tool_in_allowlist(&agent.allowed_tools, &self.name, Some(&self.server_name)) {
180+
PermissionEvalResult::Allow
181+
} else {
182+
PermissionEvalResult::Ask
188183
}
189-
190-
PermissionEvalResult::Ask
191184
}
192185
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use crate::cli::chat::tools::{
2323
};
2424
use crate::cli::chat::util::truncate_safe;
2525
use crate::os::Os;
26-
use crate::util::pattern_matching::matches_any_pattern;
26+
use crate::util::tool_permission_checker::is_tool_in_allowlist;
2727

2828
// Platform-specific modules
2929
#[cfg(windows)]
@@ -205,7 +205,7 @@ impl ExecuteCommand {
205205

206206
let Self { command, .. } = self;
207207
let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" };
208-
let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, tool_name);
208+
let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, tool_name, None);
209209
match agent.tools_settings.get(tool_name) {
210210
Some(settings) => {
211211
let Settings {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ use crate::cli::chat::{
4646
};
4747
use crate::os::Os;
4848
use crate::util::directories;
49-
use crate::util::pattern_matching::matches_any_pattern;
49+
use crate::util::tool_permission_checker::is_tool_in_allowlist;
5050

5151
#[derive(Debug, Clone, Deserialize)]
5252
pub struct FsRead {
@@ -113,7 +113,7 @@ impl FsRead {
113113
allow_read_only: bool,
114114
}
115115

116-
let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read");
116+
let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, "fs_read", None);
117117
let settings = agent
118118
.tools_settings
119119
.get("fs_read")

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ use crate::cli::agent::{
4545
use crate::cli::chat::line_tracker::FileLineTracker;
4646
use crate::os::Os;
4747
use crate::util::directories;
48-
use crate::util::pattern_matching::matches_any_pattern;
48+
use crate::util::tool_permission_checker::is_tool_in_allowlist;
4949

5050
static SYNTAX_SET: LazyLock<SyntaxSet> = LazyLock::new(SyntaxSet::load_defaults_newlines);
5151
static THEME_SET: LazyLock<ThemeSet> = LazyLock::new(ThemeSet::load_defaults);
@@ -470,7 +470,7 @@ impl FsWrite {
470470
denied_paths: Vec<String>,
471471
}
472472

473-
let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_write");
473+
let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, "fs_write", None);
474474
match agent.tools_settings.get("fs_write") {
475475
Some(settings) => {
476476
let Settings {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use crate::cli::agent::{
2020
use crate::database::settings::Setting;
2121
use crate::os::Os;
2222
use crate::util::knowledge_store::KnowledgeStore;
23-
use crate::util::pattern_matching::matches_any_pattern;
23+
use crate::util::tool_permission_checker::is_tool_in_allowlist;
2424

2525
/// The Knowledge tool allows storing and retrieving information across chat sessions.
2626
/// It provides semantic search capabilities for files, directories, and text content.
@@ -497,7 +497,7 @@ impl Knowledge {
497497
_ = self;
498498
_ = os;
499499

500-
if matches_any_pattern(&agent.allowed_tools, "knowledge") {
500+
if is_tool_in_allowlist(&agent.allowed_tools, "knowledge", None) {
501501
PermissionEvalResult::Allow
502502
} else {
503503
PermissionEvalResult::Ask

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use crate::cli::agent::{
2929
PermissionEvalResult,
3030
};
3131
use crate::os::Os;
32-
use crate::util::pattern_matching::matches_any_pattern;
32+
use crate::util::tool_permission_checker::is_tool_in_allowlist;
3333

3434
const READONLY_OPS: [&str; 6] = ["get", "describe", "list", "ls", "search", "batch_get"];
3535

@@ -187,7 +187,7 @@ impl UseAws {
187187
}
188188

189189
let Self { service_name, .. } = self;
190-
let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "use_aws");
190+
let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, "use_aws", None);
191191
match agent.tools_settings.get("use_aws") {
192192
Some(settings) => {
193193
let settings = match serde_json::from_value::<Settings>(settings.clone()) {

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ pub mod spinner;
77
pub mod system_info;
88
#[cfg(test)]
99
pub mod test;
10+
pub mod tool_permission_checker;
1011
pub mod ui;
1112

1213
use std::fmt::Display;
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
use std::collections::HashSet;
2+
3+
use tracing::debug;
4+
5+
use crate::util::MCP_SERVER_TOOL_DELIMITER;
6+
use crate::util::pattern_matching::matches_any_pattern;
7+
8+
/// Checks if a tool is allowed based on the agent's allowed_tools configuration.
9+
/// This function handles both native tools and MCP tools with wildcard pattern support.
10+
pub fn is_tool_in_allowlist(allowed_tools: &HashSet<String>, tool_name: &str, server_name: Option<&str>) -> bool {
11+
let filter_patterns = |predicate: fn(&str) -> bool| -> HashSet<String> {
12+
allowed_tools
13+
.iter()
14+
.filter(|pattern| predicate(pattern))
15+
.cloned()
16+
.collect()
17+
};
18+
19+
match server_name {
20+
// Native tool
21+
None => {
22+
let patterns = filter_patterns(|p| !p.starts_with('@'));
23+
debug!("Native patterns: {:?}", patterns);
24+
let result = matches_any_pattern(&patterns, tool_name);
25+
debug!("Native tool '{}' permission check result: {}", tool_name, result);
26+
result
27+
},
28+
// MCP tool
29+
Some(server) => {
30+
let patterns = filter_patterns(|p| p.starts_with('@'));
31+
debug!("MCP patterns: {:?}", patterns);
32+
33+
// Check server-level permission first: @server_name
34+
let server_pattern = format!("@{}", server);
35+
debug!("Checking server-level pattern: '{}'", server_pattern);
36+
if matches_any_pattern(&patterns, &server_pattern) {
37+
debug!("Server-level permission granted for '{}'", server_pattern);
38+
return true;
39+
}
40+
41+
// Check tool-specific permission: @server_name/tool_name
42+
let tool_pattern = format!("@{}{}{}", server, MCP_SERVER_TOOL_DELIMITER, tool_name);
43+
debug!("Checking tool-specific pattern: '{}'", tool_pattern);
44+
let result = matches_any_pattern(&patterns, &tool_pattern);
45+
debug!("Tool-specific permission result for '{}': {}", tool_pattern, result);
46+
result
47+
},
48+
}
49+
}
50+
51+
#[cfg(test)]
52+
mod tests {
53+
use std::collections::HashSet;
54+
55+
use super::*;
56+
57+
#[test]
58+
fn test_native_vs_mcp_separation() {
59+
let mut allowed = HashSet::new();
60+
allowed.insert("fs_*".to_string());
61+
allowed.insert("@git".to_string());
62+
63+
// Native patterns only apply to native tools
64+
assert!(is_tool_in_allowlist(&allowed, "fs_read", None));
65+
assert!(!is_tool_in_allowlist(&allowed, "fs_read", Some("server")));
66+
67+
// MCP patterns only apply to MCP tools
68+
assert!(is_tool_in_allowlist(&allowed, "status", Some("git")));
69+
assert!(!is_tool_in_allowlist(&allowed, "git", None));
70+
}
71+
72+
#[test]
73+
fn test_mcp_wildcard_patterns() {
74+
let mut allowed = HashSet::new();
75+
allowed.insert("@*quip*".to_string());
76+
allowed.insert("@git/read_*".to_string());
77+
78+
assert!(is_tool_in_allowlist(&allowed, "tool", Some("quip-server")));
79+
assert!(is_tool_in_allowlist(&allowed, "read_file", Some("git")));
80+
assert!(!is_tool_in_allowlist(&allowed, "write_file", Some("git")));
81+
}
82+
}

0 commit comments

Comments
 (0)