test: apply the supply-chain age gate to npm and pnpm in Verdaccio tests - #4347
test: apply the supply-chain age gate to npm and pnpm in Verdaccio tests#4347erickzhao wants to merge 11 commits into
Conversation
The Verdaccio tests install the freshly published monorepo into throwaway app directories, so they can never pick up the root `.yarnrc.yml`. Until now only Yarn's gate was mirrored in the harness; npm and pnpm installed without one. Both have since grown the same policy, so set it for them too and keep the shared values in one place: - npm calls it `min-release-age` (in days) and has supported it since 11.19, so only pass it when the npm on `PATH` is new enough and say once when it isn't. CI pins npm 11.19.0 because npm 12 requires a newer Node than `.nvmrc`. - pnpm calls it `minimumReleaseAge` (in minutes) but reads it from its config files only, so generate a global config and point pnpm at it with `XDG_CONFIG_HOME`. That config also has to set `registry`: pnpm honors `--registry` and its config files but not `npm_config_registry`, so the pnpm half of these tests was resolving `@electron-forge/*` from the public registry and validating the last published release instead of the local build. Two other fixes fall out of this: - `pnpm store prune` is gone in favour of a per-run `cacheDir` under the storage directory. Staleness lives in the metadata cache, not in the content-addressed store, and a cold store made pnpm hang after installing until the test runner timed out. - `COREPACK_ROOT` no longer leaks into the spawned tests. `yarn test:verdaccio` runs Yarn through Corepack, and pnpm refuses to switch to the version `create-electron-app` pins when it thinks Corepack invoked it. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
pnpm shuts its tarball worker pool down once per install, but any worker call that happens after that lazily creates a new pool that nothing ever shuts down, and the idle worker thread keeps the event loop alive. An install whose last download finishes just after pnpm prints `Done in Xs` therefore writes the lockfile, links everything, reports success and then never exits (pnpm/pnpm#13617). Installs that fetch nothing are unaffected, which is why this only shows up in these tests: they always install into a brand new project. Every test here spawns its package manager and waits for it to exit, so the hang costs the whole test rather than just the process, which is what was timing out the `pnpm` template tests on all three platforms. Put a stand-in for pnpm at the front of `PATH` for the duration of these tests that kills it once it has reported that it is done and has had a grace period to exit on its own. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Since pnpm 11, `pnpm run` silently runs an install first whenever it decides that `node_modules` is out of sync with the lockfile. These tests run `<package manager> run start` to check that the app `create-electron-app` just installed can start, so that install replaces the very thing they are checking: on Windows it rewrote the dependency tree into one where the generated `forge.config.ts` could no longer resolve the Forge plugin it imports, and `electron-forge start` failed. Set `verifyDepsBeforeRun` to `warn` so the check still runs and still reports whatever it believes is out of sync, without acting on it. While here, only watch for the exit hang on the commands that install packages. `pnpm run start` keeps running long after pnpm reports that it is done, and the last thing the exit shim should do is kill it. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
pnpm decides whether to link dependencies through a store-wide virtual store or one inside the project based on whether it believes it is running in CI, and these tests cannot keep that consistent: they install with the environment they inherit and then run the app's `start` script with a minimal one, so pnpm read the same project two different ways and reported that `node_modules` no longer matched the lockfile. Pin the setting to the value CI would pick anyway. Also report the dependency tree the package manager installed when `start` fails, since a `start` that cannot resolve the app's own configuration says nothing about the tree it was trying to resolve it from. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
The shim this removes ran pnpm and killed it 15 seconds after it printed `Done in`, on the theory that anything past that point was the leaked worker pool of pnpm/pnpm#13617 keeping a finished process alive. On Windows it was cutting installs short instead: both TypeScript templates came out of `create-electron-app` with exactly the base template's dependencies and none of their own, because the second install reports that it is done and is then killed while the very work the pnpm bug defers — copying packages into `node_modules` — is still going. Windows CI keeps its pnpm store on `C:` and its projects on `D:`, so nothing can be hardlinked and that tail takes far longer than it does anywhere else. An install that quietly loses half of a project is a worse failure than one that hangs, and the hang the shim was written for was in the install `pnpm run` used to perform behind the tests' back, which `verifyDepsBeforeRun: warn` already stopped. Also report what `create-electron-app` printed when `start` fails, since it runs its steps with listr2's `exitOnError: false` and so exits 0 with a broken project when an install fails. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Restores the watchdog removed in the commit before this one — pnpm really does finish an install and then hang, and without it the vite-typescript project times out at 240s — but stops it from leaving a project behind that cannot be repaired. Killing pnpm once it reports success is only safe if it has really stopped working, and it hasn't: the hang comes from a package that arrived after pnpm stopped expecting one, and that package is still being written. Worse, pnpm will not put it back, because it decides whether a project is up to date from the state files it keeps in `node_modules` rather than from the packages themselves — installing again into a tree it has already recorded is `Already up to date` even with packages deleted out of it, `--force` included. That is what Windows was failing on: both TypeScript projects came out of `create-electron-app` with the base template's dependencies and none of their own. So the watchdog now discards those state files after killing pnpm and installs again, which makes pnpm compare the tree against the store and write whatever is missing. A repair only has to link packages that are already in the store, so it is very unlikely to hang in turn; if every attempt does, the shim now says so and fails instead of reporting a success it cannot vouch for. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Windows keeps producing projects whose `node_modules` has the base template's dependencies and none of the template's own, and nothing says why: `create-electron-app` installs through listr2 with `exitOnError: false` and never prints the errors it collects, so a failed install leaves the task without a tick and without a word about it, and the package manager's own output is thrown away with it. So the pnpm shim now records every pnpm it runs — the command, the directory, how it ended and everything it printed — and a test whose app fails to start reports the runs for its own project, along with the `package.json` they were working from, which is what says whether the dependencies were recorded and not installed or never recorded at all. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
… passes on On Windows the real pnpm is a `.cmd` file, so the shim ran it through `cmd.exe`, which Node hands the command line to unquoted. `cmd.exe` reads `^` as its own escape character, so `pnpm add typescript@^6.0.0` reached pnpm as `typescript@6.0.0` — a version that does not exist — and the install failed with `ERR_PNPM_NO_MATCHING_VERSION`. Every other dependency the templates ask for happens to have a release at exactly the version its range starts from, which is why only the two TypeScript templates failed, and why the projects they left behind had `"@electron/fuses": "2.0.0"` where every other platform gets `"^2.1.3"`. `create-electron-app` runs its steps with listr2's `exitOnError: false` and never prints the errors it collects, so all of this was silent: the install failed, the project was left without the template's own dependencies, and the test only found out when the app could not resolve its Forge configuration. Hand the spawning to `cross-spawn`, which quotes and escapes arguments the way `cmd.exe` needs and is what Forge itself runs package managers with. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
The manifest entry was left out of the commit that started importing it, so installs with a frozen lockfile failed. `^7.0.3` is the range the rest of the project asks for, which `yarn constraints` requires it to match. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
`create-electron-app` pins every pnpm app it creates to the pnpm version this repository runs in CI, and until pnpm 11.18 adding a dependency to a project that already had some could drop a package that another package it kept still depends on. The last of the four installs `create-electron-app` runs left `rimraf` in `node_modules` without the `glob` it requires, so `forge.config.ts` could no longer load and the Verdaccio template tests could not start the app they had just created. It only showed up on Windows, for two reasons: everywhere else the platform-specific makers' dependencies pull `glob` in through a second path that keeps it in the tree, and Corepack pins `pnpm@latest` for the app it creates, which is much newer than this pin. Corepack fails on the Windows runners, so there the pin is what actually installs. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
There was a problem hiding this comment.
The change itself is correct. I checked the main claims:
- pnpm 11.21.0 ignores
npm_config_registry. Pointed it at a closed port, install still completed. pnpm reads$XDG_CONFIG_HOME/pnpm/config.yamlon every platform, Windows included. - The gate works through Verdaccio. Ran Verdaccio 6.10 with our
config.yaml. Proxied packuments keeptime. npm 11.19 withnpm_config_min_release_age=7and pnpm with the generated config both rejected a 6 hour old@types/node@26.3.0through127.0.0.1:4873. Cutoff was exactly 7 days.@scope/*excludes work in both. - The
COREPACK_ROOTfailure reproduces. - CI logs for this branch have no "does not support
min-release-age" warning and no[verdaccio harness]lines. npm gate was on, shim never fired.
Requesting changes for the comment errors inline. One of them hides a real gap. Two more installs in CI are still not locked or gated and need to be fixed before we can call CI dependency locked:
packages/api/core/spec/slow/install-dependencies.slow.spec.ts:25runsnpm install debug@^2.0.0against the public registry. This runs inyarn test:slowon Windows and macOS. No lockfile, no age gate, and the caret picks up a new 2.x immediately. Run it through the Verdaccio harness, or setnpm_config_min_release_ageand an exact version..github/workflows/ci.yml:109and:188runnpm install node-gyp@9.4.0in the Windows setup step. The pin is exact, but node-gyp's transitive deps get resolved fresh every run with no lock and no gate. Setnpm_config_min_release_age=7on that step (npm 11.19 is installed before it in slow-tests but not in fast-tests), or add a lockfile.
The devEngines issue inline predates this PR, but this PR touches that line and the new comment promises something it does not do. Fix the comment here. The actual fix can be a follow up.
| run: npm install -g pnpm@11.21.0 | ||
|
|
||
| # The Verdaccio tests age-gate their installs like the root `.yarnrc.yml` | ||
| # does, and npm only learned about `min-release-age` in 11.19, which is |
There was a problem hiding this comment.
npm 11.10.0 added min-release-age (npm/cli#8965) and 11.17.0 added min-release-age-exclude (npm/cli#9534). Not 11.19. The >= 11.19 check in spawn-verdaccio.ts still works, it is just stricter than it needs to be. Change this comment and the one in the harness to 11.17. The rest is right: Node 22.13 bundles npm 10.9.x, and npm 12 needs ^22.22.2 || ^24.15.0 || >=26.0.0.
There was a problem hiding this comment.
Fixed — the comment now cites 11.10 (npm/cli#8965) and 11.17 (npm/cli#9534). I also moved the check itself from >= 11.19 to >= 11.17 so the code and the comment agree.
| const npmSupportsAgeGate = | ||
| npmMajor > 11 || (npmMajor === 11 && npmMinor >= 19); | ||
| if (!npmSupportsAgeGate) { | ||
| console.warn( |
There was a problem hiding this comment.
This is a security control. In CI it should fail, not warn and keep going without the gate.
if (!npmSupportsAgeGate) {
const message = `npm ${npmVersion} does not support \`min-release-age\` (npm >= 11.17 required)`;
if (process.env.CI) throw new Error(message);
console.warn(`⚠️ ${message}, so npm installs in these tests are not age-gated`);
}Otherwise a future .nvmrc bump that drops the Install npm step removes the gate from the npm tests and nothing fails. Also 11.17, see above.
There was a problem hiding this comment.
Done, with your snippet at 11.17. One addition: I hoisted the check out of runCommand into checkNpmAgeGateSupport() and call it before startVerdaccio(), since as written the throw landed after ~60s of publishing. Verified on this machine (npm 11.6.2): CI=true exits 1 in 2s without publishing or running the command, unset CI warns and proceeds.
| /** | ||
| * `start` makes the package manager check the lockfile it just | ||
| * wrote, and pnpm has enforced a minimum release age of its own by | ||
| * default since 11.16, so that check rejects the project outright |
There was a problem hiding this comment.
Not true for the pnpm we run. There is no built-in default. The bundled pnpmrc is empty and the resolver does minimumReleaseAge ?? 0. With a fresh cache and no gate, pnpm 11.21.0 and 11.24.0 both installed a 6 hour old @types/node@26.3.0. Passing XDG_CONFIG_HOME to start is still right, it carries verifyDepsBeforeRun: warn, the registry, and our gate. Just fix the reason in this comment.
There was a problem hiding this comment.
Fixed — the reason is now the harness config that XDG_CONFIG_HOME carries: registry, gate, exempt list, and verifyDepsBeforeRun: warn. Took your testing on this rather than re-running it.
| // skipped and `rimraf` is the only thing left asking for `glob`. | ||
| packageJSON.devEngines = { | ||
| packageManager: 'pnpm@11.10.0', | ||
| packageManager: 'pnpm@11.21.0', |
There was a problem hiding this comment.
This does not do what the comment says. pnpm ignores devEngines.packageManager when it is a string. The spec form is an object. Tested with pnpm 11.21.0:
"packageManager": "pnpm@11.20.0"plus"devEngines": {"packageManager": "pnpm@11.21.0"}: pnpm downloaded 11.20.0 and ran it. No warning.- Same with
"devEngines": {"packageManager": {"name": "pnpm", "version": "11.21.0"}}: pnpm warned thatpackageManagerwill be ignored and ran 11.21.0.
In CI the tests run create-electron-app --package-manager=pnpm. resolvePackageManager turns that into latest. corepack use pnpm@latest writes whatever is latest on npmjs that day, 11.24.0 today, already ahead of this pin and the ci.yml pin. Corepack resolves that straight from registry.npmjs.org, it reads COREPACK_NPM_REGISTRY but not npm_config_registry, so Verdaccio is bypassed. The pnpm on PATH then downloads that version and hands over to it. I checked, the age gate does not apply to that download. So the pnpm that actually runs the template installs in CI is unpinned, ungated, and changes daily. Neither the CI pin nor this line controls it.
This predates the PR, but do not claim a floor we do not enforce. Simplest real fix is test only: have template-tests.ts pass --package-manager=pnpm@<version on PATH> so corepack pins what CI installed. The object form here would also work but it is user facing (pnpm warns whenever packageManager differs) so that is a separate PR.
There was a problem hiding this comment.
Comment fixed: it now documents the field as a record of the pnpm version CI installs rather than a floor, and says outright that pnpm ignores the string form and that Corepack's packageManager field takes precedence regardless. Real fix left as a follow-up, per your note.
The Verdaccio harness now fails when npm cannot enforce `min-release-age` in CI instead of warning and installing without a gate, and checks before publishing anything so that failure costs two seconds. The floor is 11.17, not 11.19: npm added `min-release-age` in 11.10 (npm/cli#8965) and `min-release-age-exclude` in 11.17 (npm/cli#9534). Two installs were still resolving from the public registry with no lockfile and no gate. Both now pass a `before` date, which the npm bundled with the Node version in `.nvmrc` understands and `min-release-age` is too new for: - `npm install node-gyp@9.4.0` in the Windows setup step of both test jobs, where the pin is exact but its dependencies were resolved fresh every run. - `npm install debug@^2.0.0` in `install-dependencies.slow.spec.ts`, where the caret would pick up a new 2.x the moment one was published. Two comments also claimed things that are not true, one of them hiding a real gap. pnpm has no minimum release age of its own, so the reason for passing `XDG_CONFIG_HOME` through to `start` is the harness config it carries. And pnpm ignores `devEngines.packageManager` unless it is written in the object form, while Corepack writes a `packageManager` field that takes precedence regardless, so that field is documented as a record of the version CI installs rather than the floor it cannot enforce. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
|
The two installs from your summary are gated in ccfc3c5, both with a
Separately, and not touched here: the |
MarshallOfSound
left a comment
MarshallOfSound
left a comment
There was a problem hiding this comment.
All four threads are fixed and I checked the new commit. --before and npm_config_before both work on the bundled npm 10.9.8: debug@^2.0.0 resolves to 2.6.9 with a 7 day cutoff, a 6 hour old @types/node@26.3.0 gets ETARGET, and with no gate it installs 26.3.0. You are right that Install npm runs after Windows setup, so --before was the correct call over min-release-age. Both new Windows jobs run the flag and pass.
Two notes on the Windows setup step inline. Neither blocks this. Approving.
| run: | | ||
| cd "$PROGRAMFILES/nodejs/node_modules/npm/node_modules/@npmcli/run-script" | ||
| npm install node-gyp@9.4.0 | ||
| npm install node-gyp@9.4.0 --before "$(date -u -d '7 days ago' +%Y-%m-%d)" |
There was a problem hiding this comment.
Two things about this step, neither blocking, both for a follow up:
-
This installs a lot more than node-gyp. The
cdputs us inside@npmcli/run-script, which has its ownpackage.json, so npm installs that package's full devDependency tree as well. The new Windows logs showadded 880 packagesfor this step, including tap, eslint bits and a pile of deprecated stuff. It is gated now, which is the important part, but it is a much bigger surface than the step name suggests.npm install --no-save node-gyp@9.4.0or installing node-gyp somewhere else and linking it would shrink it to node-gyp and its deps only. -
Your note about this being inert in slow-tests is probably right.
npm install -g npm@11.19.0runs after this step and puts a different npm first on PATH, so the node-gyp we patch into$PROGRAMFILES/nodejs/node_modules/npmis not the npm that runs the tests. I have not verified it either. If it is inert we should either drop the step from slow-tests or moveInstall npmbefore it so the patch lands in the npm that actually runs.
2 participants
The root
.yarnrc.ymlsetsnpmMinimalAgeGatewith an exemption list for our own packages. The Verdaccio tests install the freshly published monorepo into app directories underos.tmpdir(), so they never pick that file up, and the harness only ever mirrored the Yarn half of the policy — npm and pnpm installed with no gate at all. Both have since grown the same setting, so this mirrors it for them too, with the age and the exemption list defined once and spelled three ways.min-release-ageand counts it in days. It landed in 11.19, and older versions warn "unknown config" on every single invocation, so the harness only passes it when the npm onPATHsupports it and warns once when it doesn't. CI installs npm 11.19.0 for the slow-tests job — npm 12 requires Node^22.22.2 || ^24.15.0 || >=26and.nvmrcpins 22.13, so 12 can't be installed there at all.minimumReleaseAgeand counts it in minutes, but only reads it frompnpm-workspace.yamlor the global config file. Writing apnpm-workspace.yamlinto each generated app would make Forge's ownresolvePackageManagermisdetect the npm and Yarn cases, so the harness generates a global config under the Verdaccio storage directory and points pnpm at it withXDG_CONFIG_HOME.The pnpm leg wasn't using the local registry
That generated config also has to set
registry. pnpm honors--registryand its config files, but it ignoresnpm_config_registryfrom the environment, which is how every other package manager in the harness is pointed at Verdaccio. So the pnpm third of these tests has been resolving@electron-forge/*from the public registry and validating the last published alpha rather than the local build.Verified by baking a marker into
packages/api/cli/dist, publishing, and diffing what got installed: before the fix npm and Yarn got the marker and pnpm didn't, and pnpm's lockfile integrity matched the npmjs tarball. After it, pnpm's integrity matches the locally published one.Two fixes that fall out of it
pnpm store pruneis replaced by a per-runcacheDirunder the storage directory, whichstartVerdaccioalready deletes. The prune was there so a republished version couldn't resolve through stale metadata, but staleness lives in the metadata cache, not in the content-addressed store — and a fully cold store made pnpm hang after installing (its worker pool never shut down) until the test runner timed out. Confirmed by running twice with different markers: the warm store serves the fresh build both times.COREPACK_ROOTleakage.yarn test:verdaccioruns Yarn through Corepack, which exportsCOREPACK_ROOTto everything below it, including the package manager installing each generated app. pnpm refuses to switch to the version it's asked for when it believes Corepack invoked it, andcreate-electron-apprunscorepack use pnpm@latest, so every pnpm app failed its first install with a version mismatch. The harness now drops the variable.Testing
yarn test:verdaccio— the real entry point, through Corepack Yarn — passes 64/64 across all 10 files.Noticed but not changed
exitOnError: falseon the "Installing common dependencies" task atpackages/external/create-electron-app/src/init.ts:248means a failed dependency install is swallowed by listr2: the CLI exits 0 and leaves the user with a scaffold that has no dependencies. That's what made theCOREPACK_ROOTbug above so hard to see, and real users hitting an install failure get the same silent success. Worth fixing separately, since it changes user-facing CLI behavior.🤖 Generated with Claude Code
Co-Authored-By: Claude svc-devxp-claude@slack-corp.com