fix(bash): unquote tokens before the destructive-command scan - #506
Conversation
![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: def96035bf
ℹ️ 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".
| for invocation in &cmd.invocations { | ||
| let normalized = invocation | ||
| .iter() | ||
| .map(|t| unquote_token(t)) |
There was a problem hiding this comment.
When a destructive command is spelled as $'git' push --force, Bash executes it as git push --force, but unquote_token preserves the leading $, so this scan sees $git push --force; the raw scan and later AST check also miss it, while check_shell_injection rejects only ANSI strings containing hex/octal escapes. Consequently this direct quoting variant still passes BashTool::validate_input; normalize ANSI-C quoted tokens and add it to the evasion test cases.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
`find_destructive` matched its patterns against the raw command text, so re-spelling a command without changing what it runs walked past it. Verified end to end through `BashTool::validate_input`: chmod 777 /etc -> rejected ch\\mod 777 /etc -> ACCEPTED git push --force -> rejected 'git' push --force -> ACCEPTED Each invocation is now also scanned in normalized form — its tokens with shell quoting and backslash escapes removed, joined by single spaces. The pipeline-head check unquotes and de-paths too, so `/bin/rm` and `'rm'` are both recognised as `rm`. Both scans are additive: nothing the raw scan caught stops being caught. This is the same evasion class fixed in #495 for the name-based classifier. That fix unquoted at comparison time; this scan is a substring match over raw text and was never covered by it. Also pins a pre-existing limitation that this change does not address: the scan cannot distinguish a command from a string that merely mentions one, so `grep -r 'rm -rf' docs/` is refused. That is over-blocking, and separating arguments from commands is a design change of its own — the test documents the behaviour so a later fix has something to flip.
$'git' push --force executes git push --force, but unquote_token kept the leading $, so the normalized destructive scan, the pipeline-head scan and the AST head check all compared against $git and the spelling passed validate_input. Decode $'...' (including numeric escapes) and $"..." to the text bash hands the command, with evasion and false-positive regression tests.
def9603 to
246868c
Compare
|
@codex review — addressed the P1: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 246868cf37
ℹ️ 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".
| Some(_) => { | ||
| out.push(char::from_u32(radix_value(chars, 16, max, 0)).unwrap_or('\u{fffd}')) |
There was a problem hiding this comment.
When an ANSI-C escape evaluates to NUL, Bash discards that NUL and the remainder of the ANSI-C quoted segment. Thus $'git\u0000junk' push --force invokes git push --force, while this branch retains git\0junk, causing both the normalized scan and AST head check to miss git; \c@ has the same issue. Treat NUL as terminating the current ANSI-C segment so the destructive command remains visible.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| 'c' => match chars.next() { | ||
| // `\cX` is Ctrl-X — a control byte, never pattern text. | ||
| Some(ctl) if ctl.is_ascii() => { | ||
| out.push(char::from((ctl.to_ascii_uppercase() as u8) ^ 0x40)); | ||
| } |
There was a problem hiding this comment.
For a command such as printf %s $'\c3hutdown', Bash maps \c3 to control byte 0x13, but the XOR here maps it to printable s. The normalized scan consequently sees shutdown and rejects a command that only prints a control character; use Bash's control-character mapping (x & 0x1f, with ? producing DEL) instead.
Useful? React with 👍 / 👎.
Bash drops a NUL and the rest of the quoted segment, so $'git\x00junk' push --force executes git push --force while the decoder kept git<NUL>junk and hid the head. \cX now masks with & 0x1f (\c? is DEL) the way bash does; the previous XOR turned \c3 into a printable 's', misreading printf %s $'\c3hutdown' as shutdown.
|
@codex review — addressed both follow-ups. P1 (NUL truncation): a NUL-valued escape (\x00, \0, \u0000, \c@) now ends the ANSI-C segment exactly as bash does — the NUL and the remainder of the segment are discarded (escaped quotes in the discarded tail still pair, and text concatenated after the closing quote still appends), so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32559b63f5
ℹ️ 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".
| for segment in cmd.raw.split('|') { | ||
| let trimmed = segment.trim(); | ||
| let base = trimmed.split_whitespace().next().unwrap_or(""); | ||
| let head = trimmed.split_whitespace().next().unwrap_or(""); | ||
| let base_owned = base_name(&unquote_token(head)).to_lowercase(); |
There was a problem hiding this comment.
When a quoted argument contains |, the raw split('|') creates a synthetic segment and the new unquoting turns its unmatched trailing quote into a valid destructive head. For example, printf '%s\n' 'keep|rm' does not execute rm, but the second segment is rm', which unquote_token normalizes to rm, causing BashTool::validate_input to reject this harmless command. Use the parsed pipeline/invocation structure, or at least a quote-aware splitter, before normalizing segment heads.
Useful? React with 👍 / 👎.
… segments Splitting the raw text on | minted a synthetic segment from a quoted argument, and unquoting its trailing quote produced a valid head: printf '%s\n' 'keep|rm' was refused as rm. Normalize heads from the tree-sitter invocations, which are quote-aware; keep the historical exact-match raw scan unchanged, and unquote raw segment heads only when the command did not parse cleanly (failing closed).
|
@codex review — addressed the P2: the destructive-head scan now walks the tree-sitter invocations, which are quote-aware, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad4931568d
ℹ️ 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".
| .iter() | ||
| .map(|t| unquote_token(t)) | ||
| .collect::<Vec<_>>() | ||
| .join(" ") |
There was a problem hiding this comment.
When a quoted argument contains spaces, joining the unquoted tokens with ordinary spaces makes token-internal whitespace indistinguishable from argument boundaries. For example, printf '%s\n' 'git push' --force only prints two arguments and was safe before this change, but it normalizes to text containing git push --force, so BashTool::validate_input now rejects it as destructive. Preserve token boundaries or match patterns structurally so quoted data cannot be reconstructed as an invocation.
Useful? React with 👍 / 👎.
Joining unquoted tokens with spaces let quoted data reconstruct an invocation: printf '%s\n' 'git push' --force prints two strings but normalized to text containing git push --force and was refused. A space inside one token is data, not an argument boundary — mask it so patterns only match across real argv boundaries. The raw-text scan is untouched, so everything it caught is still caught.
|
@codex review — addressed the P2: the normalized scan now masks whitespace inside a token before joining, so a space inside one argv element is data rather than an argument boundary — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8cc052610
ℹ️ 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".
| for invocation in &cmd.invocations { | ||
| let normalized = invocation | ||
| .iter() | ||
| .map(|t| unquote_token(t).replace(' ', "\u{0}")) |
There was a problem hiding this comment.
For literal payloads passed to another shell, intra-token spaces become command boundaries during the second parse. For example, bash -c "'git' push --force" receives 'git' push --force and executes a force push, but the raw scan misses the quoted spelling and this replacement masks both payload spaces, so the normalized scan also misses it; neither the later injection nor AST checks reject this form. Recursively parse literal bash -c/eval payloads rather than treating their whitespace as inert.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
bash -c "'git' push --force" hands its argument to another shell, where intra-token spaces become command boundaries again — the raw scan misses the quoted spelling and the space masking treated the payload as inert data. Extract literal bash/sh/zsh -c (including short-option clusters like -lc) and eval payloads, unquote one layer — exactly the text the inner shell receives — re-parse, and run the full scan recursively with a depth cap.
|
@codex review — addressed the P1: the destructive scan now recurses into literal shell payloads. For |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 283e818475
ℹ️ 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".
| match head { | ||
| "bash" | "sh" | "zsh" | "dash" | "ash" | "ksh" => { | ||
| for (i, a) in args.iter().enumerate() { | ||
| let cluster = a.strip_prefix('-')?; |
There was a problem hiding this comment.
-c
Handle valid operand-taking options before -c: for bash -O extglob -c "'git' push --force" (or bash -o posix -c ...), this loop reaches extglob and strip_prefix('-')? returns None, so the payload is never recursively scanned. bash --help explicitly documents -O shopt_option, -o option, and -c command, and Bash accepts this ordering; because the outer normalized scan treats the quoted payload as one data token, the destructive command bypasses the unconditional guard.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| let Some((head, args)) = unquoted.split_first() else { | ||
| continue; | ||
| }; | ||
| let Some(payload) = shell_payload(&base_name(head).to_lowercase(), args) else { |
There was a problem hiding this comment.
Unwrap runner commands before testing the invocation head: env bash -c "'git' push --force" and command bash -c ... execute the inner shell, but only the outer env/command head is passed to shell_payload, so recursion is skipped while the quoted payload evades the outer normalized scan. env --help states that it runs the supplied COMMAND, help command documents the same forwarding behavior, and this repository already models these wrappers in bash_parse::unwrapped_tokens; use that wrapper resolution here as well.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
Locating -c exactly meant trusting bash's option grammar: -O shopt, -o option and --rcfile FILE all take operands, so bash -O extglob -c "'git' push --force" hid the payload. Treat every argument of a shell head as a candidate payload instead — immune to option ordering, and an option operand parses as a harmless bare word. Wrapper chains (env/command/nohup/setsid) now come off via bash_parse::unwrapped_argv before the head is read, so env bash -c "'git' push --force" recurses too.
|
@codex review — addressed both P1s. (1) Shell option grammar: rather than locating |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e5e0f30a7
ℹ️ 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".
| let unwrapped = unwrapped_argv(invocation); | ||
| for tokens in [Some(&unquoted), unwrapped.as_ref()].into_iter().flatten() { |
There was a problem hiding this comment.
When the shell is launched through the exec builtin, this only checks the original argv and unwrapped_argv, whose wrapper list excludes exec. Consequently, exec bash -c '"git" push --force' avoids the raw and normalized patterns, while Bash's help exec confirms that exec replaces the shell with the specified command, so the force-push still runs. Treat exec as a runner and account for its -a operand before recursing.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| // become command boundaries again — so the payload gets the whole | ||
| // scan, not the inert-data treatment. One unquote layer is exactly | ||
| // the text the inner shell receives. | ||
| if depth < MAX_SHELL_SCAN_DEPTH { |
There was a problem hiding this comment.
When a payload contains four nested bash -c layers, the scan reaches depth 3 while the current invocation is still another shell and silently stops; if the final command is quote-split (for example, 'git' push --force), none of the outer raw or normalized scans match it, so validation permits the destructive command. Keep the depth cap if needed, but classify a remaining shell/eval payload as destructive or unknown when the cap is reached rather than returning Safe.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
exec replaces the shell with the command it is given, so exec bash -c "'git' push --force" ran a force push the scan never saw; its arguments are now candidate payloads like a shell's, and its -a NAME operand parses as a bare word. Reaching the depth cap with a shell payload still in hand now reports destructive instead of returning Safe — the nested command is unknown, and unknown must not mean allowed.
|
@codex review — addressed both P1s. (1) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf1ae1256
ℹ️ 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".
| /// operand is just another bare word. | ||
| fn shell_payloads(head: &str, args: &[String]) -> Vec<String> { | ||
| match head { | ||
| "bash" | "sh" | "zsh" | "dash" | "ash" | "ksh" | "exec" => args.to_vec(), |
There was a problem hiding this comment.
When a shell is launched through an unrecognized runner, the recursive scan never reaches its payload. For example, GNU timeout documents timeout [OPTION] DURATION COMMAND [ARG]..., but timeout 10 bash -c "'git' push --force" is classified Safe: the raw scan is defeated by quoting, the normalized scan masks the payload's intra-token spaces, and the only inspected head is timeout. This bypasses the unconditional destructive-command gate when the Bash call is otherwise permitted, so runners such as timeout/sudo need to be unwrapped or scanned for their executed command.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
Keying payload extraction to the invocation head meant every runner that can precede a shell needed listing: timeout 10 bash -c "'git' push --force" was Safe because the only head inspected was timeout, and sudo, nice, xargs, stdbuf and chroot were all the same hole. A payload needs a shell to interpret it and that shell must be named in the argv, so scan for the shell name at any position instead — no runner list to keep current.
|
@codex review — addressed the P1, and generalized rather than adding |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e77596ee3
ℹ️ 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".
| } | ||
|
|
||
| /// Commands that take a command string and run it in a fresh shell. | ||
| const SHELL_LAUNCHERS: &[&str] = &["bash", "sh", "zsh", "dash", "ash", "ksh", "exec"]; |
There was a problem hiding this comment.
On hosts with Fish installed, fish -c '"git" push --force' executes the destructive command but remains classified as safe: the raw scan misses the quoted command name, the normalized scan masks spaces inside the payload token, and shell_payloads never recurses because fish is absent from this list. The same issue applies to other command-string shells such as csh and tcsh, leaving the unconditional destructive-command gate bypassable through those interpreters.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| /// Commands that take a command string and run it in a fresh shell. | ||
| const SHELL_LAUNCHERS: &[&str] = &["bash", "sh", "zsh", "dash", "ash", "ksh", "exec"]; |
There was a problem hiding this comment.
exec arguments as shell payloads
When exec launches a normal binary, its remaining arguments are argv data rather than strings interpreted by a shell. Including exec here makes a harmless command such as exec printf '%s\n' "'git' push --force" recursively parse the final printf argument and reject it as destructive even though it only prints that text. Treat exec as a runner whose later argv should be searched for an actual shell, rather than as a shell launcher itself.
Useful? React with 👍 / 👎.
fish -c, csh -c and tcsh -c interpret a command string exactly like sh -c, so a payload behind one was never recursed into; an interpreter present on the host is a usable one. exec comes off the launcher list in the other direction: exec bash -c '…' is already covered by the shell name later in the argv, while exec printf … passes plain argv data, which was being re-parsed and refused.
|
@codex review — both addressed. P1: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d3d95ef42
ℹ️ 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".
| .find_map(|argv| match unwrapped_tokens(argv) { | ||
| Unwrap::Unresolved(reason) => Some(reason), | ||
| _ => None, |
There was a problem hiding this comment.
When a selector is supplied through exec env, this resolver stops at exec and returns no unresolved wrapper because unwrapped_tokens only recognizes env, command, nohup, and setsid. Consequently, CMD=rm exec env -S '${CMD} -rf /tmp/x' reaches rm -rf while the raw/normalized scans see only ${CMD}, and neither this fail-closed path nor the AST head check rejects it. Unwrap exec and its options before resolving the nested env -S command.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| fn inherits_export_for_git_config(name: &str) -> bool { | ||
| matches!(name, "HOME" | "XDG_CONFIG_HOME") |
There was a problem hiding this comment.
In environments where XDG_CONFIG_HOME is initially unset—which is a common and valid configuration—a bare XDG_CONFIG_HOME=/tmp/h creates an unexported shell variable, so XDG_CONFIG_HOME=/tmp/h; git p does not pass that value to Git. Treating the name as unconditionally inherited/exported therefore rejects this valid command as destructive; derive inherited export attributes from the actual process environment, or restrict this special case to names guaranteed to be present.
Useful? React with 👍 / 👎.
The wrapper walk started at the first token, so a name it did not know
in front of one it did hid the unknown behind it: `exec env -S '${CMD}
-rf /tmp/x'` reported nothing wrapped and reached `rm -rf` while every
scan saw only `${CMD}`. Adding `exec` to the list would have closed
that spelling and left `stdbuf`, `sudo`, `time`, `xargs` and every
other name nobody enumerated.
What precedes a wrapper decides nothing about whether that wrapper
resolves, so resolution now runs from every word of an invocation. The
list of wrapper names no longer has to be complete for the unknown
behind an unlisted name to be reported. A head whose operands are text
still runs nothing they name, so `printf '%s\n' env -S '${CMD}'` prints
it; being wrong about that list can only report an unknown that is not
one.
`exec` joins the wrapper list too, so the command it names resolves
rather than merely failing closed, and its `-a NAME` is read as the
operand it is.
|
@codex review — pushed [P1] Treat exec as a wrapper when resolving Verified with names that are in no list at all — each now reports The one list that remains is the opposite polarity and therefore safe to be wrong about: a head whose operands are text runs nothing they name, so [P2] Do not assume XDG_CONFIG_HOME is inherited as exported — fixed by deriving, not removing. You're right that a bare assignment to an unset name creates an unexported shell variable. The check now derives from what actually holds: Gate on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9629d2c194
ℹ️ 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".
| let head_is_data = argv.first().is_some_and(|t| { | ||
| DATA_COMMANDS.contains(&base_name(&unquote_token(t)).to_lowercase().as_str()) | ||
| }); | ||
| if head_is_data { |
There was a problem hiding this comment.
When a data command is itself wrapped, such as env printf '%s\n' env -S '${CMD}', the raw head is env, so this exemption does not apply. The subsequent every-word scan treats the printed env -S operands as executable, returns Unresolved::Expansion, and causes the destructive-command and permission gates to reject an innocuous print. Determine whether the resolved head is a data command before scanning suffixes.
Useful? React with 👍 / 👎.
| /// does not come with them set, which is why `GIT_CONFIG_GLOBAL=/tmp/g; | ||
| /// git status` runs an ordinary status. | ||
| fn inherits_export_for_git_config(name: &str) -> bool { | ||
| name == "HOME" || (name == "XDG_CONFIG_HOME" && std::env::var_os(name).is_some()) |
There was a problem hiding this comment.
When Agent itself inherits an exported GIT_CONFIG_GLOBAL (or another GIT_CONFIG* selector), a later bare assignment retains that export exactly as XDG_CONFIG_HOME does. Thus GIT_CONFIG_GLOBAL=/tmp/evil; git p can load an attacker-controlled alias and execute a destructive command while this function reports that the selector is not inherited and the scan classifies it as safe. Conversely, Agent supports environments with no HOME, where hard-coding HOME as inherited over-blocks an unexported bare assignment. Derive inherited status from the actual process environment for all Git configuration selectors.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
Whether a bare assignment reaches git is not a property of the name but
of whether the environment already exports it, so it is now read from
the environment for every selector alike. A shell handed an exported
`GIT_CONFIG_GLOBAL` carries `GIT_CONFIG_GLOBAL=/tmp/evil; git p` to
git, which was reported safe; a shell with no `HOME` carries nothing,
which was reported destructive. Neither name is assumed either way.
The same rule was being asked in three spellings, so a selector could
be recognised by one walk and missed by another: an unaccounted `HOME=`
handed through `env -S` redirected git's config while the check looked
only for `GIT_CONFIG…`. All three now ask `selects_git_config`.
The head that decides whether operands are text is the one that ends up
running, so it is resolved before the operand scan: `env printf '%s\n'
env -S '${CMD}'` prints its operands exactly as `printf` alone does and
is no longer refused.
Resolution also runs only from words that name a wrapper. Running it
from every word copied the whole suffix each time, quadratic in the
argument count — 142ms for one invocation of 1600 arguments, now 0.4ms.
|
@codex review — pushed [P1] Derive inherited exports for every git config selector. Fixed as the mechanism, not the name. Whether a bare assignment reaches git is not a property of the name — it is whether the environment already exports it — so that is now read from the environment for every selector alike. A shell handed an exported Proactive pass on the same defect shape, as suggested — and it found a third site. The selector question was being asked in three different spellings, so a name could be recognised by one walk and missed by another. The unaccounted-mention check looked only for [P2] Unwrap the effective head before scanning operand suffixes. The head that decides whether operands are text is the one that ends up running, so it is resolved first: A performance regression I introduced last round, found while checking the Coverage question. Resolving from every word copied the whole suffix each time — quadratic in the argument count: 142ms for one Coverage: this is explanation (1), the wait budget — not the parser. Evidence rather than assertion:
Fixed in the harness, not the assertion: the script's last event now waits on a caller-supplied condition ( One housekeeping note: Gate on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d05b7b3f
ℹ️ 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".
| for word in words { | ||
| push_assignment(&mut pairs, &word); | ||
| } | ||
| idx += 2; |
There was a problem hiding this comment.
For an attached operand such as env -S'HOME=/dev/null' HOME=/tmp/evil git p, -S consumes only the current token; env --help describes it as processing the split string into separate arguments. Advancing by two skips the following HOME=/tmp/evil, while the collected safe HOME name causes the unaccounted-mention check to ignore the skipped override. Git therefore reads /tmp/evil/.gitconfig and can execute a destructive p alias while this classifier reports the invocation safe; distinguish attached from separate -S operands when advancing.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| const GIT_CONFIG_SELECTORS: &[&str] = &[ | ||
| "GIT_CONFIG_GLOBAL", | ||
| "GIT_CONFIG_SYSTEM", | ||
| "GIT_CONFIG", | ||
| "GIT_CONFIG_PARAMETERS", | ||
| "GIT_CONFIG_COUNT", | ||
| "HOME", | ||
| "XDG_CONFIG_HOME", |
There was a problem hiding this comment.
GIT_DIR also selects a Git configuration file ($GIT_DIR/config), but it is absent from this selector set and from the opacity check. For example, a bare repository whose config defines alias.p = !<destructive command> makes GIT_DIR=/tmp/evil.git git p execute that alias, while the new environment walk ignores the assignment and classifies the command as safe. Include this repository-config selector in the same fail-closed handling as HOME and GIT_CONFIG_GLOBAL.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
Six findings on this branch have had one shape: an `env` spelling or a config selector nobody enumerated reached the safe path. The walk was re-implementing two grammars it does not own — env's argument processing and git's selector semantics — and an unmodelled spelling read as "nothing set" rather than "not read". Invert that. An option of the `env` family whose effect on the words after it is not modelled now ends the walk and refuses: what the command runs with is unknown, and unknown is not safe. The modelled set is deliberately short, because being absent from it costs a prompt rather than a miss. This narrows what classifies as automatically safe — `env` with an option this does not model no longer passes silently, it asks. That is the trade being made deliberately: an unmodelled spelling should be inconvenient rather than invisible. Behind that, two spellings that were reported and are correct on their own: `GIT_DIR` and `GIT_COMMON_DIR` name a repository whose `config` is read like any other, and an attached `-S` operand is the whole of its token, so the assignment after it is still read. Neither is now load-bearing.
|
@codex review — pushed The pattern, and why I stopped enumeratingSix findings on this branch have had one shape: an incomplete wrapper list (twice), export inheritance applied to one variable name, a missing The categorical ruleAn option of the Declared narrowing, stated explicitly: this reduces how many commands classify as automatically safe. Ordinary shapes are unaffected and pinned: The two reported spellings, kept but no longer load-bearing
Coverage — root-caused, fixed, and it was not the parserI reproduced it locally and it is neither a slow parser nor a too-small budget. Evidence: forcing the answer to fire before arming reproduces CI exactly — same assertion, 63.01s against CI's 63.13s. With the beat gated on the real readiness signal ( Gate on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc5a9e11b
ℹ️ 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".
| fn state_is_unresolvable(raw: &str, statements: &[String]) -> bool { | ||
| if shell_text_facts(raw).control_operator { | ||
| return true; | ||
| } | ||
| statements.iter().any(|statement| { |
There was a problem hiding this comment.
Commands such as if true; then export GIT_CONFIG_GLOBAL=/tmp/evil; fi; git p can execute an alias from the selected file, but this function only recognizes &&/||/heredoc state and unusual set operands as unresolved. shell_statements splits the conditional at semicolons, leaving the carrier in a then export ... fragment that carries_git_config does not recognize, so no opaque configuration reaches the final git and validation passes. Mark conditionals, loops, and similar control-flow constructs unresolved before tracking configuration state.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| let head = invocation | ||
| .first() | ||
| .map(|t| base_name(&unquote_token(t)).to_lowercase()); | ||
| if !head.is_some_and(|h| COMMAND_RUNNER_WRAPPERS.contains(&h.as_str())) { | ||
| return (pairs, true); |
There was a problem hiding this comment.
When env is behind an unlisted runner, this early return misses its split-string environment: timeout 5 env -S'GIT_CONFIG_GLOBAL=/tmp/evil git p' executes an alias from /tmp/evil, yet returns (pairs, true) here, and the fallback sees the first assignment as -SGIT_CONFIG_GLOBAL rather than a selector. This matches the documented CLI semantics—timeout --help says it starts COMMAND, and env --help says -S processes and splits its operand—so the environment walk must locate env after arbitrary runners rather than only at argv position zero.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| if !env_option_is_modelled(&unquoted) { | ||
| unreadable = true; | ||
| break; |
There was a problem hiding this comment.
This unknown-option branch also runs for every entry in COMMAND_RUNNER_WRAPPERS, so valid options belonging to other wrappers are treated as unknown env grammar. For example, Bash's help command documents command [-pVv] command ... and says -p selects a default PATH, but command -p git status sets env_unread, becomes Destructive, and is rejected by BashTool::validate_input. Apply this refusal only while walking an actual env invocation, or model each wrapper's grammar separately.
Useful? React with 👍 / 👎.
| if env_unread { | ||
| findings.push(DestructiveFinding { | ||
| level: DestructivenessLevel::Destructive, | ||
| reason: "an env option this does not model decides the environment; what it runs cannot be determined" | ||
| .to_string(), | ||
| }); |
There was a problem hiding this comment.
The declared narrowing says unmodeled options should lose automatic approval but still receive [y]/[a]; instead this creates a destructive finding, and BashTool::validate_input consumes such findings before permission checks. Thus a valid command such as env --default-signal git status—documented by env --help as resetting signal handling for COMMAND—is returned as InvalidInput and cannot be run even after confirmation. Represent this state as unsafe for Auto classification without feeding it into the unconditional destructive-command validator.
Useful? React with 👍 / 👎.
…er it is Four review findings, two bypasses and two over-blocks. An environment the walk cannot read now reports Risky rather than Destructive. find_destructive feeds BashTool::validate_input, which refuses outright, so the fail-closed change had made valid commands such as `env --default-signal git status` unrunnable even with confirmation. Risky withholds automatic approval and leaves the prompt, which is what the declared narrowing promised; validate_input now blocks only on Destructive. Control flow is unresolved state. `if true; then export GIT_CONFIG_GLOBAL=/tmp/evil; fi; git p` split into fragments whose carrier sat behind a `then`, so the selector was invisible and the trailing git looked unconfigured. The wrapper is not always argv[0]: `timeout 5 env -S'...'` hid an env behind an unmodelled runner. The walk now starts wherever the modelled wrapper is, and a data head still names nothing it runs. env's option grammar is env's alone — applying it to every wrapper made `command -p git status` look like unknown env grammar. Also restores has_dynamic_wrapper, which main's grants.rs calls; the merge into this branch kept this branch's richer Unwrap::Unresolved and dropped the function, leaving the branch unbuildable.
|
@codex review All four findings from Unknown env options → permission prompting ( Control flow is unresolved state ( env behind arbitrary runners ( env grammar limited to env ( One thing to flag that was not in your findings: the branch did not build at Four regression tests, each verified to fail without its fix (I removed each change in turn and watched the corresponding test go red). The existing Gate: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8f380f3b0
ℹ️ 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".
| let Some(wrapper_at) = invocation | ||
| .iter() | ||
| .position(|t| COMMAND_RUNNER_WRAPPERS.contains(&name_of(t).as_str())) |
There was a problem hiding this comment.
Using .position(...) inspects only the first wrapper-shaped token, even though one simple invocation can execute multiple commands. I verified with GNU find 4.9 (find --help lists -exec COMMAND ;) that find /tmp -maxdepth 0 -exec echo env x \; -exec env -S'GIT_CONFIG_GLOBAL=/tmp/evil-gitconfig git p' \; runs both actions. Here the first, inert env is selected and the walk stops at x, so the real env -S is never examined; its external config can define p as a destructive Git alias while find_destructive sees only an ordinary git p. Continue searching after a candidate walk terminates, or model every executable wrapper occurrence.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
One invocation can execute more than one command, so taking the first wrapper-shaped token let an inert env mask a real one: find with two -exec actions selected the env inside 'echo env x', stopped at its operand, and never examined the env -S that points git at an external config. Every occurrence is now walked and what they set is pooled; an occurrence that reads straight to a command word contributes nothing.
|
@codex review Fixed at I widened the search from argv[0] to "wherever the wrapper is", but kept
Test One thing worth saying plainly about the direction of travel here: the last three rounds on this function have each been "the enumeration assumed one of something there can be many of" — one wrapper position, one wrapper per invocation, one grammar for all wrappers. Pooling every occurrence removes the last count-based assumption I can see in the walk, but if you find another, I would rather restructure than patch again. The categorical guard behind it ( Gate: fmt, clippy |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c214aa8027
ℹ️ 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".
| for (at, token) in invocation.iter().enumerate() { | ||
| if !COMMAND_RUNNER_WRAPPERS.contains(&name_of(token).as_str()) { | ||
| continue; | ||
| } | ||
| let (found, read_fully) = walk_runner_from(invocation, at, name_of(token)); |
There was a problem hiding this comment.
When a Git pathspec is literally env followed by an assignment-looking pathspec, such as git status -- env GIT_CONFIG_GLOBAL=/tmp/g, no wrapper executes—git status -h documents this tail as [--] [<pathspec>...]. This loop nevertheless walks that inert env, pools the following token as an environment assignment, and causes env_defines_opaque_git_alias to classify the harmless status command as Destructive, so BashTool::validate_input refuses it even after confirmation. Scan wrapper occurrences only where the enclosing command can execute them, or preserve boundaries such as Git's -- pathspec separator.
Useful? React with 👍 / 👎.
git status -- env GIT_CONFIG_GLOBAL=/tmp/g names two pathspecs and runs no wrapper, but both the occurrence scan and the unaccounted-mention check read them as environment, so a plain status classified Destructive and validate_input refused it outright. Both now stop at the separator — except when the head is itself a wrapper, where -- belongs to its own grammar and env -- GIT_CONFIG_GLOBAL=... git p really does hand git that file.
|
@codex review Fixed at I bounded the wrapper scan at The boundary is not unconditional. When the head is itself a wrapper, Two tests, one per direction:
That second assertion started as "not Gate: fmt, clippy |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f91d091840
ℹ️ 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".
| .map(|t| canonical_whitespace(&unquote_token(t)).to_lowercase()) | ||
| .collect(); | ||
| for pattern in DESTRUCTIVE_PATTERNS { | ||
| if normalized.contains(pattern) || per_token.iter().any(|t| t.contains(pattern)) { |
There was a problem hiding this comment.
When a Git read command uses --, this normalized whole-invocation scan still treats the following pathspecs as executable words. For example, git log -- g'it' push --force normalizes to text containing git push --force and is classified Destructive, although git log -h documents the grammar as git log ... [[--] <path>...], so these tokens are only paths and cannot push anything. Because BashTool::validate_input rejects every destructive finding, users cannot run this safe command even after confirmation; bound this scan at Git's option terminator while retaining the wrapper-head exception needed for env -- ....
Useful? React with 👍 / 👎.
…ands git log -- g'it' push --force shows a file's history, but the git scan walks every token spelled git — and a pathspec unquotes to one — so it rebuilt the tail as git push --force and validate_input refused a read-only command. The -- boundary now lives in executable_tokens and is used by the destructive-pattern scan, the environment walk, the unaccounted-mention check and the git subcommand scan, instead of each growing its own. A wrapper head still keeps every token, since env -- FOO=... git p runs what follows. The untokenized backstop scan is deliberately left alone: it has no token structure for -- to mean anything in, and exempting everything after a -- would weaken the one net that catches mis-tokenized input. A test pins that over-block so it cannot be relaxed unnoticed.
|
@codex review Fixed at The mechanism was not the scan you pointed at. Rather than add a fourth Declared, not fixed: the untokenized backstop scan over the whole command string still flags Tests: the reported command plus a Gate: fmt, clippy |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e4bab2d85
ℹ️ 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".
| match invocation.iter().position(|t| unquote_token(t) == "--") { | ||
| Some(end) => &invocation[..end], | ||
| None => invocation, |
There was a problem hiding this comment.
Do not treat every non-wrapper -- as the end of executable tokens. GNU xargs --help documents its grammar as xargs [OPTION]... COMMAND [INITIAL-ARGS]..., and xargs -- git push -uf origin main therefore executes the force push, but this slice ends before git, causing the normalized and clustered-git scans to miss it; the raw patterns also miss the clustered -uf, so the destructive validator permits the command. The boundary needs to account for commands such as xargs, find -exec, and timeout whose operands can themselves name executables.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| argv.iter() | ||
| .enumerate() | ||
| .filter(|(_, word)| { | ||
| COMMAND_WRAPPERS.contains(&base_name(&unquote_token(word)).as_str()) | ||
| }) | ||
| .find_map(|(start, _)| match unwrapped_tokens(&argv[start..]) { | ||
| Unwrap::Unresolved(reason) => Some(reason), |
There was a problem hiding this comment.
Limit this suffix scan to tokens that can actually be in command position. For example, git log -- env -S '${CMD}' treats every word after -- as a pathspec, but this loop starts wrapper resolution at the operand env, reports Unresolved::Expansion, and check_parsed_security makes BashTool::validate_input reject the otherwise read-only command. This newly bypasses the shared operand boundary used by the other token scans.
Useful? React with 👍 / 👎.
| let unwrapped = unwrapped_argv(invocation); | ||
| let tokens = unwrapped.as_ref().unwrap_or(&unquoted); | ||
| for payload in shell_payloads(tokens) { |
There was a problem hiding this comment.
--
Pass only executable-position tokens to shell_payloads for non-wrapper heads. With git log -- bash -c "'rm' -rf /tmp/x", all tokens after -- are pathspec data, yet this call treats bash as a launcher, recursively parses its apparent payload, and classifies the quoted rm as destructive; the command is therefore rejected even though no nested shell runs. This is another structured scan that does not inherit the new operand boundary.
Useful? React with 👍 / 👎.
The -- boundary was fail-open: it truncated for every non-wrapper head,
but xargs documents 'xargs [OPTION]... COMMAND [INITIAL-ARGS]...', so
xargs -- git push -uf really runs the push and every scan skipped it.
It is now an allowlist of heads whose -- introduces operands, which is
git alone. An unlisted head keeps all its tokens, so a miss over-blocks
instead of passing something through unseen.
The helper moves to bash_parse, where the wrapper-suffix scan and the
nested-payload scan can share it: git log -- env -S '${CMD}' reported an
unresolved expansion, and git log -- bash -c "'rm' -rf /tmp/x" was
recursed into as a launcher, both rejecting read-only commands.
|
@codex review Fixed at My The rule is now an allowlist of heads whose Both P2s are the same boundary reaching two more scans, so the helper moved to Tests both ways: Gate: fmt, clippy |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
find_destructivematchedDESTRUCTIVE_PATTERNSagainst the raw command text, so re-spelling a command without changing what it runs walked past the guard. Driven end to end throughBashTool::validate_input:This guard runs before the permission system and returns
InvalidInput, so it is the safety floor that no rule and no future grant can override. A hole in it is a hole under everything else.Why #495 didn't already cover this
#495 fixed the same evasion class for the name-based classifier (
check_parsed_security), by unquoting at comparison time.find_destructiveis a separate substring scan overcmd.rawand was never in its path —'rm' -rf /tmp/xis caught today only because #495's fix rejects it elsewhere, which is what masked this.Fix
Each invocation is additionally scanned in normalized form: its tokens with shell quoting and backslash escapes stripped, joined by single spaces. The pipeline-head check now unquotes and de-paths as well, so
/bin/rmand'rm'are both recognised asrm.Both scans are additive — anything the raw scan caught is still caught.
Verification
quoting_and_escaping_cannot_hide_a_destructive_command— the evasions above plus'shutdown' -h nowordinary_commands_stay_safe— the negative direction, because a guard that refuses everything is its own kind of failurePre-existing limitation, pinned not fixed
While testing the negative direction I found that
grep -r 'rm -rf' docs/,grep 'shutdown' log.txtandecho 'rm -rf /'are already classifiedDestructiveonmain— a substring scan cannot tell a command from a string that merely mentions one.That is over-blocking (failing in the safe direction), it pre-dates this change, and fixing it properly means matching patterns only at command-head position — a design change that risks weakening detection and deserves its own PR.
a_literal_argument_mentioning_a_pattern_is_still_flaggedpins the current behaviour so it is documented rather than rediscovered, and so a later fix has a test to flip.cargo test --workspace --all-targetsgreen apart from the 3bwrap_*tests, which fail on this host withsetting up uid map: Permission deniedand pass in CI.clippy --all-targets -- -D warningsandfmt --checkclean.Register
Advances D9-09 (dangerous-command list that holds regardless of grants). The list itself already runs ahead of the permission system; this makes it non-trivially spellable-around, which is the property persistent grants (D9-06/07) will depend on.