diff --git a/crates/chat-cli/src/cli/agent/mod.rs b/crates/chat-cli/src/cli/agent/mod.rs index 02b984dd2d..3b6c7fbbee 100644 --- a/crates/chat-cli/src/cli/agent/mod.rs +++ b/crates/chat-cli/src/cli/agent/mod.rs @@ -776,32 +776,14 @@ impl Agents { /// Returns a label to describe the permission status for a given tool. pub fn display_label(&self, tool_name: &str, origin: &ToolOrigin) -> String { - use crate::util::pattern_matching::matches_any_pattern; + use crate::util::tool_permission_checker::is_tool_in_allowlist; let tool_trusted = self.get_active().is_some_and(|a| { - if matches!(origin, &ToolOrigin::Native) { - return matches_any_pattern(&a.allowed_tools, tool_name); - } - - a.allowed_tools.iter().any(|name| { - name.strip_prefix("@").is_some_and(|remainder| { - remainder - .split_once(MCP_SERVER_TOOL_DELIMITER) - .is_some_and(|(_left, right)| right == tool_name) - || remainder == >::borrow(origin) - }) || { - if let Some(server_name) = name.strip_prefix("@").and_then(|s| s.split('/').next()) { - if server_name == >::borrow(origin) { - let tool_pattern = format!("@{}/{}", server_name, tool_name); - matches_any_pattern(&a.allowed_tools, &tool_pattern) - } else { - false - } - } else { - false - } - } - }) + let server_name = match origin { + ToolOrigin::Native => None, + _ => Some(>::borrow(origin)), + }; + is_tool_in_allowlist(&a.allowed_tools, tool_name, server_name) }); if tool_trusted || self.trust_all_tools { diff --git a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs index 976f2d3820..0e45205678 100644 --- a/crates/chat-cli/src/cli/chat/tools/custom_tool.rs +++ b/crates/chat-cli/src/cli/chat/tools/custom_tool.rs @@ -28,7 +28,6 @@ use crate::mcp_client::{ }; use crate::os::Os; use crate::util::MCP_SERVER_TOOL_DELIMITER; -use crate::util::pattern_matching::matches_any_pattern; #[derive(Clone, Serialize, Deserialize, Debug, Eq, PartialEq, JsonSchema)] #[serde(rename_all = "camelCase")] @@ -175,18 +174,12 @@ impl CustomTool { } pub fn eval_perm(&self, _os: &Os, agent: &Agent) -> PermissionEvalResult { - let server_name = &self.server_name; + use crate::util::tool_permission_checker::is_tool_in_allowlist; - let server_pattern = format!("@{server_name}"); - if agent.allowed_tools.contains(&server_pattern) { - return PermissionEvalResult::Allow; - } - - let tool_pattern = self.namespaced_tool_name(); - if matches_any_pattern(&agent.allowed_tools, &tool_pattern) { - return PermissionEvalResult::Allow; + if is_tool_in_allowlist(&agent.allowed_tools, &self.name, Some(&self.server_name)) { + PermissionEvalResult::Allow + } else { + PermissionEvalResult::Ask } - - PermissionEvalResult::Ask } } diff --git a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs index e53daa6ef6..200cac641a 100644 --- a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs +++ b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs @@ -23,7 +23,7 @@ use crate::cli::chat::tools::{ }; use crate::cli::chat::util::truncate_safe; use crate::os::Os; -use crate::util::pattern_matching::matches_any_pattern; +use crate::util::tool_permission_checker::is_tool_in_allowlist; // Platform-specific modules #[cfg(windows)] @@ -205,7 +205,7 @@ impl ExecuteCommand { let Self { command, .. } = self; let tool_name = if cfg!(windows) { "execute_cmd" } else { "execute_bash" }; - let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, tool_name); + let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, tool_name, None); match agent.tools_settings.get(tool_name) { Some(settings) => { let Settings { 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 5c5c0abf25..e6dc7e31ba 100644 --- a/crates/chat-cli/src/cli/chat/tools/fs_read.rs +++ b/crates/chat-cli/src/cli/chat/tools/fs_read.rs @@ -46,7 +46,7 @@ use crate::cli::chat::{ }; use crate::os::Os; use crate::util::directories; -use crate::util::pattern_matching::matches_any_pattern; +use crate::util::tool_permission_checker::is_tool_in_allowlist; #[derive(Debug, Clone, Deserialize)] pub struct FsRead { @@ -113,7 +113,7 @@ impl FsRead { allow_read_only: bool, } - let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_read"); + let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, "fs_read", None); let settings = agent .tools_settings .get("fs_read") diff --git a/crates/chat-cli/src/cli/chat/tools/fs_write.rs b/crates/chat-cli/src/cli/chat/tools/fs_write.rs index 284706f43a..d23a957b60 100644 --- a/crates/chat-cli/src/cli/chat/tools/fs_write.rs +++ b/crates/chat-cli/src/cli/chat/tools/fs_write.rs @@ -45,7 +45,7 @@ use crate::cli::agent::{ use crate::cli::chat::line_tracker::FileLineTracker; use crate::os::Os; use crate::util::directories; -use crate::util::pattern_matching::matches_any_pattern; +use crate::util::tool_permission_checker::is_tool_in_allowlist; static SYNTAX_SET: LazyLock = LazyLock::new(SyntaxSet::load_defaults_newlines); static THEME_SET: LazyLock = LazyLock::new(ThemeSet::load_defaults); @@ -470,7 +470,7 @@ impl FsWrite { denied_paths: Vec, } - let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "fs_write"); + let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, "fs_write", None); match agent.tools_settings.get("fs_write") { Some(settings) => { let Settings { diff --git a/crates/chat-cli/src/cli/chat/tools/knowledge.rs b/crates/chat-cli/src/cli/chat/tools/knowledge.rs index 7bde4f0651..7f8428e6f8 100644 --- a/crates/chat-cli/src/cli/chat/tools/knowledge.rs +++ b/crates/chat-cli/src/cli/chat/tools/knowledge.rs @@ -20,7 +20,7 @@ use crate::cli::agent::{ use crate::database::settings::Setting; use crate::os::Os; use crate::util::knowledge_store::KnowledgeStore; -use crate::util::pattern_matching::matches_any_pattern; +use crate::util::tool_permission_checker::is_tool_in_allowlist; /// The Knowledge tool allows storing and retrieving information across chat sessions. /// It provides semantic search capabilities for files, directories, and text content. @@ -497,7 +497,7 @@ impl Knowledge { _ = self; _ = os; - if matches_any_pattern(&agent.allowed_tools, "knowledge") { + if is_tool_in_allowlist(&agent.allowed_tools, "knowledge", None) { PermissionEvalResult::Allow } else { PermissionEvalResult::Ask diff --git a/crates/chat-cli/src/cli/chat/tools/use_aws.rs b/crates/chat-cli/src/cli/chat/tools/use_aws.rs index 3bb9611ea4..b7390744cd 100644 --- a/crates/chat-cli/src/cli/chat/tools/use_aws.rs +++ b/crates/chat-cli/src/cli/chat/tools/use_aws.rs @@ -29,7 +29,7 @@ use crate::cli::agent::{ PermissionEvalResult, }; use crate::os::Os; -use crate::util::pattern_matching::matches_any_pattern; +use crate::util::tool_permission_checker::is_tool_in_allowlist; const READONLY_OPS: [&str; 6] = ["get", "describe", "list", "ls", "search", "batch_get"]; @@ -187,7 +187,7 @@ impl UseAws { } let Self { service_name, .. } = self; - let is_in_allowlist = matches_any_pattern(&agent.allowed_tools, "use_aws"); + let is_in_allowlist = is_tool_in_allowlist(&agent.allowed_tools, "use_aws", None); match agent.tools_settings.get("use_aws") { Some(settings) => { let settings = match serde_json::from_value::(settings.clone()) { diff --git a/crates/chat-cli/src/util/mod.rs b/crates/chat-cli/src/util/mod.rs index 648d90cad1..48d8c94c97 100644 --- a/crates/chat-cli/src/util/mod.rs +++ b/crates/chat-cli/src/util/mod.rs @@ -7,6 +7,7 @@ pub mod spinner; pub mod system_info; #[cfg(test)] pub mod test; +pub mod tool_permission_checker; pub mod ui; use std::fmt::Display; diff --git a/crates/chat-cli/src/util/tool_permission_checker.rs b/crates/chat-cli/src/util/tool_permission_checker.rs new file mode 100644 index 0000000000..f1cc04f895 --- /dev/null +++ b/crates/chat-cli/src/util/tool_permission_checker.rs @@ -0,0 +1,82 @@ +use std::collections::HashSet; + +use tracing::debug; + +use crate::util::MCP_SERVER_TOOL_DELIMITER; +use crate::util::pattern_matching::matches_any_pattern; + +/// Checks if a tool is allowed based on the agent's allowed_tools configuration. +/// This function handles both native tools and MCP tools with wildcard pattern support. +pub fn is_tool_in_allowlist(allowed_tools: &HashSet, tool_name: &str, server_name: Option<&str>) -> bool { + let filter_patterns = |predicate: fn(&str) -> bool| -> HashSet { + allowed_tools + .iter() + .filter(|pattern| predicate(pattern)) + .cloned() + .collect() + }; + + match server_name { + // Native tool + None => { + let patterns = filter_patterns(|p| !p.starts_with('@')); + debug!("Native patterns: {:?}", patterns); + let result = matches_any_pattern(&patterns, tool_name); + debug!("Native tool '{}' permission check result: {}", tool_name, result); + result + }, + // MCP tool + Some(server) => { + let patterns = filter_patterns(|p| p.starts_with('@')); + debug!("MCP patterns: {:?}", patterns); + + // Check server-level permission first: @server_name + let server_pattern = format!("@{}", server); + debug!("Checking server-level pattern: '{}'", server_pattern); + if matches_any_pattern(&patterns, &server_pattern) { + debug!("Server-level permission granted for '{}'", server_pattern); + return true; + } + + // Check tool-specific permission: @server_name/tool_name + let tool_pattern = format!("@{}{}{}", server, MCP_SERVER_TOOL_DELIMITER, tool_name); + debug!("Checking tool-specific pattern: '{}'", tool_pattern); + let result = matches_any_pattern(&patterns, &tool_pattern); + debug!("Tool-specific permission result for '{}': {}", tool_pattern, result); + result + }, + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use super::*; + + #[test] + fn test_native_vs_mcp_separation() { + let mut allowed = HashSet::new(); + allowed.insert("fs_*".to_string()); + allowed.insert("@git".to_string()); + + // Native patterns only apply to native tools + assert!(is_tool_in_allowlist(&allowed, "fs_read", None)); + assert!(!is_tool_in_allowlist(&allowed, "fs_read", Some("server"))); + + // MCP patterns only apply to MCP tools + assert!(is_tool_in_allowlist(&allowed, "status", Some("git"))); + assert!(!is_tool_in_allowlist(&allowed, "git", None)); + } + + #[test] + fn test_mcp_wildcard_patterns() { + let mut allowed = HashSet::new(); + allowed.insert("@*quip*".to_string()); + allowed.insert("@git/read_*".to_string()); + + assert!(is_tool_in_allowlist(&allowed, "tool", Some("quip-server"))); + assert!(is_tool_in_allowlist(&allowed, "read_file", Some("git"))); + assert!(!is_tool_in_allowlist(&allowed, "write_file", Some("git"))); + } +}