From 274b1f3ac950b275b30777fdae0bfb01178d9352 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Thu, 8 Jan 2026 22:49:15 -0500 Subject: [PATCH] Add tool permission evaluation logic (#46155) This builds on https://github.com/zed-industries/zed/pull/46112 (which should be merged first) and adds the permission evaluation logic and integrates it with the terminal tool as part of the tool permissions feature. This is a separate PR for [stacked diffs](https://newsletter.pragmaticengineer.com/p/stacked-diffs) to make review easier. ## Changes - **Add `tool_permissions.rs`** with `decide_permission()` function that implements: - `deny > confirm > allow` precedence hierarchy (security-critical) - Case-sensitive and case-insensitive regex matching - Integration with existing `always_allow_tool_actions` setting - Comprehensive unit tests (15 tests) - **Integrate with terminal tool**: - Commands matching deny rules are blocked immediately with an error - Commands matching allow rules proceed without confirmation dialog - Commands requiring confirmation show the dialog as before - Added integration tests for deny and allow rule scenarios Co-Authored-By: Claude Opus 4.5 Release Notes: - N/A --------- Co-authored-by: Amp --- crates/agent/src/agent.rs | 2 + crates/agent/src/tests/mod.rs | 235 +++++++++ crates/agent/src/thread.rs | 8 + crates/agent/src/tool_permissions.rs | 547 ++++++++++++++++++++ crates/agent/src/tools/terminal_tool.rs | 25 +- crates/agent_settings/src/agent_settings.rs | 151 +++++- 6 files changed, 940 insertions(+), 28 deletions(-) create mode 100644 crates/agent/src/tool_permissions.rs diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index 2975ffa0d26dbaa956c1668aa4087a753491729d..5a212352c7f3addf614bec789fd004ef3a9c4613 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -8,6 +8,7 @@ mod templates; #[cfg(test)] mod tests; mod thread; +mod tool_permissions; mod tools; use context_server::ContextServerId; @@ -16,6 +17,7 @@ pub use history_store::*; pub use native_agent_server::NativeAgentServer; pub use templates::*; pub use thread::*; +pub use tool_permissions::*; pub use tools::*; use acp_thread::{AcpThread, AgentModelSelector, UserMessageId}; diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 389f1e7624bd5a94196a10d0395c3bbea7ac6fbc..6a81d3daa0b4f37affc1cebb853010f6940718bf 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -93,6 +93,25 @@ impl FakeTerminalHandle { } } + fn new_with_immediate_exit(cx: &mut App, exit_code: u32) -> Self { + let killed = Arc::new(AtomicBool::new(false)); + let stopped_by_user = Arc::new(AtomicBool::new(false)); + let (exit_sender, _exit_receiver) = futures::channel::oneshot::channel(); + + let wait_for_exit = cx + .spawn(async move |_cx| acp::TerminalExitStatus::new().exit_code(exit_code)) + .shared(); + + Self { + killed, + stopped_by_user, + exit_sender: std::cell::RefCell::new(Some(exit_sender)), + wait_for_exit, + output: acp::TerminalOutputResponse::new("command output".to_string(), false), + id: acp::TerminalId::new("fake_terminal".to_string()), + } + } + fn was_killed(&self) -> bool { self.killed.load(Ordering::SeqCst) } @@ -3596,3 +3615,219 @@ async fn test_tokens_before_message_after_truncate(cx: &mut TestAppContext) { ); }); } + +#[gpui::test] +async fn test_terminal_tool_permission_rules(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree("/root", json!({})).await; + let project = Project::test(fs, ["/root".as_ref()], cx).await; + + // Test 1: Deny rule blocks command + { + let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_never_exits(cx))); + let environment = Rc::new(FakeThreadEnvironment { + handle: handle.clone(), + }); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.tool_permissions.tools.insert( + "terminal".into(), + agent_settings::ToolRules { + default_mode: settings::ToolPermissionMode::Confirm, + always_allow: vec![], + always_deny: vec![ + agent_settings::CompiledRegex::new(r"rm\s+-rf", false).unwrap(), + ], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + #[allow(clippy::arc_with_non_send_sync)] + let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment)); + let (event_stream, _rx) = crate::ToolCallEventStream::test(); + + let task = cx.update(|cx| { + tool.run( + crate::TerminalToolInput { + command: "rm -rf /".to_string(), + cd: ".".to_string(), + timeout_ms: None, + }, + event_stream, + cx, + ) + }); + + let result = task.await; + assert!( + result.is_err(), + "expected command to be blocked by deny rule" + ); + assert!( + result.unwrap_err().to_string().contains("blocked"), + "error should mention the command was blocked" + ); + } + + // Test 2: Allow rule skips confirmation (and overrides default_mode: Deny) + { + let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_with_immediate_exit(cx, 0))); + let environment = Rc::new(FakeThreadEnvironment { + handle: handle.clone(), + }); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.always_allow_tool_actions = false; + settings.tool_permissions.tools.insert( + "terminal".into(), + agent_settings::ToolRules { + default_mode: settings::ToolPermissionMode::Deny, + always_allow: vec![ + agent_settings::CompiledRegex::new(r"^echo\s", false).unwrap(), + ], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + #[allow(clippy::arc_with_non_send_sync)] + let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment)); + let (event_stream, mut rx) = crate::ToolCallEventStream::test(); + + let task = cx.update(|cx| { + tool.run( + crate::TerminalToolInput { + command: "echo hello".to_string(), + cd: ".".to_string(), + timeout_ms: None, + }, + event_stream, + cx, + ) + }); + + let update = rx.expect_update_fields().await; + assert!( + update.content.iter().any(|blocks| { + blocks + .iter() + .any(|c| matches!(c, acp::ToolCallContent::Terminal(_))) + }), + "expected terminal content (allow rule should skip confirmation and override default deny)" + ); + + let result = task.await; + assert!( + result.is_ok(), + "expected command to succeed without confirmation" + ); + } + + // Test 3: Confirm rule forces confirmation even with always_allow_tool_actions=true + { + let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_with_immediate_exit(cx, 0))); + let environment = Rc::new(FakeThreadEnvironment { + handle: handle.clone(), + }); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.always_allow_tool_actions = true; + settings.tool_permissions.tools.insert( + "terminal".into(), + agent_settings::ToolRules { + default_mode: settings::ToolPermissionMode::Allow, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![ + agent_settings::CompiledRegex::new(r"sudo", false).unwrap(), + ], + invalid_patterns: vec![], + }, + ); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + #[allow(clippy::arc_with_non_send_sync)] + let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment)); + let (event_stream, mut rx) = crate::ToolCallEventStream::test(); + + let _task = cx.update(|cx| { + tool.run( + crate::TerminalToolInput { + command: "sudo rm file".to_string(), + cd: ".".to_string(), + timeout_ms: None, + }, + event_stream, + cx, + ) + }); + + let auth = rx.expect_authorization().await; + assert!( + auth.tool_call.fields.title.is_some(), + "expected authorization request for sudo command despite always_allow_tool_actions=true" + ); + } + + // Test 4: default_mode: Deny blocks commands when no pattern matches + { + let handle = Rc::new(cx.update(|cx| FakeTerminalHandle::new_never_exits(cx))); + let environment = Rc::new(FakeThreadEnvironment { + handle: handle.clone(), + }); + + cx.update(|cx| { + let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); + settings.always_allow_tool_actions = true; + settings.tool_permissions.tools.insert( + "terminal".into(), + agent_settings::ToolRules { + default_mode: settings::ToolPermissionMode::Deny, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + agent_settings::AgentSettings::override_global(settings, cx); + }); + + #[allow(clippy::arc_with_non_send_sync)] + let tool = Arc::new(crate::TerminalTool::new(project.clone(), environment)); + let (event_stream, _rx) = crate::ToolCallEventStream::test(); + + let task = cx.update(|cx| { + tool.run( + crate::TerminalToolInput { + command: "echo hello".to_string(), + cd: ".".to_string(), + timeout_ms: None, + }, + event_stream, + cx, + ) + }); + + let result = task.await; + assert!( + result.is_err(), + "expected command to be blocked by default_mode: Deny" + ); + assert!( + result.unwrap_err().to_string().contains("disabled"), + "error should mention the tool is disabled" + ); + } +} diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index dcb7712da74b2ad7830f7ec69e594805d079c179..4ed1e537f0536c24e3b5ccd79e9d9fb052b293b2 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -2663,6 +2663,14 @@ impl ToolCallEventStream { return Task::ready(Ok(())); } + self.authorize_required(title, cx) + } + + /// Like `authorize`, but always prompts for confirmation regardless of + /// the `always_allow_tool_actions` setting. Use this when tool-specific + /// permission rules (like `always_confirm` patterns) have already determined + /// that confirmation is required. + pub fn authorize_required(&self, title: impl Into, cx: &mut App) -> Task> { let (response_tx, response_rx) = oneshot::channel(); self.stream .0 diff --git a/crates/agent/src/tool_permissions.rs b/crates/agent/src/tool_permissions.rs new file mode 100644 index 0000000000000000000000000000000000000000..1102610de92a90e0511a8bac5925d65667b8985a --- /dev/null +++ b/crates/agent/src/tool_permissions.rs @@ -0,0 +1,547 @@ +use agent_settings::{AgentSettings, ToolPermissions, ToolRules}; +use settings::ToolPermissionMode; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ToolPermissionDecision { + Allow, + Deny(String), + Confirm, +} + +/// Determines the permission decision for a tool invocation based on configured rules. +/// +/// # Precedence Order (highest to lowest) +/// +/// 1. **`always_deny`** - If any deny pattern matches, the tool call is blocked immediately. +/// This takes precedence over all other rules for security. +/// 2. **`always_confirm`** - If any confirm pattern matches (and no deny matched), +/// the user is prompted for confirmation regardless of other settings. +/// 3. **`always_allow`** - If any allow pattern matches (and no deny/confirm matched), +/// the tool call proceeds without prompting. +/// 4. **`default_mode`** - If no patterns match, falls back to the tool's default mode. +/// 5. **`always_allow_tool_actions`** - Global setting used as fallback when no tool-specific +/// rules are configured, or when `default_mode` is `Confirm`. +/// +/// # Pattern Matching Tips +/// +/// Patterns are matched as regular expressions against the tool input (e.g., the command +/// string for the terminal tool). Some tips for writing effective patterns: +/// +/// - Use word boundaries (`\b`) to avoid partial matches. For example, pattern `rm` will +/// match "storm" and "arms", but `\brm\b` will only match the standalone word "rm". +/// This is important for security rules where you want to block specific commands +/// without accidentally blocking unrelated commands that happen to contain the same +/// substring. +/// - Patterns are case-insensitive by default. Set `case_sensitive: true` for exact matching. +/// - Use `^` and `$` anchors to match the start/end of the input. +pub fn decide_permission( + tool_name: &str, + input: &str, + permissions: &ToolPermissions, + always_allow_tool_actions: bool, +) -> ToolPermissionDecision { + let rules = permissions.tools.get(tool_name); + + let rules = match rules { + Some(rules) => rules, + None => { + return if always_allow_tool_actions { + ToolPermissionDecision::Allow + } else { + ToolPermissionDecision::Confirm + }; + } + }; + + // Check for invalid regex patterns before evaluating rules. + // If any patterns failed to compile, block the tool call entirely. + if let Some(error) = check_invalid_patterns(tool_name, rules) { + return ToolPermissionDecision::Deny(error); + } + + if rules.always_deny.iter().any(|r| r.is_match(input)) { + return ToolPermissionDecision::Deny(format!( + "Command blocked by security rule for {} tool", + tool_name + )); + } + + if rules.always_confirm.iter().any(|r| r.is_match(input)) { + return ToolPermissionDecision::Confirm; + } + + if rules.always_allow.iter().any(|r| r.is_match(input)) { + return ToolPermissionDecision::Allow; + } + + match rules.default_mode { + ToolPermissionMode::Deny => { + ToolPermissionDecision::Deny(format!("{} tool is disabled", tool_name)) + } + ToolPermissionMode::Allow => ToolPermissionDecision::Allow, + ToolPermissionMode::Confirm => { + if always_allow_tool_actions { + ToolPermissionDecision::Allow + } else { + ToolPermissionDecision::Confirm + } + } + } +} + +/// Checks if the tool rules contain any invalid regex patterns. +/// Returns an error message if invalid patterns are found. +fn check_invalid_patterns(tool_name: &str, rules: &ToolRules) -> Option { + if rules.invalid_patterns.is_empty() { + return None; + } + + let count = rules.invalid_patterns.len(); + let pattern_word = if count == 1 { "pattern" } else { "patterns" }; + + Some(format!( + "The {} tool cannot run because {} regex {} failed to compile. \ + Please fix the invalid patterns in your tool_permissions settings.", + tool_name, count, pattern_word + )) +} + +/// Convenience wrapper that extracts permission settings from `AgentSettings`. +/// +/// This is the primary entry point for tools to check permissions. It extracts +/// `tool_permissions` and `always_allow_tool_actions` from the settings and +/// delegates to [`decide_permission`]. +pub fn decide_permission_from_settings( + tool_name: &str, + input: &str, + settings: &AgentSettings, +) -> ToolPermissionDecision { + decide_permission( + tool_name, + input, + &settings.tool_permissions, + settings.always_allow_tool_actions, + ) +} + +#[cfg(test)] +mod tests { + use super::*; + use agent_settings::{CompiledRegex, InvalidRegexPattern, ToolRules}; + use std::sync::Arc; + + fn empty_permissions() -> ToolPermissions { + ToolPermissions { + tools: collections::HashMap::default(), + } + } + + fn terminal_rules_with_deny(patterns: &[&str]) -> ToolPermissions { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Confirm, + always_allow: vec![], + always_deny: patterns + .iter() + .filter_map(|p| CompiledRegex::new(p, false)) + .collect(), + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + ToolPermissions { tools } + } + + fn terminal_rules_with_allow(patterns: &[&str]) -> ToolPermissions { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Confirm, + always_allow: patterns + .iter() + .filter_map(|p| CompiledRegex::new(p, false)) + .collect(), + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + ToolPermissions { tools } + } + + #[test] + fn test_deny_takes_precedence_over_allow() { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![CompiledRegex::new("dangerous", false).unwrap()], + always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + let decision = decide_permission("terminal", "run dangerous command", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + } + + #[test] + fn test_deny_takes_precedence_over_confirm() { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![], + always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()], + always_confirm: vec![CompiledRegex::new("dangerous", false).unwrap()], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + let decision = decide_permission("terminal", "run dangerous command", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + } + + #[test] + fn test_confirm_takes_precedence_over_allow() { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![CompiledRegex::new("risky", false).unwrap()], + always_deny: vec![], + always_confirm: vec![CompiledRegex::new("risky", false).unwrap()], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + let decision = decide_permission("terminal", "do risky thing", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Confirm); + } + + #[test] + fn test_no_tool_rules_uses_global_setting() { + let permissions = empty_permissions(); + + let decision = decide_permission("terminal", "any command", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Confirm); + + let decision = decide_permission("terminal", "any command", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Allow); + } + + #[test] + fn test_default_mode_fallthrough() { + // default_mode: Allow - should allow regardless of global setting + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + let decision = decide_permission("terminal", "any command", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Allow); + + // default_mode: Deny - should deny regardless of global setting + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Deny, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + let decision = decide_permission("terminal", "any command", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + // default_mode: Confirm - respects global always_allow_tool_actions + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Confirm, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + let decision = decide_permission("terminal", "any command", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Confirm); + let decision = decide_permission("terminal", "any command", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Allow); + } + + #[test] + fn test_empty_input() { + let permissions = terminal_rules_with_deny(&["rm"]); + + // Empty input doesn't match the deny pattern, so falls through to default_mode (Confirm) + let decision = decide_permission("terminal", "", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Confirm); + + // With always_allow_tool_actions=true and default_mode=Confirm, it returns Allow + let decision = decide_permission("terminal", "", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Allow); + } + + #[test] + fn test_multiple_patterns_any_match() { + // Multiple deny patterns - any match should deny + let permissions = terminal_rules_with_deny(&["rm", "dangerous", "delete"]); + + let decision = decide_permission("terminal", "run dangerous command", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + let decision = decide_permission("terminal", "delete file", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + // Multiple allow patterns - any match should allow + let permissions = terminal_rules_with_allow(&["^cargo", "^npm", "^git"]); + + let decision = decide_permission("terminal", "cargo build", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Allow); + + let decision = decide_permission("terminal", "npm install", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Allow); + + // No pattern matches - falls through to default + let decision = decide_permission("terminal", "rm file", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Confirm); + } + + #[test] + fn test_case_insensitive_matching() { + // Case-insensitive by default (case_sensitive: false) + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Confirm, + always_allow: vec![], + always_deny: vec![CompiledRegex::new(r"\brm\b", false).unwrap()], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + // Should match regardless of case + let decision = decide_permission("terminal", "RM file.txt", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + let decision = decide_permission("terminal", "Rm file.txt", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + let decision = decide_permission("terminal", "rm file.txt", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + } + + #[test] + fn test_case_sensitive_matching() { + // Case-sensitive matching when explicitly enabled + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Confirm, + always_allow: vec![], + always_deny: vec![CompiledRegex::new("DROP TABLE", true).unwrap()], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + // Should only match exact case + let decision = decide_permission("terminal", "DROP TABLE users", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + // Should NOT match different case + let decision = decide_permission("terminal", "drop table users", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Allow); + } + + #[test] + fn test_multi_tool_isolation() { + // Rules for terminal should not affect edit_file + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Deny, + always_allow: vec![], + always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + tools.insert( + Arc::from("edit_file"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + // Terminal with "dangerous" should be denied + let decision = decide_permission("terminal", "run dangerous command", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + // edit_file with "dangerous" should be allowed (no deny rules for edit_file) + let decision = decide_permission("edit_file", "dangerous_file.txt", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Allow); + + // Terminal without "dangerous" should still be denied due to default_mode: Deny + let decision = decide_permission("terminal", "safe command", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + } + + #[test] + fn test_invalid_patterns_block_tool() { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![CompiledRegex::new("echo", false).unwrap()], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![InvalidRegexPattern { + pattern: "[invalid(regex".to_string(), + rule_type: "always_deny".to_string(), + error: "unclosed character class".to_string(), + }], + }, + ); + let permissions = ToolPermissions { tools }; + + // Even though "echo" matches always_allow, the tool should be blocked + // because there are invalid patterns + let decision = decide_permission("terminal", "echo hello", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + + if let ToolPermissionDecision::Deny(msg) = decision { + assert!( + msg.contains("regex"), + "error message should mention regex: {}", + msg + ); + assert!( + msg.contains("settings"), + "error message should mention settings: {}", + msg + ); + assert!( + msg.contains("terminal"), + "error message should mention the tool name: {}", + msg + ); + } + } + + #[test] + fn test_same_pattern_in_deny_and_allow_deny_wins() { + // When the same pattern appears in both deny and allow lists, deny should win + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![CompiledRegex::new("deploy", false).unwrap()], + always_deny: vec![CompiledRegex::new("deploy", false).unwrap()], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + let decision = decide_permission("terminal", "deploy production", &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + } + + #[test] + fn test_same_pattern_in_confirm_and_allow_confirm_wins() { + // When the same pattern appears in both confirm and allow lists, confirm should win + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("terminal"), + ToolRules { + default_mode: ToolPermissionMode::Allow, + always_allow: vec![CompiledRegex::new("deploy", false).unwrap()], + always_deny: vec![], + always_confirm: vec![CompiledRegex::new("deploy", false).unwrap()], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + let decision = decide_permission("terminal", "deploy production", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Confirm); + } + + #[test] + fn test_partial_tool_name_does_not_match() { + // Rules for "term" should not affect "terminal" + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from("term"), + ToolRules { + default_mode: ToolPermissionMode::Deny, + always_allow: vec![], + always_deny: vec![], + always_confirm: vec![], + invalid_patterns: vec![], + }, + ); + let permissions = ToolPermissions { tools }; + + // "terminal" should not be affected by "term" rules, falls back to global setting + let decision = decide_permission("terminal", "echo hello", &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Allow); + + let decision = decide_permission("terminal", "echo hello", &permissions, false); + assert_eq!(decision, ToolPermissionDecision::Confirm); + } + + #[test] + fn test_very_long_input() { + // Test that very long inputs are handled correctly + let permissions = terminal_rules_with_deny(&[r"\brm\b"]); + + // Long input without the pattern should not match + let long_safe_input = "echo ".to_string() + &"a".repeat(100_000); + let decision = decide_permission("terminal", &long_safe_input, &permissions, true); + assert_eq!(decision, ToolPermissionDecision::Allow); + + // Long input with the pattern should match + let long_dangerous_input = "a".repeat(50_000) + " rm " + &"b".repeat(50_000); + let decision = decide_permission("terminal", &long_dangerous_input, &permissions, true); + assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + } +} diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index 914a0d1f3262334a69ce5cf6b0c8633149dcb61c..aaf7ea9ebb0c87979b7bce58ebaaaee1d0a1116f 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -1,10 +1,12 @@ use agent_client_protocol as acp; +use agent_settings::AgentSettings; use anyhow::Result; use futures::FutureExt as _; use gpui::{App, Entity, SharedString, Task}; use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use settings::Settings; use std::{ path::{Path, PathBuf}, rc::Rc, @@ -13,7 +15,10 @@ use std::{ }; use util::markdown::MarkdownInlineCode; -use crate::{AgentTool, ThreadEnvironment, ToolCallEventStream}; +use crate::{ + AgentTool, ThreadEnvironment, ToolCallEventStream, ToolPermissionDecision, + decide_permission_from_settings, +}; const COMMAND_OUTPUT_LIMIT: u64 = 16 * 1024; @@ -103,9 +108,23 @@ impl AgentTool for TerminalTool { Err(err) => return Task::ready(Err(err)), }; - let authorize = event_stream.authorize(self.initial_title(Ok(input.clone()), cx), cx); + let settings = AgentSettings::get_global(cx); + let decision = decide_permission_from_settings("terminal", &input.command, settings); + + let authorize = match decision { + ToolPermissionDecision::Allow => None, + ToolPermissionDecision::Deny(reason) => { + return Task::ready(Err(anyhow::anyhow!("{}", reason))); + } + ToolPermissionDecision::Confirm => { + // Use authorize_required since permission rules already determined confirmation is needed + Some(event_stream.authorize_required(self.initial_title(Ok(input.clone()), cx), cx)) + } + }; cx.spawn(async move |cx| { - authorize.await?; + if let Some(authorize) = authorize { + authorize.await?; + } let terminal = self .environment diff --git a/crates/agent_settings/src/agent_settings.rs b/crates/agent_settings/src/agent_settings.rs index fe24a66a6cda0d648c052447f3fdede7ee2ae3e6..eab9e59545daa6055f156b2626986895e8fe669a 100644 --- a/crates/agent_settings/src/agent_settings.rs +++ b/crates/agent_settings/src/agent_settings.rs @@ -162,12 +162,42 @@ pub struct ToolPermissions { pub tools: collections::HashMap, ToolRules>, } +impl ToolPermissions { + /// Returns all invalid regex patterns across all tools. + pub fn invalid_patterns(&self) -> Vec<&InvalidRegexPattern> { + self.tools + .values() + .flat_map(|rules| rules.invalid_patterns.iter()) + .collect() + } + + /// Returns true if any tool has invalid regex patterns. + pub fn has_invalid_patterns(&self) -> bool { + self.tools + .values() + .any(|rules| !rules.invalid_patterns.is_empty()) + } +} + +/// Represents a regex pattern that failed to compile. +#[derive(Clone, Debug)] +pub struct InvalidRegexPattern { + /// The pattern string that failed to compile. + pub pattern: String, + /// Which rule list this pattern was in (e.g., "always_deny", "always_allow", "always_confirm"). + pub rule_type: String, + /// The error message from the regex compiler. + pub error: String, +} + #[derive(Clone, Debug)] pub struct ToolRules { pub default_mode: ToolPermissionMode, pub always_allow: Vec, pub always_deny: Vec, pub always_confirm: Vec, + /// Patterns that failed to compile. If non-empty, tool calls should be blocked. + pub invalid_patterns: Vec, } impl Default for ToolRules { @@ -177,6 +207,7 @@ impl Default for ToolRules { always_allow: Vec::new(), always_deny: Vec::new(), always_confirm: Vec::new(), + invalid_patterns: Vec::new(), } } } @@ -199,15 +230,14 @@ impl std::fmt::Debug for CompiledRegex { impl CompiledRegex { pub fn new(pattern: &str, case_sensitive: bool) -> Option { + Self::try_new(pattern, case_sensitive).ok() + } + + pub fn try_new(pattern: &str, case_sensitive: bool) -> Result { let regex = regex::RegexBuilder::new(pattern) .case_insensitive(!case_sensitive) - .build() - .map_err(|e| { - log::warn!("Invalid regex pattern '{}': {}", pattern, e); - e - }) - .ok()?; - Some(Self { + .build()?; + Ok(Self { pattern: pattern.to_string(), case_sensitive, regex, @@ -272,20 +302,47 @@ fn compile_tool_permissions(content: Option) - .tools .into_iter() .map(|(tool_name, rules_content)| { - let rules = ToolRules { - default_mode: rules_content.default_mode.unwrap_or_default(), - always_allow: rules_content - .always_allow - .map(|v| compile_regex_rules(v.0)) - .unwrap_or_default(), - always_deny: rules_content - .always_deny - .map(|v| compile_regex_rules(v.0)) - .unwrap_or_default(), - always_confirm: rules_content + let mut invalid_patterns = Vec::new(); + + let (always_allow, allow_errors) = compile_regex_rules( + rules_content.always_allow.map(|v| v.0).unwrap_or_default(), + "always_allow", + ); + invalid_patterns.extend(allow_errors); + + let (always_deny, deny_errors) = compile_regex_rules( + rules_content.always_deny.map(|v| v.0).unwrap_or_default(), + "always_deny", + ); + invalid_patterns.extend(deny_errors); + + let (always_confirm, confirm_errors) = compile_regex_rules( + rules_content .always_confirm - .map(|v| compile_regex_rules(v.0)) + .map(|v| v.0) .unwrap_or_default(), + "always_confirm", + ); + invalid_patterns.extend(confirm_errors); + + // Log invalid patterns for debugging. Users will see an error when they + // attempt to use a tool with invalid patterns in their settings. + for invalid in &invalid_patterns { + log::error!( + "Invalid regex pattern in tool_permissions for '{}' tool ({}): '{}' - {}", + tool_name, + invalid.rule_type, + invalid.pattern, + invalid.error, + ); + } + + let rules = ToolRules { + default_mode: rules_content.default_mode.unwrap_or_default(), + always_allow, + always_deny, + always_confirm, + invalid_patterns, }; (tool_name, rules) }) @@ -294,11 +351,28 @@ fn compile_tool_permissions(content: Option) - ToolPermissions { tools } } -fn compile_regex_rules(rules: Vec) -> Vec { - rules - .into_iter() - .filter_map(|rule| CompiledRegex::new(&rule.pattern, rule.case_sensitive.unwrap_or(false))) - .collect() +fn compile_regex_rules( + rules: Vec, + rule_type: &str, +) -> (Vec, Vec) { + let mut compiled = Vec::new(); + let mut errors = Vec::new(); + + for rule in rules { + let case_sensitive = rule.case_sensitive.unwrap_or(false); + match CompiledRegex::try_new(&rule.pattern, case_sensitive) { + Ok(regex) => compiled.push(regex), + Err(error) => { + errors.push(InvalidRegexPattern { + pattern: rule.pattern, + rule_type: rule_type.to_string(), + error: error.to_string(), + }); + } + } + } + + (compiled, errors) } #[cfg(test)] @@ -448,13 +522,16 @@ mod tests { } #[test] - fn test_invalid_regex_is_skipped_not_fail() { + fn test_invalid_regex_is_tracked_and_valid_ones_still_compile() { let json = json!({ "tools": { "terminal": { "always_deny": [ { "pattern": "[invalid(regex" }, { "pattern": "valid_pattern" } + ], + "always_allow": [ + { "pattern": "[another_bad" } ] } } @@ -464,8 +541,32 @@ mod tests { let permissions = compile_tool_permissions(Some(content)); let terminal = permissions.tools.get("terminal").unwrap(); + + // Valid patterns should still be compiled assert_eq!(terminal.always_deny.len(), 1); assert!(terminal.always_deny[0].is_match("valid_pattern")); + + // Invalid patterns should be tracked (order depends on processing order) + assert_eq!(terminal.invalid_patterns.len(), 2); + + let deny_invalid = terminal + .invalid_patterns + .iter() + .find(|p| p.rule_type == "always_deny") + .expect("should have invalid pattern from always_deny"); + assert_eq!(deny_invalid.pattern, "[invalid(regex"); + assert!(!deny_invalid.error.is_empty()); + + let allow_invalid = terminal + .invalid_patterns + .iter() + .find(|p| p.rule_type == "always_allow") + .expect("should have invalid pattern from always_allow"); + assert_eq!(allow_invalid.pattern, "[another_bad"); + + // ToolPermissions helper methods should work + assert!(permissions.has_invalid_patterns()); + assert_eq!(permissions.invalid_patterns().len(), 2); } #[test]