fix(readonly-bash): anchor git verbs to subcommand position + wrapper/indirection handling (#577)#697
Conversation
… wrapper/indirection (#577) The classifier matched git/curl verb regexes against the whole command string, which cannot distinguish a verb in COMMAND position from the same word in an ARGUMENT. That over-blocked recon (`git log -S push`, `git help push`) and under-blocked mutations reached via a wrapper (env/xargs/command/sudo/busybox/eval), a shell variable (`g=git; $g push`), or a quoted flag value (`curl -X "POST"`). Replace the six anywhere-in-segment git-verb regexes with a small command-line tokenizer (Pass 1c) that splits pipeline/chaining segments, resolves one level of `VAR=val;$VAR` indirection, unwraps no-op wrappers, and recurses into `$(…)`/backtick substitutions, judging the verb in subcommand position. A `hiddenGitMutation` backstop keeps obfuscated mutations (`eval git push`, `{ git push; }`, `\git push`) blocked so the change is strictly MORE conservative than the old regex for the security cage, never less. Also widen PIPE_TO_SHELL (more shells + piped interpreters) and INTERPRETER_WRITE_API (os.system/os.popen/subprocess./ Deno.*), and add tokenizer-based curl `-X <method>` detection. git config stays regex-matched (its quoted value must be read raw). Tests: 185 existing + 72 new (#577 acceptance cases + obfuscation backstop + one asserted residual); consumers (dispatcher/plan-mode-gate/nesting/ parser/plan-mode-addendum) green; tsc --noEmit clean. Adversarially verified against the pre-change classifier: the only behavioral deltas are the intended over-block fixes + tighter curl/indirection blocking. One documented residual: $(…)-in-command-position obfuscation, out of the module's best-effort recon-cage threat model.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 2d6a5899fc
ℹ️ 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".
| while (idx < tokens.length && (/^[A-Za-z_]\w*=/.test(tokens[idx]!) || tokens[idx]!.startsWith('-'))) { | ||
| const am = /^([A-Za-z_]\w*)=(.*)$/.exec(tokens[idx]!); | ||
| if (am !== null) vars.set(am[1]!, am[2]!); | ||
| idx++; | ||
| } |
There was a problem hiding this comment.
When env is used with an option that takes a separate operand, this loop skips only the flag and then treats that operand as the wrapped command. I checked env --help: -u, --unset=NAME and -C, --chdir=DIR are options before [COMMAND [ARG]...]; therefore env -u FOO git stash executes git stash, but resolveSegment returns command FOO, and because git stash was removed from STRIPPED_RULES while the backstop only covers always-mutating verbs, the classifier now allows this mutating command. Consume env option operands before deciding where the command starts.
Useful? React with 👍 / 👎.
| while (idx < tokens.length && tokens[idx]!.startsWith('-')) { | ||
| const f = tokens[idx]!; | ||
| idx++; | ||
| if (/^-[In]$/.test(f) && idx < tokens.length) idx++; // -I {} / -n N consume an arg | ||
| } |
There was a problem hiding this comment.
For xargs options whose value is in the next token, this parser stops at the value and never unwraps the real command. I checked xargs --help: usage is [OPTION]... COMMAND and -n, --max-args=MAX-ARGS consumes an argument; find . | xargs --max-args 1 git tag -d v1 runs the mutating git tag -d, but the classifier treats 1 as the command and allows it because conditional git tag/stash/branch mutations no longer have regex backstops. Consume operands for long/short xargs options like --max-args, --max-lines, and --max-procs before returning.
Useful? React with 👍 / 👎.
…itional git mutations (#577) Addresses Codex review on #697 (both P2): - env `-u NAME` / `-C DIR` (+ `--unset`/`--chdir`/`-S`) and xargs `--max-args N` / `-n N` / `-L N` / `--max-procs N` / `-I R` (+ long forms) take a SEPARATE operand. The unwrap loops now consume it, so the operand is no longer mistaken for the wrapped command — `env -u FOO git stash` and `xargs --max-args 1 git tag -d` resolve `git` to command position. - The hidden-git backstop now re-runs the full subcommand classifier (gitSegmentReason) on the tokens after any bare `git`, so CONDITIONAL mutations (stash / tag -d / branch -D / remote add / worktree remove) reached via a wrapper or compound form are caught too — not just the always-mutating verbs. Defense-in-depth against unenumerated wrapper options. Tests: +13 (env/xargs operand + conditional-mutation cases); 270 classifier + dispatcher/plan-mode-gate/nesting consumers green; tsc --noEmit clean.
|
Thanks @codex — both P2 findings were valid and are fixed in 1. env option operands — 2. xargs option operands — Plus a defense-in-depth fix for the root gap you identified — the hidden-git backstop now re-runs the full subcommand classifier on tokens after any bare +13 regression tests (270 classifier + consumers green, tsc clean). |
Done
Testing
|
1 participant
Closes #577.
Problem
classifyBashCommandmatched git/curl verb regexes against the whole command string. A string match cannot tell a verb in command position from the same word in an argument, causing both defects #577 tracks:git log -S push,git log -G merge,git log --grep revert,git help push,git push --help. This hurtgit-investigator, whose core job is git-history search.env/xargs/command/sudo/busybox), a shell variable (g=git; $g push), a pipe-to-interpreter (| python), or a quoted flag value (curl -X "POST") slipped through.Fix
Replace the six anywhere-in-segment git-verb regexes with a small command-line tokenizer (Pass 1c) that:
; | & ( )), quote-aware;VAR=val; $VARindirection (shared across segments);env/xargs/command/sudo/nice/time/timeout/nohup/stdbuf/busybox/eval);$(…)/backtick substitutions;…then judges git by subcommand position (
gitSegmentReason) and curl by parsed argv (curlSegmentMutating, catches the quoted-X "POST"). Also widensPIPE_TO_SHELL(more shells + piped interpreterspython/perl/…) andINTERPRETER_WRITE_API(os.system/os.popen/subprocess./Deno.*).git configstays regex-matched — its mutation signal (a quoted<value>) must be read on the raw string.Conservative-by-construction (security)
Positional anchoring is more precise but was initially less conservative than the old blunt regex — an adversarial differential caught that it regressed ~12 git-mutation obfuscations the old code caught (
eval git push,{ git push; },for…do git push,env g=git $g push,\git push,timeout -k 5 10 git push). AhiddenGitMutationbackstop (+eval/env/-khandling) restores them, so the classifier is strictly ≥ the old regex for the cage. A broad differential over ~55 realistic commands shows the only behavioral deltas vs. the pre-change classifier are the intended Tier-2 loosenings + tighter curl/indirection blocking — no new over-blocks on recon, no new bypasses.Documented residual (asserted in a test)
$(echo git) push— a command substitution producing the command name — is not statically resolvable without executing it, so (likesh -c "git push") it's out of scope per the module's stated best-effort recon cage, not hostile-process sandbox threat model. Layer-1write_file/edit_filestrip remains the real boundary.Acceptance criteria
git log -S push/--grep push/git help pushread-only;git push/git -c x=y commitmutating.find | xargs rm,env rm -rf,g=git; $g push,| python,os.system(...)mutating.curl -X "POST" https://xmutating;grep -rn "curl -X POST" .read-only.Tests
readonly-bash.test.ts: 257 pass (185 existing + 72 new — acceptance cases, obfuscation backstop, asserted residual).dispatcher(104),plan-mode-gate(16),nesting(28),agents/parser(13),plan-mode-addendum(12).tsc --noEmit(strict): clean.classifyBashCommandsignature unchanged — zero API-compat impact.🤖 Implemented + adversarially verified via agent-afk.