From 9ffca00032dfaf96c3bd5eeee5aaadb6334ae26e Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 13:29:37 +1000 Subject: [PATCH 1/4] fix(supply-chain): suppress install detection inside data-utility argv (#141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #141 on top of develop @ c4f6e55, composing cleanly with the now-merged #142/#143/#146 work (line-continuation preprocess, quote-aware tokeniser, recursion + depth-cap synthetic Unvettable). The original #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-#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-#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) --- src/hooks/supply_chain_gate.rs | 317 ++++++++++++++++++++++++++++++++- 1 file changed, 315 insertions(+), 2 deletions(-) diff --git a/src/hooks/supply_chain_gate.rs b/src/hooks/supply_chain_gate.rs index 6284c15921..131a21ccd4 100644 --- a/src/hooks/supply_chain_gate.rs +++ b/src/hooks/supply_chain_gate.rs @@ -803,6 +803,117 @@ fn is_shell_operator(tok: &str) -> bool { ) } +/// Data-consuming utilities — their argv is data, not a command-of-commands. +/// When the head verb of a command segment is one of these, install-shaped +/// substrings inside that segment are treated as plain data (not real install +/// invocations) and the gate suppresses detection for that segment. +/// +/// This is the #141 pragmatic bandaid (approach 1 of three): a head-verb +/// allowlist. Approaches (2) "token-position constraint" and (3) "quote- +/// context exclusion" remain the more principled fixes; this list should +/// shrink (or retire) once they ship. +/// +/// Kept deliberately narrow: only utilities whose canonical role is to EMIT +/// or SEARCH their argv as data. Stateful utilities like `cd`, `env`, +/// `xargs`, `sudo`, `nohup` are NOT on the list — they invoke a subsequent +/// command and that command is the real head. +const DATA_CONSUMING_UTILITIES: &[&str] = &[ + "echo", "printf", "cat", "tac", "grep", "egrep", "fgrep", "rg", + "awk", "gawk", "sed", "head", "tail", "tee", "nl", +]; + +/// True iff the first non-whitespace token of `segment`, basename-normalised, +/// matches a known data-consuming utility (see [`DATA_CONSUMING_UTILITIES`]). +/// +/// Basename normalisation handles `/bin/echo`, `./echo`, `/usr/bin/grep` etc. +/// — the same path-bypass concern that #139 closed for installer heads +/// applies symmetrically here. +/// +/// Returns `false` on an empty / whitespace-only segment, on segments that +/// begin with a shell operator, and on any unknown head. +#[allow(dead_code)] // exercised by tests + reserved for principled-path follow-up +fn command_head_is_data_utility(segment: &str) -> bool { + let tokens = shell_tokens(segment); + let head = match tokens.first() { + Some((_, t)) => t.as_str(), + None => return false, + }; + // A leading shell operator (e.g. trailing chain fragment with no head) + // is not a data utility — it's just empty. + if is_shell_operator(head) { + return false; + } + let basename = installer_basename(head); + DATA_CONSUMING_UTILITIES.contains(&basename) +} + +/// Mask command segments whose head verb is a data-consuming utility (see +/// [`command_head_is_data_utility`]). Returns a string of the same byte +/// length as `cmd` — bytes inside masked segments are replaced with ASCII +/// spaces, chain operators and unmasked segments are preserved verbatim. +/// +/// Preserving byte offsets matters: the regex prefix-anchor in NPM_RE / +/// PIP_RE / etc. relies on the boundary char immediately before the install +/// verb (space, `&&`, `||`, `;`, `/`, `\`), and the downstream `claimed` +/// dedup uses absolute indices into the command. Replacing with spaces +/// (not deleting) keeps both invariants intact. +/// +/// Segment boundaries are shell operator tokens (`;`, `|`, `||`, `&`, `&&`, +/// `>`, `>>`, `<`, `<<`) as produced by [`shell_tokens`]. Each segment's +/// head is the first non-operator token; the segment span is `[head_off, +/// next_op_off)` — using the next operator's source offset as the segment +/// end avoids the trap that `shell_tokens` strips quote characters from +/// token payloads (so `tok.len()` no longer equals the source-span length +/// for a quoted token). +fn mask_data_utility_segments(cmd: &str) -> String { + let tokens = shell_tokens(cmd); + let bytes = cmd.as_bytes(); + let mut out: Vec = bytes.to_vec(); + + let mask_range = |buf: &mut Vec, start: usize, end: usize| { + for b in buf.iter_mut().take(end).skip(start) { + // Preserve newlines so any multi-line invariants survive; we + // are not currently aware of one, but it costs nothing. + if *b != b'\n' { + *b = b' '; + } + } + }; + + let mut seg_head_off: Option = None; + let mut seg_head_is_data_util = false; + + for (off, tok) in &tokens { + if is_shell_operator(tok) { + // Close the in-flight segment at the operator's source offset. + if let (Some(start), true) = (seg_head_off, seg_head_is_data_util) { + mask_range(&mut out, start, *off); + } + seg_head_off = None; + seg_head_is_data_util = false; + continue; + } + // First word of a fresh segment establishes the head. + if seg_head_off.is_none() { + seg_head_off = Some(*off); + let basename = installer_basename(tok); + seg_head_is_data_util = DATA_CONSUMING_UTILITIES.contains(&basename); + } + } + // Flush the final segment — it runs to end-of-string. + if let (Some(start), true) = (seg_head_off, seg_head_is_data_util) { + mask_range(&mut out, start, bytes.len()); + } + + // Safety: we only overwrite ASCII word bytes with ASCII spaces — ASCII + // space is never a UTF-8 continuation byte, so multi-byte sequences + // inside masked spans get replaced byte-by-byte with valid UTF-8. The + // result is therefore valid UTF-8 and `from_utf8` cannot fail in + // practice; fall back to the original `cmd` on the impossible case + // rather than panic, honouring the gate's no-panic contract. + String::from_utf8(out).unwrap_or_else(|_| cmd.to_string()) +} + /// Detect package-manager install invocations in a shell command. /// /// ## Scope (codified after Codex + agy peer review on #143) @@ -908,6 +1019,30 @@ fn installs_equivalent(a: &ParsedInstall, b: &ParsedInstall) -> bool { const MAX_RECURSION_DEPTH: u8 = 6; fn detect_installs_into(cmd: &str, depth: u8, out: &mut Vec) { + // #141: mask any segment whose head verb is a data-consuming utility + // (echo, printf, cat, grep, rg, awk, sed, …). The masked string has the + // SAME byte offsets — bytes inside masked segments are turned into + // spaces — so all downstream regex anchors, the `claimed` index + // bookkeeping, AND `mask_quoted_operators` (which also produces a + // same-length string) compose cleanly. + // + // Applied at every recursion depth — wrappers like `sh -c 'echo "npm + // install foo"'` extract the inner `echo "..."` payload via + // `extract_recursion_segments` and recurse here; the inner head is also + // a data utility and must be masked at the inner depth, not just at the + // top level. + // + // Crucially, the recursion sweep below runs against the ORIGINAL + // `cmd_raw` (NOT the masked copy). Substitution bodies inside a masked + // data-utility segment still surface for recursion — masking only + // suppresses the surface-level regex pass for that segment, not the + // recursion sweep. This is intentional: `echo "$(npm install foo)"` + // does execute the substitution at runtime, so the gate must still vet + // its body. + let cmd_raw = cmd; + let masked_data = mask_data_utility_segments(cmd_raw); + let cmd: &str = masked_data.as_str(); + // Run UV before PIP so `uv pip install foo` is claimed by the UV pattern // and PIP_RE matching the inner `pip install foo` substring is suppressed // for that span. @@ -999,10 +1134,13 @@ fn detect_installs_into(cmd: &str, depth: u8, out: &mut Vec) { // unresolved recursion segments, surface a synthetic Unvettable // ParsedInstall so the caller fails CLOSED (Verdict::Ask) instead. if depth < MAX_RECURSION_DEPTH { - for inner in extract_recursion_segments(cmd) { + // Recurse against the original (unmasked) cmd: a substitution body + // nested inside a data-utility segment still executes at runtime + // (`echo "$(npm install foo)"`) and must be vetted. + for inner in extract_recursion_segments(cmd_raw) { detect_installs_into(&inner, depth + 1, out); } - } else if !extract_recursion_segments(cmd).is_empty() { + } else if !extract_recursion_segments(cmd_raw).is_empty() { out.push(ParsedInstall { ecosystem: Ecosystem::Npm, // ecosystem-agnostic; pick one packages: Vec::new(), @@ -3292,4 +3430,179 @@ mod tests { texts ); } + + // ─── #141: plaintext false-positive guard (data-utility allowlist) ────── + // + // The supply-chain regexes match install-shaped substrings anywhere in a + // command, including inside the ARGUMENTS of ordinary data-consuming + // utilities. Hit live on 2026-05-23: `echo "/usr/bin/npm install foo"` is + // just data printed to stdout, but the gate treated the substring as a + // real install verb and blocked. + // + // Fix: head-verb allowlist (echo, printf, cat, tac, grep, egrep, fgrep, + // rg, awk, gawk, sed, head, tail, tee, nl). When the head of a command + // segment basename-normalises to one of these, the segment bytes are + // overwritten with ASCII spaces in a same-length working copy before the + // install-detection regexes run. Byte offsets are preserved so the + // existing regex prefix-anchors and `claimed`-span dedup keep working. + // + // Re-implementation of #141 on top of develop @ c4f6e55, composing + // cleanly with the now-merged #142/#143/#146 changes (line-continuation + // preprocess, quote-aware tokeniser, recursion + depth-cap). + + #[test] + fn echo_with_install_substring_is_not_an_install() { + // The classic FP — observed in live peer review on 2026-05-23. + let v = detect_installs(r#"echo "/usr/bin/npm install foo""#); + assert!( + v.is_empty(), + "echo printing an install-shaped string must not trigger the gate, got: {:?}", + v + ); + } + + #[test] + fn grep_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"grep "/opt/bin/pip install" README.md"#); + assert!( + v.is_empty(), + "grep searching for an install-shaped pattern must not trigger the gate, got: {:?}", + v + ); + } + + #[test] + fn printf_format_is_not_an_install() { + let v = detect_installs(r#"printf '%s\n' "npm install lodash""#); + assert!( + v.is_empty(), + "printf data argument must not trigger the gate, got: {:?}", + v + ); + } + + #[test] + fn cat_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"cat /tmp/notes.txt # contains npm install foo"#); + assert!( + v.is_empty(), + "cat reading a file with install-shaped content must not trigger, got: {:?}", + v + ); + } + + #[test] + fn rg_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"rg "npm install" src/"#); + assert!( + v.is_empty(), + "rg searching for an install pattern must not trigger, got: {:?}", + v + ); + } + + // ─── #141: positive guards — these MUST NOT regress ───────────────────── + + #[test] + fn plain_npm_install_still_detected_141() { + // Sanity: the data-utility guard must not break the base case. + let v = detect_installs("npm install foo"); + assert_eq!(v.len(), 1, "vanilla npm install must still be detected"); + assert_eq!(names(&v[0]), vec!["foo"]); + } + + #[test] + fn abs_path_npm_install_still_detected_141() { + // The absolute-path bypass closed by #139 must remain closed — + // the guard must not regress it. + let v = detect_installs("/usr/local/bin/npm install foo"); + assert_eq!(v.len(), 1, "abs-path npm install must still be detected"); + assert_eq!(names(&v[0]), vec!["foo"]); + } + + #[test] + fn chain_after_data_utility_still_detected_141() { + // `cd` is not in the data-utility list (cd is a stateful builtin, + // not data-printing). The second segment after `&&` MUST be + // evaluated independently. + let v = detect_installs("cd foo && npm install bar"); + assert_eq!(v.len(), 1, "install after && must still be detected"); + assert_eq!(names(&v[0]), vec!["bar"]); + } + + #[test] + fn echo_chained_with_real_install_still_detects_install_141() { + // Tricky chain: the LEFT segment is suppressed (echo head), the + // RIGHT segment is a real install and must be detected. + let v = detect_installs(r#"echo "x" && npm install foo"#); + assert_eq!( + v.len(), + 1, + "real install after && must survive even when left segment is suppressed, got: {:?}", + v + ); + assert_eq!(names(&v[0]), vec!["foo"]); + } + + #[test] + fn pipe_into_install_is_detected_141() { + // `echo foo | npm install bar` — npm is the head of the right-hand + // pipe segment (it's being invoked, with `foo` piped into stdin), + // so the install MUST be detected. + let v = detect_installs("echo foo | npm install bar"); + assert_eq!( + v.len(), + 1, + "npm as RHS-of-pipe head must still be detected, got: {:?}", + v + ); + assert_eq!(names(&v[0]), vec!["bar"]); + } + + #[test] + fn semicolon_chain_with_data_utility_141() { + // `;` is a segment boundary, same as `&&`. + let v = detect_installs(r#"echo "npm install foo" ; npm install real"#); + assert_eq!( + v.len(), + 1, + "echo suppressed, second segment must detect, got: {:?}", + v + ); + assert_eq!(names(&v[0]), vec!["real"]); + } + + #[test] + fn data_utility_with_abs_path_head_is_suppressed_141() { + // The head check must basename-normalise — `/bin/echo "..."` is + // still an echo head. + let v = detect_installs(r#"/bin/echo "/usr/bin/npm install foo""#); + assert!( + v.is_empty(), + "abs-path echo head must still be recognised as a data utility, got: {:?}", + v + ); + } + + #[test] + fn command_head_is_data_utility_unit() { + // Direct unit test on the helper, in case it's reused. + assert!(command_head_is_data_utility("echo foo")); + assert!(command_head_is_data_utility(" echo foo")); + assert!(command_head_is_data_utility("printf '%s' x")); + assert!(command_head_is_data_utility("grep pattern file")); + assert!(command_head_is_data_utility("/bin/echo hi")); + assert!(command_head_is_data_utility("/usr/bin/cat foo")); + + assert!(!command_head_is_data_utility("npm install foo")); + assert!(!command_head_is_data_utility("/usr/bin/npm install foo")); + assert!(!command_head_is_data_utility("pip install x")); + assert!(!command_head_is_data_utility("")); + assert!(!command_head_is_data_utility(" ")); + // `cd` is a stateful builtin, not a data utility — its segment is + // benign because it has no install verb, but it must not be on + // the allowlist (otherwise `cd && npm install` semantics get + // muddied if cd ever gains install-shaped argv). + assert!(!command_head_is_data_utility("cd foo")); + } } From 76ecc84fae65833adc90b5c0c90bfecda1985c63 Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 13:36:17 +1000 Subject: [PATCH 2/4] harden(supply-chain): remove execution-capable utils from data-mask allowlist (agy #148 BLOCKER) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/hooks/supply_chain_gate.rs | 91 +++++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 2 deletions(-) diff --git a/src/hooks/supply_chain_gate.rs b/src/hooks/supply_chain_gate.rs index 131a21ccd4..dca0c961c8 100644 --- a/src/hooks/supply_chain_gate.rs +++ b/src/hooks/supply_chain_gate.rs @@ -817,9 +817,27 @@ fn is_shell_operator(tok: &str) -> bool { /// or SEARCH their argv as data. Stateful utilities like `cd`, `env`, /// `xargs`, `sudo`, `nohup` are NOT on the list — they invoke a subsequent /// command and that command is the real head. +/// +/// **EXCLUDED (deliberately) — execution-capable utilities** (agy BLOCKER +/// on PR #148): `awk` / `gawk` / `mawk` / `sed` are Turing-complete and +/// have native command-execution primitives: +/// - `awk 'BEGIN { system("npm install evil") }'` +/// - GNU `sed 's/.*/npm install .../e'` +/// Masking these would create a silent bypass class — the entire point of +/// allowlisting is that the matched utility cannot spawn an install. If +/// you ever want to handle them, route through `extract_recursion_segments` +/// to walk the script body, NOT through the data-utility allowlist. const DATA_CONSUMING_UTILITIES: &[&str] = &[ - "echo", "printf", "cat", "tac", "grep", "egrep", "fgrep", "rg", - "awk", "gawk", "sed", "head", "tail", "tee", "nl", + // Emit-as-data + "echo", "printf", "cat", "tac", "tee", + // Search-as-data + "grep", "egrep", "fgrep", "rg", + // Slice / trim-as-data + "head", "tail", "nl", + // Encode / decode (cannot execute) + "base64", "xxd", "od", "hexdump", + // Structured-text parse (jq has no shell-out; `--exec`-style flags do not exist) + "jq", ]; /// True iff the first non-whitespace token of `segment`, basename-normalised, @@ -3491,6 +3509,75 @@ mod tests { ); } + // ─── #148 round-1 BLOCKER (agy) — execution-capable utilities excluded ─ + + #[test] + fn awk_system_call_install_must_be_detected_not_masked() { + // `awk 'BEGIN { system("...") }'` is a real command-execution path. + // If awk were in DATA_CONSUMING_UTILITIES, the entire segment would + // be masked to spaces and the install would silently bypass. This + // test pins awk OUT of the allowlist. + let v = detect_installs(r#"awk 'BEGIN { system("npm install evil") }'"#); + assert!( + !v.is_empty(), + "awk segment must NOT be masked — system() can spawn an install: {v:?}" + ); + } + + #[test] + fn gawk_system_call_install_must_be_detected_not_masked() { + let v = detect_installs(r#"gawk 'BEGIN { system("pip install bad") }'"#); + assert!( + !v.is_empty(), + "gawk system() install must not be masked: {v:?}" + ); + } + + #[test] + fn sed_exec_flag_install_must_be_detected_not_masked() { + // GNU sed's `s///e` flag executes the replacement as a shell command. + let v = detect_installs(r#"sed 's/.*/npm install nasty/e' /tmp/x"#); + assert!( + !v.is_empty(), + "sed `e` flag install must not be masked — sed can spawn shell: {v:?}" + ); + } + + // ─── #148 round-1 MEDIUM (agy) — allowlist coverage additions ────────── + + #[test] + fn jq_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"jq '.scripts | select(. == "npm install foo")' package.json"#); + assert!(v.is_empty(), "jq filter is data processing, not install: {v:?}"); + } + + #[test] + fn base64_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"base64 -d <<< "bnBtIGluc3RhbGwgZm9v""#); + assert!(v.is_empty(), "base64 decode is data, not install: {v:?}"); + } + + #[test] + fn xxd_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"xxd /tmp/notes # echoes hex of 'pip install x'"#); + assert!(v.is_empty(), "xxd is data, not install: {v:?}"); + } + + #[test] + fn od_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"od -c /tmp/log | grep 'npm install'"#); + assert!( + v.is_empty(), + "od piped to grep — both data utilities, not an install: {v:?}" + ); + } + + #[test] + fn hexdump_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"hexdump -C /tmp/payload"#); + assert!(v.is_empty(), "hexdump is data, not install: {v:?}"); + } + #[test] fn rg_with_install_substring_is_not_an_install() { let v = detect_installs(r#"rg "npm install" src/"#); From f0fc794a505e29f6bdd185c24bebc6c84d48b95e Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 13:41:27 +1000 Subject: [PATCH 3/4] harden(supply-chain): remove rg from data-mask allowlist (Codex #148 BLOCKER) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex peer review on PR #148 caught an execution-capable utility agy missed: `rg --pre ` runs the named executable as a preprocessor on every file. With `rg` on the allowlist, the segment was masked and the install hidden — silent bypass. Fix: remove `rg` from DATA_CONSUMING_UTILITIES. A plain `rg "" src/` is no longer masked either, so a false-positive Ask is possible for the rare case where the pattern is install-shaped. Net trade: better to over-Ask than under-detect. Doc-comment block updated with the third execution-capable exclusion alongside awk/gawk/mawk and sed. +1 regression test: rg_pre_install_must_be_detected_not_masked (the old rg_with_install_substring_is_not_an_install test was replaced — its expected behaviour reversed once rg left the allowlist). 2,661 tests pass, clippy clean. Out of scope, tracking separately: - Codex WARN: `claimed.iter().any(...)` and `dedup_installs()` are O(n²) in the number of detected install spans. Not a bypass; a performance pathology with a crafted command containing many repeated install-shaped segments. Worth a follow-up issue. Both peer-review BLOCKERs (#148 round-1) are now closed: - agy: awk/gawk/sed removed in 76ecc84. - Codex: rg removed here. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/hooks/supply_chain_gate.rs | 46 ++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/src/hooks/supply_chain_gate.rs b/src/hooks/supply_chain_gate.rs index dca0c961c8..cbc0bde426 100644 --- a/src/hooks/supply_chain_gate.rs +++ b/src/hooks/supply_chain_gate.rs @@ -818,20 +818,28 @@ fn is_shell_operator(tok: &str) -> bool { /// `xargs`, `sudo`, `nohup` are NOT on the list — they invoke a subsequent /// command and that command is the real head. /// -/// **EXCLUDED (deliberately) — execution-capable utilities** (agy BLOCKER -/// on PR #148): `awk` / `gawk` / `mawk` / `sed` are Turing-complete and -/// have native command-execution primitives: -/// - `awk 'BEGIN { system("npm install evil") }'` -/// - GNU `sed 's/.*/npm install .../e'` -/// Masking these would create a silent bypass class — the entire point of -/// allowlisting is that the matched utility cannot spawn an install. If -/// you ever want to handle them, route through `extract_recursion_segments` -/// to walk the script body, NOT through the data-utility allowlist. +/// **EXCLUDED (deliberately) — execution-capable utilities** (agy + Codex +/// BLOCKERs on PR #148): tools that can execute arbitrary shell as a +/// side effect of normal flags do NOT belong on a "data only" allowlist, +/// because masking their segment hides any install verb from the gate +/// while the runtime still spawns it. +/// +/// Confirmed execution-capable, NEVER mask: +/// - `awk` / `gawk` / `mawk` — Turing-complete with `system()`: +/// `awk 'BEGIN { system("npm install evil") }'` +/// - GNU `sed` — `s///e` flag runs the replacement as a shell command: +/// `sed 's/.*/npm install .../e'` +/// - `rg` (ripgrep) — `--pre ` runs the preprocessor on +/// each file (Codex BLOCKER): `rg --pre /tmp/script.sh pattern .` +/// +/// If you ever want to surface real installs invoked through these +/// utilities, walk the script body via `extract_recursion_segments` — do +/// NOT add them back to the allowlist. const DATA_CONSUMING_UTILITIES: &[&str] = &[ // Emit-as-data "echo", "printf", "cat", "tac", "tee", - // Search-as-data - "grep", "egrep", "fgrep", "rg", + // Search-as-data (grep family — no `--pre`-style executable flag) + "grep", "egrep", "fgrep", // Slice / trim-as-data "head", "tail", "nl", // Encode / decode (cannot execute) @@ -3579,12 +3587,18 @@ mod tests { } #[test] - fn rg_with_install_substring_is_not_an_install() { - let v = detect_installs(r#"rg "npm install" src/"#); + fn rg_pre_install_must_be_detected_not_masked() { + // Codex BLOCKER on #148: `rg --pre ` runs the named + // executable as a preprocessor on every file. If `rg` were on the + // allowlist, the segment would be masked and the install hidden. + // `rg` is OUT of DATA_CONSUMING_UTILITIES for this reason. + // (A plain `rg "" src/` is also no longer masked — that + // produces a false-positive Ask in the rare case the pattern is + // install-shaped. Net: better to over-Ask than under-detect.) + let v = detect_installs(r#"rg --pre /tmp/installer.sh 'npm install evil'"#); assert!( - v.is_empty(), - "rg searching for an install pattern must not trigger, got: {:?}", - v + !v.is_empty(), + "rg --pre install must NOT be masked — preprocessor can spawn shell: {v:?}" ); } From e949b73407e1bdaeb230679c5dbe99e9df82435a Mon Sep 17 00:00:00 2001 From: Daniel Hoffman Date: Sat, 23 May 2026 14:34:37 +1000 Subject: [PATCH 4/4] harden(supply-chain): round-3 follow-ups for #148 (agy lows) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the two low-severity findings from the round-3 peer review on PR #148, both surfaced by agy after both round-2 BLOCKERs were closed: 1. Stale comments in two locations still enumerated rg/awk/gawk/sed as data utilities — out of date since those four were deliberately excluded from DATA_CONSUMING_UTILITIES. - src/hooks/supply_chain_gate.rs:1049 — replace `rg, awk, sed` example tail with `jq, base64`, which ARE on the allowlist. - src/hooks/supply_chain_gate.rs:3469 — rewrite the allowlist enumeration to match DATA_CONSUMING_UTILITIES exactly, and call out the exclusions explicitly so future readers don't try to add them back. 2. Seven allowlisted utilities (tac, tee, egrep, fgrep, head, tail, nl) were silently relying on indirect masking coverage — no dedicated "must-be-masked" test pinned them. Add one regression test each so any future trim of the allowlist surfaces as an explicit test failure instead of a quiet behavioural change. Verification: cargo test --bin contextcrawler → 2668 passed; 0 failed; 7 ignored Co-Authored-By: Claude Opus 4.7 (1M context) --- src/hooks/supply_chain_gate.rs | 61 +++++++++++++++++++++++++++++++--- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/src/hooks/supply_chain_gate.rs b/src/hooks/supply_chain_gate.rs index cbc0bde426..fcc86ca5ab 100644 --- a/src/hooks/supply_chain_gate.rs +++ b/src/hooks/supply_chain_gate.rs @@ -1046,7 +1046,7 @@ const MAX_RECURSION_DEPTH: u8 = 6; fn detect_installs_into(cmd: &str, depth: u8, out: &mut Vec) { // #141: mask any segment whose head verb is a data-consuming utility - // (echo, printf, cat, grep, rg, awk, sed, …). The masked string has the + // (echo, printf, cat, grep, jq, base64, …). The masked string has the // SAME byte offsets — bytes inside masked segments are turned into // spaces — so all downstream regex anchors, the `claimed` index // bookkeeping, AND `mask_quoted_operators` (which also produces a @@ -3465,9 +3465,11 @@ mod tests { // just data printed to stdout, but the gate treated the substring as a // real install verb and blocked. // - // Fix: head-verb allowlist (echo, printf, cat, tac, grep, egrep, fgrep, - // rg, awk, gawk, sed, head, tail, tee, nl). When the head of a command - // segment basename-normalises to one of these, the segment bytes are + // Fix: head-verb allowlist (echo, printf, cat, tac, tee, grep, egrep, + // fgrep, head, tail, nl, base64, xxd, od, hexdump, jq — all execution- + // incapable; rg/awk/gawk/sed deliberately excluded, see #148). When the + // head of a command segment basename-normalises to one of these, the + // segment bytes are // overwritten with ASCII spaces in a same-length working copy before the // install-detection regexes run. Byte offsets are preserved so the // existing regex prefix-anchors and `claimed`-span dedup keep working. @@ -3586,6 +3588,57 @@ mod tests { assert!(v.is_empty(), "hexdump is data, not install: {v:?}"); } + // ─── #148 round-3 (agy) — explicit masking coverage for the rest of + // DATA_CONSUMING_UTILITIES (tac, tee, egrep, fgrep, head, tail, nl). + // Round-2 only pinned the exec-capable removals and the high-traffic + // data utilities. These seven were silently relying on indirect + // coverage; pin them here so future trims of the allowlist surface as + // an obvious test failure. + + #[test] + fn tac_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"tac /tmp/log # contains 'npm install foo'"#); + assert!(v.is_empty(), "tac is reverse-cat, pure data: {v:?}"); + } + + #[test] + fn tee_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"echo data | tee /tmp/out # 'pip install x' in body"#); + // `tee` is the head of the post-pipe segment; both `echo` and `tee` + // are data utilities, so neither segment must trigger detection. + assert!(v.is_empty(), "tee writes stdin to file+stdout, no exec: {v:?}"); + } + + #[test] + fn egrep_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"egrep "npm install|pip install" /tmp/notes"#); + assert!(v.is_empty(), "egrep is grep -E, no exec: {v:?}"); + } + + #[test] + fn fgrep_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"fgrep "npm install foo" /tmp/notes"#); + assert!(v.is_empty(), "fgrep is grep -F, no exec: {v:?}"); + } + + #[test] + fn head_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"head -20 /tmp/install-instructions.md"#); + assert!(v.is_empty(), "head prints first N lines, no exec: {v:?}"); + } + + #[test] + fn tail_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"tail -f /var/log/npm-install.log"#); + assert!(v.is_empty(), "tail prints last N lines, no exec: {v:?}"); + } + + #[test] + fn nl_with_install_substring_is_not_an_install() { + let v = detect_installs(r#"nl /tmp/notes # numbered 'npm install foo'"#); + assert!(v.is_empty(), "nl numbers lines, no exec: {v:?}"); + } + #[test] fn rg_pre_install_must_be_detected_not_masked() { // Codex BLOCKER on #148: `rg --pre ` runs the named