fix(install): repeat specs in global allow-scripts suggestion - #9840
fix(install): repeat specs in global allow-scripts suggestion#9840Ashish-CodeJourney wants to merge 2 commits into
Conversation
The blocked-install-scripts warning suggested `npm install -g --allow-scripts=<pkg>`, which has no install targets, so the command falls back to installing the current directory and fails with ENOENT reading package.json for anyone not sitting in a project. Build the suggestion from the command that was actually run and its positional specs, so `npm install -g esbuild` now suggests `npm install -g esbuild --allow-scripts=esbuild`. Commands invoked without specs (`npm update -g`) keep the bare form, which works. Fixes: npm#9835
| // suggestion would fail with ENOENT reading package.json. | ||
| const globalAllowScripts = (npm, names) => { | ||
| const command = npm.command || 'install' | ||
| const specs = npm.argv?.length ? ` ${npm.argv.join(' ')}` : '' |
There was a problem hiding this comment.
npm.argv cannot safely reconstruct the original command. It drops flags, so an install from a private registry would be retried against the default registry while explicitly allowing that package's scripts. The values are also unquoted, so a spec like 'pkg@>=1.2.0' turns > into shell redirection. Could we avoid replaying the install and operate on the installed package instead
There was a problem hiding this comment.
Confirmed, and dropped the replay entirely. lib/npm.js:91 sets npm.argv from
config.parsedArgv.remain, which is positionals only, so an install from a
private registry would have been suggested back as a default-registry install
that also allows that package's scripts to run. The unquoted > in a spec like
pkg@>=1.2.0 is a problem too.
Rather than replaying the install, the warning now names the flag to add to the
install you already ran:
Re-run your install with
--allow-scripts=esbuildto allow these scripts once,
or runnpm config set allow-scripts=esbuild --location=userto allow them for
all global installs.
No command reconstruction, so no flag loss, and the emitted value is shell-quoted
when it needs to be.
| mock.npm.finish() | ||
|
|
||
| const warn = mock.logs.warn.byTitle('install-scripts').join('\n') | ||
| t.match(warn, /npm update -g --allow-scripts=esbuild/) |
There was a problem hiding this comment.
This checks the warning text, but not whether the suggested command works. If the first update installed the latest esbuild with its script blocked, running npm update -g --allow-scripts=esbuild finds nothing to update and does not rerun the script. The test should execute the remediation and confirm the script runs.
There was a problem hiding this comment.
That test went away with the replay it was covering. The underlying point stands
though: npm update -g --allow-scripts=esbuild finds nothing to update and never
reruns the script, so no suggested install command fully closes this.
Fixing it properly means making approve-scripts work for global installs, which
currently throws EGLOBAL at lib/utils/allow-scripts-cmd.js:71. I kept that out
of this PR since it is a design change rather than a bug fix, and this PR now at
least stops printing a command that cannot work. Happy to take it on here if you
would rather have both together.
| const globalAllowScripts = (npm, names) => { | ||
| const command = npm.command || 'install' | ||
| const specs = npm.argv?.length ? ` ${npm.argv.join(' ')}` : '' | ||
| return `npm ${command} -g${specs} --allow-scripts=${names.join(',')}` |
There was a problem hiding this comment.
Package names only work as policy keys for registry dependencies. URL, git, and file dependencies are matched by their resolved source, so --allow-scripts=tool would still leave those scripts blocked. This needs the trusted policy identity for each node rather than its display name.
There was a problem hiding this comment.
Fixed and it turned out to predate this PR: lib/commands/rebuild.js and
lib/utils/strict-allow-scripts-preflight.js build their npm config set
suggestions from trustedDisplay(node).name too, so they had the same bug on
latest.
All three now go through a shared policyKeyFor(node), which builds candidate
keys (trusted registry name → node.resolved → resolvedSourceSpecs) and
verifies each against the node with the real matches() from
script-allowed.js. That way the key we hand the user is one the matcher
actually accepts, rather than one that looks right: git, file, remote and tarball
deps now get their resolved source. Those carry shell metacharacters — # in a
git committish starts a comment — so the value is quoted when needed.
`npm install -g esbuild` warned "Run `npm install -g --allow-scripts=esbuild`", which has no specs and so installs the current directory, failing with ENOENT reading package.json. The command cannot be reconstructed either: `npm.argv` carries positionals only, so flags like `--registry` would be dropped from a suggestion that also allows that package's scripts to run, and unquoted specs such as `pkg@>=1.2.0` turn `>` into shell redirection. Suggest the flag to add to the install the user already ran instead of replaying it. Also derive the suggested policy keys with the real matcher rather than the display name. Only registry deps are matched by name; git, file, remote and tarball deps are matched by their resolved source, so the previous suggestions left those scripts blocked. Resolved sources contain shell metacharacters, so the value is quoted when needed.
The blocked-install-scripts warning suggested
npm install -g --allow-scripts=<pkg>, which has no install targets, so the command falls back to installing the current directory and fails with ENOENT reading package.json for anyone not sitting in a project.Stop suggesting a command at all: name the flag to add to the install that was already run. Reconstructing the command is not safe —
npm.argvcarries positional specs only, so flags are silently dropped. Also derive the suggested--allow-scriptsvalues from the policy identity rather than the display name, so git, file and remote deps get a key the matcher accepts.Fixes: #9835
What / Why
Global installs have no project
package.json, sonpm install-scripts approvecannot be used. The warning instead points at--allow-scripts, but the command it prints is not runnable:npm install -gwith no positional specs installs the current directory, which a global installer is generally not sitting in, so the suggested remediation dead-ends on ENOENT.An earlier revision of this PR rebuilt the command from
npm.commandandnpm.argv. Per review, that is not safe:lib/npm.js:91setsnpm.argvfromconfig.parsedArgv.remain, which is positional specs only. Flags are dropped, so an install from a private registry would be suggested back as a default-registry install that also allows that package's scripts to run.npm i -g 'pkg@>=1.2.0'turns>into shell redirection.Review also surfaced a second bug on the same line, present on
latest: the suggested--allow-scripts/npm config set allow-scriptsvalues are built fromtrustedDisplay(node).name, which is a display name. Perworkspaces/arborist/lib/script-allowed.js, only registry deps are matched by name — git, file, remote and tarball deps are matched by their resolved source. Following the advice left those scripts blocked.lib/commands/rebuild.jsandlib/utils/strict-allow-scripts-preflight.jsbuild their suggestions the same way and have the same bug.How
remediationLines()inlib/utils/reify-output.jsno longer prints a command. It names the flag to add to the install the user already ran, via a newallowScriptsFlag()helper next to the existingconfigSetAllowScripts()inlib/utils/allow-scripts-remediation.js:Nothing is reconstructed, so no flags can be lost and no spec needs re-quoting into a command.
The values themselves now come from a new shared
policyKeyFor(node)in the same module. It builds candidate keys (trusted registry name →node.resolved→resolvedSourceSpecs) and verifies each against the node with the realmatches()fromscript-allowed.js, so the key handed to the user is one the policy accepts rather than one that merely looks right.rebuild.jsandstrict-allow-scripts-preflight.jsuse it too. Resolved sources carry shell metacharacters —#in a git committish starts a comment — so the emitted value is quoted when needed.The same non-working example was documented in
npm install-scriptsandnpm approve-scripts; both are corrected tonpm install -g canvas sharp --allow-scripts=canvas,sharp, which is runnable as written.Tests
Written failing first.
New
test/lib/utils/allow-scripts-remediation.js:test/lib/utils/reify-output.js:test/lib/utils/strict-allow-scripts-preflight.js:The two command-replay tests from the earlier revision are removed along with the helper they covered.
Known gap
npm update -g --allow-scripts=<pkg>still will not rerun a blocked script when there is nothing to update — the remediation is only reached on an install that actually reifies the package. Closing that means makingapprove-scriptswork for global installs, which currently throwsEGLOBALatlib/utils/allow-scripts-cmd.js:71. Left out of this PR as a separate change; happy to fold it in if preferred.References
Fixes #9835