fix(execpolicy): fail closed on unresolved shell parsing and tighten prefix matching - #6675
2 commits merged into
Conversation
…efix matching
Cause: deny rules were matched against the command text as written, so a
command word the shell only resolves when it runs (a variable, substitution,
glob or brace list, ANSI-C escapes, a shell reading its script from a pipe or
here-string) never met the rule. Commands after reserved words (`if`, `then`,
`while`, `!`), `function f { ... }` bodies and `find -exec` payloads were not
scanned as commands. Wrapper words were stripped without their option grammar,
so `chroot DIR cmd`, `sudo --user NAME cmd` and `timeout -s SIG N cmd` hid
`cmd`. A trusted prefix such as `git status` also covered options placed
before the subcommand (`git -c key=value status`, `git --exec-path=... status`),
and the parallel read-only classifier accepted parentheses.
Fix: the shell expander now reports whether a command word is dynamic and
whether code runs nested inside another command. While any deny rule is
configured, a dynamic command word is refused; allow and trusted prefixes no
longer cover dynamic or nested code. Reserved words are dropped before the
command word, `{`/`}` split commands, `find -exec` payloads are recorded, and
wrappers are unwrapped by a per-wrapper option table (ambiguous options are
read both ways). Prefix allow matching requires the matched words to be
spelled at the front of the command. Typed deny rules also match a
path-qualified command word. `command -v NAME` is treated as a lookup, not a
run. Parentheses disqualify a command from parallel read-only.
Tests: new crates/execpolicy/tests/shell_parse_policy.rs fails on the pre-fix
source (test result: FAILED. 0 passed; 5 failed) and passes with it.
cargo test -p codewhale-execpolicy: 217 passed; 0 failed (unit), 1 passed,
5 passed (redirection_policy), 5 passed (shell_parse_policy), 1 passed (doc).
cargo clippy -p codewhale-execpolicy --all-targets -D warnings: clean.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7abbe26c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if matches!(word.as_str(), "{" | "}") { | ||
| push_command(commands, &mut piece); | ||
| continue; |
There was a problem hiding this comment.
When a literal brace is quoted or escaped, it is an ordinary argument rather than shell grouping, but Line no longer retains that distinction and this branch splits every token equal to { or }. With an unrelated rm deny rule, a valid data-producing command such as printf '%s\n' '{' rm is therefore expanded into a separate rm target and forbidden even though the shell only passes rm to printf; only unquoted reserved braces should create a boundary.
Useful? React with 👍 / 👎.
| if tokens[..=head] | ||
| .iter() | ||
| .zip(&line.dynamic) | ||
| .any(|(token, dynamic)| *dynamic && !is_env_assignment(token)) |
There was a problem hiding this comment.
When a wrapper has a quoted dynamic option value, this scans every token through the eventual command head and treats the option value as an unresolved command word. For example, sudo -u "$USER" ls has a statically known ls command and the quotes guarantee one username operand, yet any unrelated deny prefix such as rm now makes expansion.dynamic true and causes the engine to forbid it. Track dynamic wrapper names/command positions separately from quoted option operands instead of rejecting the whole prefix slice.
Useful? React with 👍 / 👎.
| if depth > 0 && !input.trim().is_empty() { | ||
| self.nested = true; |
There was a problem hiding this comment.
Every recursive parse is marked as executable nested code, but ${...} bodies are also recursively parsed even when they contain only a parameter name. Consequently a trusted command such as ls "${DIR}" sets nested = true and unexpectedly requires approval, while the equivalent ls "$DIR" remains trusted; only actual command/process substitutions found inside the parameter expression should set this flag.
Useful? React with 👍 / 👎.
| while let Some((index, state)) = stack.pop() { | ||
| if index >= tokens.len() || seen.len() >= MAX_HEAD_STATES || !seen.insert((index, state)) { | ||
| continue; |
There was a problem hiding this comment.
The state limit silently stops traversal instead of marking the expansion unresolved, so a sufficiently long wrapper chain bypasses a literal deny rule. Each wrapper consumes both a Command and a Wrapper state; therefore 32 repetitions in env env ... env rm x exhaust the 64-state budget before rm becomes a head, leaving only env-prefixed targets that do not match a deny prefix of rm, even though the shell eventually executes it. Budget exhaustion must set dynamic or emit a failure rather than continuing with incomplete deny targets.
AGENTS.md reference: AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
| || rulesets.iter().any(|ruleset| { | ||
| ruleset | ||
| .ask_rules | ||
| .iter() | ||
| .any(|rule| rule.action == PermissionAction::Deny && rule.tool == tool) |
There was a problem hiding this comment.
This treats every typed deny for the same tool as active without applying the workspace and path predicates used by matching_ask_rule. For example, a deny rule for command rm scoped to /repo/a causes a genuinely dynamic but otherwise permitted command such as v=ls; $v in /repo/b to be forbidden, even though that deny rule cannot match in the current workspace. Filter self-contained scope predicates before deciding that a relevant deny rule is configured.
Useful? React with 👍 / 👎.
| match script { | ||
| Some(index) if !from_stdin => ShellInput::Script(index), | ||
| _ => ShellInput::Stdin, | ||
| } |
There was a problem hiding this comment.
A shell invocation with no script operand is always classified as reading a runtime script from stdin, including exit-only invocations such as bash --version and bash --help. With any deny rule configured, these harmless commands now become dynamically unresolved and are forbidden. bash --help lists both as GNU long options, and bash --version exits successfully without consuming stdin, so terminal long options need a non-stdin outcome.
Useful? React with 👍 / 👎.
| "eof", | ||
| "replace", |
There was a problem hiding this comment.
The wrapper table marks xargs --replace and --eof as consuming the following word, so xargs --replace rm -f file skips rm as an option value and never emits it as a command head, allowing a deny prefix of rm to be bypassed. xargs --help documents these as --replace[=R] and --eof[=END]: when no = value is present, the next word is the command, not the option argument. Model these optional arguments separately so the deny scan reaches rm.
AGENTS.md reference: AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
Cause: the expander could stop looking without saying so. Output past 256 command lines and wrapper walks past the state budget were dropped silently. Wrapper short options missing from the tables were assumed to take no value (`env -P DIR cmd`, `xargs -J % cmd`, `chroot -u USER`). `$( … )` bodies were closed by paren counting that ignored quotes and `case` patterns. `<<` inside `(( … ))` was read as a heredoc that swallowed the following lines. `xargs` and `find -exec` replacement strings were trusted as static command words and shell code. Several wrappers and code-as-string commands were not known (`trap`, `su -c`, `watch`, `cmd /c`, PowerShell `-Command`, `caffeinate`, `arch`, `.exe` spellings). Outside the expander: `task_shell_start` and task gate commands skipped the shell deny rules, session grants keyed on a flag-stripped prefix covered interposed options, chains and nested code, and the sub-agent read-only shell admitted a leading unquoted `*` whose matches can read as options. Fix: a truncated parse, an unterminated quote/substitution/heredoc, or a `case` inside `$( … )` marks the command unresolved, so it fails closed while deny rules exist. Substitution bodies are read quote-aware. Wrapper tables list known switches, and anything else is read both ways; the wrapper's flags are also kept in a target so deny matching can skip them. `(( … ))` bodies are scanned on their own. Replacement strings mark their words as run-time values. New wrappers and payload handlers are added, and names fold `.exe`. Unresolved commands prompt under OnRequest/UnlessTrusted and stay refused in other modes (OnFailure also serves auto-approving sessions). Task shell tools route through the shell policy (an allow rule does not waive their own approval). The session grouping key falls back to an exact key for interposed options, chains, nested or unresolved code. Leading unquoted `*` is rejected on the sub-agent read-only surface. Tests (CARGO_BUILD_JOBS=4): - cargo test -p codewhale-execpolicy: lib 217 passed; 0 failed, shell_parse_policy 7 passed; 0 failed (plus 1, 5, 1 passed in other targets). The new cases on the previous source: test result: FAILED. 3 passed; 4 failed (every new case listed not denied). - cargo test -p codewhale-tui --lib -- approval_cache task_shell_tools_answer grouping_key: 26 passed; 0 failed. Without the fix: 0 passed; 2 failed. - cargo test -p codewhale-tui --lib -- readonly read_only: 138 passed; 0 failed. - cargo clippy -p codewhale-execpolicy --tests: clean. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Hmbown
changed the title
crates/execpolicy/src/command_safety.rs: #6637 (already merged) replaced the agent read-only classifier with a quote-aware lexer (lex_readonly_command / agent_readonly_verdict) that admits chains and three redirects; #6675 hardened the old classifier by rejecting a word that starts with an unquoted `*` (has_leading_glob), because its matches can begin with `-` and become options after the option allowlist ran. Kept #6637's lexer as the single read-only authority and ported #6675's rule into it: an unquoted `*` whose word has no literal prefix yet (empty quotes count as no prefix, so `''*` is refused as #6675's test requires) is an `operator` refusal. has_leading_glob had no remaining caller and is removed. Doc bullet takes #6675's `*` wording within #6637's list. Test updates: `echo *` is now refused by the glob rule before the echo literal rule (row updated, `echo a*` added for the literal rule); a new unit test covers the glob rule on the lexer incl. chains. crates/tui/src/tools/approval_cache.rs: #6670 (already merged) replaced command_prefix with shell_command_grant_scope (family key only for simple, non-wrapper, inert-option, known families; else shell:cmd:<command>); #6675 edited the old command_prefix to fall back to an exact key for dynamic/nested/multi-command expansions or when the canonical prefix is not leading. Folded #6675's conditions into #6670's family branch, so either rule failing gives the full-command key; kept #6670's key format. Verified: cargo test -p codewhale-execpolicy: 221+1+5+7+1 passed, 0 failed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Each fix below reconciles two PRs that merged textually but not semantically. - snapshot/repo.rs, receipts.rs: #6645/#6682 added SnapshotRepo::changed_paths_between(from, to) -> Vec<PathBuf> (undo, turn artifacts) and #6591 added a different changed_paths_between(from, to, limit) -> (Vec<SnapshotPathChange>, bool) (receipts). Duplicate definition; #6591's is renamed path_changes_between and its one caller (receipts) updated. - core/engine/turn_loop.rs: #6673's repl-fence approval match did not cover #6601's ApprovalResult::TimedOut. A timed-out card now refunds the tool-call budget slot and reports a timeout, like direct and code-mode calls; the audit line records "timeout" instead of "denied". - runtime_api/sessions.rs: #6640's session-owner 409 predates #6645's ApiError.code field; code: None. - tools/verifier.rs: #6671's env-scrub test called run_gate(gate) without the session_id argument run_gate takes on main (#6508). - skills/install.rs + integration harness: #6679 made install.rs read downloads through crate::utils::read_response_body_capped, but the integration harness #[path]-includes install.rs and has no utils module, so the integration test target did not compile (also on the #6679 branch). The capped reader moves to utils/response_body.rs (re-exported from utils, unchanged API) and the harness includes just that file as crate::utils. - Test files where an add-only conflict was auto-resolved by concatenating both sides lost the shared closing lines of the first test: commands/groups/debug/tests.rs (#6682 + #6591), tui/ui/tests.rs (#6635 + main), tools/shell/tests.rs (#6674 + #6679), and runtime_api/tests.rs (#6645 merge in round 1). Restored the missing `}` / `);` so each test is whole again; no assertions were dropped. - tools/shell/tests.rs: #6637's executor test expected `sort * | cat` to be admitted and then fail at run time; with #6675's rule ported into the #6637 lexer (see the #6675 merge), a word-leading unquoted `*` is refused before anything runs. The test now asserts that refusal and still checks the sentinel and option-named files are untouched. - core/engine/tests.rs -> tui/history/tests.rs: #6601's engine test asserted crate::tui::history on the trust warning, raising the runtime->UI test reference ratchet 40 -> 41 (check-command-crate- boundaries FAIL). That assertion moved to a tui::history test on workspace_trust_runtime_message, so the ratchet is back at 40. - scripts/check-blocking-calls-budget.json: runtime_api/git.rs 5 -> 6. #6648 justified this budget in its PR body (working-tree fingerprint reads in sync fns reached only from spawn_blocking); its own branch already has six such sites (the File::open used for hashing), so the recorded 5 was stale. subagent/worktree.rs tightened 7 -> 6. Checks: cargo check --workspace --tests clean (no warnings); cargo test -p codewhale-execpolicy 221+1+5+7+1 passed; cargo test -p codewhale-tui --test integration 187 passed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex (read-only) reviewed every hand-resolved merge of this round. Five findings, all verified in the code and fixed: - tools/shell.rs readonly_git_dirs (#6637 x #6679, silent merge): the git-filter hardening found git segments by splitting on `|` only, while #6637's classifier and executor also run `&&`, `||` and `;` chains. In `pwd && git diff` no override was installed, so a repository clean filter could run. It now splits with agent_readonly_verdict's segments (falling back to `|` only if the verdict fails); test added. - compaction.rs (#6680 merge): starts_with on the bare legacy marker missed the real pre-v0.9.6 checkpoint text, which opened with "## 📋 Conversation Summary (Auto-Generated)", optionally after the "## Pinned Facts (User Anchors)" section. Those openings are recognised again (so restore replaces them instead of stacking), while a message that only quotes the marker mid-text still is not; test added. - core/engine/turn_loop.rs (#6673 x #6601): the inline REPL fence refunded the tool-call budget only on timeout; Denied, RetryWithPolicy and approval errors kept the slot although nothing ran. Every refusal now refunds, as direct and code-mode calls do. - command_safety.rs (#6675 port): the leading-glob rule treated any quote-only raw prefix as empty, so `cat '"'*` (literal `"` prefix) was refused. It now decodes the prefix with shlex and refuses only when the decoded prefix is empty (`''*` still refused); tests added. - tools/git.rs (#6671 x #6679, silent merge): two hardened git runners (run_git_command via read_only_git_command, and run_git_review_command) both wrapped Git::review_command with different NotFound handling. run_git_review_command is removed and its callers use run_git_command, keeping their REVIEW_DIFF_ARGS. Checks: cargo check --workspace --tests clean; cargo test -p codewhale-execpolicy 221+1+5+7+1 passed; cargo test -p codewhale-tui --lib -- compaction tools::shell tools::git turn_loop repl approval readonly checkpoint: 1133 passed, 1 failed (approval::tests::required_tool_execution_uses_typed_host_decisions_not_approval_claims, a 5s event deadline under load; core::engine::approval:: rerun twice: 15 passed, 0 failed). Ratchets: boundaries, blocking budget, dead-code budget, module graph, cargo fmt --check all pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Summary
This hardens shell command policy in
crates/execpolicy, plus the shell-policy entry points in the TUI.Words known only at run time. The shell expander now reports when a command word is resolved only when the command runs. That covers a variable, a substitution, a glob or brace list, ANSI-C escapes, a shell reading its script from a pipe, here-string or process substitution, and a replacement string from
xargs -I/find -execused as the command or as shell code. It also reports when the text does not parse cleanly: an unterminated quote, substitution or heredoc, or acaseinside$( … ). It reports when a parse budget runs out (output lines, wrapper states) too. While any deny rule is configured, such a command is refused. UnderOnRequest/UnlessTrustedit asks instead.OnFailurealso serves auto-approving sessions, so it keeps refusing. With no deny rules, behaviour is unchanged.Reserved words and bodies. Commands are scanned like any other command when they:
if,then,while,until,do,!and similar wordsfunction f { ... }body or afind -execpayload(( … ))body, where<<is a shift and not a heredocBodies of
$( … ),<( … )and${ … }are read with quotes taken into account.Wrapper option grammar. Wrappers are unwrapped by a per-wrapper option table. The table lists value-taking options and switches, and any other option is read both ways. BSD and macOS options are covered (
env -P,xargs -J,chroot -u,doas -a). Recognized wrappers now includecaffeinate,arch,sandbox-exec,nsenter,unshare,runuser,flock,watch,wsl,noglobandnocorrect, and.exespellings fold. Code passed as a string is checked as a command line:trap,su -c,flock -c,script -c,watch,cmd /cand PowerShell-Command.-EncodedCommandis treated as unresolved.command -v NAMEis still a lookup.Prefix allow rules. A trusted or allow prefix such as
git statusmust appear at the front of the command. So it no longer coversgit -c key=value statusorgit --exec-path=... status. Allow and trusted prefixes also stop covering nested code and run-time command words. Typed deny rules match a path-qualified command word (/bin/rm).Session grants. An "approve for session" grant for a shell command covers only flag variants of the command as written. A command with options before the subcommand, a chain, or nested or unresolved code gets an exact key.
Task shell tools.
task_shell_startandtasksgate_runnow go through the shell deny and ask rules. A shell allow rule does not waive their own approval.Read-only classifiers. Commands with
(or)no longer count as parallel read-only. The sub-agent read-only shell rejects a word that starts with an unquoted*.Known limits are documented in
shell_expand.rs:cmd/PowerShell payloads are scanned with the POSIX grammar.Testing
All cargo runs used
CARGO_BUILD_JOBS=4.cargo test -p codewhale-execpolicy:217 passed; 0 failedshell_parse_policy:7 passed; 0 failed1 passed,5 passed,1 passedshell_parse_policygivestest result: FAILED. 3 passed; 4 failed, and every new case is listed as not denied.cargo test -p codewhale-tui --lib -- approval_cache task_shell_tools_answer grouping_key:26 passed; 0 failed. With the production change reverted, the two new tests fail:0 passed; 2 failed.cargo test -p codewhale-tui --lib -- readonly read_only:138 passed; 0 failed.cargo clippy -p codewhale-execpolicy --tests: clean.🤖 Generated with Claude Code