fix: require all subcommands covered for shell allow rules (#3440)

3 files changed, 208 insertions, 42 deletions

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<ShellCommand>) {
 /// - `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)]