You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(install): strip npm_config_allow_scripts from the per-client installs - #2713
The reporter has allow-scripts set in an npmrc (user or global). npm accepts it there, so the root install succeeds. But npm exports every non-default config value, whichever npmrc set it, to lifecycle scripts as npm_config_* (@npmcli/config/lib/set-envs.js). So scripts/install-clients.mjs inherits npm_config_allow_scripts, and the npm install it spawns in clients/web reads that as the CLI/env layer. npm 11.19+ rejects that layer in any project install (lib/utils/resolve-allow-scripts.js → EALLOWSCRIPTS).
This is not specific to macOS. It affects any from-source v2 install on npm ≥ 11.19 with allow-scripts set in an npmrc. npx users never run the cascade.
Fix
The per-client installs run with npm_config_allow_scripts removed from their environment, matched case-insensitively as npm matches it. Nothing is lost: each child npm reads the same npmrc files itself, where the setting is allowed.
The logic is now an exported childInstallEnv() behind a main guard, so it can be tested. The script ships in the tarball, so it stays self-contained.
The main guard compares real paths. Node resolves symlinks for import.meta.url but not for argv[1], so a checkout reached through a symlink (e.g. macOS /var → /private/var) would otherwise skip the cascade silently. The new test hit exactly that.
Tests
scripts/install-clients.test.mjs:
Unit cases for childInstallEnv: strips the key in any case, keeps other config, doesn't mutate its input.
A premise case against real npm: allow-scripts arriving through the env fails with EALLOWSCRIPTS. It is skipped on npm < 11.19.
An end-to-end case that runs the script against a throwaway repo layout. The HOME npmrc sets allow-scripts and the parent-npm env var is present, and the install is offline. I confirmed it fails with the env: line removed and passes with it.
npm run local:gate is green.
Related
#2712 (filed alongside this PR) is the separate question of v2's own install-script policy: denying esbuild/fsevents as v1 did in #2077, plus a guard against unreviewed scripts. This PR doesn't touch policy.
…alls (#2518)
npm exports every non-default config value to lifecycle scripts as
npm_config_*, whichever npmrc set it. With allow-scripts in a user's
~/.npmrc, the root install accepted it and then handed it to the
install-clients postinstall as npm_config_allow_scripts; the child
`npm install` read that as --allow-scripts on the command line, which
npm 11.19+ rejects in a project install (EALLOWSCRIPTS), so a from-source
install failed at clients/web.
The child installs now run with that one variable removed. Nothing is
lost: each child npm reads the same npmrc files itself, where the setting
is allowed.
The script's main guard compares real paths, since Node resolves symlinks
for import.meta.url but not for argv[1], so the cascade cannot skip
silently when the checkout is reached through a symlink.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
…t a shell (#2518)
The symlink case now launches the fixture through a deliberate symlink
(a junction on Windows) and the plain case runs from a canonical path, so
reverting the realpath guard fails on every platform rather than only
where tmpdir happens to be an alias. Node is spawned without a shell, so
a Windows temp path containing a space is not re-split.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
This version-gated premise leaves the actual spawn wiring unprotected in CI. The build job runs test:scripts on Node 22's bundled npm 10.x (.github/workflows/main.yml:67-96; the workflow confirms that bundle at lines 352-353), so this case is skipped and the end-to-end case below still passes if env: childInstallEnv(process.env) is removed. Add a deterministic wiring test using a stub npm executable that fails when npm_config_allow_scripts is present, or run this suite under npm >=11.19, so the regression fails in CI.
… runner's npm (#2518)
CI's Node 22 bundles npm 10, which predates the allow-scripts gate, so the
real-npm cases skip or pass there whether or not the strip is wired in. A
stub npm on PATH fails whenever npm_config_allow_scripts reaches it, so
dropping the env wiring now fails on every runner.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Previously missed — CI test skips spawn env wiring on bundled npm 10 (from the Suppressed/previously-missed block, so there's no thread to reply in): fixed in 1ac6e2f. A stub npm on PATH (npm + npm.cmd) exits 1 with EALLOWSCRIPTS whenever npm_config_allow_scripts reaches it, so it behaves like npm 11.19+ on any runner. The case first checks that the stub rejects the variable, then runs the cascade through it. Verified: removing env: childInstallEnv(process.env) now fails this case, independent of the installed npm.
Add deterministic symlink-path coverage: still listed as open, but it is the round-1 thread, already fixed in e3c76ab and answered there.
Canonicalize import.meta.url before symlink-aware entrypoint comparison
scripts/install-clients.mjs:101
This guard can still silently skip the cascade when the checkout is reached through a symlink and Node is launched with --preserve-symlinks-main (including via NODE_OPTIONS). That flag makes import.meta.url retain the symlink path, while only argv[1] is canonicalized here, so the comparison is false in the exact scenario this guard is intended to support. Canonicalize both paths before comparing them; extending the symlink test to launch Node with this flag would pin the behavior.
--preserve-symlinks-main keeps the symlink in import.meta.url, so
canonicalizing only argv[1] still skipped the cascade silently for a
checkout reached through a symlink under that flag. The symlink case now
runs with and without it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Previously missed — canonicalize import.meta.url before the symlink-aware entrypoint comparison (no thread to reply in): fixed in 996500e. Both sides of the main guard now go through realpathSync, and the symlinked-checkout case also runs under --preserve-symlinks-main. Verified that canonicalizing only argv[1] fails the new variant.
Copilot review closed: round 4 came back clean (no inline comments, no findings in the headline or the suppressed/previously-missed block). All four rounds' findings were fixed. None were declined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2518
Cause
The reporter has
allow-scriptsset in an npmrc (user or global). npm accepts it there, so the root install succeeds. But npm exports every non-default config value, whichever npmrc set it, to lifecycle scripts asnpm_config_*(@npmcli/config/lib/set-envs.js). Soscripts/install-clients.mjsinheritsnpm_config_allow_scripts, and thenpm installit spawns inclients/webreads that as the CLI/env layer. npm 11.19+ rejects that layer in any project install (lib/utils/resolve-allow-scripts.js→EALLOWSCRIPTS).This is not specific to macOS. It affects any from-source v2 install on npm ≥ 11.19 with
allow-scriptsset in an npmrc.npxusers never run the cascade.Fix
npm_config_allow_scriptsremoved from their environment, matched case-insensitively as npm matches it. Nothing is lost: each child npm reads the same npmrc files itself, where the setting is allowed.childInstallEnv()behind a main guard, so it can be tested. The script ships in the tarball, so it stays self-contained.import.meta.urlbut not forargv[1], so a checkout reached through a symlink (e.g. macOS/var→/private/var) would otherwise skip the cascade silently. The new test hit exactly that.Tests
scripts/install-clients.test.mjs:childInstallEnv: strips the key in any case, keeps other config, doesn't mutate its input.allow-scriptsarriving through the env fails withEALLOWSCRIPTS. It is skipped on npm < 11.19.HOMEnpmrc setsallow-scriptsand the parent-npm env var is present, and the install is offline. I confirmed it fails with theenv:line removed and passes with it.npm run local:gateis green.Related
#2712 (filed alongside this PR) is the separate question of v2's own install-script policy: denying esbuild/fsevents as v1 did in #2077, plus a guard against unreviewed scripts. This PR doesn't touch policy.
🤖 Generated with Claude Code