harden(supply-chain): quote-aware tokeniser closes wrapped-install bypass (#140) - #146
Merged
Merged
Conversation
…pass (#140) PR #139 review (Codex + Antigravity) flagged a HIGH bypass class: the naive shell_tokens() did not honour shell quoting, so several install shapes slipped past the gate. 1. sh -c '<install>' / bash -c '<install>' wrapped installs 2. $(<install>) / backtick <install> command-substitution installs 3. 'npm' install pkg / "pip" install pkg quoted-head installs 4. operator inside a quoted flag value (--description="install & test") truncated [^|;&<>]+ and dropped packages after the flag Implementation: - shell_tokens() walks bytes with quote-state for single, double, and ANSI-C $'…' quotes. Token text is the literal payload with quote chars stripped, so a quoted head matches npm/pip/etc. - mask_quoted_operators() produces a same-length copy where operator bytes inside quoted regions become spaces. Package-list regexes run against this; the captured arg span is re-sliced from the original command to keep real characters in package names. - extract_recursion_segments() pulls sh -c arg / bash -c arg / $(…) / backtick bodies out as flat strings. detect_installs_into() recurses on each (depth-capped at MAX_RECURSION_DEPTH = 6). - Regex anchors now treat ' and " as command-start delimiters so 'npm' install lodash is caught by the outer pass. - Structural dedup collapses duplicates when the outer regex + recursion both fire on the same install. Adversarial fixtures (TDD, all real shell forms): - quoted_sh_c_install_detected - quoted_bash_c_install_detected - command_substitution_install_detected - backtick_substitution_install_detected - quoted_head_install_detected - quoted_operator_in_flag_value_does_not_truncate_scan - ansi_c_quoted_install_detected - quoted_install_as_data_argument_still_flagged (echo "$(npm install foo)" — conservatively flagged: the install runs before echo prints, so the side-effect already happened) - nested_sh_c_install_detected — defence in depth - unmatched_quote_falls_back_safely — no panic on malformed input - shell_tokens unit tests for quote stripping + operator-in-quote Out of scope (documented in code comments): heredocs (<<EOF … EOF), eval with variable expansion, process substitution <(…). These would need shell-style word expansion, not just tokenisation. Test count: 2582 -> 2596 (+14). All 2596 pass, 0 failures. Clippy: 49 warnings before, 49 after — no new warnings on changed code. Pre-existing type_complexity on parse_package_args left untouched per task constraints. Closes #140. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…c clusters, depth fail-open) Codex + agy peer review on PR #146 surfaced 2 BLOCKERs + 1 HIGH + 1 LOW. All four addressed here; +8 regression tests. 1. BLOCKER (both reviewers) — quoted SUBCOMMAND verb bypass. `npm 'install' lodash`, `pip "install" requests`, `'npm' 'install' x`. The package-bearing regexes did not allow quote chars around the verb subcommand, so the outer match missed. shell_tokens strips quotes so the bare-install path then saw a package after the verb and flipped `has_package=true` — silent skip. Fix: wrap each subcommand in optional `['"]?` in all 7 regexes: - NPM/PNPM: `\s+['"]?(?i:i|install|add)['"]?` - YARN: `\s+['"]?(?i:add)['"]?` - PIP/PIPX: `\s+['"]?(?i:install)['"]?` - UV: `(?:['"]?(?i:pip)['"]?\s+)?['"]?(?i:install|add)['"]?` - POETRY: `\s+['"]?(?i:add)['"]?` 2. HIGH (both reviewers) — `-c` combined-option bypass. `bash -lc '<install>'`, `bash -xc '<install>'`, `bash -x -c '<install>'` skipped recursion because `extract_recursion_segments` matched only the exact token `-c`. Fix: locate a shell head, then scan forward through short-option clusters for any cluster ending in `c` (handles -c, -lc, -xc, -ic, etc., plus `-x -c` order). Long options (`--`) and non-option tokens end the scan without recursion. 3. BLOCKER (both reviewers) — recursion depth-limit FAIL-OPEN. At `MAX_RECURSION_DEPTH` (6) the recursion silently dropped any installs at deeper nesting. A 7-deep nest payload (`sh -c "$(sh -c "$(sh -c '...')")"`) auto-allowed at the gate — the gate is supposed to be conservative. Fix: at the cap, if there are still unresolved recursion segments, push a synthetic Unvettable ParsedInstall so the caller fails CLOSED (Verdict::Ask/Unavailable) rather than Verdict::Skip. 4. LOW (agy) — order-sensitive dedup. `npm install a b && npm install b a` didn't collapse because the package-list comparison was order-strict. Fix: `installs_equivalent` now sorts cheap clones before comparison. Original ordering preserved in the input. Tests (+8): - quoted_install_verb_detected - double_quoted_install_verb_detected - quoted_install_verb_combined_with_quoted_head - bash_lc_combined_option_install_detected - bash_xc_combined_option_install_detected - bash_separate_options_install_detected - recursion_depth_limit_surfaces_unvettable_not_skip (with a 7-deep nest payload — proves fail-closed) - dedup_collapses_reordered_package_lists 2,639 tests pass (+8 from prior #146 head), clippy clean. Per the peer-review-codex-agy-mandatory rule: re-review on this head is required before the merge question opens. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…c-anywhere, -- marker) agy round-2 peer review on PR #146 surfaced two new option-parsing bypasses in the recursion extractor: 1. BLOCKER — `bash -c -- '<install>'`. POSIX end-of-options marker `--` was being treated as the recursion body, so the actual command after the marker silently bypassed the gate. Fix: after locating the `-c` cluster, skip any `--` token before reading the body. 2. HIGH — `bash -ce '<install>'`, `bash -cce '<install>'`. Short-option clusters where `c` is NOT the last char (e.g. `-ce` = `-c -e`, `-cce` = `-c -c -e`) still take the next token as the `-c` command argument in bash, but the original `ends_with('c')` check missed them entirely. Fix: `contains('c')` instead — every cluster carrying `c` triggers recursion. +1 combined regression test (`bash_c_double_dash_install_detected`) asserting both shapes detect. 2,640 tests pass (+1), clippy clean. Mandate gate: Codex round-2 review still in flight on commit 2263116. Will consolidate with that on return before opening the merge question. This commit lands the agy round-2 findings unconditionally — both are real bypasses regardless of what Codex finds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 23, 2026
thehoff
added a commit
that referenced
this pull request
May 23, 2026
…llowlist (agy #148 BLOCKER) agy round-1 peer review on PR #148 (re-implementation of #141) caught a BLOCKER: `awk`, `gawk`, and `sed` were in DATA_CONSUMING_UTILITIES, but those tools are Turing-complete with native command-execution primitives: - `awk 'BEGIN { system("npm install evil") }'` - `gawk` (and `mawk`) — same `system()` primitive - GNU `sed 's/.*/npm install bad/e'` — the `e` flag executes the replacement string as a shell command Masking the segment to spaces removed the install from the regex pass AND from the recursion sweep (recursion uses cmd_raw, but no shell-c extractor walks awk/sed script bodies). Net: silent bypass. Fix: remove `awk`/`gawk`/`sed` from the allowlist. The allowlist now contains ONLY utilities that genuinely cannot spawn a shell. Added a doc-comment block explicitly enumerating execution-capable exclusions so a future contributor doesn't re-add them. Also picked up agy's MEDIUM (allowlist coverage): added `jq`, `base64`, `xxd`, `od`, `hexdump` — all genuinely data-consuming, no shell-out flags, currently produce false-positive blocks when they read manifests or payloads containing install-shaped text. New allowlist (15 → 17 entries, awk/gawk/sed dropped, base64/xxd/od/ hexdump/jq added): echo, printf, cat, tac, tee, grep, egrep, fgrep, rg, head, tail, nl, base64, xxd, od, hexdump, jq +8 regression tests: - awk/gawk/sed `system()`/`e`-flag installs MUST be detected (not masked). - jq/base64/xxd/od/hexdump with install-shaped substrings must NOT classify as installs. 2,661 tests pass (+8), clippy clean. Out of scope, noted for tracking: - agy MEDIUM: multi-byte UTF-8 corruption in shell_tokens unquoted payloads (pre-existing, post-#146 contract). Worth a follow-up issue when context allows. Codex peer review on #148 still in flight — this commit lands the agy-found BLOCKER unconditionally given its severity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thehoff
added a commit
that referenced
this pull request
May 23, 2026
Manual version bump capping the #141 plaintext-FP guard cluster (#139, #140/#146, #141/#148, #142, #143). Headline: zero-execution data-utility allowlist (echo/printf/cat/grep/jq/base64/…) so that install-shaped substrings printed-as-data no longer trigger the supply-chain gate, with hard exclusions for rg / awk / gawk / sed which can spawn subprocesses (Codex + agy round-3 GREEN/GREEN). 46 merged PRs since v0.1.9. See GH release v0.1.10 for the full PR list + notes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
noogalabs
pushed a commit
to noogalabs/contextcrawler
that referenced
this pull request
Jun 4, 2026
thehoff#141) The supply-chain gate's install-detection regexes match install-shaped substrings anywhere in a command, including inside the ARGUMENTS of ordinary data-consuming utilities. Live FP from peer review: echo "/usr/bin/npm install foo" is data printed to stdout, but the gate treated the inner substring as a real install verb and blocked. Re-implementation of thehoff#141 on top of develop @ c4f6e55, composing cleanly with the now-merged thehoff#142/thehoff#143/thehoff#146 work (line-continuation preprocess, quote-aware tokeniser, recursion + depth-cap synthetic Unvettable). The original thehoff#141 implementation (commit 7a6935b on feat/plaintext-fp-guard-141) accumulated too many conflicts with the follow-on merges to rebase cleanly — this is a fresh re-write of the same byte-masking contract. Design — head-verb allowlist, byte-mask preserving offsets: - DATA_CONSUMING_UTILITIES — echo, printf, cat, tac, grep, egrep, fgrep, rg, awk, gawk, sed, head, tail, tee, nl. Same list as the original; deliberately narrow, only utilities whose canonical role is to EMIT or SEARCH their argv as data. - command_head_is_data_utility(segment) — basename-normalises the first non-operator token (handles /bin/echo, /usr/bin/grep) and matches against the allowlist. - mask_data_utility_segments(cmd) — walks the shell_tokens stream, identifies segments whose head is a data utility, overwrites their bytes with ASCII spaces in a same-length working copy. Chain operators (&&, ||, ;, |) and unmasked segments are preserved verbatim, so the regex prefix-anchors and the claimed-span dedup bookkeeping continue to work unchanged. Composition with post-thehoff#146 develop — masking layer placement: - Wired into detect_installs_into() (NOT detect_installs()), so it applies at every recursion depth. Wrappers like sh -c 'echo "npm install foo"' extract the inner echo payload via extract_recursion_segments and recurse; the inner head is also a data utility and must be masked at the inner depth. - The recursion sweep below the masking now runs against cmd_raw (the unmasked input), NOT the masked copy. Reason: a substitution body nested inside a data-utility segment still executes at runtime — echo "$(npm install foo)" does install the package — so the gate must still vet that body. Masking suppresses the surface-level regex pass for that segment ONLY; the recursion sweep is left intact. - Composes cleanly with mask_quoted_operators() (also same-length) and the LINE_CONT_RE collapse upstream. Segment-end calculation differs from the original implementation — post-thehoff#146 shell_tokens strips quote characters from token payloads (so the literal token for "foo" is foo, payload len 3, source span 5 bytes including the quotes). The original mask used seg_end = off + tok.len() which would under-mask quoted tokens. This re-write tracks segment end via the next operator's source offset (or cmd.len() at end-of-stream), which is robust to the quote-stripping payload contract. Tests: 13 new tests covering the FP cases (echo/grep/printf/cat/rg with install-shaped argv), positive guards (vanilla + abs-path npm install MUST still detect), tricky chain semantics (echo "x" && npm install foo, echo … ; npm install …, echo foo | npm install bar, cd foo && npm install bar), abs-path data-utility heads (/bin/echo …), plus a direct unit test on command_head_is_data_utility. 2,653 tests pass (was 2,640), zero regressions. Out of scope (deferred to principled-fix follow-up): token-position constraint and quote-context exclusion. They are the principled fixes — this remains a head-verb allowlist bandaid and the DATA_CONSUMING_UTILITIES list should shrink (or retire) once those ship. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #140. Rebased onto develop after #142/#143 merged — combined cleanly with no semantic conflicts (regex prefix anchor now
(?:^|[\s;/\\'"]|&&|\|\|), verb has optional['"]?after the head, then(?:\.(?i:cmd|exe|bat))*chained-suffix from #142/#143, then capture excluding\r\nfrom #142, plus develop'sLINE_CONT_REpreprocess indetect_installs).What this closes
Closes the HIGH bypass class both Codex and agy flagged when reviewing the original abs-path PR (#139): shell-quoted and command-substitution wrapped installs slip past the regex anchor.
How
Three layers added to
src/hooks/supply_chain_gate.rs:Tests (14 new)
quoted_sh_c_install_detected,command_substitution_install_detected,backtick_substitution_install_detected,quoted_head_install_detected,quoted_operator_in_flag_value_does_not_truncate_scan, recursion-bound guard, dedup verifies, plus regression guards on the existing detection paths.2,631 tests pass (+14 over develop's 2,617), clippy clean.
Per-the-mandate
Multi-model peer review (Codex + agy) will run on this rebased head before the merge question opens.
Out of scope (#141)
Plaintext-data false-positive guard. The quote-aware tokeniser lays the foundation for the principled fix (approach 2/3 in the issue body); #141 has shipped approach 1 (utility-prefix allowlist) as a pragmatic bandaid. Rebase + PR of #141 is the next move after this lands.
🤖 Generated with Claude Code