From 34d13bb0d3f1a7f08338133a0cdea7a394ec372c Mon Sep 17 00:00:00 2001
From: Ashish Vaghela <ashish.vaghela@nelkinda.com>
Date: Tue, 4 Aug 2026 16:40:08 +0530
Subject: [PATCH 1/3] fix(install): repeat specs in global allow-scripts
 suggestion

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: https://github.com/npm/cli/issues/9835
---
 .../content/commands/npm-approve-scripts.md   |  4 +-
 .../content/commands/npm-install-scripts.md   |  4 +-
 lib/utils/allow-scripts-remediation.js        | 13 ++++-
 lib/utils/reify-output.js                     |  5 +-
 test/lib/utils/reify-output.js                | 52 +++++++++++++++++++
 5 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/docs/lib/content/commands/npm-approve-scripts.md b/docs/lib/content/commands/npm-approve-scripts.md
index 55c892fbf96b1..d73c052286e8d 100644
--- a/docs/lib/content/commands/npm-approve-scripts.md
+++ b/docs/lib/content/commands/npm-approve-scripts.md
@@ -25,8 +25,8 @@ it with `--global` (`-g`) fails with an `EGLOBAL` error, since global
 installs (`npm install -g`) and one-off executions (`npm exec` / `npx`) have
 no project `package.json` to write to. To allow install scripts in those
 contexts, use the `--allow-scripts` flag at install time (for example
-`npm install -g --allow-scripts=canvas,sharp`) or persist the setting with
-`npm config set allow-scripts=canvas,sharp --location=user`.
+`npm install -g canvas sharp --allow-scripts=canvas,sharp`) or persist the
+setting with `npm config set allow-scripts=canvas,sharp --location=user`.
 
 There are three modes:
 
diff --git a/docs/lib/content/commands/npm-install-scripts.md b/docs/lib/content/commands/npm-install-scripts.md
index 32f05a8577039..e83a72025f821 100644
--- a/docs/lib/content/commands/npm-install-scripts.md
+++ b/docs/lib/content/commands/npm-install-scripts.md
@@ -25,8 +25,8 @@ it with `--global` (`-g`) fails with an `EGLOBAL` error, since global
 installs (`npm install -g`) and one-off executions (`npm exec` / `npx`) have
 no project `package.json` to write to. To allow install scripts in those
 contexts, use the `--allow-scripts` flag at install time (for example
-`npm install -g --allow-scripts=canvas,sharp`) or persist the setting with
-`npm config set allow-scripts=canvas,sharp --location=user`.
+`npm install -g canvas sharp --allow-scripts=canvas,sharp`) or persist the
+setting with `npm config set allow-scripts=canvas,sharp --location=user`.
 
 There are four subcommands:
 
diff --git a/lib/utils/allow-scripts-remediation.js b/lib/utils/allow-scripts-remediation.js
index ff8c9b75a81fe..c3c99316b6f0e 100644
--- a/lib/utils/allow-scripts-remediation.js
+++ b/lib/utils/allow-scripts-remediation.js
@@ -6,4 +6,15 @@
 const configSetAllowScripts = (names) =>
   `npm config set allow-scripts=${names.join(',')} --location=user`
 
-module.exports = { configSetAllowScripts }
+// Builds the one-off `npm <cmd> -g ... --allow-scripts=<names>` command
+// suggested to global users. The specs the user asked for have to be
+// repeated: `npm install -g --allow-scripts=foo` with no specs installs the
+// current directory, which global users usually are not sitting in, so the
+// 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(' ')}` : ''
+  return `npm ${command} -g${specs} --allow-scripts=${names.join(',')}`
+}
+
+module.exports = { configSetAllowScripts, globalAllowScripts }
diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js
index e50d4ac72d967..aadc2da58ed82 100644
--- a/lib/utils/reify-output.js
+++ b/lib/utils/reify-output.js
@@ -16,7 +16,7 @@ const npmAuditReport = require('npm-audit-report')
 const { readTree: getFundingInfo } = require('libnpmfund')
 const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
 const auditError = require('./audit-error.js')
-const { configSetAllowScripts } = require('./allow-scripts-remediation.js')
+const { configSetAllowScripts, globalAllowScripts } = require('./allow-scripts-remediation.js')
 
 const reifyOutput = (npm, arb, extras = {}) => {
   const { diff, actualTree } = arb
@@ -276,9 +276,8 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
 // one-off, or `npm config set allow-scripts` to persist it.
 const remediationLines = (npm, names) => {
   if (npm.global) {
-    const list = names.join(',')
     return [
-      `Run \`npm install -g --allow-scripts=${list}\` to allow these scripts ` +
+      `Run \`${globalAllowScripts(npm, names)}\` to allow these scripts ` +
       `once, or \`${configSetAllowScripts(names)}\` to allow them for ` +
       'all global installs.',
     ]
diff --git a/test/lib/utils/reify-output.js b/test/lib/utils/reify-output.js
index 0678e9cabeb28..7d559fe027221 100644
--- a/test/lib/utils/reify-output.js
+++ b/test/lib/utils/reify-output.js
@@ -539,6 +539,58 @@ t.test('global install suggests --allow-scripts, not approve-scripts', async t =
   t.notMatch(warn, /approve-scripts/)
 })
 
+t.test('global install repeats the requested specs in the suggestion', async t => {
+  const mock = await mockNpm(t, {
+    command: 'install',
+    argv: ['esbuild', 'canvas@2'],
+    config: { global: true },
+  })
+  Object.defineProperty(mock.npm, 'command', {
+    get () {
+      return 'install'
+    },
+    enumerable: true,
+  })
+
+  reifyOutput(mock.npm, {
+    actualTree: { name: 'host', inventory: { has: () => false } },
+    diff: { children: [] },
+  }, {
+    unreviewedScripts: [{
+      node: { packageName: 'esbuild', name: 'esbuild', version: '0.28.1', path: '/x/esbuild' },
+      scripts: { postinstall: 'node install.js' },
+    }],
+  })
+  mock.npm.finish()
+
+  const warn = mock.logs.warn.byTitle('install-scripts').join('\n')
+  t.match(warn, /npm install -g esbuild canvas@2 --allow-scripts=esbuild/)
+})
+
+t.test('global command without specs suggests that command', async t => {
+  const mock = await mockNpm(t, { command: 'update', config: { global: true } })
+  Object.defineProperty(mock.npm, 'command', {
+    get () {
+      return 'update'
+    },
+    enumerable: true,
+  })
+
+  reifyOutput(mock.npm, {
+    actualTree: { name: 'host', inventory: { has: () => false } },
+    diff: { children: [] },
+  }, {
+    unreviewedScripts: [{
+      node: { packageName: 'esbuild', name: 'esbuild', version: '0.28.1', path: '/x/esbuild' },
+      scripts: { postinstall: 'node install.js' },
+    }],
+  })
+  mock.npm.finish()
+
+  const warn = mock.logs.warn.byTitle('install-scripts').join('\n')
+  t.match(warn, /npm update -g --allow-scripts=esbuild/)
+})
+
 t.test('single unreviewed script uses singular wording', async t => {
   const mockReifyWithExtras = async (t, reify, extras) => {
     const mock = await mockNpm(t, {})

From 8d81415f460edb227a4582a0c3466fa3ca2ce1ac Mon Sep 17 00:00:00 2001
From: Ashish Vaghela <ashish.vaghela@nelkinda.com>
Date: Tue, 11 Aug 2026 10:44:16 +0530
Subject: [PATCH 2/3] fix(install): stop suggesting a broken global
 allow-scripts command

`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.
---
 lib/commands/rebuild.js                       | 10 ++-
 lib/utils/allow-scripts-remediation.js        | 58 ++++++++++---
 lib/utils/reify-output.js                     | 21 +++--
 lib/utils/strict-allow-scripts-preflight.js   | 14 ++--
 test/lib/utils/allow-scripts-remediation.js   | 81 ++++++++++++++++++
 test/lib/utils/reify-output.js                | 82 ++++++++++++++-----
 .../utils/strict-allow-scripts-preflight.js   | 19 +++++
 7 files changed, 232 insertions(+), 53 deletions(-)
 create mode 100644 test/lib/utils/allow-scripts-remediation.js

diff --git a/lib/commands/rebuild.js b/lib/commands/rebuild.js
index 0f98b41e8c168..bcb188fd8f502 100644
--- a/lib/commands/rebuild.js
+++ b/lib/commands/rebuild.js
@@ -2,12 +2,11 @@ const { resolve } = require('node:path')
 const { log, output } = require('proc-log')
 const npa = require('npm-package-arg')
 const semver = require('semver')
-const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
 const ArboristWorkspaceCmd = require('../arborist-cmd.js')
 const checkAllowScripts = require('../utils/check-allow-scripts.js')
 const resolveAllowScripts = require('../utils/resolve-allow-scripts.js')
 const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js')
-const { configSetAllowScripts } = require('../utils/allow-scripts-remediation.js')
+const { configSetAllowScripts, policyKeyFor } = require('../utils/allow-scripts-remediation.js')
 
 class Rebuild extends ArboristWorkspaceCmd {
   static description = 'Rebuild a package'
@@ -77,9 +76,12 @@ class Rebuild extends ArboristWorkspaceCmd {
       // `npm install-scripts` writes to a project package.json, which doesn't
       // exist for global rebuilds. Point global users at `npm config set`,
       // which writes the `allow-scripts` setting to their user .npmrc.
-      const names = unreviewed.map(({ node }) => trustedDisplay(node).name)
+      // Use the policy identity, not the display name: only registry deps
+      // are matched by name, so a name-based suggestion would leave git,
+      // file and tarball deps blocked.
+      const keys = unreviewed.map(({ node }) => policyKeyFor(node))
       const remediation = this.npm.global
-        ? `Run \`${configSetAllowScripts(names)}\` to allow their scripts.`
+        ? `Run \`${configSetAllowScripts(keys)}\` to allow their scripts.`
         : 'Run `npm install-scripts ls` to review.'
       log.warn(
         'rebuild',
diff --git a/lib/utils/allow-scripts-remediation.js b/lib/utils/allow-scripts-remediation.js
index c3c99316b6f0e..1db719963d3e0 100644
--- a/lib/utils/allow-scripts-remediation.js
+++ b/lib/utils/allow-scripts-remediation.js
@@ -1,20 +1,52 @@
+const {
+  getTrustedRegistryIdentity,
+  matches,
+  resolvedSourceSpecs,
+  trustedDisplay,
+} = require('@npmcli/arborist/lib/script-allowed.js')
+
+// Policy keys come straight from resolved sources, which carry characters
+// the shell acts on: `#` in a git committish starts a comment, and `&`,
+// `?` or spaces in a tarball URL break the command apart. Quote whenever
+// the value is not plainly safe, so the suggestion can be pasted as-is.
+const SHELL_SAFE = /^[\w@,./:-]+$/
+
+const shellQuote = (value) =>
+  SHELL_SAFE.test(value) ? value : `'${value.replace(/'/g, `'\\''`)}'`
+
+// The blocked-scripts summary shows a human-readable name, but the
+// allowScripts policy only matches registry deps by name. git, file, remote
+// and tarball deps are matched by their resolved source, so a suggestion
+// built from display names would leave their scripts blocked. Verify each
+// candidate against the node with the real matcher, so the key we hand the
+// user is one the policy will actually accept.
+const policyKeyFor = (node) => {
+  const trusted = getTrustedRegistryIdentity(node)
+  const candidates = [trusted && trusted.name, node.resolved, ...resolvedSourceSpecs(node)]
+  for (const candidate of candidates) {
+    if (typeof candidate === 'string' && candidate !== '' && matches(node, candidate, false)) {
+      return candidate
+    }
+  }
+  /* istanbul ignore next: defensive fallback for nodes without name */
+  return trustedDisplay(node).name || '<unknown>'
+}
+
 // Builds the `npm config set allow-scripts` command suggested to global
 // users, who have no project package.json for `npm approve-scripts` to
 // write to. `--location=user` keeps the setting in the user .npmrc instead
 // of trying (and, for global installs, failing) to write it to the local
 // project config.
-const configSetAllowScripts = (names) =>
-  `npm config set allow-scripts=${names.join(',')} --location=user`
+const configSetAllowScripts = (keys) =>
+  `npm config set allow-scripts=${shellQuote(keys.join(','))} --location=user`
 
-// Builds the one-off `npm <cmd> -g ... --allow-scripts=<names>` command
-// suggested to global users. The specs the user asked for have to be
-// repeated: `npm install -g --allow-scripts=foo` with no specs installs the
-// current directory, which global users usually are not sitting in, so the
-// 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(' ')}` : ''
-  return `npm ${command} -g${specs} --allow-scripts=${names.join(',')}`
-}
+// Builds the `--allow-scripts=<keys>` flag global users add to the install
+// they just ran. Deliberately not a whole command: npm.argv holds
+// positionals only, so a reconstructed `npm install -g <specs>` would drop
+// flags like --registry and retry against the default registry while
+// allowing that package's scripts to run. A spec-less
+// `npm install -g --allow-scripts=<keys>` is no better: it installs the
+// current directory and fails with ENOENT reading package.json.
+const allowScriptsFlag = (keys) => `--allow-scripts=${shellQuote(keys.join(','))}`
 
-module.exports = { configSetAllowScripts, globalAllowScripts }
+module.exports = { allowScriptsFlag, configSetAllowScripts, policyKeyFor }
diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js
index aadc2da58ed82..9ddfb70cd85c2 100644
--- a/lib/utils/reify-output.js
+++ b/lib/utils/reify-output.js
@@ -16,7 +16,11 @@ const npmAuditReport = require('npm-audit-report')
 const { readTree: getFundingInfo } = require('libnpmfund')
 const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
 const auditError = require('./audit-error.js')
-const { configSetAllowScripts, globalAllowScripts } = require('./allow-scripts-remediation.js')
+const {
+  allowScriptsFlag,
+  configSetAllowScripts,
+  policyKeyFor,
+} = require('./allow-scripts-remediation.js')
 
 const reifyOutput = (npm, arb, extras = {}) => {
   const { diff, actualTree } = arb
@@ -246,12 +250,12 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
   const header =
     `${count} ${pkg} install scripts blocked because they are not covered by allowScripts:`
 
-  const names = []
+  const nodes = []
   const lines = unreviewedScripts.map(({ node, scripts }) => {
     const { name, version } = trustedDisplay(node)
     /* istanbul ignore next: every test node has a name */
     const display = name || '<unknown>'
-    names.push(display)
+    nodes.push(node)
     const ver = version ? `@${version}` : ''
     const events = Object.entries(scripts)
       .map(([event, cmd]) => `${event}: ${cmd}`)
@@ -265,7 +269,7 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
       header,
       ...lines,
       '',
-      ...remediationLines(npm, names),
+      ...remediationLines(npm, nodes),
     ].join('\n')
   )
 }
@@ -274,12 +278,13 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
 // exist for global installs (it throws EGLOBAL). For those, point users at
 // the mechanism that does work globally: the `--allow-scripts` flag for a
 // one-off, or `npm config set allow-scripts` to persist it.
-const remediationLines = (npm, names) => {
+const remediationLines = (npm, nodes) => {
   if (npm.global) {
+    const keys = nodes.map(policyKeyFor)
     return [
-      `Run \`${globalAllowScripts(npm, names)}\` to allow these scripts ` +
-      `once, or \`${configSetAllowScripts(names)}\` to allow them for ` +
-      'all global installs.',
+      `Re-run your install with \`${allowScriptsFlag(keys)}\` to allow these ` +
+      `scripts once, or run \`${configSetAllowScripts(keys)}\` to allow them ` +
+      'for all global installs.',
     ]
   }
   return [
diff --git a/lib/utils/strict-allow-scripts-preflight.js b/lib/utils/strict-allow-scripts-preflight.js
index 0c500018184c2..786f32834e86b 100644
--- a/lib/utils/strict-allow-scripts-preflight.js
+++ b/lib/utils/strict-allow-scripts-preflight.js
@@ -1,6 +1,5 @@
 const checkAllowScripts = require('./check-allow-scripts.js')
-const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
-const { configSetAllowScripts } = require('./allow-scripts-remediation.js')
+const { configSetAllowScripts, policyKeyFor } = require('./allow-scripts-remediation.js')
 
 // Pre-flight check for `--strict-allow-scripts`. Call after arborist has
 // been constructed but before `arb.reify()` runs, so that install scripts
@@ -51,13 +50,14 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => {
   // `npm install-scripts` writes to a project package.json, which doesn't
   // exist for global installs. Point global users at the `--allow-scripts`
   // flag and `npm config set allow-scripts`, which both work for global
-  // installs. Use the trusted display identity so the suggested `npm config
-  // set` value matches what the policy matches on, not the tarball's
-  // self-reported name.
-  const names = unreviewed.map(({ node }) => trustedDisplay(node).name)
+  // installs. Use the policy identity so the suggested `npm config set`
+  // value is a key the matcher accepts: only registry deps are matched by
+  // name, so a name-based suggestion would leave git, file and tarball deps
+  // blocked, and the name itself is the tarball's self-reported one.
+  const keys = unreviewed.map(({ node }) => policyKeyFor(node))
   const remediation = npm.global
     ? 'Allow them with `--allow-scripts`, persist them with ' +
-      `\`${configSetAllowScripts(names)}\`, or bypass this ` +
+      `\`${configSetAllowScripts(keys)}\`, or bypass this ` +
       'check with `--dangerously-allow-all-scripts`.'
     : 'Approve them with `npm install-scripts approve`, deny them with ' +
       '`npm install-scripts deny`, or bypass this check with ' +
diff --git a/test/lib/utils/allow-scripts-remediation.js b/test/lib/utils/allow-scripts-remediation.js
new file mode 100644
index 0000000000000..18521bfc18797
--- /dev/null
+++ b/test/lib/utils/allow-scripts-remediation.js
@@ -0,0 +1,81 @@
+const t = require('tap')
+
+const {
+  allowScriptsFlag,
+  configSetAllowScripts,
+  policyKeyFor,
+} = require('../../../lib/utils/allow-scripts-remediation.js')
+
+t.test('registry deps are keyed by their trusted name', async t => {
+  const node = {
+    name: 'canvas',
+    version: '2.11.0',
+    resolved: 'https://registry.npmjs.org/canvas/-/canvas-2.11.0.tgz',
+  }
+  t.equal(policyKeyFor(node), 'canvas')
+})
+
+// An alias installs `naughty` at `node_modules/trusted`. The policy matches
+// on the registered name, so the suggestion has to name it too.
+t.test('aliased registry deps are keyed by the registered name', async t => {
+  const node = {
+    name: 'trusted',
+    version: '1.0.0',
+    resolved: 'https://registry.npmjs.org/naughty/-/naughty-1.0.0.tgz',
+  }
+  t.equal(policyKeyFor(node), 'naughty')
+})
+
+// Non-registry deps are matched by their resolved source. Keying them by
+// name would produce a suggestion the matcher rejects, leaving the scripts
+// blocked after the user followed the advice.
+t.test('tarball deps are keyed by their resolved URL', async t => {
+  const node = { name: 'tool', version: '1.0.0', resolved: 'https://example.com/tool.tgz' }
+  t.equal(policyKeyFor(node), 'https://example.com/tool.tgz')
+})
+
+t.test('file deps are keyed by their resolved path', async t => {
+  const node = { name: 'local', version: '1.0.0', resolved: 'file:../local' }
+  t.equal(policyKeyFor(node), 'file:../local')
+})
+
+t.test('git deps are keyed by their resolved git URL', async t => {
+  const resolved = `git+ssh://git@github.com/o/r.git#${'a'.repeat(40)}`
+  const node = { name: 'forked', version: '1.0.0', resolved }
+  t.equal(policyKeyFor(node), resolved)
+})
+
+// Bundled deps can never be allowlisted, so no candidate matches. Fall back
+// to the display name rather than emitting nothing.
+t.test('falls back to the display name when nothing matches', async t => {
+  const node = {
+    name: 'bundled',
+    version: '1.0.0',
+    inBundle: true,
+    resolved: 'https://registry.npmjs.org/bundled/-/bundled-1.0.0.tgz',
+  }
+  t.equal(policyKeyFor(node), 'bundled')
+})
+
+t.test('plain keys are left unquoted', async t => {
+  t.equal(
+    configSetAllowScripts(['canvas', 'sharp']),
+    'npm config set allow-scripts=canvas,sharp --location=user'
+  )
+  t.equal(allowScriptsFlag(['canvas', 'sharp']), '--allow-scripts=canvas,sharp')
+})
+
+// `#` starts a shell comment, which would silently truncate the committish
+// off a pasted suggestion.
+t.test('shell-unsafe keys are quoted', async t => {
+  const key = `git+ssh://git@github.com/o/r.git#${'a'.repeat(40)}`
+  t.equal(
+    configSetAllowScripts([key]),
+    `npm config set allow-scripts='${key}' --location=user`
+  )
+  t.equal(allowScriptsFlag([key]), `--allow-scripts='${key}'`)
+})
+
+t.test('single quotes in a key are escaped', async t => {
+  t.equal(allowScriptsFlag(["file:../it's"]), `--allow-scripts='file:../it'\\''s'`)
+})
diff --git a/test/lib/utils/reify-output.js b/test/lib/utils/reify-output.js
index 7d559fe027221..2fd05df4b81c3 100644
--- a/test/lib/utils/reify-output.js
+++ b/test/lib/utils/reify-output.js
@@ -534,22 +534,21 @@ t.test('global install suggests --allow-scripts, not approve-scripts', async t =
   const warn = mock.logs.warn.byTitle('install-scripts').join('\n')
   t.match(warn, /2 packages had install scripts blocked because they are not covered by allowScripts/)
   t.match(warn, /canvas@2\.11\.0 \(install: node-gyp rebuild\)/)
-  t.match(warn, /npm install -g --allow-scripts=canvas,sharp/)
-  t.match(warn, /npm config set allow-scripts=canvas,sharp/)
+  t.match(warn, /Re-run your install with `--allow-scripts=canvas,sharp`/)
+  t.match(warn, /npm config set allow-scripts=canvas,sharp --location=user/)
   t.notMatch(warn, /approve-scripts/)
 })
 
-t.test('global install repeats the requested specs in the suggestion', async t => {
+// A copy-pasteable `npm install -g --allow-scripts=<names>` has no specs, so
+// it installs the current directory and fails with ENOENT reading
+// package.json. The command cannot be reconstructed safely either: npm.argv
+// carries positionals only, so flags like --registry would be silently
+// dropped from a suggestion that also allows scripts to run.
+t.test('global remediation never suggests a spec-less install command', async t => {
   const mock = await mockNpm(t, {
     command: 'install',
-    argv: ['esbuild', 'canvas@2'],
-    config: { global: true },
-  })
-  Object.defineProperty(mock.npm, 'command', {
-    get () {
-      return 'install'
-    },
-    enumerable: true,
+    argv: ['esbuild'],
+    config: { global: true, registry: 'https://internal.example.com/' },
   })
 
   reifyOutput(mock.npm, {
@@ -564,31 +563,72 @@ t.test('global install repeats the requested specs in the suggestion', async t =
   mock.npm.finish()
 
   const warn = mock.logs.warn.byTitle('install-scripts').join('\n')
-  t.match(warn, /npm install -g esbuild canvas@2 --allow-scripts=esbuild/)
+  t.notMatch(warn, /npm install -g --allow-scripts=/)
+  t.notMatch(warn, /npm install -g esbuild/)
+  t.match(warn, /Re-run your install with `--allow-scripts=esbuild`/)
 })
 
-t.test('global command without specs suggests that command', async t => {
-  const mock = await mockNpm(t, { command: 'update', config: { global: true } })
-  Object.defineProperty(mock.npm, 'command', {
-    get () {
-      return 'update'
-    },
-    enumerable: true,
+// Package names are only valid policy keys for registry deps. git, file and
+// remote deps are matched by their resolved source, so a suggestion built
+// from the display name would leave their scripts blocked.
+t.test('global remediation uses resolved sources as policy keys', async t => {
+  const mock = await mockNpm(t, { command: 'install', config: { global: true } })
+
+  reifyOutput(mock.npm, {
+    actualTree: { name: 'host', inventory: { has: () => false } },
+    diff: { children: [] },
+  }, {
+    unreviewedScripts: [
+      {
+        node: {
+          name: 'tool',
+          version: '1.0.0',
+          path: '/x/tool',
+          resolved: 'https://example.com/tool.tgz',
+        },
+        scripts: { postinstall: 'node install.js' },
+      },
+      {
+        node: {
+          name: 'local',
+          version: '2.0.0',
+          path: '/x/local',
+          resolved: 'file:../local',
+        },
+        scripts: { install: 'make' },
+      },
+    ],
   })
+  mock.npm.finish()
+
+  const warn = mock.logs.warn.byTitle('install-scripts').join('\n')
+  t.match(warn, /allow-scripts=https:\/\/example\.com\/tool\.tgz,file:\.\.\/local/)
+})
+
+// Resolved sources contain characters the shell treats specially — `#` in a
+// git committish starts a comment — so the suggested value has to be quoted.
+t.test('global remediation quotes shell-unsafe policy keys', async t => {
+  const mock = await mockNpm(t, { command: 'install', config: { global: true } })
+  const sha = 'a'.repeat(40)
 
   reifyOutput(mock.npm, {
     actualTree: { name: 'host', inventory: { has: () => false } },
     diff: { children: [] },
   }, {
     unreviewedScripts: [{
-      node: { packageName: 'esbuild', name: 'esbuild', version: '0.28.1', path: '/x/esbuild' },
+      node: {
+        name: 'forked',
+        version: '1.0.0',
+        path: '/x/forked',
+        resolved: `git+ssh://git@github.com/o/r.git#${sha}`,
+      },
       scripts: { postinstall: 'node install.js' },
     }],
   })
   mock.npm.finish()
 
   const warn = mock.logs.warn.byTitle('install-scripts').join('\n')
-  t.match(warn, /npm update -g --allow-scripts=esbuild/)
+  t.match(warn, new RegExp(`allow-scripts='git\\+ssh://git@github\\.com/o/r\\.git#${sha}'`))
 })
 
 t.test('single unreviewed script uses singular wording', async t => {
diff --git a/test/lib/utils/strict-allow-scripts-preflight.js b/test/lib/utils/strict-allow-scripts-preflight.js
index c67a6e4853a12..fdf4bbe2ee577 100644
--- a/test/lib/utils/strict-allow-scripts-preflight.js
+++ b/test/lib/utils/strict-allow-scripts-preflight.js
@@ -232,3 +232,22 @@ t.test('global error points at --allow-scripts, not approve-scripts', async t =>
     }
   )
 })
+
+// A bare name is only a valid policy key for a registry dep. A tarball dep
+// is matched by its resolved URL, so suggesting its name would leave the
+// scripts blocked after the user followed the advice.
+t.test('global error suggests the resolved source for a tarball dep', async t => {
+  const tarball = {
+    ...node({ name: 'tool' }),
+    resolved: 'https://example.com/tool.tgz',
+  }
+  const arb = makeArb({ ideal: tree([tarball]) })
+  await t.rejects(
+    preflight({
+      arb,
+      npm: { global: true, flatOptions: { strictAllowScripts: true } },
+      idealTreeOpts: {},
+    }),
+    { message: /npm config set allow-scripts=https:\/\/example\.com\/tool\.tgz/ }
+  )
+})

From d76a6da83f4cd21c5303206e894779e54b5dfda8 Mon Sep 17 00:00:00 2001
From: Ashish Vaghela <ashish.vaghela@nelkinda.com>
Date: Tue, 8 Sep 2026 10:20:20 +0530
Subject: [PATCH 3/3] fix(allow-scripts-remediation): address JamieMagee's PR
 review feedback

- Strip query parameters from registry URLs to avoid exposing credentials
- Return null for non-registry URLs with unsafe shell characters (&, |, >, <)
- Return null for non-registry URLs with commas (which break parser)
- Filter out null keys when generating config set commands
- Add comprehensive tests for all three issues raised by JamieMagee
---
 lib/utils/allow-scripts-remediation.js      | 167 +++++++++++++++++++-
 test/lib/utils/allow-scripts-remediation.js |  83 ++++++++++
 2 files changed, 245 insertions(+), 5 deletions(-)

diff --git a/lib/utils/allow-scripts-remediation.js b/lib/utils/allow-scripts-remediation.js
index 1db719963d3e0..bda7ef64d200d 100644
--- a/lib/utils/allow-scripts-remediation.js
+++ b/lib/utils/allow-scripts-remediation.js
@@ -5,6 +5,50 @@ const {
   trustedDisplay,
 } = require('@npmcli/arborist/lib/script-allowed.js')
 
+// Copy of isRegistryNode from arborist to check if a node is a registry dependency
+// This is needed to determine if we can safely extract a package name
+const isRegistryNode = (node) => {
+  // arborist Node objects have an isRegistryDependency getter
+  if (typeof node.isRegistryDependency === 'boolean') {
+    return node.isRegistryDependency
+  }
+  // Fall back to URL parsing for nodes without the getter (e.g., test fixtures)
+  if (!node.resolved) {
+    return !!node.version
+  }
+  // Registry tarballs live at `<host>/<pkg-name>/-/<pkg-name>-<version>.tgz`
+  // Strip query parameters and hash for matching
+  const urlWithoutQuery = node.resolved.split('?')[0].split('#')[0]
+  return /^https?:\/\/[^\/]+\/.+\/-\/[^\/]+-\d/.test(urlWithoutQuery)
+}
+
+// Extract package name from a registry URL, handling query parameters and auth
+// This is similar to getTrustedRegistryIdentity but works even when the URL has query params
+const extractPackageNameFromRegistryUrl = (url) => {
+  // Strip query parameters and hash
+  const cleanUrl = url.split('?')[0].split('#')[0]
+  
+  const { URL } = require('node:url')
+  try {
+    const u = new URL(cleanUrl)
+    const parts = u.pathname.slice(1).split('/-/')
+    if (parts.length >= 2) {
+      // The part before /-/ is the package name (or scope/pkg-name)
+      return parts[0]
+    }
+  } catch {
+    // If URL parsing fails, try to extract from the path
+  }
+  
+  // Fallback: try to extract from the path
+  const match = cleanUrl.match(/\/([^\/]+)\/-\/[^\/]+-\d/)
+  if (match) {
+    return match[1]
+  }
+  
+  return null
+}
+
 // Policy keys come straight from resolved sources, which carry characters
 // the shell acts on: `#` in a git committish starts a comment, and `&`,
 // `?` or spaces in a tarball URL break the command apart. Quote whenever
@@ -14,20 +58,131 @@ const SHELL_SAFE = /^[\w@,./:-]+$/
 const shellQuote = (value) =>
   SHELL_SAFE.test(value) ? value : `'${value.replace(/'/g, `'\\''`)}'`
 
+// Characters that are unsafe in shell commands, even when quoted.
+// `&` in cmd.exe runs the following text as a command.
+// `|` pipes output.
+// `>` and `<` redirect output/input.
+// Query parameters (`?`) in URLs can contain these characters.
+// We avoid suggesting copy-paste commands for URLs with these characters.
+// Note: `#` is safe in cmd.exe (only starts comments in POSIX), so we allow it
+// and rely on shellQuoting to handle it for POSIX shells.
+const UNSAFE_FOR_SHELL = /[&|><\s?]/
+
+// Check if a URL is a registry URL and extract the package name safely.
+// For registry URLs with query parameters or auth, this extracts the
+// package name without the sensitive parts.
+const safeRegistryKey = (node) => {
+  // If we already have a trusted identity, use it
+  const trusted = getTrustedRegistryIdentity(node)
+  if (trusted && trusted.name) {
+    return trusted.name
+  }
+
+  // For registry URLs without trusted identity (e.g., due to query params),
+  // try to extract the package name from the URL
+  if (node.resolved && typeof node.resolved === 'string' && isRegistryNode(node)) {
+    const pkgName = extractPackageNameFromRegistryUrl(node.resolved)
+    if (pkgName) {
+      return pkgName
+    }
+  }
+
+  return null
+}
+
+// Check if a key is safe to include in a copy-paste shell command.
+// Returns true if the key can be safely quoted and pasted into a shell.
+// For non-registry deps with URLs containing unsafe characters, we return false
+// to avoid suggesting commands that could execute arbitrary code when pasted.
+const isSafeForShell = (key) => {
+  // Registry package names are always safe (alphanumeric, hyphens, underscores, dots)
+  // Git URLs, file paths, and tarball URLs may contain unsafe characters
+  if (typeof key !== 'string' || key === '') {
+    return false
+  }
+
+  // Check if it looks like a registry package name (no URL scheme)
+  if (!key.includes('://') && !key.includes('/') && !UNSAFE_FOR_SHELL.test(key)) {
+    return true
+  }
+
+  // For URLs, check if they contain characters that are unsafe even when quoted
+  // Also check for commas, which break the comma-separated list parsing
+  if (UNSAFE_FOR_SHELL.test(key) || key.includes(',')) {
+    return false
+  }
+
+  return true
+}
+
 // The blocked-scripts summary shows a human-readable name, but the
 // allowScripts policy only matches registry deps by name. git, file, remote
 // and tarball deps are matched by their resolved source, so a suggestion
 // built from display names would leave their scripts blocked. Verify each
 // candidate against the node with the real matcher, so the key we hand the
 // user is one the policy will actually accept.
+//
+// For registry deps, we prefer the package name (without version or URL)
+// to avoid exposing credentials or query parameters in the suggestion.
+// For non-registry deps, we use the resolved source, but only if it's safe
+// for shell use (no characters that could be exploited in shell commands).
 const policyKeyFor = (node) => {
-  const trusted = getTrustedRegistryIdentity(node)
-  const candidates = [trusted && trusted.name, node.resolved, ...resolvedSourceSpecs(node)]
+  // Create a cleaned node for matching purposes (strip query params and hash)
+  // This allows getTrustedRegistryIdentity to work even when the original
+  // node.resolved contains query parameters with tokens
+  const cleanNode = { ...node }
+  if (node.resolved && typeof node.resolved === 'string') {
+    cleanNode.resolved = node.resolved.split('?')[0].split('#')[0]
+  }
+
+  // Try to get the trusted identity from the cleaned node
+  const trusted = getTrustedRegistryIdentity(cleanNode)
+  if (trusted && trusted.name) {
+    // For registry deps, verify that the package name matches
+    // We use the cleaned node for matching to avoid issues with query params
+    if (matches(cleanNode, trusted.name, false)) {
+      return trusted.name
+    }
+  }
+
+  // For registry URLs without trusted identity (e.g., due to unusual URL format),
+  // try to extract the package name from the URL
+  if (node.resolved && typeof node.resolved === 'string' && isRegistryNode(node)) {
+    const pkgName = extractPackageNameFromRegistryUrl(node.resolved)
+    if (pkgName) {
+      // Verify with the cleaned node
+      if (matches(cleanNode, pkgName, false)) {
+        return pkgName
+      }
+    }
+  }
+
+  // For non-registry deps or fallback, try the original candidates
+  const candidates = [node.resolved, ...resolvedSourceSpecs(node)]
   for (const candidate of candidates) {
     if (typeof candidate === 'string' && candidate !== '' && matches(node, candidate, false)) {
-      return candidate
+      // Only return keys that are safe for shell use
+      if (isSafeForShell(candidate)) {
+        return candidate
+      }
+      // For registry nodes, also accept the candidate even if not shell-safe
+      // (we already tried to get a safe version above)
+      if (isRegistryNode(node)) {
+        // Strip query params from registry URLs to avoid exposing tokens
+        const cleaned = candidate.split('?')[0].split('#')[0]
+        const pkgName = extractPackageNameFromRegistryUrl(cleaned)
+        if (pkgName) {
+          return pkgName
+        }
+        return cleaned
+      }
+      // For non-registry nodes with unsafe URLs (commas, &, etc.),
+      // we cannot safely include them in a comma-separated list.
+      // Return null to indicate this.
+      return null
     }
   }
+
   /* istanbul ignore next: defensive fallback for nodes without name */
   return trustedDisplay(node).name || '<unknown>'
 }
@@ -37,8 +192,10 @@ const policyKeyFor = (node) => {
 // write to. `--location=user` keeps the setting in the user .npmrc instead
 // of trying (and, for global installs, failing) to write it to the local
 // project config.
+//
+// Filter out null/undefined keys to avoid suggesting invalid entries
 const configSetAllowScripts = (keys) =>
-  `npm config set allow-scripts=${shellQuote(keys.join(','))} --location=user`
+  `npm config set allow-scripts=${shellQuote(keys.filter(k => k).join(','))} --location=user`
 
 // Builds the `--allow-scripts=<keys>` flag global users add to the install
 // they just ran. Deliberately not a whole command: npm.argv holds
@@ -47,6 +204,6 @@ const configSetAllowScripts = (keys) =>
 // allowing that package's scripts to run. A spec-less
 // `npm install -g --allow-scripts=<keys>` is no better: it installs the
 // current directory and fails with ENOENT reading package.json.
-const allowScriptsFlag = (keys) => `--allow-scripts=${shellQuote(keys.join(','))}`
+const allowScriptsFlag = (keys) => `--allow-scripts=${shellQuote(keys.filter(k => k).join(','))}`
 
 module.exports = { allowScriptsFlag, configSetAllowScripts, policyKeyFor }
diff --git a/test/lib/utils/allow-scripts-remediation.js b/test/lib/utils/allow-scripts-remediation.js
index 18521bfc18797..2751b5b8b37a1 100644
--- a/test/lib/utils/allow-scripts-remediation.js
+++ b/test/lib/utils/allow-scripts-remediation.js
@@ -79,3 +79,86 @@ t.test('shell-unsafe keys are quoted', async t => {
 t.test('single quotes in a key are escaped', async t => {
   t.equal(allowScriptsFlag(["file:../it's"]), `--allow-scripts='file:../it'\\''s'`)
 })
+
+// Test for issue: URLs with query parameters containing tokens should not be exposed
+t.test('URLs with auth tokens are not used as policy keys', async t => {
+  const node = {
+    name: 'private-pkg',
+    version: '1.0.0',
+    resolved: 'https://registry.npmjs.org/private-pkg/-/private-pkg-1.0.0.tgz?npm_token=secret123',
+  }
+  // Should use package name instead of URL with token
+  t.equal(policyKeyFor(node), 'private-pkg')
+})
+
+// Test for issue: URLs with commas should not break round-trip
+t.test('URLs with commas are handled safely', async t => {
+  // When a URL contains a comma, it should either be rejected or encoded
+  // The parser splits on commas, so we need to ensure the round-trip works.
+  // For non-registry deps, we return null to avoid breaking the comma-separated list.
+  const parseAllowScriptsList = require('@npmcli/config/lib/parse-allow-scripts-list.js')
+  const node = {
+    name: 'tool',
+    version: '1.0.0',
+    resolved: 'https://example.com/tool,prod.tgz',
+  }
+  const key = policyKeyFor(node)
+  
+  // For non-registry deps with commas, we return null to avoid breaking the parser
+  t.equal(key, null, 'should return null for non-registry URLs with commas')
+  
+  // Verify that null keys are filtered out in config generation
+  const cmd = configSetAllowScripts([key, 'safe-pkg'])
+  t.ok(cmd, 'should generate a command even with null keys')
+  // The command should not include the null key
+  t.equal(cmd.includes('safe-pkg'), true, 'should include safe keys')
+  t.equal(cmd.includes('tool,prod'), false, 'should not include unsafe keys')
+})
+
+// Test for issue: Shell unsafe characters (like &) should be handled for Windows
+t.test('shell unsafe characters like & are escaped or avoided', async t => {
+  const node = {
+    name: 'pkg',
+    version: '1.0.0',
+    resolved: 'https://example.com/pkg.tgz?x=1&whoami',
+  }
+  const key = policyKeyFor(node)
+  // For non-registry deps with unsafe URLs (&, |, >, <), we return null
+  // to avoid suggesting commands that could execute arbitrary code on Windows
+  t.equal(key, null, 'should return null for non-registry URLs with &')
+})
+
+// Round-trip test: parser and matcher should handle the generated keys correctly
+// Note: shell quoting is for the command line only. When the user pastes the command,
+// the shell removes the quotes before passing to npm config set, which stores the unquoted value.
+t.test('round-trip: generated config should parse back to original keys', async t => {
+  const parseAllowScriptsList = require('@npmcli/config/lib/parse-allow-scripts-list.js')
+  const keys = ['canvas', 'sharp']
+  const cmd = configSetAllowScripts(keys)
+  // Extract the allow-scripts value from the command
+  const match = cmd.match(/allow-scripts=([^\s]+)/)
+  t.ok(match, 'should have allow-scripts value')
+  let value = match[1]
+  // Simulate shell unquoting: remove outer single quotes if present
+  if (value.startsWith("'") && value.endsWith("'")) {
+    value = value.slice(1, -1)
+  }
+  const parsed = parseAllowScriptsList(value)
+  t.same(parsed, keys, 'parsed keys should match original')
+})
+
+// Round-trip test with special characters
+t.test('round-trip: keys with shell-unsafe characters should parse correctly', async t => {
+  const parseAllowScriptsList = require('@npmcli/config/lib/parse-allow-scripts-list.js')
+  const key = `git+ssh://git@github.com/o/r.git#${'a'.repeat(40)}`
+  const cmd = configSetAllowScripts([key])
+  const match = cmd.match(/allow-scripts=([^\s]+)/)
+  t.ok(match, 'should have allow-scripts value')
+  let value = match[1]
+  // Simulate shell unquoting: remove outer single quotes if present
+  if (value.startsWith("'") && value.endsWith("'")) {
+    value = value.slice(1, -1)
+  }
+  const parsed = parseAllowScriptsList(value)
+  t.same(parsed, [key], 'parsed keys should match original')
+})