Repository navigation
Harden npm supply-chain posture - #381
Conversation
e3f183d to
6e71499
Compare
b53351d to
b3f4701
Compare
npapagna
left a comment
There was a problem hiding this comment.
🤖 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.
npapagna
left a comment
There was a problem hiding this comment.
Works great @MatiasFernandez!
Left a couple of comments.
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.
…tricter without failing the linter
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.
cc3b15f to
6c0ece6
Compare
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.
.nvmrc→ 24.19.0, rootdevEngines) — the minimum version that supportsstrict-allow-scripts/npm approve-scripts, with theprepareCI job andacceptance/Dockerfilefixed to match.strict-allow-scripts=true+ a version-pinnedallowScriptstable inpackage.jsonmakes an unreviewed install script fail-closed instead of silently running. Approves the tree's 5 current install-script deps, deniesfsevents, and backs the control with a newlint:supply-chainCI check sorm .npmrccan't silently disable it.min-release-age=7guards localnpm 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 aninstall-scriptsgroup soESTRICTALLOWSCRIPTSbumps land in one predictable PR per cycle.mime/saxlockfile entries — a prior release-bump find-and-replace left theirversionfield disagreeing with the tarball their ownresolvedURL points to, which silently breaks advisory scanning and hard-blocksnpm audit signatures.lockfile-lint(host/HTTPS/integrity),npm audit signatures, and a new version-vs-resolveddrift check that closes the exact gap that letmime/saxdrift unnoticed for two months.docs/split (explanation/how-to/reference) for the new controls, with an index.Test plan
npm cisucceeds with noEBADDEVENGINES/ESTRICTALLOWSCRIPTSnpm run lint && npm run format:check && npm run compile && npm testnpm run package(vsce's dependency walk, ships koffi's native binary) +npm run test:gciagainst a live stone.vsixcontents exclude.npmrc/.nvmrcallowScriptsentry,lint:supply-chaincatches a missing.npmrcand a drift lockfile entry,lockfile-lintcatches a tamperedresolvedhostnpm audit signaturespasses over the full treeprepare,lint, and floor jobs green end to end