This PR hardens the terminal tool's hardcoded security rules for destructive commands like `rm -rf /`, and adds path normalization to prevent traversal-based bypasses. ## Path normalization Adds `normalize_path` which resolves `..`, `.`, and redundant path separators, and `decide_permission_for_path` which checks permissions against both raw and normalized paths (taking the most restrictive result). This prevents attacks like `rm -rf /tmp/../../` which previously bypassed the `rm -rf /` rule. ## rm command expansion Adds `expand_rm_to_single_path_commands` which splits multi-argument rm commands into individual single-path commands for checking. This catches cases like `rm -rf /tmp /` where the dangerous path is the second argument. ## Regex hardening - **FLAGS**: Now accepts digits, underscores, and uppercase in long flags (e.g. `--no-preserve-root`) - **`--flag=value`**: Correctly matched as a single flag token - **Trailing flags**: Handles GNU rm's acceptance of flags after path operands (e.g. `rm / -rf`) - **`--` marker**: Detects end-of-options bypass attempts (e.g. `rm -rf -- /`) - **Whitespace**: Handles tabs and other whitespace, not just spaces ## `$HOME`/`${HOME}` handling Normalizes the suffix after `$HOME`/`${HOME}` variable references so that traversal attacks like `rm -rf $HOME/./` or `rm -rf ${HOME}/foo/..` are correctly detected. Release Notes: - Strengthened terminal security rules to detect path traversal attacks in destructive commands like `rm -rf`.
Unified · Split
| @@ -3,6 +3,7 @@ use crate::shell_parser::extract_commands; · 2 unmodified lines | |||
| 3 | use crate::tools::TerminalTool; | 3 | use crate::tools::TerminalTool; |
| 4 | use agent_settings::{AgentSettings, CompiledRegex, ToolPermissions, ToolRules}; | 4 | use agent_settings::{AgentSettings, CompiledRegex, ToolPermissions, ToolRules}; |
| 5 | use settings::ToolPermissionMode; | 5 | use settings::ToolPermissionMode; |
| 6 | use std::path::{Component, Path}; | ||
| 6 | use std::sync::LazyLock; | 7 | use std::sync::LazyLock; |
| 7 | use util::shell::ShellKind; | 8 | use util::shell::ShellKind; |
| 8 | 9 | ||
| @@ -16,24 +17,44 @@ pub struct HardcodedSecurityRules { · 7 unmodified lines | |||
| 16 | } | 17 | } |
| 17 | 18 | ||
| 18 | pub static HARDCODED_SECURITY_RULES: LazyLock<HardcodedSecurityRules> = LazyLock::new(|| { | 19 | pub static HARDCODED_SECURITY_RULES: LazyLock<HardcodedSecurityRules> = LazyLock::new(|| { |
| 20 | // Flag group matches any short flags (-rf, -rfv, -v, etc.) or long flags (--recursive, --force, etc.) | ||
| 21 | // This ensures extra flags like -rfv, -v -rf, --recursive --force don't bypass the rules. | ||
| 22 | const FLAGS: &str = r"(--[a-zA-Z0-9][-a-zA-Z0-9_]*(=[^\s]*)?\s+|-[a-zA-Z]+\s+)*"; | ||
| 23 | // Trailing flags that may appear after the path operand (GNU rm accepts flags after operands) | ||
| 24 | const TRAILING_FLAGS: &str = r"(\s+--[a-zA-Z0-9][-a-zA-Z0-9_]*(=[^\s]*)?|\s+-[a-zA-Z]+)*\s*"; | ||
| 25 | |||
| 19 | HardcodedSecurityRules { | 26 | HardcodedSecurityRules { |
| 20 | // Case-insensitive; `(-[rf]+\s+)*` handles `-rf`, `-fr`, `-RF`, `-r -f`, etc. | ||
| 21 | terminal_deny: vec![ | 27 | terminal_deny: vec![ |
| 22 | // Recursive deletion of root - "rm -rf /" or "rm -rf / " | 28 | // Recursive deletion of root - "rm -rf /", "rm -rfv /", "rm -rf /*", "rm / -rf" |
| 23 | CompiledRegex::new(r"rm\s+(-[rf]+\s+)*/\s*$", false) | 29 | CompiledRegex::new( |
| 24 | .expect("hardcoded regex should compile"), | 30 | &format!(r"\brm\s+{FLAGS}(--\s+)?/\*?{TRAILING_FLAGS}$"), |
| 25 | // Recursive deletion of home - "rm -rf ~" or "rm -rf ~/" (but not ~/subdir) | 31 | false, |
| 26 | CompiledRegex::new(r"rm\s+(-[rf]+\s+)*~/?\s*$", false) | 32 | ) |
| 27 | .expect("hardcoded regex should compile"), | 33 | .expect("hardcoded regex should compile"), |
| 28 | // Recursive deletion of home via $HOME - "rm -rf $HOME" or "rm -rf ${HOME}" | 34 | // Recursive deletion of home - "rm -rf ~" or "rm -rf ~/" or "rm -rf ~/*" or "rm ~ -rf" (but not ~/subdir) |
| 29 | CompiledRegex::new(r"rm\s+(-[rf]+\s+)*(\$HOME|\$\{HOME\})/?\s*$", false) | 35 | CompiledRegex::new( |
| 30 | .expect("hardcoded regex should compile"), | 36 | &format!(r"\brm\s+{FLAGS}(--\s+)?~/?\*?{TRAILING_FLAGS}$"), |
| 31 | // Recursive deletion of current directory - "rm -rf ." or "rm -rf ./" | 37 | false, |
| 32 | CompiledRegex::new(r"rm\s+(-[rf]+\s+)*\./?\s*$", false) | 38 | ) |
| 33 | .expect("hardcoded regex should compile"), | 39 | .expect("hardcoded regex should compile"), |
| 34 | // Recursive deletion of parent directory - "rm -rf .." or "rm -rf ../" | 40 | // Recursive deletion of home via $HOME - "rm -rf $HOME" or "rm -rf ${HOME}" or "rm $HOME -rf" or with /* |
| 35 | CompiledRegex::new(r"rm\s+(-[rf]+\s+)*\.\./?\s*$", false) | 41 | CompiledRegex::new( |
| 36 | .expect("hardcoded regex should compile"), | 42 | &format!(r"\brm\s+{FLAGS}(--\s+)?(\$HOME|\$\{{HOME\}})/?(\*)?{TRAILING_FLAGS}$"), |
| 43 | false, | ||
| 44 | ) | ||
| 45 | .expect("hardcoded regex should compile"), | ||
| 46 | // Recursive deletion of current directory - "rm -rf ." or "rm -rf ./" or "rm -rf ./*" or "rm . -rf" | ||
| 47 | CompiledRegex::new( | ||
| 48 | &format!(r"\brm\s+{FLAGS}(--\s+)?\./?\*?{TRAILING_FLAGS}$"), | ||
| 49 | false, | ||
| 50 | ) | ||
| 51 | .expect("hardcoded regex should compile"), | ||
| 52 | // Recursive deletion of parent directory - "rm -rf .." or "rm -rf ../" or "rm -rf ../*" or "rm .. -rf" | ||
| 53 | CompiledRegex::new( | ||
| 54 | &format!(r"\brm\s+{FLAGS}(--\s+)?\.\./?\*?{TRAILING_FLAGS}$"), | ||
| 55 | false, | ||
| 56 | ) | ||
| 57 | .expect("hardcoded regex should compile"), | ||
| 37 | ], | 58 | ], |
| 38 | } | 59 | } |
| 39 | }); | 60 | }); |
| @@ -53,25 +74,21 @@ fn check_hardcoded_security_rules( · 13 unmodified lines | |||
| 53 | let rules = &*HARDCODED_SECURITY_RULES; | 74 | let rules = &*HARDCODED_SECURITY_RULES; |
| 54 | let terminal_patterns = &rules.terminal_deny; | 75 | let terminal_patterns = &rules.terminal_deny; |
| 55 | 76 | ||
| 56 | // First: check the original input as-is | 77 | // First: check the original input as-is (and its path-normalized form) |
| 57 | for pattern in terminal_patterns { | 78 | if matches_hardcoded_patterns(input, terminal_patterns) { |
| 58 | if pattern.is_match(input) { | 79 | return Some(ToolPermissionDecision::Deny( |
| 59 | return Some(ToolPermissionDecision::Deny( | 80 | HARDCODED_SECURITY_DENIAL_MESSAGE.into(), |
| 60 | HARDCODED_SECURITY_DENIAL_MESSAGE.into(), | 81 | )); |
| 61 | )); | ||
| 62 | } | ||
| 63 | } | 82 | } |
| 64 | 83 | ||
| 65 | // Second: parse and check individual sub-commands (for chained commands) | 84 | // Second: parse and check individual sub-commands (for chained commands) |
| 66 | if shell_kind.supports_posix_chaining() { | 85 | if shell_kind.supports_posix_chaining() { |
| 67 | if let Some(commands) = extract_commands(input) { | 86 | if let Some(commands) = extract_commands(input) { |
| 68 | for command in &commands { | 87 | for command in &commands { |
| 69 | for pattern in terminal_patterns { | 88 | if matches_hardcoded_patterns(command, terminal_patterns) { |
| 70 | if pattern.is_match(command) { | 89 | return Some(ToolPermissionDecision::Deny( |
| 71 | return Some(ToolPermissionDecision::Deny( | 90 | HARDCODED_SECURITY_DENIAL_MESSAGE.into(), |
| 72 | HARDCODED_SECURITY_DENIAL_MESSAGE.into(), | 91 | )); |
| 73 | )); | ||
| 74 | } | ||
| 75 | } | 92 | } |
| 76 | } | 93 | } |
| 77 | } | 94 | } |
| @@ -80,6 +97,105 @@ fn check_hardcoded_security_rules( · 2 unmodified lines | |||
| 80 | None | 97 | None |
| 81 | } | 98 | } |
| 82 | 99 | ||
| 100 | /// Checks a single command against hardcoded patterns, both as-is and with | ||
| 101 | /// path arguments normalized (to catch traversal bypasses like `rm -rf /tmp/../../` | ||
| 102 | /// and multi-path bypasses like `rm -rf /tmp /`). | ||
| 103 | fn matches_hardcoded_patterns(command: &str, patterns: &[CompiledRegex]) -> bool { | ||
| 104 | for pattern in patterns { | ||
| 105 | if pattern.is_match(command) { | ||
| 106 | return true; | ||
| 107 | } | ||
| 108 | } | ||
| 109 | |||
| 110 | for expanded in expand_rm_to_single_path_commands(command) { | ||
| 111 | for pattern in patterns { | ||
| 112 | if pattern.is_match(&expanded) { | ||
| 113 | return true; | ||
| 114 | } | ||
| 115 | } | ||
| 116 | } | ||
| 117 | |||
| 118 | false | ||
| 119 | } | ||
| 120 | |||
| 121 | /// For rm commands, expands multi-path arguments into individual single-path | ||
| 122 | /// commands with normalized paths. This catches both traversal bypasses like | ||
| 123 | /// `rm -rf /tmp/../../` and multi-path bypasses like `rm -rf /tmp /`. | ||
| 124 | fn expand_rm_to_single_path_commands(command: &str) -> Vec<String> { | ||
| 125 | let trimmed = command.trim(); | ||
| 126 | |||
| 127 | let first_token = trimmed.split_whitespace().next(); | ||
| 128 | if !first_token.is_some_and(|t| t.eq_ignore_ascii_case("rm")) { | ||
| 129 | return vec![]; | ||
| 130 | } | ||
| 131 | |||
| 132 | let parts: Vec<&str> = trimmed.split_whitespace().collect(); | ||
| 133 | let mut flags = Vec::new(); | ||
| 134 | let mut paths = Vec::new(); | ||
| 135 | let mut past_double_dash = false; | ||
| 136 | |||
| 137 | for part in parts.iter().skip(1) { | ||
| 138 | if !past_double_dash && *part == "--" { | ||
| 139 | past_double_dash = true; | ||
| 140 | flags.push(*part); | ||
| 141 | continue; | ||
| 142 | } | ||
| 143 | if !past_double_dash && part.starts_with('-') { | ||
| 144 | flags.push(*part); | ||
| 145 | } else { | ||
| 146 | paths.push(*part); | ||
| 147 | } | ||
| 148 | } | ||
| 149 | |||
| 150 | let flags_str = if flags.is_empty() { | ||
| 151 | String::new() | ||
| 152 | } else { | ||
| 153 | format!("{} ", flags.join(" ")) | ||
| 154 | }; | ||
| 155 | |||
| 156 | let mut results = Vec::new(); | ||
| 157 | for path in &paths { | ||
| 158 | if path.starts_with('$') { | ||
| 159 | let home_prefix = if path.starts_with("${HOME}") { | ||
| 160 | Some("${HOME}") | ||
| 161 | } else if path.starts_with("$HOME") { | ||
| 162 | Some("$HOME") | ||
| 163 | } else { | ||
| 164 | None | ||
| 165 | }; | ||
| 166 | |||
| 167 | if let Some(prefix) = home_prefix { | ||
| 168 | let suffix = &path[prefix.len()..]; | ||
| 169 | if suffix.is_empty() { | ||
| 170 | results.push(format!("rm {flags_str}{path}")); | ||
| 171 | } else if suffix.starts_with('/') { | ||
| 172 | let normalized_suffix = normalize_path(suffix); | ||
| 173 | let reconstructed = if normalized_suffix == "/" { | ||
| 174 | prefix.to_string() | ||
| 175 | } else { | ||
| 176 | format!("{prefix}{normalized_suffix}") | ||
| 177 | }; | ||
| 178 | results.push(format!("rm {flags_str}{reconstructed}")); | ||
| 179 | } else { | ||
| 180 | results.push(format!("rm {flags_str}{path}")); | ||
| 181 | } | ||
| 182 | } else { | ||
| 183 | results.push(format!("rm {flags_str}{path}")); | ||
| 184 | } | ||
| 185 | continue; | ||
| 186 | } | ||
| 187 | |||
| 188 | let mut normalized = normalize_path(path); | ||
| 189 | if normalized.is_empty() && !Path::new(path).has_root() { | ||
| 190 | normalized = ".".to_string(); | ||
| 191 | } | ||
| 192 | |||
| 193 | results.push(format!("rm {flags_str}{normalized}")); | ||
| 194 | } | ||
| 195 | |||
| 196 | results | ||
| 197 | } | ||
| 198 | |||
| 83 | #[derive(Debug, Clone, PartialEq, Eq)] | 199 | #[derive(Debug, Clone, PartialEq, Eq)] |
| 84 | pub enum ToolPermissionDecision { | 200 | pub enum ToolPermissionDecision { |
| 85 | Allow, | 201 | Allow, |
| @@ -92,28 +208,27 @@ impl ToolPermissionDecision { · 6 unmodified lines | |||
| 92 | /// | 208 | /// |
| 93 | /// # Precedence Order (highest to lowest) | 209 | /// # Precedence Order (highest to lowest) |
| 94 | /// | 210 | /// |
| 95 | /// 1. **`always_allow_tool_actions`** - When enabled, allows all tool actions without | 211 | /// 1. **Hardcoded security rules** - Critical safety checks (e.g., blocking `rm -rf /`) |
| 96 | /// prompting. This global setting bypasses all other checks including deny patterns. | 212 | /// that cannot be bypassed by any user settings, including `always_allow_tool_actions`. |
| 97 | /// Use with caution as it disables all security rules. | 213 | /// 2. **`always_allow_tool_actions`** - When enabled, allows all tool actions without |
| 98 | /// 2. **`always_deny`** - If any deny pattern matches, the tool call is blocked immediately. | 214 | /// prompting. This global setting bypasses user-configured deny/confirm/allow patterns, |
| 215 | /// but does **not** bypass hardcoded security rules. | ||
| 216 | /// 3. **`always_deny`** - If any deny pattern matches, the tool call is blocked immediately. | ||
| 99 | /// This takes precedence over `always_confirm` and `always_allow` patterns. | 217 | /// This takes precedence over `always_confirm` and `always_allow` patterns. |
| 100 | /// 3. **`always_confirm`** - If any confirm pattern matches (and no deny matched), | 218 | /// 4. **`always_confirm`** - If any confirm pattern matches (and no deny matched), |
| 101 | /// the user is prompted for confirmation. | 219 | /// the user is prompted for confirmation. |
| 102 | /// 4. **`always_allow`** - If any allow pattern matches (and no deny/confirm matched), | 220 | /// 5. **`always_allow`** - If any allow pattern matches (and no deny/confirm matched), |
| 103 | /// the tool call proceeds without prompting. | 221 | /// the tool call proceeds without prompting. |
| 104 | /// 5. **`default_mode`** - If no patterns match, falls back to the tool's default mode. | 222 | /// 6. **`default_mode`** - If no patterns match, falls back to the tool's default mode. |
| 105 | /// | 223 | /// |
| 106 | /// # Shell Compatibility (Terminal Tool Only) | 224 | /// # Shell Compatibility (Terminal Tool Only) |
| 107 | /// | 225 | /// |
| 108 | /// For the terminal tool, commands are parsed to extract sub-commands for security. | 226 | /// For the terminal tool, commands are parsed to extract sub-commands for security. |
| 109 | /// This parsing only works for shells with POSIX-like `&&` / `||` / `;` / `|` syntax: | 227 | /// All currently supported `ShellKind` variants are treated as compatible because |
| 110 | /// | 228 | /// brush-parser can handle their command chaining syntax. If a new `ShellKind` |
| 111 | /// **Compatible shells:** Posix (sh, bash, dash, zsh), Fish 3.0+, PowerShell 7+/Pwsh, | 229 | /// variant is added that brush-parser cannot safely parse, it should be excluded |
| 112 | /// Cmd, Xonsh, Csh, Tcsh | 230 | /// from `ShellKind::supports_posix_chaining()`, which will cause `always_allow` |
| 113 | /// | 231 | /// patterns to be disabled for that shell. |
| 114 | /// **Incompatible shells:** Nushell, Elvish, Rc (Plan 9) | ||
| 115 | /// | ||
| 116 | /// For incompatible shells, `always_allow` patterns are disabled for safety. | ||
| 117 | /// | 232 | /// |
| 118 | /// # Pattern Matching Tips | 233 | /// # Pattern Matching Tips |
| 119 | /// | 234 | /// |
| @@ -303,15 +418,119 @@ pub fn decide_permission_from_settings( · 183 unmodified lines | |||
| 303 | ) | 418 | ) |
| 304 | } | 419 | } |
| 305 | 420 | ||
| 421 | /// Normalizes a path by collapsing `.` and `..` segments without touching the filesystem. | ||
| 422 | fn normalize_path(raw: &str) -> String { | ||
| 423 | let is_absolute = Path::new(raw).has_root(); | ||
| 424 | let mut components: Vec<&str> = Vec::new(); | ||
| 425 | for component in Path::new(raw).components() { | ||
| 426 | match component { | ||
| 427 | Component::CurDir => {} | ||
| 428 | Component::ParentDir => { | ||
| 429 | if components.last() == Some(&"..") { | ||
| 430 | components.push(".."); | ||
| 431 | } else if !components.is_empty() { | ||
| 432 | components.pop(); | ||
| 433 | } else if !is_absolute { | ||
| 434 | components.push(".."); | ||
| 435 | } | ||
| 436 | } | ||
| 437 | Component::Normal(segment) => { | ||
| 438 | if let Some(s) = segment.to_str() { | ||
| 439 | components.push(s); | ||
| 440 | } | ||
| 441 | } | ||
| 442 | Component::RootDir | Component::Prefix(_) => {} | ||
| 443 | } | ||
| 444 | } | ||
| 445 | let joined = components.join("/"); | ||
| 446 | if is_absolute { | ||
| 447 | format!("/{joined}") | ||
| 448 | } else { | ||
| 449 | joined | ||
| 450 | } | ||
| 451 | } | ||
| 452 | |||
| 453 | /// Decides permission by checking both the raw input path and a simplified/canonicalized | ||
| 454 | /// version. Returns the most restrictive decision (Deny > Confirm > Allow). | ||
| 455 | pub fn decide_permission_for_path( | ||
| 456 | tool_name: &str, | ||
| 457 | raw_path: &str, | ||
| 458 | settings: &AgentSettings, | ||
| 459 | ) -> ToolPermissionDecision { | ||
| 460 | let raw_decision = decide_permission_from_settings(tool_name, raw_path, settings); | ||
| 461 | |||
| 462 | let simplified = normalize_path(raw_path); | ||
| 463 | if simplified == raw_path { | ||
| 464 | return raw_decision; | ||
| 465 | } | ||
| 466 | |||
| 467 | let simplified_decision = decide_permission_from_settings(tool_name, &simplified, settings); | ||
| 468 | |||
| 469 | most_restrictive(raw_decision, simplified_decision) | ||
| 470 | } | ||
| 471 | |||
| 472 | fn most_restrictive( | ||
| 473 | a: ToolPermissionDecision, | ||
| 474 | b: ToolPermissionDecision, | ||
| 475 | ) -> ToolPermissionDecision { | ||
| 476 | match (&a, &b) { | ||
| 477 | (ToolPermissionDecision::Deny(_), _) => a, | ||
| 478 | (_, ToolPermissionDecision::Deny(_)) => b, | ||
| 479 | (ToolPermissionDecision::Confirm, _) | (_, ToolPermissionDecision::Confirm) => { | ||
| 480 | ToolPermissionDecision::Confirm | ||
| 481 | } | ||
| 482 | _ => a, | ||
| 483 | } | ||
| 484 | } | ||
| 485 | |||
| 306 | #[cfg(test)] | 486 | #[cfg(test)] |
| 307 | mod tests { | 487 | mod tests { |
| 308 | use super::*; | 488 | use super::*; |
| 309 | use crate::AgentTool; | 489 | use crate::AgentTool; |
| 310 | use crate::pattern_extraction::extract_terminal_pattern; | 490 | use crate::pattern_extraction::extract_terminal_pattern; |
| 311 | use crate::tools::{EditFileTool, TerminalTool}; | 491 | use crate::tools::{EditFileTool, TerminalTool}; |
| 312 | use agent_settings::{CompiledRegex, InvalidRegexPattern, ToolRules}; | 492 | use agent_settings::{AgentProfileId, CompiledRegex, InvalidRegexPattern, ToolRules}; |
| 493 | use gpui::px; | ||
| 494 | use settings::{DefaultAgentView, DockPosition, DockSide, NotifyWhenAgentWaiting}; | ||
| 313 | use std::sync::Arc; | 495 | use std::sync::Arc; |
| 314 | 496 | ||
| 497 | fn test_agent_settings( | ||
| 498 | tool_permissions: ToolPermissions, | ||
| 499 | always_allow_tool_actions: bool, | ||
| 500 | ) -> AgentSettings { | ||
| 501 | AgentSettings { | ||
| 502 | enabled: true, | ||
| 503 | button: true, | ||
| 504 | dock: DockPosition::Right, | ||
| 505 | agents_panel_dock: DockSide::Left, | ||
| 506 | default_width: px(300.), | ||
| 507 | default_height: px(600.), | ||
| 508 | default_model: None, | ||
| 509 | inline_assistant_model: None, | ||
| 510 | inline_assistant_use_streaming_tools: false, | ||
| 511 | commit_message_model: None, | ||
| 512 | thread_summary_model: None, | ||
| 513 | inline_alternatives: vec![], | ||
| 514 | favorite_models: vec![], | ||
| 515 | default_profile: AgentProfileId::default(), | ||
| 516 | default_view: DefaultAgentView::Thread, | ||
| 517 | profiles: Default::default(), | ||
| 518 | always_allow_tool_actions, | ||
| 519 | notify_when_agent_waiting: NotifyWhenAgentWaiting::default(), | ||
| 520 | play_sound_when_agent_done: false, | ||
| 521 | single_file_review: false, | ||
| 522 | model_parameters: vec![], | ||
| 523 | enable_feedback: false, | ||
| 524 | expand_edit_card: true, | ||
| 525 | expand_terminal_card: true, | ||
| 526 | cancel_generation_on_terminal_stop: true, | ||
| 527 | use_modifier_to_send: true, | ||
| 528 | message_editor_min_lines: 1, | ||
| 529 | tool_permissions, | ||
| 530 | show_turn_stats: false, | ||
| 531 | } | ||
| 532 | } | ||
| 533 | |||
| 315 | fn pattern(command: &str) -> &'static str { | 534 | fn pattern(command: &str) -> &'static str { |
| 316 | Box::leak( | 535 | Box::leak( |
| 317 | extract_terminal_pattern(command) | 536 | extract_terminal_pattern(command) |
| @@ -1125,6 +1344,18 @@ mod tests { · 807 unmodified lines | |||
| 1125 | t("rm -r -f /").is_deny(); | 1344 | t("rm -r -f /").is_deny(); |
| 1126 | t("rm -f -r /").is_deny(); | 1345 | t("rm -f -r /").is_deny(); |
| 1127 | t("RM -RF /").is_deny(); | 1346 | t("RM -RF /").is_deny(); |
| 1347 | // Long flags | ||
| 1348 | t("rm --recursive --force /").is_deny(); | ||
| 1349 | t("rm --force --recursive /").is_deny(); | ||
| 1350 | // Extra short flags | ||
| 1351 | t("rm -rfv /").is_deny(); | ||
| 1352 | t("rm -v -rf /").is_deny(); | ||
| 1353 | // Glob wildcards | ||
| 1354 | t("rm -rf /*").is_deny(); | ||
| 1355 | t("rm -rf /* ").is_deny(); | ||
| 1356 | // End-of-options marker | ||
| 1357 | t("rm -rf -- /").is_deny(); | ||
| 1358 | t("rm -- /").is_deny(); | ||
| 1128 | } | 1359 | } |
| 1129 | 1360 | ||
| 1130 | #[test] | 1361 | #[test] |
| @@ -1141,6 +1372,40 @@ mod tests { · 10 unmodified lines | |||
| 1141 | t("rm -FR ${HOME}/").is_deny(); | 1372 | t("rm -FR ${HOME}/").is_deny(); |
| 1142 | t("rm -R -F ${HOME}/").is_deny(); | 1373 | t("rm -R -F ${HOME}/").is_deny(); |
| 1143 | t("RM -RF ~").is_deny(); | 1374 | t("RM -RF ~").is_deny(); |
| 1375 | // Long flags | ||
| 1376 | t("rm --recursive --force ~").is_deny(); | ||
| 1377 | t("rm --recursive --force ~/").is_deny(); | ||
| 1378 | t("rm --recursive --force $HOME").is_deny(); | ||
| 1379 | t("rm --force --recursive ${HOME}/").is_deny(); | ||
| 1380 | // Extra short flags | ||
| 1381 | t("rm -rfv ~").is_deny(); | ||
| 1382 | t("rm -v -rf ~/").is_deny(); | ||
| 1383 | // Glob wildcards | ||
| 1384 | t("rm -rf ~/*").is_deny(); | ||
| 1385 | t("rm -rf $HOME/*").is_deny(); | ||
| 1386 | t("rm -rf ${HOME}/*").is_deny(); | ||
| 1387 | // End-of-options marker | ||
| 1388 | t("rm -rf -- ~").is_deny(); | ||
| 1389 | t("rm -rf -- ~/").is_deny(); | ||
| 1390 | t("rm -rf -- $HOME").is_deny(); | ||
| 1391 | } | ||
| 1392 | |||
| 1393 | #[test] | ||
| 1394 | fn hardcoded_blocks_rm_rf_home_with_traversal() { | ||
| 1395 | // Path traversal after $HOME / ${HOME} should still be blocked | ||
| 1396 | t("rm -rf $HOME/./").is_deny(); | ||
| 1397 | t("rm -rf $HOME/foo/..").is_deny(); | ||
| 1398 | t("rm -rf ${HOME}/.").is_deny(); | ||
| 1399 | t("rm -rf ${HOME}/./").is_deny(); | ||
| 1400 | t("rm -rf $HOME/a/b/../..").is_deny(); | ||
| 1401 | t("rm -rf ${HOME}/foo/bar/../..").is_deny(); | ||
| 1402 | // Subdirectories should NOT be blocked | ||
| 1403 | t("rm -rf $HOME/subdir") | ||
| 1404 | .mode(ToolPermissionMode::Allow) | ||
| 1405 | .is_allow(); | ||
| 1406 | t("rm -rf ${HOME}/Documents") | ||
| 1407 | .mode(ToolPermissionMode::Allow) | ||
| 1408 | .is_allow(); | ||
| 1144 | } | 1409 | } |
| 1145 | 1410 | ||
| 1146 | #[test] | 1411 | #[test] |
| @@ -1156,6 +1421,18 @@ mod tests { · 9 unmodified lines | |||
| 1156 | t("rm -R -F ../").is_deny(); | 1421 | t("rm -R -F ../").is_deny(); |
| 1157 | t("RM -RF .").is_deny(); | 1422 | t("RM -RF .").is_deny(); |
| 1158 | t("RM -RF ..").is_deny(); | 1423 | t("RM -RF ..").is_deny(); |
| 1424 | // Long flags | ||
| 1425 | t("rm --recursive --force .").is_deny(); | ||
| 1426 | t("rm --force --recursive ../").is_deny(); | ||
| 1427 | // Extra short flags | ||
| 1428 | t("rm -rfv .").is_deny(); | ||
| 1429 | t("rm -v -rf ../").is_deny(); | ||
| 1430 | // Glob wildcards | ||
| 1431 | t("rm -rf ./*").is_deny(); | ||
| 1432 | t("rm -rf ../*").is_deny(); | ||
| 1433 | // End-of-options marker | ||
| 1434 | t("rm -rf -- .").is_deny(); | ||
| 1435 | t("rm -rf -- ../").is_deny(); | ||
| 1159 | } | 1436 | } |
| 1160 | 1437 | ||
| 1161 | #[test] | 1438 | #[test] |
| @@ -1210,4 +1487,320 @@ mod tests { · 48 unmodified lines | |||
| 1210 | t("echo hello; rm -rf .").is_deny(); | 1487 | t("echo hello; rm -rf .").is_deny(); |
| 1211 | t("echo hello; rm -rf ..").is_deny(); | 1488 | t("echo hello; rm -rf ..").is_deny(); |
| 1212 | } | 1489 | } |
| 1490 | |||
| 1491 | #[test] | ||
| 1492 | fn hardcoded_blocks_rm_with_trailing_flags() { | ||
| 1493 | // GNU rm accepts flags after operands by default | ||
| 1494 | t("rm / -rf").is_deny(); | ||
| 1495 | t("rm / -fr").is_deny(); | ||
| 1496 | t("rm / -RF").is_deny(); | ||
| 1497 | t("rm / -r -f").is_deny(); | ||
| 1498 | t("rm / --recursive --force").is_deny(); | ||
| 1499 | t("rm / -rfv").is_deny(); | ||
| 1500 | t("rm /* -rf").is_deny(); | ||
| 1501 | // Mixed: some flags before path, some after | ||
| 1502 | t("rm -r / -f").is_deny(); | ||
| 1503 | t("rm -f / -r").is_deny(); | ||
| 1504 | // Home | ||
| 1505 | t("rm ~ -rf").is_deny(); | ||
| 1506 | t("rm ~/ -rf").is_deny(); | ||
| 1507 | t("rm ~ -r -f").is_deny(); | ||
| 1508 | t("rm $HOME -rf").is_deny(); | ||
| 1509 | t("rm ${HOME} -rf").is_deny(); | ||
| 1510 | // Dot / dotdot | ||
| 1511 | t("rm . -rf").is_deny(); | ||
| 1512 | t("rm ./ -rf").is_deny(); | ||
| 1513 | t("rm . -r -f").is_deny(); | ||
| 1514 | t("rm .. -rf").is_deny(); | ||
| 1515 | t("rm ../ -rf").is_deny(); | ||
| 1516 | t("rm .. -r -f").is_deny(); | ||
| 1517 | // Trailing flags in chained commands | ||
| 1518 | t("ls && rm / -rf").is_deny(); | ||
| 1519 | t("echo hello; rm ~ -rf").is_deny(); | ||
| 1520 | // Safe paths with trailing flags should NOT be blocked | ||
| 1521 | t("rm ./build -rf") | ||
| 1522 | .mode(ToolPermissionMode::Allow) | ||
| 1523 | .is_allow(); | ||
| 1524 | t("rm /tmp/test -rf") | ||
| 1525 | .mode(ToolPermissionMode::Allow) | ||
| 1526 | .is_allow(); | ||
| 1527 | t("rm ~/Documents -rf") | ||
| 1528 | .mode(ToolPermissionMode::Allow) | ||
| 1529 | .is_allow(); | ||
| 1530 | } | ||
| 1531 | |||
| 1532 | #[test] | ||
| 1533 | fn hardcoded_blocks_rm_with_flag_equals_value() { | ||
| 1534 | // --flag=value syntax should not bypass the rules | ||
| 1535 | t("rm --no-preserve-root=yes -rf /").is_deny(); | ||
| 1536 | t("rm --no-preserve-root=yes --recursive --force /").is_deny(); | ||
| 1537 | t("rm -rf --no-preserve-root=yes /").is_deny(); | ||
| 1538 | t("rm --interactive=never -rf /").is_deny(); | ||
| 1539 | t("rm --no-preserve-root=yes -rf ~").is_deny(); | ||
| 1540 | t("rm --no-preserve-root=yes -rf .").is_deny(); | ||
| 1541 | t("rm --no-preserve-root=yes -rf ..").is_deny(); | ||
| 1542 | t("rm --no-preserve-root=yes -rf $HOME").is_deny(); | ||
| 1543 | // Trailing --flag=value after path | ||
| 1544 | t("rm / --no-preserve-root=yes -rf").is_deny(); | ||
| 1545 | t("rm ~ -rf --no-preserve-root=yes").is_deny(); | ||
| 1546 | // Safe paths with --flag=value should NOT be blocked | ||
| 1547 | t("rm --no-preserve-root=yes -rf ./build") | ||
| 1548 | .mode(ToolPermissionMode::Allow) | ||
| 1549 | .is_allow(); | ||
| 1550 | t("rm --interactive=never -rf /tmp/test") | ||
| 1551 | .mode(ToolPermissionMode::Allow) | ||
| 1552 | .is_allow(); | ||
| 1553 | } | ||
| 1554 | |||
| 1555 | #[test] | ||
| 1556 | fn hardcoded_blocks_rm_with_path_traversal() { | ||
| 1557 | // Traversal to root via .. | ||
| 1558 | t("rm -rf /tmp/../../").is_deny(); | ||
| 1559 | t("rm -rf /tmp/../..").is_deny(); | ||
| 1560 | t("rm -rf /var/log/../../").is_deny(); | ||
| 1561 | // Root via /./ | ||
| 1562 | t("rm -rf /./").is_deny(); | ||
| 1563 | t("rm -rf /.").is_deny(); | ||
| 1564 | // Double slash (equivalent to /) | ||
| 1565 | t("rm -rf //").is_deny(); | ||
| 1566 | // Home traversal via ~/./ | ||
| 1567 | t("rm -rf ~/./").is_deny(); | ||
| 1568 | t("rm -rf ~/.").is_deny(); | ||
| 1569 | // Dot traversal via indirect paths | ||
| 1570 | t("rm -rf ./foo/..").is_deny(); | ||
| 1571 | t("rm -rf ../foo/..").is_deny(); | ||
| 1572 | // Traversal in chained commands | ||
| 1573 | t("ls && rm -rf /tmp/../../").is_deny(); | ||
| 1574 | t("echo hello; rm -rf /./").is_deny(); | ||
| 1575 | // Traversal cannot be bypassed by global or allow patterns | ||
| 1576 | t("rm -rf /tmp/../../").global(true).is_deny(); | ||
| 1577 | t("rm -rf /./").allow(&[".*"]).is_deny(); | ||
| 1578 | // Safe paths with traversal should still be allowed | ||
| 1579 | t("rm -rf /tmp/../tmp/foo") | ||
| 1580 | .mode(ToolPermissionMode::Allow) | ||
| 1581 | .is_allow(); | ||
| 1582 | t("rm -rf ~/Documents/./subdir") | ||
| 1583 | .mode(ToolPermissionMode::Allow) | ||
| 1584 | .is_allow(); | ||
| 1585 | } | ||
| 1586 | |||
| 1587 | #[test] | ||
| 1588 | fn hardcoded_blocks_rm_multi_path_with_dangerous_last() { | ||
| 1589 | t("rm -rf /tmp /").is_deny(); | ||
| 1590 | t("rm -rf /tmp/foo /").is_deny(); | ||
| 1591 | t("rm -rf /var/log ~").is_deny(); | ||
| 1592 | t("rm -rf /safe $HOME").is_deny(); | ||
| 1593 | } | ||
| 1594 | |||
| 1595 | #[test] | ||
| 1596 | fn hardcoded_blocks_rm_multi_path_with_dangerous_first() { | ||
| 1597 | t("rm -rf / /tmp").is_deny(); | ||
| 1598 | t("rm -rf ~ /var/log").is_deny(); | ||
| 1599 | t("rm -rf . /tmp/foo").is_deny(); | ||
| 1600 | t("rm -rf .. /safe").is_deny(); | ||
| 1601 | } | ||
| 1602 | |||
| 1603 | #[test] | ||
| 1604 | fn hardcoded_allows_rm_multi_path_all_safe() { | ||
| 1605 | t("rm -rf /tmp /home/user") | ||
| 1606 | .mode(ToolPermissionMode::Allow) | ||
| 1607 | .is_allow(); | ||
| 1608 | t("rm -rf ./build ./dist") | ||
| 1609 | .mode(ToolPermissionMode::Allow) | ||
| 1610 | .is_allow(); | ||
| 1611 | t("rm -rf /var/log/app /tmp/cache") | ||
| 1612 | .mode(ToolPermissionMode::Allow) | ||
| 1613 | .is_allow(); | ||
| 1614 | } | ||
| 1615 | |||
| 1616 | #[test] | ||
| 1617 | fn hardcoded_blocks_rm_multi_path_with_traversal() { | ||
| 1618 | t("rm -rf /safe /tmp/../../").is_deny(); | ||
| 1619 | t("rm -rf /tmp/../../ /safe").is_deny(); | ||
| 1620 | t("rm -rf /safe /var/log/../../").is_deny(); | ||
| 1621 | } | ||
| 1622 | |||
| 1623 | #[test] | ||
| 1624 | fn normalize_path_relative_no_change() { | ||
| 1625 | assert_eq!(normalize_path("foo/bar"), "foo/bar"); | ||
| 1626 | } | ||
| 1627 | |||
| 1628 | #[test] | ||
| 1629 | fn normalize_path_relative_with_curdir() { | ||
| 1630 | assert_eq!(normalize_path("foo/./bar"), "foo/bar"); | ||
| 1631 | } | ||
| 1632 | |||
| 1633 | #[test] | ||
| 1634 | fn normalize_path_relative_with_parent() { | ||
| 1635 | assert_eq!(normalize_path("foo/bar/../baz"), "foo/baz"); | ||
| 1636 | } | ||
| 1637 | |||
| 1638 | #[test] | ||
| 1639 | fn normalize_path_absolute_preserved() { | ||
| 1640 | assert_eq!(normalize_path("/etc/passwd"), "/etc/passwd"); | ||
| 1641 | } | ||
| 1642 | |||
| 1643 | #[test] | ||
| 1644 | fn normalize_path_absolute_with_traversal() { | ||
| 1645 | assert_eq!(normalize_path("/tmp/../etc/passwd"), "/etc/passwd"); | ||
| 1646 | } | ||
| 1647 | |||
| 1648 | #[test] | ||
| 1649 | fn normalize_path_root() { | ||
| 1650 | assert_eq!(normalize_path("/"), "/"); | ||
| 1651 | } | ||
| 1652 | |||
| 1653 | #[test] | ||
| 1654 | fn normalize_path_parent_beyond_root_clamped() { | ||
| 1655 | assert_eq!(normalize_path("/../../../etc/passwd"), "/etc/passwd"); | ||
| 1656 | } | ||
| 1657 | |||
| 1658 | #[test] | ||
| 1659 | fn normalize_path_curdir_only() { | ||
| 1660 | assert_eq!(normalize_path("."), ""); | ||
| 1661 | } | ||
| 1662 | |||
| 1663 | #[test] | ||
| 1664 | fn normalize_path_empty() { | ||
| 1665 | assert_eq!(normalize_path(""), ""); | ||
| 1666 | } | ||
| 1667 | |||
| 1668 | #[test] | ||
| 1669 | fn normalize_path_relative_traversal_above_start() { | ||
| 1670 | assert_eq!(normalize_path("../../../etc/passwd"), "../../../etc/passwd"); | ||
| 1671 | } | ||
| 1672 | |||
| 1673 | #[test] | ||
| 1674 | fn normalize_path_relative_traversal_with_curdir() { | ||
| 1675 | assert_eq!(normalize_path("../../."), "../.."); | ||
| 1676 | } | ||
| 1677 | |||
| 1678 | #[test] | ||
| 1679 | fn normalize_path_relative_partial_traversal_above_start() { | ||
| 1680 | assert_eq!(normalize_path("foo/../../bar"), "../bar"); | ||
| 1681 | } | ||
| 1682 | |||
| 1683 | #[test] | ||
| 1684 | fn most_restrictive_deny_vs_allow() { | ||
| 1685 | assert!(matches!( | ||
| 1686 | most_restrictive( | ||
| 1687 | ToolPermissionDecision::Deny("x".into()), | ||
| 1688 | ToolPermissionDecision::Allow | ||
| 1689 | ), | ||
| 1690 | ToolPermissionDecision::Deny(_) | ||
| 1691 | )); | ||
| 1692 | } | ||
| 1693 | |||
| 1694 | #[test] | ||
| 1695 | fn most_restrictive_allow_vs_deny() { | ||
| 1696 | assert!(matches!( | ||
| 1697 | most_restrictive( | ||
| 1698 | ToolPermissionDecision::Allow, | ||
| 1699 | ToolPermissionDecision::Deny("x".into()) | ||
| 1700 | ), | ||
| 1701 | ToolPermissionDecision::Deny(_) | ||
| 1702 | )); | ||
| 1703 | } | ||
| 1704 | |||
| 1705 | #[test] | ||
| 1706 | fn most_restrictive_deny_vs_confirm() { | ||
| 1707 | assert!(matches!( | ||
| 1708 | most_restrictive( | ||
| 1709 | ToolPermissionDecision::Deny("x".into()), | ||
| 1710 | ToolPermissionDecision::Confirm | ||
| 1711 | ), | ||
| 1712 | ToolPermissionDecision::Deny(_) | ||
| 1713 | )); | ||
| 1714 | } | ||
| 1715 | |||
| 1716 | #[test] | ||
| 1717 | fn most_restrictive_confirm_vs_deny() { | ||
| 1718 | assert!(matches!( | ||
| 1719 | most_restrictive( | ||
| 1720 | ToolPermissionDecision::Confirm, | ||
| 1721 | ToolPermissionDecision::Deny("x".into()) | ||
| 1722 | ), | ||
| 1723 | ToolPermissionDecision::Deny(_) | ||
| 1724 | )); | ||
| 1725 | } | ||
| 1726 | |||
| 1727 | #[test] | ||
| 1728 | fn most_restrictive_deny_vs_deny() { | ||
| 1729 | assert!(matches!( | ||
| 1730 | most_restrictive( | ||
| 1731 | ToolPermissionDecision::Deny("a".into()), | ||
| 1732 | ToolPermissionDecision::Deny("b".into()) | ||
| 1733 | ), | ||
| 1734 | ToolPermissionDecision::Deny(_) | ||
| 1735 | )); | ||
| 1736 | } | ||
| 1737 | |||
| 1738 | #[test] | ||
| 1739 | fn most_restrictive_confirm_vs_allow() { | ||
| 1740 | assert_eq!( | ||
| 1741 | most_restrictive( | ||
| 1742 | ToolPermissionDecision::Confirm, | ||
| 1743 | ToolPermissionDecision::Allow | ||
| 1744 | ), | ||
| 1745 | ToolPermissionDecision::Confirm | ||
| 1746 | ); | ||
| 1747 | } | ||
| 1748 | |||
| 1749 | #[test] | ||
| 1750 | fn most_restrictive_allow_vs_confirm() { | ||
| 1751 | assert_eq!( | ||
| 1752 | most_restrictive( | ||
| 1753 | ToolPermissionDecision::Allow, | ||
| 1754 | ToolPermissionDecision::Confirm | ||
| 1755 | ), | ||
| 1756 | ToolPermissionDecision::Confirm | ||
| 1757 | ); | ||
| 1758 | } | ||
| 1759 | |||
| 1760 | #[test] | ||
| 1761 | fn most_restrictive_allow_vs_allow() { | ||
| 1762 | assert_eq!( | ||
| 1763 | most_restrictive(ToolPermissionDecision::Allow, ToolPermissionDecision::Allow), | ||
| 1764 | ToolPermissionDecision::Allow | ||
| 1765 | ); | ||
| 1766 | } | ||
| 1767 | |||
| 1768 | #[test] | ||
| 1769 | fn decide_permission_for_path_no_dots_early_return() { | ||
| 1770 | // When the path has no `.` or `..`, normalize_path returns the same string, | ||
| 1771 | // so decide_permission_for_path returns the raw decision directly. | ||
| 1772 | let settings = test_agent_settings( | ||
| 1773 | ToolPermissions { | ||
| 1774 | tools: Default::default(), | ||
| 1775 | }, | ||
| 1776 | false, | ||
| 1777 | ); | ||
| 1778 | let decision = decide_permission_for_path(EditFileTool::NAME, "src/main.rs", &settings); | ||
| 1779 | assert_eq!(decision, ToolPermissionDecision::Confirm); | ||
| 1780 | } | ||
| 1781 | |||
| 1782 | #[test] | ||
| 1783 | fn decide_permission_for_path_traversal_triggers_deny() { | ||
| 1784 | let deny_regex = CompiledRegex::new("/etc/passwd", false).unwrap(); | ||
| 1785 | let mut tools = collections::HashMap::default(); | ||
| 1786 | tools.insert( | ||
| 1787 | Arc::from(EditFileTool::NAME), | ||
| 1788 | ToolRules { | ||
| 1789 | default_mode: ToolPermissionMode::Allow, | ||
| 1790 | always_allow: vec![], | ||
| 1791 | always_deny: vec![deny_regex], | ||
| 1792 | always_confirm: vec![], | ||
| 1793 | invalid_patterns: vec![], | ||
| 1794 | }, | ||
| 1795 | ); | ||
| 1796 | let settings = test_agent_settings(ToolPermissions { tools }, false); | ||
| 1797 | |||
| 1798 | let decision = | ||
| 1799 | decide_permission_for_path(EditFileTool::NAME, "/tmp/../etc/passwd", &settings); | ||
| 1800 | assert!( | ||
| 1801 | matches!(decision, ToolPermissionDecision::Deny(_)), | ||
| 1802 | "expected Deny for traversal to /etc/passwd, got {:?}", | ||
| 1803 | decision | ||
| 1804 | ); | ||
| 1805 | } | ||
| 1213 | } | 1806 | } |