Skip to content

Close remaining check-lint-pins.sh gaps from #38 - #40

Merged
charlesgreen merged 2 commits into
mainfrom
fix/lint-pin-gaps-followup-38
Sep 9, 2026
Merged

charlesgreen merged 2 commits into
mainfrom
fix/lint-pin-gaps-followup-38

Conversation

@charlesgreen

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #37/#39 (via #38). Closes four items with code, closes one as not-applicable after investigation, and leaves one as an already-documented deliberate trade.

Fixed:

  • pnpm dlx / yarn dlx — same registry-fetch defect as npx, matched broadly in the shared tool-name group since neither tool has any other legitimate use of the word "dlx".
  • npm exec — same defect, but scoped to npm specifically ((^|[^[:alnum:]_./-])npm[^#]*[[:space:]]exec) rather than the shared group: yarn exec/pnpm exec are common, legitimate commands that just run an already-installed local binary and fetch nothing, so matching them the way -g is matched across all four tools would be a real false-positive, not the same trade -g makes (where all four tools genuinely share the flag).
  • bunx — joins npx in the bare-keyword alternative (it's bun's name for the same thing, not a flag on bun).
  • npm_config_global=true / NPM_CONFIG_GLOBAL=true — npm's environment-variable spelling of --global.
  • package.json's own scripts block now gets the same backslash-continuation join file scanning already had. Refactored join_continuations to read stdin instead of a file argument so it can sit in front of jq's output too — a lifecycle hook value can carry a real embedded newline via JSON's \n escape (e.g. "preinstall": "npm install \\\n -g pkg"), which previously scanned as two independent physical lines and missed the flag entirely.
  • A broken/dangling tracked symlink now hard-fails with a clear message instead of silently scanning as clean — its open failure was landing inside the existing || true meant only to tolerate grep's ordinary "no match".

Closed without a code change, after investigation:

  • -g attached directly to its value (-gpkg) — not valid npm syntax. -g is a boolean flag; npm's arg parser doesn't accept an attached value on it, so there's no real evasion here to close.
  • A # in a non-comment position, e.g. npm install https://github.com/a/b#v1 -g — stays the documented, deliberate trade from Close check-lint-pins.sh scan gaps from #37 #39. Examining content after a # would reopen the exact comment-swallowing false-clears that fix closed (a comment ending in \ glued onto the next real command, and the reverse). Narrower and more contrived than what was traded away.

Caught during review of this diff's own pattern change (not present in the merged PR, fixed before this one): scoping exec to npm without a left boundary matched pnpm exec too, because npm is a literal substring of pnpm. Fixed with the same boundary style already used for npx/bunx. Also dropped a require_readable call site in scan_scanned_paths that turned out to be dead code — find -type f excludes symlinks by type before the guard ever runs, so the only reachable call site is scan_stray_scripts (via git ls-files, which lists tracked symlinks regardless of type). A guard that can never trigger is exactly the kind of declared-but-unwritten check this repo's own gate philosophy warns against, so it's removed rather than left for appearances.

Closes #38.

Test plan

  • All 4 new spellings caught: pnpm dlx, yarn dlx, npm exec, bunx, both npm_config_global=true casings
  • False-positive guards for the new patterns: yarn exec/pnpm exec stay green, npm run build:exec-helper and npm run something-dlx-thing stay green
  • package.json lifecycle hook with a real backslash-continuation now caught; a normal hook (husky install) stays green
  • Broken tracked symlink hard-fails with a clear message instead of reporting clean
  • Full regression re-run of all 18 fixtures from Close check-lint-pins.sh scan gaps from #37 #39's fix (gaps, controls, false-positive guards, comment handling both directions, CRLF, yarn global, dedupe, content/ isolation, self-exclusion, the RCE-filename regression test, manifest shape guard) — all still pass after the join_continuations stdin refactor
  • shellcheck clean, bash -n clean, real repo baseline stays green throughout (31 fixtures total)

Adds coverage for pnpm/yarn's registry-fetch spellings (dlx, matched
broadly since neither tool has any other use of that word) and npm's
(exec, scoped to npm specifically since yarn/pnpm's own exec commonly
just runs an already-installed local binary and fetches nothing -- unlike
-g, which all four tools genuinely share). Adds bunx alongside npx in the
bare-keyword alternative, and npm_config_global=true (either casing) as
npm's environment-variable spelling of --global.

Refactors join_continuations to read stdin instead of a file argument,
so package.json's own scripts block gets the same backslash-continuation
join file scanning already had -- a lifecycle hook value can carry a real
embedded newline via JSON's \n escape, which previously scanned each
resulting physical line independently and missed a flag split across them.

Adds a readability guard so a broken/dangling tracked symlink hard-fails
with a clear message instead of silently scanning as clean (its open
failure was landing inside the existing || true meant to tolerate grep's
ordinary "no match").

Two items from #38 investigated and closed without a code change:
attaching -g directly to its value (-gpkg) isn't valid npm syntax --
-g is boolean and takes no attached value, so there's no real evasion
to close. A #-in-URL install (npm install https://.../pkg#v1 -g) stays
a documented, deliberate trade from #39: examining content after a #
would reopen the comment-swallowing false-clears that fix closed.

Verified with 31 fixtures covering every #37/#38 case plus the new
false-positive guards this diff's own pattern changes needed (yarn
exec / pnpm exec must stay green; npm exec, pnpm dlx, yarn dlx, bunx,
and both env-var casings must still fail). shellcheck clean.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 9, 2026

Copy link
Copy Markdown

Deploying aixgo with  Cloudflare Pages  Cloudflare Pages

Latest commit: e3c6070
Status: ✅  Deploy successful!
Preview URL: https://49aeb010.aixgo.pages.dev
Branch Preview URL: https://fix-lint-pin-gaps-followup-3.aixgo.pages.dev

View logs

require_readable() used -r alone, which passes for a tracked symlink
pointing at a directory (then hangs downstream in an awk I/O error
swallowed by the existing || true -- silently clean again) or a device
file (awk blocks reading it, hanging the gate rather than failing it).
Now requires -f as well.

The awk -v tag="$tag" tagging step -- introduced to close the sed-based
RCE from the previous round -- still had a narrower issue: POSIX
mandates -v assignments go through the same escape processing as a
program-text string literal, so a tracked filename containing the two
characters \n becomes a real embedded newline once awk unescapes it,
letting an attacker-controlled filename forge extra lines into the
gate's own FAIL output. Switched both call sites to pass the tag via
the environment (`tag=... awk '... ENVIRON["tag"]'`), which is read
verbatim.

npm_config_global=true (added last commit) turns out to be one of two
real spellings of the same env-var mechanism: npm accepts 1 as well as
true for this flag, and npm_config_location=global is the same env-var
treatment of the already-caught --location=global. Both are now covered.

Added npm x, npm exec's documented one-letter alias, scoped the same
way exec is (npm specifically, not the shared tool group, since it's
not a spelling pnpm/yarn/bun share). Also caught and fixed, before it
shipped, a bug in this addition's own first draft: an unbounded npm
prefix would have matched inside stray mentions of things like .npmrc
combined with unrelated "exec" text later on the same line -- the same
substring-boundary class of bug as the pnpm/npm mixup fixed in the
previous commit. A parallel `bun x` alternative was attempted and
dropped: nobody in this review had bun installed to confirm the alias
is real, and it introduced a similar boundary problem (`bun` is also
the first three letters of `bundle`) that couldn't be resolved without
either failing to match the real case or reopening a new false-positive
on an unrelated trailing token. `bunx`, the confirmed and dominant real
spelling, was already covered and needs no change.

Verified with the full existing fixture suite (33 cases) plus new ones
for each fix above, including the specific false-positive/negative
pairs each fix needed: npm x / npm exec still fail, npm run
build:exec-helper and bundle exec x_test.rb stay green, pnpm exec /
yarn exec stay green, bunx still fails. shellcheck clean.
@charlesgreen

Copy link
Copy Markdown
Contributor Author

Update: this PR was independently reviewed by security-engineer and test-engineer (isolated git worktrees, PR head at the time: `6e89655`). Summary of what they found and the follow-up commit (`e3c6070`) that addresses it:

Fixed in this PR (pushed as a follow-up commit):

  • `require_readable()` used `-r` alone, which passes for a tracked symlink pointing at a directory (then hits an awk I/O error swallowed by the existing `|| true` — silently clean again) or a device file (awk blocks reading it, hanging the gate rather than failing it). Now requires `-f` as well.
  • The `awk -v tag="$tag"` tagging step — introduced in the prior PR to close a sed-based RCE — had a narrower issue: POSIX mandates `-v` assignments go through the same escape processing as program-text, so a tracked filename containing the two characters `\n` (literal backslash-n) becomes a real embedded newline once awk unescapes it, letting an attacker-controlled filename forge extra lines into the gate's own FAIL output. Both call sites now pass the tag via the environment (`ENVIRON["tag"]`), read verbatim.
  • `npm_config_global=true` (added earlier in this PR) turns out to be one of two real spellings: npm also accepts `1`, and `npm_config_location=global` is the env-var form of the already-caught `--location=global`. Both now covered.
  • Added `npm x` (npm exec's documented one-letter alias). Caught and fixed, before shipping, a bug in this addition's own first draft: an unbounded `npm` prefix would have matched inside stray mentions like `.npmrc` combined with unrelated "exec" text later on the same line — the same substring-boundary class of bug as the `pnpm`/`npm` mixup fixed earlier in this PR. A parallel `bun x` alternative was attempted and dropped: unverified (nobody in review had `bun` installed to confirm the alias is real) and it reopened a similar boundary problem (`bun` is the first three letters of `bundle`) that couldn't be resolved cleanly. `bunx` — confirmed, dominant, already covered — needs no change.

Deferred to #41 (lower-severity, judgment calls, or no live impact under current invocation): a newline-in-filename bypass affecting non-`.sh` files under `.github`/`scripts`, `is_self()`'s fragility under a hypothetical future absolute-path invocation, a quoted `#` inside a lifecycle hook, and two diagnostics-only rough edges (an unreadable file suppressing real hits found elsewhere in the same run; two manifest script entries gluing together across a trailing backslash).

Full regression re-run after the follow-up commit: 33 fixtures (all prior gaps/controls plus every new case from both review rounds), shellcheck clean, real repo baseline green throughout.

@charlesgreen
charlesgreen merged commit c25f5a2 into main Sep 9, 2026
5 checks passed
@charlesgreen
charlesgreen deleted the fix/lint-pin-gaps-followup-38 branch September 9, 2026 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

check-lint-pins.sh: remaining scan gaps after #37

1 participant