diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index 4ca877cdb8..0f7a40b3b4 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -2,6 +2,7 @@ pub mod bash_arity; pub mod shell_expand; use std::collections::HashSet; +use std::sync::{Arc, RwLock}; use anyhow::Result; use bash_arity::BashArityDict; @@ -312,10 +313,20 @@ pub struct ExecPolicyContext<'a> { pub struct ExecPolicyEngine { /// Layered rulesets (builtin → agent → user). When non-empty, takes precedence /// over the legacy flat lists below. - rulesets: Vec, + /// + /// Shared behind an `Arc>` so that [`Self::set_ruleset`] applied + /// through one clone is observed by every clone. Hosts clone the engine + /// into long-lived side executors (nested sub-agent tool registries); a + /// plain `Vec` would leave those executors on a stale ruleset after a live + /// permission update, reopening an enforcement gap the parent no longer + /// has. + rulesets: Arc>>, /// Legacy flat lists kept for backward compatibility with `new()`. trusted_prefixes: Vec, denied_prefixes: Vec, + /// Deliberately clone-private, unlike `rulesets`: a remembered grant is a + /// decision the parent session made for its own calls, so it must not + /// silently authorize a delegated call in a cloned executor. approved_for_session: HashSet, /// Arity dictionary for command-prefix allow-rule matching. arity_dict: BashArityDict, @@ -325,7 +336,7 @@ impl ExecPolicyEngine { /// Legacy constructor: wraps the two vecs into a User-layer ruleset. pub fn new(trusted_prefixes: Vec, denied_prefixes: Vec) -> Self { Self { - rulesets: vec![], + rulesets: Arc::new(RwLock::new(vec![])), trusted_prefixes, denied_prefixes, approved_for_session: HashSet::new(), @@ -338,7 +349,7 @@ impl ExecPolicyEngine { pub fn with_rulesets(mut rulesets: Vec) -> Self { rulesets.sort_by_key(|r| r.layer); Self { - rulesets, + rulesets: Arc::new(RwLock::new(rulesets)), trusted_prefixes: vec![], denied_prefixes: vec![], approved_for_session: HashSet::new(), @@ -348,17 +359,39 @@ impl ExecPolicyEngine { /// Add a ruleset layer (re-sorts internally). pub fn add_ruleset(&mut self, ruleset: Ruleset) { - self.rulesets.push(ruleset); - self.rulesets.sort_by_key(|r| r.layer); + let mut guard = Self::lock_rulesets(&self.rulesets); + guard.push(ruleset); + guard.sort_by_key(|r| r.layer); } /// Replace the ruleset at one priority layer without clearing approvals /// remembered for the current session. pub fn set_ruleset(&mut self, ruleset: Ruleset) { + let mut guard = Self::lock_rulesets(&self.rulesets); + guard.retain(|existing| existing.layer != ruleset.layer); + guard.push(ruleset); + guard.sort_by_key(|existing| existing.layer); + } + + /// Lock the shared ruleset list for reading or writing. + /// + /// A poisoned lock (a panic held the guard mid-mutation) is recovered from: + /// the ruleset vec is plain data and a torn update sorts itself out on the + /// next `set_ruleset`, while refusing to answer policy checks would fail + /// closed for every command in the process. + fn lock_rulesets( + rulesets: &Arc>>, + ) -> std::sync::RwLockWriteGuard<'_, Vec> { + rulesets + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + + /// Read-only snapshot of the shared ruleset list. + fn read_rulesets(&self) -> std::sync::RwLockReadGuard<'_, Vec> { self.rulesets - .retain(|existing| existing.layer != ruleset.layer); - self.rulesets.push(ruleset); - self.rulesets.sort_by_key(|existing| existing.layer); + .read() + .unwrap_or_else(std::sync::PoisonError::into_inner) } /// Resolve the effective trusted/denied prefix sets by merging all rulesets. @@ -368,17 +401,19 @@ impl ExecPolicyEngine { /// semantics: any matching deny prefix blocks the command regardless of layer. /// Trusted rules are only consulted after deny checks pass. fn resolve_prefixes(&self) -> (Vec, Vec) { - if self.rulesets.is_empty() { + let rulesets = self.read_rulesets(); + if rulesets.is_empty() { return (self.trusted_prefixes.clone(), self.denied_prefixes.clone()); } // Collect all trusted/denied across all layers, highest-priority last so they // shadow lower-priority entries with the same prefix. let mut trusted: Vec = vec![]; let mut denied: Vec = vec![]; - for rs in &self.rulesets { + for rs in rulesets.iter() { trusted.extend(rs.trusted_prefixes.iter().cloned()); denied.extend(rs.denied_prefixes.iter().cloned()); } + drop(rulesets); // Also merge legacy flat lists as user-layer. trusted.extend(self.trusted_prefixes.iter().cloned()); denied.extend(self.denied_prefixes.iter().cloned()); @@ -391,7 +426,8 @@ impl ExecPolicyEngine { .path .and_then(|path| normalize_workspace_relative_path(path, ctx.cwd)); - self.rulesets + let rulesets = self.read_rulesets(); + let matched = rulesets .iter() .flat_map(|ruleset| { ruleset @@ -411,18 +447,31 @@ impl ExecPolicyEngine { None => true, }) .filter(|(_, rule)| match (rule.path.as_deref(), ctx.path) { - (Some(pattern), Some(_)) => match ( - normalize_workspace_relative_path(pattern, ctx.cwd), - normalized_path.as_deref(), - ) { - (Some(pattern), Some(path)) => pattern == path, - _ => false, - }, + (Some(pattern), Some(call_path)) => { + let ws_rule = normalize_workspace_relative_path(pattern, ctx.cwd); + match (ws_rule, normalized_path.as_deref()) { + // Workspace-relative normalization fails for a call + // outside the workspace or a rule that names one, and + // on a POSIX host a Windows-spelled rule/call pair + // parses as unrelated relative forms. A rule spelling + // an ABSOLUTE path must still be able to match such a + // call exactly, or pinned locations (a real home, + // `/root`, a Windows profile) are unmatchable. The + // helper only fires for rooted rules, so relative + // semantics are unchanged. + (Some(ws_rule), Some(ws_call)) => { + ws_rule == ws_call || absolute_path_rule_matches(pattern, call_path) + } + _ => absolute_path_rule_matches(pattern, call_path), + } + } (Some(_), None) => false, (None, _) => true, }) .max_by_key(|(layer, rule)| (*layer, rule.action, ask_rule_specificity(rule))) - .map(|(_, rule)| rule.clone()) + .map(|(_, rule)| (*rule).clone()); + drop(rulesets); + matched } /// Records an approval key for the current session so subsequent checks skip approval. @@ -726,6 +775,16 @@ fn command_is_chained(command: &str) -> bool { /// direction. Matching stays anchored at the first positional token, so a /// non-flag token that isn't in the rule ends it — `git push` does not block /// `git checkout push`, and `rm` does not block `rmdir`. +/// +/// Two rule-side spellings widen what a rule can name. cmd.exe-style +/// single-letter `/` flags (`del /f /s /q`) in the *command* are skippable like +/// `-` flags, in any position. And a rule token of exactly `*` is a middle +/// wildcard matching zero or more consecutive command tokens regardless of +/// shape, so a rule can anchor on a tail (`grep * ~/.ssh/id_rsa`, +/// `dd * of=/dev/sda`) without enumerating every flag spelling. A wildcard +/// widens the deny face of a rule — each one must be justified by the rule +/// author. This engine is deliberately permissive; the rulesets that feed it +/// own the false-positive discipline of keeping wildcards narrow. fn denied_prefix_matches(rule: &str, command: &str) -> bool { let rule_tokens: Vec = normalize_command(rule) .split_whitespace() @@ -758,6 +817,24 @@ fn denied_prefix_matches(rule: &str, command: &str) -> bool { if j == rule_tokens.len() { return true; } + // A rule token of exactly `*` is a middle wildcard: it matches zero or + // more consecutive command tokens regardless of shape — that is its + // point, since `grep -i PATTERN ~/.ssh/id_rsa` interleaves flags and + // positionals no flag rule could enumerate. `(i, j+1)` lets it match + // nothing; `(i+1, j)` skips one more command token. `seen` keeps the + // run of states finite. This branch runs BEFORE the end-of-command + // bail below so a trailing `*` can still match zero tokens once the + // command is exhausted, degrading to plain prefix semantics, and a + // wildcard is never itself treated as a command word. + if rule_tokens[j] == "*" { + if seen.insert((i, j)) { + stack.push((i, j + 1)); + if i < command_tokens.len() { + stack.push((i + 1, j)); + } + } + continue; + } if i >= command_tokens.len() || !seen.insert((i, j)) { continue; } @@ -776,11 +853,16 @@ fn denied_prefix_matches(rule: &str, command: &str) -> bool { if matches_rule_token { stack.push((i + 1, j + 1)); } - if token.starts_with('-') { + if token.starts_with('-') || is_single_letter_slash_flag(token) { // An unrelated flag is skippable — alone, and (when it could take // a separate value) together with the token after it. Consuming it - // as a rule token above takes priority, so a rule that names a flag - // (`cargo test --danger`) still matches it. + // as a rule token above takes priority, so a rule that names a + // flag (`cargo test --danger`) still matches it. cmd.exe spells + // its flags the same way shells spell paths, so only the + // single-letter shape (`/f`, `/s`, `/q`, `/y`) may skip; anything + // longer is a POSIX path (`/tmp`, `/etc`, `/usr`, `/dev`) and must + // stay positional, or `cp /tmp/new_key ~/.ssh/authorized_keys` + // would slip past a rule guarding `~/.ssh/authorized_keys`. stack.push((i + 1, j)); if !token.contains('=') { stack.push((i + 2, j)); @@ -792,6 +874,18 @@ fn denied_prefix_matches(rule: &str, command: &str) -> bool { false } +/// True for a cmd.exe-style single-letter flag such as `/f`, `/s`, `/q`, `/y`. +/// +/// cmd.exe flags are a slash plus exactly one letter (`del /f /s /q`, `xcopy +/// /e /y`), so only that shape may skip like a `-` flag. The narrowness is +/// load-bearing: multi-character `/`-tokens are real POSIX paths (`/tmp`, +/// `/etc`, `/usr`, `/dev`) and must keep matching positionally. Case needs no +/// handling here — `normalize_command` has already lowercased the token. +fn is_single_letter_slash_flag(token: &str) -> bool { + let bytes = token.as_bytes(); + bytes.len() == 2 && bytes[0] == b'/' && bytes[1].is_ascii_alphabetic() +} + /// Whether a command word matches a deny rule's command word. /// /// Exact first, then the command's basename — `/bin/rm`, `./rm`, and @@ -799,6 +893,13 @@ fn denied_prefix_matches(rule: &str, command: &str) -> bool { /// direction only: a rule that spells a path (`/usr/bin/rm`) still requires /// that path, because the rule author asked for it specifically. Both /// separators are honored so a Windows spelling cannot slip past. +/// +/// A trailing `.exe` on the command's basename also folds: Windows spells the +/// same binary `cat.exe` or `C:\Windows\System32\cat.exe`, and a `cat +/// ~/.ssh/id_rsa` rule must hold against that spelling too. The fold is one +/// direction only — when the RULE itself ends in `.exe` (`control.exe`) it +/// keeps requiring that spelling, and `catalog` never matches `cat` because +/// only a whole `.exe` suffix strips, never a prefix. fn command_word_matches(rule_token: &str, command_token: &str) -> bool { if command_token == rule_token { return true; @@ -807,10 +908,15 @@ fn command_word_matches(rule_token: &str, command_token: &str) -> bool { if rule_token.contains('/') || rule_token.contains('\\') { return false; } - let basename = command_token + let mut basename = command_token .rsplit(['/', '\\']) .next() .unwrap_or(command_token); + if !rule_token.ends_with(".exe") + && let Some(stem) = basename.strip_suffix(".exe") + { + basename = stem; + } !basename.is_empty() && basename == rule_token } @@ -1025,6 +1131,33 @@ fn is_windows_absolute_path(value: &str) -> bool { bytes.len() >= 3 && bytes[0].is_ascii_alphabetic() && bytes[1] == b':' && bytes[2] == b'/' } +/// Exact-match fallback for a typed path rule that names an ABSOLUTE path. +/// +/// The primary match normalizes both sides to workspace-relative form, which +/// only succeeds when the call lives inside the workspace — so a rule pinning +/// a location outside it (a real home, `/root`, another user's home, or a +/// literal `~` spelling the tool passed through unexpanded) could never match. +/// This fallback fires only when workspace normalization failed on either +/// side, and only for a ROOTED rule (leading `/`, `~`, or a Windows drive): +/// separators fold to `/`, case folds on case-insensitive platforms, and the +/// comparison is plain equality. A relative rule never reaches it, so +/// workspace-relative semantics are unchanged, and because there are no +/// wildcards the deny direction keeps its precision while the allow direction +/// can only ever match the exact path the rule spells. +fn absolute_path_rule_matches(rule_path: &str, call_path: &str) -> bool { + let fold = |value: &str| { + let value = value.trim().replace('\\', "/"); + if platform_paths_are_case_insensitive() { + value.to_ascii_lowercase() + } else { + value + } + }; + let rule = fold(rule_path); + let rooted = rule.starts_with('/') || rule.starts_with("~/") || is_windows_absolute_path(&rule); + rooted && rule == fold(call_path) +} + fn has_windows_drive_prefix(value: &str) -> bool { let bytes = value.as_bytes(); bytes.len() >= 2 && bytes[0].is_ascii_alphabetic() && bytes[1] == b':' @@ -1394,6 +1527,232 @@ mod tests { assert!(allowed.allow, "rmdir must not be denied: {allowed:?}"); } + #[test] + fn denied_prefix_skips_cmd_exe_single_letter_slash_flags() { + // cmd.exe spells its flags `/f`, `/s`, `/q` — a slash plus exactly one + // letter, in any order and position. A deny rule must hold against + // every interleaving (`del /f /s /q`, `del /q /s /f`, ...); the app + // would otherwise have to enumerate canonical flag sequences, so the + // engine skips the shape itself, like `-` flags. + let engine = ExecPolicyEngine::new( + vec![], + vec![ + r"del c:\users\x\file".to_string(), + r"xcopy c:\src d:\dst".to_string(), + ], + ); + for command in [ + r"del c:\users\x\file", + r"del /f c:\users\x\file", + r"del /f /s /q c:\users\x\file", + r"del /q /s /f c:\users\x\file", + r"del /f c:\users\x\file /s /q", + r"xcopy /e /y c:\src d:\dst", + ] { + let decision = engine.check(ctx(command, AskForApproval::Never)).unwrap(); + assert!( + !decision.allow, + "cmd.exe flag spelling evaded deny: {command:?} -> {decision:?}" + ); + } + // A rule that NAMES a `/x` flag still consumes it as a rule token — + // the rule-token branch is tried before the skip branches. + let named = ExecPolicyEngine::new(vec![], vec![r"del /q c:\x".to_string()]); + let decision = named + .check(ctx(r"del /q c:\x", AskForApproval::Never)) + .unwrap(); + assert!( + !decision.allow, + "rule naming a slash flag missed: {decision:?}" + ); + } + + #[test] + fn denied_prefix_slash_skipping_keeps_multi_char_slash_tokens_positional() { + // The single-letter constraint is load-bearing: `/tmp` is a POSIX + // directory, not a flag. If multi-character `/`-tokens skipped, an + // exfil command could hide its real operand behind a skipped path and + // slip past a rule guarding the sensitive target. + let engine = ExecPolicyEngine::new(vec![], vec!["cp ~/.ssh/authorized_keys".to_string()]); + for command in [ + "cp /tmp/new_key ~/.ssh/authorized_keys", + "cp /etc/passwd ~/.ssh/authorized_keys", + ] { + let decision = engine + .check(ctx(command, AskForApproval::UnlessTrusted)) + .unwrap(); + assert!( + decision.allow, + "POSIX path argument wrongly treated as a flag: {command:?} -> {decision:?}" + ); + } + // The guarded target itself still denies, skip branches or not. + let denied = engine + .check(ctx( + "cp ~/.ssh/authorized_keys ~/.ssh/authorized_keys.bak", + AskForApproval::Never, + )) + .unwrap(); + assert!(!denied.allow, "guarded target must stay denied: {denied:?}"); + } + + #[test] + fn denied_prefix_middle_wildcard_matches_zero_or_more_tokens() { + // A rule token of exactly `*` matches zero or more consecutive command + // tokens REGARDLESS of shape — flags, flag values, extra positionals — + // so a rule can anchor on its sensitive tail without the app + // enumerating every flag spelling. + let engine = ExecPolicyEngine::new( + vec![], + vec![ + "grep * ~/.ssh/id_rsa".to_string(), + "dd * of=/dev/sda".to_string(), + ], + ); + for command in [ + "grep root ~/.ssh/id_rsa", + "grep -i root ~/.ssh/id_rsa", + "grep -r root ~/.ssh/id_rsa", + // The wildcard matches nothing at all. + "grep ~/.ssh/id_rsa", + // `dd` has no dash flags at all: its operands are `key=value`. + "dd if=/dev/zero of=/dev/sda", + "dd if=boot.img bs=1M of=/dev/sda", + // A trailing `*` is allowed and degrades to plain prefix + // semantics: once reached, the rule matches. + "grep -i root ~/.ssh/id_rsa > /tmp/out", + ] { + let decision = engine.check(ctx(command, AskForApproval::Never)).unwrap(); + assert!( + !decision.allow, + "wildcard rule missed {command:?}: {decision:?}" + ); + } + + // A rule whose LAST token is `*` still matches a shorter command — + // prefix semantics, not suffix equality. + let trailing = ExecPolicyEngine::new(vec![], vec!["grep * ~/.ssh/id_rsa *".to_string()]); + for command in [ + "grep root ~/.ssh/id_rsa", + "grep -i root ~/.ssh/id_rsa backup", + ] { + let decision = trailing.check(ctx(command, AskForApproval::Never)).unwrap(); + assert!( + !decision.allow, + "trailing-wildcard rule missed {command:?}: {decision:?}" + ); + } + } + + #[test] + fn denied_prefix_wildcard_stays_anchored_on_the_tail_token() { + // The wildcard bridges the MIDDLE of a rule; it does not relax the + // tail. A rule is still a prefix match: when the tail token never + // appears in the segment, there is no deny — here or inside a chain. + let engine = ExecPolicyEngine::new(vec![], vec!["grep * /home/z".to_string()]); + for command in ["grep x /etc/y", "ls && grep x /etc/y"] { + let decision = engine + .check(ctx(command, AskForApproval::UnlessTrusted)) + .unwrap(); + assert!( + decision.allow, + "wildcard rule over-matched {command:?}: {decision:?}" + ); + } + // Chained segments are still scanned individually: a wildcard rule + // denies when its anchor appears in ANY segment, and does not leak + // across the chain boundary in either direction. + let chain = ExecPolicyEngine::new(vec![], vec!["grep * ~/.ssh/id_rsa".to_string()]); + let denied = chain + .check(ctx( + "echo hi && grep root ~/.ssh/id_rsa", + AskForApproval::Never, + )) + .unwrap(); + assert!(!denied.allow, "chained segment must still deny: {denied:?}"); + let shielded = chain + .check(ctx("grep x /etc/y && echo done", AskForApproval::Never)) + .unwrap(); + assert!( + shielded.allow, + "wildcard must not reach into unrelated segments: {shielded:?}" + ); + } + + #[test] + fn denied_prefix_leading_wildcard_follows_generic_wildcard_semantics() { + // Rules in practice anchor their first token, but a leading `*` is not + // an error: the generic DFS gives it the same two branches and it is + // never treated as a command word. Documented consequence of keeping + // the anchor at the rule's literal first token: the command word after + // a leading wildcard is matched exactly, so `/bin/rm` is NOT folded to + // `rm` for it. Rule authors should not start rules with `*`; this test + // only pins the behavior the generic DFS produces. + let engine = ExecPolicyEngine::new(vec![], vec!["* rm -rf /".to_string()]); + let bare = engine + .check(ctx("rm -rf /", AskForApproval::Never)) + .unwrap(); + assert!( + !bare.allow, + "leading-wildcard rule must match its bare spelling: {bare:?}" + ); + let path = engine + .check(ctx("/bin/rm -rf /", AskForApproval::Never)) + .unwrap(); + assert!( + path.allow, + "leading wildcard must not gain command-word folding: {path:?}" + ); + } + + #[test] + fn denied_prefix_folds_windows_exe_suffix_on_the_command_word() { + // Windows spells the same binary `cat.exe` or + // `C:\Windows\System32\cat.exe`; a `cat ~/.ssh/id_rsa` rule must hold + // against those spellings. The fold is one-directional: a rule that + // names `.exe` itself keeps requiring it, and only a WHOLE `.exe` + // suffix strips — `catalog` never becomes `cat`. + let engine = ExecPolicyEngine::new(vec![], vec!["cat ~/.ssh/id_rsa".to_string()]); + for command in [ + "cat ~/.ssh/id_rsa", + "cat.exe ~/.ssh/id_rsa", + "cat.EXE ~/.ssh/id_rsa", + r"C:\Windows\System32\cat.exe ~/.ssh/id_rsa", + ] { + let decision = engine.check(ctx(command, AskForApproval::Never)).unwrap(); + assert!( + !decision.allow, + "`.exe` spelling evaded deny: {command:?} -> {decision:?}" + ); + } + + // A rule ending in `.exe` must still require that spelling: the bare + // `control` is a different binary and must not match `control.exe`. + let control = ExecPolicyEngine::new(vec![], vec!["control.exe".to_string()]); + let spelled = control + .check(ctx("control.exe", AskForApproval::Never)) + .unwrap(); + assert!(!spelled.allow, "control.exe must be denied: {spelled:?}"); + let bare = control + .check(ctx("control", AskForApproval::UnlessTrusted)) + .unwrap(); + assert!( + bare.allow, + "bare `control` must not match rule `control.exe`: {bare:?}" + ); + + // Only a whole `.exe` suffix folds, never a word prefix. + for command in ["catalog ~/.ssh/id_rsa", "catalog.exe ~/.ssh/id_rsa"] { + let decision = engine + .check(ctx(command, AskForApproval::UnlessTrusted)) + .unwrap(); + assert!( + decision.allow, + "prefix word must not fold into the rule word: {command:?} -> {decision:?}" + ); + } + } + #[test] fn path_rules_respect_filesystem_case_sensitivity() { // #4725: on a case-sensitive filesystem `config/allowed.toml` and @@ -1889,6 +2248,162 @@ mod tests { assert!(decision.requires_approval); } + #[test] + fn typed_ask_absolute_path_rule_matches_absolute_call_outside_workspace() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_ask_rules( + vec![ToolAskRule { + tool: "read_file".into(), + command: None, + command_exact: false, + path: Some("/root/.ssh/config".into()), + workspace: None, + action: PermissionAction::Deny, + }], + )]); + + // An absolute rule must reach a call outside the workspace that the + // workspace-relative normalization cannot express. + let decision = engine + .check(ExecPolicyContext { + command: "", + cwd: "/workspace", + tool: Some("read_file"), + path: Some("/root/.ssh/config"), + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + assert_eq!(decision.matched_action, Some(PermissionAction::Deny)); + + // A different absolute path must not match. + let decision = engine + .check(ExecPolicyContext { + command: "", + cwd: "/workspace", + tool: Some("read_file"), + path: Some("/root/.ssh/known_hosts"), + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + assert_eq!(decision.matched_rule, None); + + // The fallback is exact: a traversal spelling of the same file is a + // different token string and must stay unmatchable (the documented + // "traversal is never matchable" stance, pinned through the rooted + // fallback too). + let decision = engine + .check(ExecPolicyContext { + command: "", + cwd: "/workspace", + tool: Some("read_file"), + path: Some("/root/../root/.ssh/config"), + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + assert_eq!(decision.matched_rule, None); + } + + #[test] + fn typed_ask_literal_tilde_rule_matches_unexpanded_call_spelling() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_ask_rules( + vec![ToolAskRule { + tool: "read_file".into(), + command: None, + command_exact: false, + path: Some("~/.ssh/config".into()), + workspace: None, + action: PermissionAction::Deny, + }], + )]); + + let decision = engine + .check(ExecPolicyContext { + command: "", + cwd: "/workspace", + tool: Some("read_file"), + path: Some("~/.ssh/config"), + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + assert_eq!(decision.matched_action, Some(PermissionAction::Deny)); + + // The tilde-rooted channel is exact as well: a traversal spelling of + // the same file must not match (never-matchable-traversal stance). + let decision = engine + .check(ExecPolicyContext { + command: "", + cwd: "/workspace", + tool: Some("read_file"), + path: Some("~/.ssh/../ssh/config"), + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + assert_eq!(decision.matched_rule, None); + } + + #[test] + fn typed_ask_relative_path_rule_still_rejects_absolute_call() { + // The absolute fallback is rooted-rule-only: a relative rule keeps + // its workspace-relative semantics and must not reach an absolute + // call path through it. + let engine = ExecPolicyEngine::with_rulesets(vec![ + Ruleset::user(vec![], vec![]) + .with_ask_rules(vec![ToolAskRule::file_path("edit_file", "src/a.rs")]), + ]); + + let decision = engine + .check(ExecPolicyContext { + command: "", + cwd: "/workspace", + tool: Some("edit_file"), + path: Some("/src/a.rs"), + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + assert_eq!(decision.matched_rule, None); + } + + #[test] + fn typed_ask_absolute_path_rule_folds_separators_and_case_on_windows() { + let engine = + ExecPolicyEngine::with_rulesets(vec![Ruleset::user(vec![], vec![]).with_ask_rules( + vec![ToolAskRule { + tool: "read_file".into(), + command: None, + command_exact: false, + path: Some("C:/Users/u/.aws/credentials".into()), + workspace: None, + action: PermissionAction::Deny, + }], + )]); + + let decision = engine + .check(ExecPolicyContext { + command: "", + cwd: r"C:\workspace", + tool: Some("read_file"), + path: Some(r"C:\Users\U\.AWS\credentials"), + ask_for_approval: AskForApproval::OnFailure, + sandbox_mode: Some("workspace-write"), + }) + .unwrap(); + // The rule folds `C:/Users/u/...` and the call folds `C:\Users\U\...` + // to the same form on a case-insensitive platform; on a + // case-sensitive one the case difference is a different file. + if platform_paths_are_case_insensitive() { + assert_eq!(decision.matched_action, Some(PermissionAction::Deny)); + } else { + assert_eq!(decision.matched_rule, None); + } + } + // ── deny / allow action tests ────────────────────────────────────────── #[test] diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 6fd398d6d7..e7a3cd3aca 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -2607,6 +2607,10 @@ impl Engine { .with_speech_output_dir(self.config.speech_output_dir.clone()) .with_mcp_pool(mcp_pool) .with_parent_mode(self.current_mode) + // Typed permission rules must bind delegated calls like + // they bind the parent's own; the handle shares the + // live rulesets, so mid-session updates stay effective. + .with_exec_policy_engine(self.config.exec_policy_engine.clone()) // #4810: no `with_todos` here — this runtime *is* the // spawned background agent, and `background_runtime()` // gives it its own list. Binding the session list would @@ -4317,7 +4321,11 @@ impl Engine { .with_todos(self.config.todos.clone()) .with_parent_completion_tx(self.tx_subagent_completion.clone()) .with_runtime_cost_owner(self.config.compaction.runtime_cost_owner.as_deref()) - .with_parent_mode(input_policy.mode); + .with_parent_mode(input_policy.mode) + // Typed permission rules must bind delegated calls like they + // bind the parent's own; the handle shares the live rulesets, + // so mid-session updates stay effective. + .with_exec_policy_engine(self.config.exec_policy_engine.clone()); if matches!(input_policy.mode, AppMode::Plan) { rt.worker_profile = WorkerRuntimeProfile::for_role(FleetRole::Planner); } @@ -6131,7 +6139,7 @@ fn goal_objective_for_prompt( // outside messages[0]. #[derive(Debug, Clone, PartialEq, Eq)] -pub(super) enum ToolAskRuleDecision { +pub(crate) enum ToolAskRuleDecision { Allow, Prompt(String), Block(String), @@ -6244,6 +6252,26 @@ pub(super) fn exec_shell_ask_rule_decision( tool_input: &Value, workspace: &Path, approval_mode: crate::tui::approval::ApprovalMode, +) -> Option { + exec_shell_ask_rule_decision_for_engine( + &config.exec_policy_engine, + tool_name, + tool_input, + workspace, + approval_mode, + ) +} + +/// [`exec_shell_ask_rule_decision`] keyed on a bare [`ExecPolicyEngine`] +/// handle instead of the full [`EngineConfig`], so executors that hold only +/// the session engine (the nested sub-agent tool registry) evaluate exactly +/// the same typed exec-rule decision the main-line turn loop would. +pub(crate) fn exec_shell_ask_rule_decision_for_engine( + exec_policy_engine: &codewhale_execpolicy::ExecPolicyEngine, + tool_name: &str, + tool_input: &Value, + workspace: &Path, + approval_mode: crate::tui::approval::ApprovalMode, ) -> Option { let policy_tool_name = crate::tools::canonical_action::canonical_action_alias(tool_name, tool_input); @@ -6252,7 +6280,7 @@ pub(super) fn exec_shell_ask_rule_decision( } let command = tool_input.get("command").and_then(Value::as_str)?; tool_ask_rule_decision_for_context( - config, + exec_policy_engine, policy_tool_name, command, None, @@ -6267,13 +6295,31 @@ pub(super) fn file_tool_ask_rule_decision( tool_input: &Value, workspace: &Path, approval_mode: crate::tui::approval::ApprovalMode, +) -> Option { + file_tool_ask_rule_decision_for_engine( + &config.exec_policy_engine, + tool_name, + tool_input, + workspace, + approval_mode, + ) +} + +/// [`file_tool_ask_rule_decision`] keyed on a bare [`ExecPolicyEngine`] +/// handle; see [`exec_shell_ask_rule_decision_for_engine`]. +pub(crate) fn file_tool_ask_rule_decision_for_engine( + exec_policy_engine: &codewhale_execpolicy::ExecPolicyEngine, + tool_name: &str, + tool_input: &Value, + workspace: &Path, + approval_mode: crate::tui::approval::ApprovalMode, ) -> Option { let policy_tool_name = crate::tools::canonical_action::canonical_action_alias(tool_name, tool_input); let paths = file_tool_permission_paths(policy_tool_name, tool_input)?; if paths.is_empty() { return tool_ask_rule_decision_for_context( - config, + exec_policy_engine, policy_tool_name, "", None, @@ -6286,7 +6332,7 @@ pub(super) fn file_tool_ask_rule_decision( let mut all_allowed = true; for path in paths { match tool_ask_rule_decision_for_context( - config, + exec_policy_engine, policy_tool_name, "", Some(&path), @@ -6314,7 +6360,7 @@ pub(super) fn file_tool_ask_rule_decision( } fn tool_ask_rule_decision_for_context( - config: &EngineConfig, + exec_policy_engine: &codewhale_execpolicy::ExecPolicyEngine, tool_name: &str, command: &str, path: Option<&str>, @@ -6328,8 +6374,7 @@ fn tool_ask_rule_decision_for_context( | crate::tui::approval::ApprovalMode::Bypass | crate::tui::approval::ApprovalMode::Suggest => AskForApproval::OnFailure, }; - let decision = config - .exec_policy_engine + let decision = exec_policy_engine .check(ExecPolicyContext { command, cwd: cwd.as_ref(), diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs index 73e83b19c9..82b4414309 100644 --- a/crates/tui/src/lib.rs +++ b/crates/tui/src/lib.rs @@ -10510,7 +10510,11 @@ async fn build_direct_workflow_tool( .with_speech_output_dir(config.speech_output_dir()) .with_mcp_pool(mcp_pool) .with_todos(new_shared_todo_list()) - .with_parent_mode(mode); + .with_parent_mode(mode) + // Typed permission rules must bind delegated calls like they bind the + // parent's own; the handle shares the live rulesets, so mid-session + // updates stay effective. + .with_exec_policy_engine(config.exec_policy_engine.clone()); Ok(( crate::tools::workflow::WorkflowTool::new(manager, runtime).with_explicit_cli_approval(), diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index ec16708819..b08ab6b0dc 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -2217,6 +2217,13 @@ pub struct SubAgentRuntime { pub todos: SharedTodoList, /// Session mode of the orchestrating parent at spawn time (Wave 7 M4/M5). pub parent_mode: AppMode, + /// The parent session's exec-policy engine. Because the engine shares its + /// live rulesets across clones, every child registry holding this handle + /// evaluates the same typed permission rules — including rules installed + /// mid-session — that the parent turn loop enforces on its own tool calls. + /// Defaults to an empty engine (no rules), which leaves child behavior + /// unchanged for embedders that never thread one. + pub exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine, } impl SubAgentRuntime { @@ -2268,6 +2275,7 @@ impl SubAgentRuntime { speech_output_dir: None, todos: crate::tools::todo::new_shared_todo_list(), parent_mode: AppMode::Agent, + exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine::new(Vec::new(), Vec::new()), } } @@ -2278,6 +2286,20 @@ impl SubAgentRuntime { self } + /// Carry the parent session's exec-policy engine into child registries so + /// typed deny rules bind sub-agent tool calls the same way they bind the + /// parent's. The engine shares its live rulesets across clones, so this + /// handle tracks later `set_ruleset` updates instead of freezing a + /// spawn-time snapshot. + #[must_use] + pub fn with_exec_policy_engine( + mut self, + exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine, + ) -> Self { + self.exec_policy_engine = exec_policy_engine; + self + } + /// Match generated worker display names to the active session language. #[must_use] pub fn with_locale_tag(mut self, locale_tag: impl Into) -> Self { @@ -2555,6 +2577,7 @@ impl SubAgentRuntime { // opt-in forked child as immutable `fork_context` text. todos: crate::tools::todo::new_shared_todo_list(), parent_mode: self.parent_mode, + exec_policy_engine: self.exec_policy_engine.clone(), } } @@ -12704,6 +12727,11 @@ struct SubAgentToolRegistry { /// admitted worker identity. Production registries always enforce claims. enforce_write_claim: bool, registry: ToolRegistry, + /// The parent session's exec-policy engine (shared live rulesets; see + /// [`SubAgentRuntime::exec_policy_engine`]). Consulted by `execute` so a + /// command the parent turn loop would hard-deny cannot be delegated to + /// this child and run anyway. Empty by default → checks are no-ops. + exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine, } impl SubAgentToolRegistry { @@ -12809,6 +12837,7 @@ impl SubAgentToolRegistry { coordination_manager, enforce_write_claim: true, registry, + exec_policy_engine: runtime.exec_policy_engine.clone(), } } @@ -13319,6 +13348,61 @@ impl SubAgentToolRegistry { ) .map_err(|refusal| anyhow!(refusal))?; } + // Fork delta (execpolicy wiring): delegated calls run through the same + // typed execpolicy gate the parent turn loop applies between hooks and + // approval (`exec_shell_ask_rule_decision` / `file_tool_ask_rule_decision`, + // reused verbatim). Positioned after the execution envelope so this + // child's own posture still speaks first. A hard Block always refuses. + // A Prompt decision follows the main line's #3790 rule — the approval + // posture is the authority: when the inherited session auto-approves + // (YOLO), the main line would auto-run, so the call passes; otherwise + // the main line would surface an approval prompt, which a child has + // no surface to do, so refusing there is the fail-closed answer for + // every prompting posture and for the fail-closed `Never` session + // alike (the Never wording differs, since the engine still maps to + // `OnFailure` here, but the refusal outcome matches). All non-Never + // modes map to `OnFailure`, so `ApprovalMode::Auto` is + // decision-equivalent to the parent's mode whenever auto-approve is + // on. The engine handle shares the parent's live rulesets, so a rule + // installed mid-session binds delegated calls too, and an empty + // engine (no rules) leaves this check a no-op. + let ask_rule_decision = crate::core::engine::exec_shell_ask_rule_decision_for_engine( + &self.exec_policy_engine, + name, + &input, + &self.registry.context().workspace, + crate::tui::approval::ApprovalMode::Auto, + ) + .or_else(|| { + crate::core::engine::file_tool_ask_rule_decision_for_engine( + &self.exec_policy_engine, + name, + &input, + &self.registry.context().workspace, + crate::tui::approval::ApprovalMode::Auto, + ) + }); + match ask_rule_decision { + Some(crate::core::engine::ToolAskRuleDecision::Block(reason)) => { + // Mirror the main line's blocked refusal so a child model sees + // the same familiar wording the parent would have received. + return Err(anyhow!(reason)); + } + // The child cannot show the approval prompt the main line would + // show for this rule, so without parent auto-approve the only + // correct answer is to refuse and point the model at the main + // conversation. + Some(crate::core::engine::ToolAskRuleDecision::Prompt(reason)) + if !self.auto_approve => + { + return Err(anyhow!(format!( + "Delegated tool call requires approval: {reason}. Sub-agents cannot show an approval prompt; run this tool call in the main conversation so it can be approved." + ))); + } + // `Allow` is a user-authored allow rule, and a Prompt under parent + // auto-approve matches the main line's auto-run: both pass. + _ => {} + } let scope_aware_write = matches!( name, "write_file" | "edit_file" | "apply_patch" | "fim_edit" diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index f2c45cfb36..ae18b5582d 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -6306,6 +6306,259 @@ async fn scout_shell_respects_parent_shell_and_network_ceilings() { } } +/// An auto-approved Worker registry (shell-capable, everything else default) +/// carrying the given exec-policy engine — the smallest fixture that reaches +/// the delegated-call execpolicy gate past every posture gate. +fn auto_approved_worker_registry_with_engine( + tmp: &tempfile::TempDir, + exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine, +) -> SubAgentToolRegistry { + worker_registry_with_engine_and_approval(tmp, exec_policy_engine, true) +} + +/// The same fixture with the parent session's approval posture under the +/// caller's control: `false` models every prompting/fail-closed posture +/// (Suggest/Auto/Never), where the main line would surface an approval +/// prompt the child has no surface for. +fn worker_registry_with_engine_and_approval( + tmp: &tempfile::TempDir, + exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine, + auto_approve: bool, +) -> SubAgentToolRegistry { + let mut runtime = + stub_runtime().with_agent_tool_surface_options(enabled_agent_surface_options()); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.context.auto_approve = auto_approve; + runtime.allow_shell = true; + runtime.worker_profile = WorkerRuntimeProfile::for_role(FleetRole::Worker); + SubAgentToolRegistry::new( + runtime.with_exec_policy_engine(exec_policy_engine), + FleetRole::Worker, + None, + crate::tools::todo::new_shared_todo_list(), + crate::tools::plan::new_shared_plan_state(), + ) +} + +/// Fork-delta behavior test: a command the parent session's execpolicy +/// hard-denies must fail closed inside the child with the main line's deny +/// wording — the auto-approve escape hatch this wiring closes — while a +/// command no rule names still dispatches for real through the same registry. +/// A typed ask rule mirrors the main line's #3790 posture authority: under a +/// prompting/fail-closed parent the child refuses (it cannot show the +/// approval prompt), while under an auto-approving parent it passes like the +/// main line's auto-run. +#[tokio::test] +async fn forkguard_subagent_execpolicy_deny_matches_main_line() { + let tmp = tempdir().expect("tempdir"); + let engine = codewhale_execpolicy::ExecPolicyEngine::with_rulesets(vec![ + codewhale_execpolicy::Ruleset::user(Vec::new(), vec!["sudo".to_string()]), + ]); + let registry = auto_approved_worker_registry_with_engine(&tmp, engine); + + let error = registry + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "sudo --version"}), + ) + .await + .expect_err("a main-line deny rule must block delegated calls too") + .to_string(); + assert!( + error.contains("denied prefix rule 'sudo'"), + "child must see the main-line deny reason: {error}" + ); + + let output = registry + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "ls"}), + ) + .await + .expect("an allowed command must pass the execpolicy gate untouched"); + assert!( + !output.starts_with("Error:"), + "allowed command must really dispatch: {output}" + ); + + // Ask-rule face: a prompting/fail-closed parent session cannot let the + // child run what the main line would only run after approval. + let ask_engine = codewhale_execpolicy::ExecPolicyEngine::with_rulesets(vec![ + codewhale_execpolicy::Ruleset::user(Vec::new(), Vec::new()) + .with_ask_rules(vec![codewhale_execpolicy::ToolAskRule::exec_shell("curl")]), + ]); + let prompting = worker_registry_with_engine_and_approval(&tmp, ask_engine.clone(), false); + let error = prompting + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "curl --version"}), + ) + .await + .expect_err("an ask rule under a non-auto parent must refuse in the child") + .to_string(); + assert!( + error.contains("requires approval"), + "refusal must point at the approval surface: {error}" + ); + + let output = prompting + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "ls"}), + ) + .await + .expect("a command no rule names must dispatch regardless of posture"); + assert!(!output.starts_with("Error:"), "{output}"); + + let yolo = worker_registry_with_engine_and_approval(&tmp, ask_engine, true); + let output = yolo + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "curl --version"}), + ) + .await + .expect("under parent auto-approve the ask rule must pass like the main line"); + assert!(!output.starts_with("Error:"), "{output}"); +} + +/// Without a ruleset the delegated path is byte-identical to the unwired +/// behavior: the gate is a no-op, and the very same call that a deny rule +/// blocks runs when the rule is absent. +#[tokio::test] +async fn subagent_execpolicy_empty_ruleset_keeps_auto_approve_behavior() { + let tmp = tempdir().expect("tempdir"); + let registry = auto_approved_worker_registry_with_engine( + &tmp, + codewhale_execpolicy::ExecPolicyEngine::new(Vec::new(), Vec::new()), + ); + let output = registry + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "echo execpolicy_unblocked"}), + ) + .await + .expect("an empty engine must leave the delegated call unchanged"); + assert!(output.contains("execpolicy_unblocked"), "{output}"); + + let denied_registry = auto_approved_worker_registry_with_engine( + &tmp, + codewhale_execpolicy::ExecPolicyEngine::with_rulesets(vec![ + codewhale_execpolicy::Ruleset::user(Vec::new(), vec!["echo".to_string()]), + ]), + ); + let error = denied_registry + .execute( + "agent_policy_denied", + "Bash", + json!({"action": "run", "command": "echo execpolicy_unblocked"}), + ) + .await + .expect_err("the deny rule, not the wiring, must be what blocks") + .to_string(); + assert!(error.contains("denied prefix rule 'echo'"), "{error}"); +} + +/// A typed Deny rule on a file path blocks the child's File read of that path +/// while a workspace-relative read outside the rule still lands. +#[tokio::test] +async fn subagent_execpolicy_blocks_denied_file_read_but_allows_workspace_relative_read() { + let tmp = tempdir().expect("tempdir"); + std::fs::create_dir_all(tmp.path().join("secret")).expect("secret dir"); + std::fs::write(tmp.path().join("secret/key.txt"), "rot thirteen").expect("key fixture"); + std::fs::write(tmp.path().join("notes.txt"), "plain notes").expect("notes fixture"); + let mut rule = codewhale_execpolicy::ToolAskRule::file_path("read_file", "secret/key.txt"); + rule.action = codewhale_execpolicy::PermissionAction::Deny; + let engine = codewhale_execpolicy::ExecPolicyEngine::with_rulesets(vec![ + codewhale_execpolicy::Ruleset::user(Vec::new(), Vec::new()).with_ask_rules(vec![rule]), + ]); + let registry = auto_approved_worker_registry_with_engine(&tmp, engine); + + let error = registry + .execute( + "agent_policy", + "File", + json!({"action": "read", "path": "secret/key.txt"}), + ) + .await + .expect_err("a denied path must not be readable through the child") + .to_string(); + assert!(error.contains("explicitly denies"), "{error}"); + + let output = registry + .execute( + "agent_policy", + "File", + json!({"action": "read", "path": "notes.txt"}), + ) + .await + .expect("a workspace-relative read outside the deny rule passes"); + assert!(output.contains("plain notes"), "{output}"); +} + +/// Typed ask rules are prompt decisions on the main line. A child has no +/// prompt surface, so the decision must degrade to pass-through — the call +/// runs — never to a refusal. +#[tokio::test] +async fn subagent_execpolicy_prompt_decision_does_not_block_delegated_calls() { + let tmp = tempdir().expect("tempdir"); + let engine = codewhale_execpolicy::ExecPolicyEngine::with_rulesets(vec![ + codewhale_execpolicy::Ruleset::user(Vec::new(), Vec::new()).with_ask_rules(vec![ + codewhale_execpolicy::ToolAskRule::exec_shell("echo askme"), + ]), + ]); + let registry = auto_approved_worker_registry_with_engine(&tmp, engine); + + let output = registry + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "echo askme"}), + ) + .await + .expect("an ask-rule (prompt) decision must not block a child"); + assert!(output.contains("askme"), "{output}"); +} + +/// The registry's engine handle is the parent's live engine, not a snapshot: +/// a ruleset installed after the registry was built binds its delegated calls. +#[tokio::test] +async fn subagent_execpolicy_ruleset_update_binds_already_delegated_calls() { + let tmp = tempdir().expect("tempdir"); + let mut engine = codewhale_execpolicy::ExecPolicyEngine::new(Vec::new(), Vec::new()); + let registry = auto_approved_worker_registry_with_engine(&tmp, engine.clone()); + + let output = registry + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "echo not_yet_denied"}), + ) + .await + .expect("no rule exists yet, so the delegated call runs"); + assert!(output.contains("not_yet_denied"), "{output}"); + + engine.set_ruleset(codewhale_execpolicy::Ruleset::user( + Vec::new(), + vec!["echo".to_string()], + )); + let error = registry + .execute( + "agent_policy", + "Bash", + json!({"action": "run", "command": "echo not_yet_denied"}), + ) + .await + .expect_err("a mid-session rule must reach the already-built registry") + .to_string(); + assert!(error.contains("denied prefix rule 'echo'"), "{error}"); +} + #[test] fn implementer_catalog_inherits_patch_and_fim_when_enabled() { let tmp = tempdir().expect("tempdir"); @@ -10761,6 +11014,7 @@ pub(crate) fn stub_runtime() -> SubAgentRuntime { tool_timeout: DEFAULT_TOOL_TIMEOUT, speech_output_dir: None, todos: crate::tools::todo::new_shared_todo_list(), + exec_policy_engine: codewhale_execpolicy::ExecPolicyEngine::new(Vec::new(), Vec::new()), } }