Skip to content

Harden npm supply-chain posture - #381

Merged
MatiasFernandez merged 14 commits into
mainfrom
mfernandez/harden-npm-supply-chain
Aug 11, 2026
Merged

MatiasFernandez merged 14 commits into
mainfrom
mfernandez/harden-npm-supply-chain

Conversation

@MatiasFernandez

@MatiasFernandez MatiasFernandez commented Aug 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Adds preventive supply-chain controls so a compromised dependency (the Shai-Hulud / SHA1-Hulud worm class) can't execute arbitrary code at install time — closes #246.

  • Raise the dev-toolchain npm floor to 11.16 (.nvmrc → 24.19.0, root devEngines) — the minimum version that supports strict-allow-scripts/npm approve-scripts, with the prepare CI job and acceptance/Dockerfile fixed to match.
  • Adopt npm's install-script allowlist — strict-allow-scripts=true + a version-pinned allowScripts table in package.json makes an unreviewed install script fail-closed instead of silently running. Approves the tree's 5 current install-script deps, denies fsevents, and backs the control with a new lint:supply-chain CI check so rm .npmrc can't silently disable it.
  • Add a release-age cooldown and Dependabot config — min-release-age=7 guards local npm install/update (the only path that can still pull a freshly published malicious release, since the lockfile pins every version), mirrored by a 7-day Dependabot cooldown with an install-scripts group so ESTRICTALLOWSCRIPTS bumps land in one predictable PR per cycle.
  • Repair drifted mime/sax lockfile entries — a prior release-bump find-and-replace left their version field disagreeing with the tarball their own resolved URL points to, which silently breaks advisory scanning and hard-blocks npm audit signatures.
  • Enforce lockfile integrity and registry signatures in CI — lockfile-lint (host/HTTPS/integrity), npm audit signatures, and a new version-vs-resolved drift check that closes the exact gap that let mime/sax drift unnoticed for two months.
  • Seeds a Diátaxis-organized docs/ split (explanation/how-to/reference) for the new controls, with an index.

Test plan

  • npm ci succeeds with no EBADDEVENGINES / ESTRICTALLOWSCRIPTS
  • npm run lint && npm run format:check && npm run compile && npm test
  • npm run package (vsce's dependency walk, ships koffi's native binary) + npm run test:gci against a live stone
  • .vsix contents exclude .npmrc/.nvmrc
  • Negative tests: allowlist fail-closed on a removed allowScripts entry, lint:supply-chain catches a missing .npmrc and a drift lockfile entry, lockfile-lint catches a tampered resolved host
  • npm audit signatures passes over the full tree
  • CI: prepare, lint, and floor jobs green end to end

@MatiasFernandez
MatiasFernandez force-pushed the mfernandez/harden-npm-supply-chain branch 6 times, most recently from e3f183d to 6e71499 Compare August 6, 2026 18:32
@MatiasFernandez MatiasFernandez changed the title WIP: NPM supply chain hardening Harden npm supply-chain posture Aug 6, 2026
@MatiasFernandez
MatiasFernandez force-pushed the mfernandez/harden-npm-supply-chain branch 8 times, most recently from b53351d to b3f4701 Compare August 10, 2026 18:18
@MatiasFernandez
MatiasFernandez marked this pull request as ready for review August 10, 2026 18:57

@npapagna npapagna left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Claude's code review of this branch. Six findings below, all inline except one: client/src/__tests__/rowanLoad.test.ts isn't part of this diff, so a line comment can't be anchored there, but flagging it here since it's directly relevant to the last finding on lintSupplyChain.test.ts — updateGitRepo's setup() helper (line 161) creates a second tempdir via mkdtempSync that is never cleaned up anywhere in the file (only the module-level root is removed, in the top-level afterEach). Every run of the two tests that call setup() (lines 191, 197) leaks a directory (seed repo, bare remote, clone, and any bump()-created pusher clones) into the OS temp folder permanently. This is exactly the class of bug withTemporaryFolderDo (client/src/__tests__/support/file.ts:33) already exists to prevent via its finally-block cleanup — worth a follow-up fix even though it's outside this PR's diff.

Comment thread .github/workflows/health-check.yml Resolved
Comment thread .github/workflows/health-check.yml Resolved
Comment thread scripts/lint-supply-chain.mjs Resolved
Comment thread .github/workflows/health-check.yml Resolved

@npapagna npapagna left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great @MatiasFernandez!
Left a couple of comments.

MatiasFernandez and others added 11 commits August 10, 2026 19:36
strict-allow-scripts (needed by the next commits) is only recognized
from npm 11.16.0; the repo was on 11.12.1 via .nvmrc 24.15.0, where an
unknown .npmrc key is merely warned about.

- .nvmrc: bump to 24.19.0, the newest Node 24 LTS release, which
  bundles npm 11.17.0.
- package.json: add devEngines, root-scoped and distinct from
  engines.node — enforces the npm floor without risking engine-strict's
  tree-wide blast radius on a future transitive dep's engines.node.
- health-check.yml: run the workflow-timeout lint script with bash
  rather than through `npm run`, keeping the prepare job free of npm —
  a bare `npm run` now fails with EBADDEVENGINES on the runner's
  ambient npm 10, and the job would otherwise need a setup-node step it
  has never had. Trim its sparse-checkout to match, and update the
  floor job's now-unreachable sunset comment.
- acceptance/Dockerfile: pin npm to @11.17.0 before npm ci, since the
  playwright base image ships 11.13.0. That exact version is the npm
  bundled by .nvmrc's Node, so the container matches a contributor's
  toolchain rather than running ahead of it.
- Document the floor in CONTRIBUTING.md and raising-the-version-floor.md,
  and scope version-pins.md to .nvmrc.
strict-allow-scripts (npm ≥11.16, satisfied by the floor just raised)
makes the per-package allowScripts allowlist fail-closed: an
unreviewed install script throws ESTRICTALLOWSCRIPTS before reify
instead of running with a warning, which is what let SHA1-Hulud-class
worms execute at install time. Approvals are version-pinned, so a
compromised patch of an already-trusted package reverts to
unreviewed and fails the install too.

Approve the tree's 5 current install-script deps, deny fsevents
(optional, macOS-only, absent from Linux CI anyway), and back the
control with a CI check (lint:supply-chain) so `rm .npmrc` can't
silently disable it. Seed the docs/ diátaxis split so the why
(explanation), the day-to-day procedure (how-to), and the current
state (reference) each have one home, without duplicating npm's own
docs or hardcoding versions that already live in package.json.
min-release-age=7 guards local npm install/update, the only path
that can still pull a freshly published malicious release since the
lockfile already pins every version. Dependabot mirrors the same
7-day cooldown so both controls tell one story, with an
install-scripts group so ESTRICTALLOWSCRIPTS bumps land in one
predictable PR per cycle instead of scattering across the queue.
A release-bump find-and-replace (cd0bff1) rewrote every
"version": "1.6.0" to "1.6.1", sweeping up mime and sax even though
neither had moved. mime's resolved tarball still points at 1.6.0
(1.6.1 doesn't exist upstream); sax's pointed at 1.6.0 while the
registry has 1.6.1. Left uncorrected, advisory scanners evaluate
versions that aren't installed, and npm audit signatures errors
outright on the nonexistent mime@1.6.1.

Regenerated via delete-entries + npm install --package-lock-only +
rm -rf node_modules && npm ci. mime is corrected back to 1.6.0 (same
tarball); sax is genuinely bumped to 1.6.1 (new resolved + integrity).
Add lockfile-lint (host/HTTPS/integrity/package-name checks) and
npm audit signatures to the lint job, plus a version-vs-resolved
drift check in lint-supply-chain.mjs. The drift check closes a gap
none of the others cover: a lockfile entry whose version disagrees
with its own resolved tarball, which is exactly what let mime/sax
drift unnoticed for two months.
npm's allowScripts accepts a bare package name (unpinned) or name@version
(pinned). An unpinned allow trusts every future version of that package
forever, defeating the re-review-on-bump behavior the rest of this
script's checks assume is in place. Deny entries are always written
unpinned by npm approve-scripts, so only allows are gated.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
npm@11 as a floating major drifts as new patches release; pin to
11.17.0 — the npm bundled by .nvmrc's Node 24.19.0, matching the
Dockerfile's pin — so CI runs the same npm a contributor does rather
than racing ahead of it. Note in the docs that Dependabot's
github-actions ecosystem won't bump either pin, since it only reads
uses:, not run: strings.
The linter spawned `npm config get` once per key, five npm cold
starts per run — and the test suite runs the whole script once per
case, so that dominated its runtime (10.5s to 2.5s over 25 runs).
One batched call returns `key=value` lines whose values are
byte-identical to the single-key form. A key missing from that
output now throws instead of reading as a failed assertion, so a
changed output shape can't masquerade as a control being off.

`npm config list --json` would batch too, but hands back JSON types
that every comparison and message would have to re-stringify.

On the test side: `entry()` takes an optional `resolvedVersion`, so
the drift cases stop hand-writing the lockfile-entry shape the
helper already builds, and `runLint` goes through
`withTemporaryFolderDo` instead of batching cleanup in an
`afterEach`. That helper is now generic over its return type so the
`spawnSync` result can escape the callback.

rowanLoad.test.ts leaked a temp dir per `updateGitRepo` test — a
bare repo plus two or more clones, never removed. Fixed here since
it is the same idiom.
Both read only the lockfile and npm config, so nothing they depend
on comes out of Prettier or ESLint. Running them first means a
dropped .npmrc key or a drifted lockfile entry fails in under a
second instead of after the full format+lint pass over all three
workspaces.

`npm audit signatures` stays last — it is lockfile-only too, but it
hits the registry, so it is not in the same class.
@MatiasFernandez
MatiasFernandez force-pushed the mfernandez/harden-npm-supply-chain branch from cc3b15f to 6c0ece6 Compare August 10, 2026 22:36
@MatiasFernandez
MatiasFernandez added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit ba4149b Aug 11, 2026
15 checks passed
@MatiasFernandez
MatiasFernandez deleted the mfernandez/harden-npm-supply-chain branch August 11, 2026 16:06
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.

Harden npm supply-chain posture (ignore-scripts, dependency cooldown, SHA-pinned actions)

2 participants