fix(install): repeat specs in global allow-scripts suggestion

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:
 

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:
 

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',

lib/utils/allow-scripts-remediation.js

@@ -1,9 +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 `--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 }
+module.exports = { allowScriptsFlag, configSetAllowScripts, policyKeyFor }

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 } = 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,13 +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 list = names.join(',')
+    const keys = nodes.map(policyKeyFor)
     return [
-      `Run \`npm install -g --allow-scripts=${list}\` 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 [

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 ' +

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'`)
+})

test/lib/utils/reify-output.js

@@ -534,11 +534,103 @@ 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/)
 })
 
+// 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'],
+    config: { global: true, registry: 'https://internal.example.com/' },
+  })
+
+  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.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`/)
+})
+
+// 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: {
+        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, new RegExp(`allow-scripts='git\\+ssh://git@github\\.com/o/r\\.git#${sha}'`))
+})
+
 t.test('single unreviewed script uses singular wording', async t => {
   const mockReifyWithExtras = async (t, reify, extras) => {
     const mock = await mockNpm(t, {})