harden(supply-chain): close value-flag, case, versioned-pip, line-cont bypasses - #143
Merged
Merged
Conversation
…t bypasses Round-two peer review of #142 by Codex + agy converged on a BLOCKER and three HIGHs. All four landed here. Both reviewers verdict on #142 was "hold"; #142 merged on the strength of what it DID close (per Hoff direction), with this branch carving the remainder. 1. BLOCKER (Codex + agy AGREED). Value-consuming flag bypass on the new bare-lockfile surface. `poetry install --with dev`, `uv sync --extra dev`, `uv sync --group dev` — `--with`/`--extra`/`--group` consume the next token, but the `has_package` scan treated that value as a package and silently dropped the install. Fix: thread `accepts_packages: bool` through the per-verb match. `poetry install`, `uv sync`, `uv pip sync`, `npm ci`, `yarn install`, and bare `yarn` never take a positional package — for them the package scan is skipped, always-bare. `npm install` / `pnpm install` / `pnpm i` keep the scan as before (they do take a positional, and the named-install case is normally claimed by NPM_RE / PNPM_RE upstream). 2. HIGH (Codex + agy AGREED). Windows launcher case sensitivity. `NPM.CMD install foo`, `Npm.Cmd install foo`, `PIP.EXE install x` slipped past both the regex verb check and the bare-install token compare. Fix: - `installer_basename` strips `.cmd`/`.exe`/`.bat` case-insensitively in a loop (so chained extensions `npm.cmd.exe` also collapse). - The bare-install token match lowercases the basename before comparing. - All 7 install-detect regexes use `(?i:…)` scoped flags on the verb and the suffix groups: `(?i:npm)(?:\.(?i:cmd|exe|bat))*\s+(?i:i|install|add)`. - The optional suffix group changed from `?` to `*` so the regex matches chained extensions (`npm.cmd.exe install foo`). 3. HIGH (Codex only). Versioned pip launchers. `pip3.12.exe install x` stripped `.exe` to `pip3.12`, which didn't match `pip|pip3`. The PIP_RE verb is now `(?i:pip\d*(?:\.\d+)*)`, matching `pip`, `pip3`, `pip3.12`, `pip3.12.0`, etc., paired with the chained-suffix group. 4. HIGH (agy only). Line-continuation bypass — my own #142 regression. `\r\n` in the arg-capture's negated class truncated `npm install \<nl> foo` at the trailing backslash, so packages on the continuation line went undetected. Fix: a `LINE_CONT_RE` (`\\\r?\n\s*`) preprocess at the top of `detect_installs` collapses backslash-newline-indent sequences to a single space BEFORE detection runs. The newline guard from #142 is preserved for genuine multi-command newline separation (test `line_continuation_does_not_merge_distinct_commands`). Also folds in agy MEDIUM (double-extension `npm.cmd.exe`) since the loop strip + `*` regex quantifier address it for free. 18 new regression tests: - poetry_install_with_{value_consuming,only,without_and_extras}_flag_still_bare - uv_sync_with_{extra,group}_value_still_bare - npm_ci_with_value_consuming_flag_still_bare - yarn_install_with_value_consuming_flag_still_bare - windows_launcher_{uppercase_npm_cmd,mixed_case,full_uppercase_no_suffix,pip_exe_uppercase,double_extension}_detected - versioned_pip_{3_12,3_12_exe,abs_path}_detected - line_continuation_{multiline,crlf}_install_caught - line_continuation_does_not_merge_distinct_commands 2,612 tests pass (+18 from develop's 2,594), clippy clean on the changed file (the line 688 warning is the pre-existing `parse_package_args` type_complexity, unchanged). Per the new peer-review-mandatory rule in memory, this PR will be reviewed by Codex + agy before the merge question is opened. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to agy peer review on #143 (BLOCKER). The earlier `LINE_CONT_RE` (`\\\r?\n\s*`) only collapsed `\n` and `\r\n` line endings, so a backslash + bare `\r` (legacy Mac convention, still emitted by some tools and very-old scripts) survived as a real continuation to the shell but bypassed the gate's normalisation. Regex now: `\\(?:\r\n|\n|\r)\s*` — covers POSIX, Windows, and legacy Mac line endings. +1 regression test: `line_continuation_legacy_mac_cr_only_caught`. Out of scope from agy's #143 review (tracked separately): bun ecosystem, pdm/pipenv/conda/mamba ecosystems, PEP 508 extras stripping, inline `#` comment handling, pip/uv value-consuming-flag values being parsed as packages. Each is its own scope, not a #143 closure. 2,613 tests pass (+1), clippy clean. Codex peer review on #143 is still in flight; this commit is a narrow agy-finding fix, not the consolidated action. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 23, 2026
…boundary Codex peer review on #143 found two issues: LOW: the bare-yarn rule over-matched any dash-prefixed next-token, so `yarn --version`, `yarn -v`, `yarn --help`, `yarn -h` (diagnostic invocations that touch nothing) were escalated as bare lockfile installs. Fixed by introducing `is_yarn_help_or_version_flag(tok)` and short-circuiting the bare arm for the four canonical query flags. `yarn --frozen-lockfile` (an actual install flag) still classifies as bare — pinned by `yarn_with_other_flag_still_bare_install`. MEDIUM: the gate's install-detection surface (npm/pnpm/yarn/pip/uv/ poetry/pipx) is narrower than "all dependency acquisition", and the absence of a documented boundary could give a false sense of completeness. Per Codex's own recommendation ("document … OR add detectors"), the boundary is now codified as a doc-comment block on `detect_installs`. Ecosystem expansion (bun, pdm, pipenv, conda, mamba, brew, plus `pnpm update`, `poetry update`) is tracked in #144 — this commit narrates the scope explicitly, not as a silent omission. +4 regression tests: - yarn_version_flag_not_a_bare_install - yarn_help_flag_not_a_bare_install - yarn_short_version_not_a_bare_install - yarn_with_other_flag_still_bare_install (sanity) 2,617 tests pass (+4), clippy clean. Consolidated review on #143 (Codex + agy): - BLOCKER (CR-only line continuation, agy) — closed in 6ebe953. - LOW (yarn over-match, Codex) — closed here. - MEDIUM (coverage gap, both) — scope boundary documented, expansion tracked in #144 + #145 (per Codex's documented-OR-detect option). - All other agy findings — tracked as scope-expansion or parse-quality in #144 and #145. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…aviour Codex re-review INFO finding on PR #143: the scope-boundary doc-comment opened with "anything outside this list returns Skip" which was slightly stronger than the implementation. The `yarn` arm at the `next.starts_with('-')` branch classifies `yarn --frozen-lockfile` (and any other non-diagnostic flag form) as a bare install — that case IS in scope (it's Yarn's shorthand for `yarn install`), and a test (`yarn_with_other_flag_still_bare_install`) already pins it. The comment now lists `yarn <install-flag>` explicitly alongside `yarn install` / bare `yarn`, and clarifies that the diagnostic-flag exclusion is what filters the out-of-scope variants. Documentation-only — no runtime change. 2,617 tests pass, clippy clean. Codex + agy re-review verdict on #143 this round: MANDATE CLEAR. 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
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.
Third hardening pass on the supply-chain gate, addressing the BLOCKER + three HIGHs that Codex and agy both surfaced when peer-reviewing #142. #142 merged on the strength of what it DID close (per [thehoff] direction); this PR carves the remainder.
What lands
1. BLOCKER — value-consuming flag bypass (both reviewers AGREED)
poetry install --with dev,uv sync --extra dev,uv sync --group dev: `--with` / `--extra` / `--group` consume the next token as their value, but the `has_package` scan in `detect_bare_lockfile_installs` treated that value as a positional package, flipped `has_package=true`, and silently dropped the install from detection.Fix: thread `accepts_packages: bool` through the per-verb match. Verbs that never take a positional (`poetry install`, `uv sync`, `uv pip sync`, `npm ci`, `yarn install`, bare `yarn`) skip the package scan unconditionally and always emit as bare. Verbs that do take a positional (`npm install`, `pnpm install`, `pnpm i`) keep the scan — the named case is normally claimed upstream by NPM_RE / PNPM_RE; the scan is the fallback.
2. HIGH — Windows launcher case-insensitivity (both reviewers AGREED)
`NPM.CMD install foo`, `Npm.Cmd install foo`, `PIP.EXE install x` slipped past both the regex verb check and the bare-install token comparison.
Fix: `installer_basename` now strips `.cmd`/`.exe`/`.bat` case-insensitively in a loop, so chained extensions (`npm.cmd.exe`) collapse. The bare-install token match lowercases the basename before comparing. All 7 install-detect regexes use `(?i:…)` scoped flags on the verb token and the suffix group: `(?i:npm)(?:\.(?i:cmd|exe|bat))\s+(?i:i|install|add)`. The suffix group quantifier changed from `?` to `` so chained extensions match via the regex too.
3. HIGH — Versioned pip launchers (Codex)
`pip3.12.exe install x` stripped `.exe` → `pip3.12`, which didn't match `pip|pip3`. The PIP_RE verb is now `(?i:pip\d*(?:\.\d+)*)`, matching `pip`, `pip3`, `pip3.12`, `pip3.12.0`, etc., paired with the chained-suffix group.
4. HIGH — Line-continuation bypass (agy) — own #142 regression
`\r\n` in the arg-capture's negated class (added in #142 to stop newline chain-swallowing) truncated `npm install \ foo` at the trailing backslash, so packages on the continuation line went undetected. My fix in #142 was overcorrective.
Fix: a `LINE_CONT_RE` (`\\\r?\n\s*`) preprocess at the top of `detect_installs` collapses backslash-newline-indent sequences to a single space BEFORE detection runs. The newline guard from #142 is preserved for genuine multi-command newline separation — pinned by `line_continuation_does_not_merge_distinct_commands`.
Bonus
agy MEDIUM (double-extension `npm.cmd.exe`) is closed for free by the loop strip + `*` regex quantifier above.
Tests
18 new regression tests across the 4 classes:
```
poetry_install_with_{value_consuming,only,without_and_extras}flag_still_bare
uv_sync_with{extra,group}value_still_bare
npm_ci_with_value_consuming_flag_still_bare
yarn_install_with_value_consuming_flag_still_bare
windows_launcher{uppercase_npm_cmd,mixed_case,full_uppercase_no_suffix,pip_exe_uppercase,double_extension}detected
versioned_pip{3_12,3_12_exe,abs_path}detected
line_continuation{multiline,crlf,does_not_merge_distinct_commands}
```
2,612 tests pass (+18 from develop's 2,594), clippy clean on the changed file.
Per-the-rule notice
The new `peer-review-codex-agy-mandatory` memory rule (locked in this session) says: do not open the "Merge?" question on a security/hardening PR without first having run both reviews and surfaced consolidated findings. This PR will get a Codex + agy review pass before merge.
🤖 Generated with Claude Code