From 9355e281a08fd7d173e317a2a39778df7e7fc23d Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 21 Apr 2026 23:00:25 -0700 Subject: fix: require all subcommands covered for shell allow rules (#3440) --- crates/atuin-ai/src/permissions/check.rs | 15 ++-- crates/atuin-ai/src/permissions/shell.rs | 102 ++++++++++++++++-------- crates/atuin-ai/src/tools/mod.rs | 133 ++++++++++++++++++++++++++++++- 3 files changed, 208 insertions(+), 42 deletions(-) (limited to 'crates') diff --git a/crates/atuin-ai/src/permissions/check.rs b/crates/atuin-ai/src/permissions/check.rs index 6b908b93..96abc3ab 100644 --- a/crates/atuin-ai/src/permissions/check.rs +++ b/crates/atuin-ai/src/permissions/check.rs @@ -57,15 +57,12 @@ impl PermissionChecker { } } - for rule in &file.content.permissions.allow { - if request.call.matches_rule(rule) { - tracing::debug!( - "Permission 'ALLOW' by rule: {} in file: {}", - rule, - file.path.display() - ); - return Ok(PermissionResponse::Allowed); - } + if request.call.all_covered_by(&file.content.permissions.allow) { + tracing::debug!( + "Permission 'ALLOW' by rules in file: {}", + file.path.display() + ); + return Ok(PermissionResponse::Allowed); } } diff --git a/crates/atuin-ai/src/permissions/shell.rs b/crates/atuin-ai/src/permissions/shell.rs index 7a2eee2e..29b9f5d8 100644 --- a/crates/atuin-ai/src/permissions/shell.rs +++ b/crates/atuin-ai/src/permissions/shell.rs @@ -341,10 +341,22 @@ fn push_segment(segment: &mut String, commands: &mut Vec) { /// - `ls*` (no space before `*`) — matches `lsof`, `ls`, `ls -a` (prefix/glob) /// - `rm` (no wildcard) — matches exactly `rm` /// - `git * amend` — matches `git commit amend` (middle wildcard matches zero+ words) -pub(crate) fn any_subcommand_matches(subcommands: &[ShellCommand], scope: &str) -> bool { +/// +/// When `prefix_bare` is true, a bare pattern without wildcards (e.g. `rm`) +/// uses word-boundary prefix matching — `rm` matches `rm -rf /`. When false, +/// bare patterns require an exact match — `rm` only matches `rm`. +/// +/// Allow rules should pass `prefix_bare: false` (strict), while deny/ask rules +/// should pass `prefix_bare: true` (broad) so that denying `rm` also blocks +/// `rm -rf /`. +pub(crate) fn any_subcommand_matches( + subcommands: &[ShellCommand], + prefix_bare: bool, + scope: &str, +) -> bool { let scope = scope.trim(); - if scope == "*" { + if scope.is_empty() || scope == "*" { return true; } @@ -373,11 +385,16 @@ pub(crate) fn any_subcommand_matches(subcommands: &[ShellCommand], scope: &str) .any(|cmd| scope_matches_words(scope, cmd.full.split_whitespace().collect())); } - // No wildcard: word-boundary prefix match + // No wildcard: exact or prefix depending on context let scope_words: Vec<&str> = scope.split_whitespace().collect(); subcommands.iter().any(|cmd| { let cmd_words: Vec<&str> = cmd.full.split_whitespace().collect(); - cmd_words.len() >= scope_words.len() && cmd_words[..scope_words.len()] == scope_words[..] + if prefix_bare { + cmd_words.len() >= scope_words.len() + && cmd_words[..scope_words.len()] == scope_words[..] + } else { + cmd_words == scope_words + } }) } @@ -542,7 +559,7 @@ mod tests { full: "npm test".into(), }, ]; - assert!(any_subcommand_matches(&commands, "*")); + assert!(any_subcommand_matches(&commands, true, "*")); } #[test] @@ -557,11 +574,18 @@ mod tests { full: "npm test".into(), }, ]; - assert!(any_subcommand_matches(&commands, "git commit *")); - assert!(any_subcommand_matches(&commands, "git commit")); - assert!(!any_subcommand_matches(&commands, "git push *")); - assert!(!any_subcommand_matches(&commands, "git push")); - assert!(any_subcommand_matches(&commands, "npm *")); + assert!(any_subcommand_matches(&commands, true, "git commit *")); + assert!(!any_subcommand_matches(&commands, true, "git push *")); + assert!(!any_subcommand_matches(&commands, true, "git push")); + assert!(any_subcommand_matches(&commands, true, "npm *")); + assert!(any_subcommand_matches(&commands, true, "npm test")); + + // prefix_bare=true: bare "git commit" prefix-matches "git commit -m msg" (deny/ask) + assert!(any_subcommand_matches(&commands, true, "git commit")); + // prefix_bare=false: bare "git commit" does NOT match "git commit -m msg" (allow) + assert!(!any_subcommand_matches(&commands, false, "git commit")); + // Exact match works in both modes when command has no extra args + assert!(any_subcommand_matches(&commands, false, "npm test")); } #[test] @@ -577,12 +601,12 @@ mod tests { }, ]; // `ls *` — word boundary: matches `ls -a` but not `lsof` - assert!(any_subcommand_matches(&commands, "ls *")); - assert!(!any_subcommand_matches(&commands, "cat *")); - assert!(any_subcommand_matches(&commands, "lsof *")); + assert!(any_subcommand_matches(&commands, true, "ls *")); + assert!(!any_subcommand_matches(&commands, true, "cat *")); + assert!(any_subcommand_matches(&commands, true, "lsof *")); // `ls*` — glob/prefix: matches both `ls -a` and `lsof` - assert!(any_subcommand_matches(&commands, "ls*")); + assert!(any_subcommand_matches(&commands, true, "ls*")); } #[test] @@ -591,8 +615,8 @@ mod tests { name: "ls".into(), full: "ls".into(), }]; - assert!(any_subcommand_matches(&commands, "ls")); - assert!(!any_subcommand_matches(&commands, "cat")); + assert!(any_subcommand_matches(&commands, true, "ls")); + assert!(!any_subcommand_matches(&commands, true, "cat")); } #[cfg(feature = "tree-sitter")] @@ -678,9 +702,13 @@ mod tests { name: "git".into(), full: "git commit -m amend".into(), }]; - assert!(any_subcommand_matches(&commands, "git * amend")); - assert!(any_subcommand_matches(&commands, "git commit * amend")); - assert!(!any_subcommand_matches(&commands, "git push * amend")); + assert!(any_subcommand_matches(&commands, true, "git * amend")); + assert!(any_subcommand_matches( + &commands, + true, + "git commit * amend" + )); + assert!(!any_subcommand_matches(&commands, true, "git push * amend")); } #[test] @@ -690,7 +718,7 @@ mod tests { full: "git commit".into(), }]; // `*` matches zero words, so `git * commit` should match `git commit` - assert!(any_subcommand_matches(&commands, "git * commit")); + assert!(any_subcommand_matches(&commands, true, "git * commit")); } #[test] @@ -699,8 +727,8 @@ mod tests { name: "docker".into(), full: "docker run --rm alpine".into(), }]; - assert!(any_subcommand_matches(&commands, "* alpine")); - assert!(!any_subcommand_matches(&commands, "* ubuntu")); + assert!(any_subcommand_matches(&commands, true, "* alpine")); + assert!(!any_subcommand_matches(&commands, true, "* ubuntu")); } #[test] @@ -709,8 +737,12 @@ mod tests { name: "git".into(), full: "git rebase -i HEAD~5".into(), }]; - assert!(any_subcommand_matches(&commands, "git * -i * HEAD~5")); - assert!(!any_subcommand_matches(&commands, "git * -i * HEAD~10")); + assert!(any_subcommand_matches(&commands, true, "git * -i * HEAD~5")); + assert!(!any_subcommand_matches( + &commands, + true, + "git * -i * HEAD~10" + )); } } @@ -1232,7 +1264,7 @@ mod adversarial { full: "ls".into(), }]; // Empty scope matches everything (nothing to constrain) - assert!(any_subcommand_matches(&commands, "")); + assert!(any_subcommand_matches(&commands, true, "")); } #[test] @@ -1242,7 +1274,7 @@ mod adversarial { full: "ls".into(), }]; // " *" with empty prefix = match anything - assert!(any_subcommand_matches(&commands, " *")); + assert!(any_subcommand_matches(&commands, true, " *")); } #[test] @@ -1252,7 +1284,7 @@ mod adversarial { full: "ls".into(), }]; // `ls*` matches `ls` (prefix match with nothing after) - assert!(any_subcommand_matches(&commands, "ls*")); + assert!(any_subcommand_matches(&commands, true, "ls*")); } #[test] @@ -1262,7 +1294,7 @@ mod adversarial { name: "git".into(), full: "git commit".into(), }]; - assert!(any_subcommand_matches(&commands, "git * commit")); + assert!(any_subcommand_matches(&commands, true, "git * commit")); } #[test] @@ -1272,7 +1304,7 @@ mod adversarial { name: "git".into(), full: "git commit".into(), }]; - assert!(any_subcommand_matches(&commands, "git ** commit")); + assert!(any_subcommand_matches(&commands, true, "git ** commit")); } #[test] @@ -1281,8 +1313,14 @@ mod adversarial { name: "LS".into(), full: "LS -la".into(), }]; - assert!(!any_subcommand_matches(&commands, "ls")); - assert!(any_subcommand_matches(&commands, "LS")); + // Wildcard: case matters + assert!(!any_subcommand_matches(&commands, true, "ls *")); + assert!(any_subcommand_matches(&commands, true, "LS *")); + // prefix_bare=true: bare "LS" prefix-matches "LS -la" + assert!(!any_subcommand_matches(&commands, true, "ls")); + assert!(any_subcommand_matches(&commands, true, "LS")); + // prefix_bare=false: bare "LS" does NOT match "LS -la" + assert!(!any_subcommand_matches(&commands, false, "LS")); } #[test] @@ -1292,6 +1330,6 @@ mod adversarial { name: "git".into(), full: "git commit-amend".into(), }]; - assert!(!any_subcommand_matches(&commands, "git commit")); + assert!(!any_subcommand_matches(&commands, true, "git commit")); } } diff --git a/crates/atuin-ai/src/tools/mod.rs b/crates/atuin-ai/src/tools/mod.rs index 8a670be0..e66d64b8 100644 --- a/crates/atuin-ai/src/tools/mod.rs +++ b/crates/atuin-ai/src/tools/mod.rs @@ -248,6 +248,15 @@ impl ClientToolCall { pub(crate) trait PermissableToolCall { /// Checks if this tool call matches the given permission rule. fn matches_rule(&self, rule: &Rule) -> bool; + + /// Check if every part of this tool call is covered by at least one rule in + /// the set. For compound operations (e.g. shell pipelines), each sub-part + /// must be individually covered. The default treats the call as atomic — + /// any single matching rule is sufficient. + fn all_covered_by(&self, rules: &[Rule]) -> bool { + rules.iter().any(|r| self.matches_rule(r)) + } + /// Returns the target directory of this tool call, if applicable, for checking against directory-based rules. fn target_dir(&self) -> Option<&Path> { None @@ -259,6 +268,13 @@ impl PermissableToolCall for ClientToolCall { self.matches_rule(rule) } + fn all_covered_by(&self, rules: &[Rule]) -> bool { + match self { + ClientToolCall::Shell(tool) => tool.all_covered_by(rules), + _ => rules.iter().any(|r| self.matches_rule(r)), + } + } + fn target_dir(&self) -> Option<&Path> { self.target_dir() } @@ -771,7 +787,38 @@ impl PermissableToolCall for ShellToolCall { let shell_kind = crate::permissions::shell::ShellKind::from_shell_name(&self.shell); let parsed = crate::permissions::shell::parse_shell_command(&self.command, shell_kind); - crate::permissions::shell::any_subcommand_matches(&parsed.subcommands, scope) + // Deny/ask path: prefix_bare = true so `deny = ["Shell(rm)"]` blocks `rm -rf /` + crate::permissions::shell::any_subcommand_matches(&parsed.subcommands, true, scope) + } + + /// For compound shell commands, every subcommand must be individually + /// covered by at least one rule. This ensures that `allow = ["Shell(git *)"]` + /// does not silently permit `git add . && rm -rf /`. + fn all_covered_by(&self, rules: &[Rule]) -> bool { + use crate::permissions::shell; + + let shell_kind = shell::ShellKind::from_shell_name(&self.shell); + let parsed = shell::parse_shell_command(&self.command, shell_kind); + + // If parsing yields nothing, don't vacuously allow — fall through to ask. + !parsed.subcommands.is_empty() + && parsed.subcommands.iter().all(|subcmd| { + rules.iter().any(|rule| { + if rule.tool != "Shell" { + return false; + } + match rule.scope.as_deref() { + None | Some("*") => true, + // Allow path: prefix_bare = false so `Shell(git commit)` + // only allows exactly `git commit`, not `git commit --amend` + Some(scope) => shell::any_subcommand_matches( + std::slice::from_ref(subcmd), + false, + scope, + ), + } + }) + }) } } @@ -1237,6 +1284,90 @@ mod tests { assert!(!tool.matches_rule(&read_rule(Some("crates/**/*.py")))); } + // ── all_covered_by tests (compound shell command semantics) ── + + fn shell_rule(scope: Option<&str>) -> Rule { + Rule { + tool: "Shell".to_string(), + scope: scope.map(String::from), + } + } + + fn shell_tool(command: &str) -> ShellToolCall { + ShellToolCall { + dir: None, + command: command.to_string(), + shell: "bash".to_string(), + timeout_secs: 30, + description: None, + } + } + + #[test] + fn all_covered_by_simple_command() { + let rules = vec![shell_rule(Some("git *"))]; + assert!(shell_tool("git add .").all_covered_by(&rules)); + assert!(!shell_tool("npm test").all_covered_by(&rules)); + } + + #[test] + fn all_covered_by_compound_all_covered() { + let rules = vec![shell_rule(Some("git *")), shell_rule(Some("npm *"))]; + assert!(shell_tool("git add . && npm test").all_covered_by(&rules)); + } + + #[test] + fn all_covered_by_compound_partially_covered() { + // Only git is allowed — npm subcommand is not covered, so the + // compound command must not be auto-allowed. + let rules = vec![shell_rule(Some("git *"))]; + assert!(!shell_tool("git add . && npm test").all_covered_by(&rules)); + } + + #[test] + fn all_covered_by_unscoped_shell_rule() { + // Shell without scope covers everything + let rules = vec![shell_rule(None)]; + assert!(shell_tool("git add . && rm -rf /").all_covered_by(&rules)); + } + + #[test] + fn all_covered_by_wildcard_shell_rule() { + let rules = vec![shell_rule(Some("*"))]; + assert!(shell_tool("git add . && npm test").all_covered_by(&rules)); + } + + #[test] + fn all_covered_by_non_shell_tool_unchanged() { + // Non-shell tools use the default (any single rule matches) + let rules = vec![read_rule(Some("*.md"))]; + assert!(read_tool("notes.md").all_covered_by(&rules)); + assert!(!read_tool("notes.txt").all_covered_by(&rules)); + } + + #[test] + fn matches_rule_still_uses_any_semantics() { + // matches_rule (used for deny/ask) still triggers on any subcommand + let rule = shell_rule(Some("rm *")); + assert!(shell_tool("git add . && rm -rf /").matches_rule(&rule)); + } + + #[test] + fn bare_pattern_asymmetry() { + // Deny (matches_rule, prefix_bare=true): bare "rm" blocks "rm -rf /" + let deny_rule = shell_rule(Some("rm")); + assert!(shell_tool("rm -rf /").matches_rule(&deny_rule)); + + // Allow (all_covered_by, prefix_bare=false): bare "rm" only allows exactly "rm" + let allow_rules = vec![shell_rule(Some("rm"))]; + assert!(shell_tool("rm").all_covered_by(&allow_rules)); + assert!(!shell_tool("rm -rf /").all_covered_by(&allow_rules)); + + // Bare prefix match is word-boundary, not substring — "rm" must not match "rmbackup" + assert!(!shell_tool("rmbackup").matches_rule(&deny_rule)); + assert!(!shell_tool("rmbackup /tmp").matches_rule(&deny_rule)); + } + // ── Unix-specific tests (absolute paths with forward slashes) ── #[cfg(unix)] -- cgit v1.3.1