fix: suggest --allow-scripts for global installs in unreviewed-scripts warnings (#9481)

Backport of #9469 to `release/v11`.

Co-authored-by: Jamie Magee <jamie.magee@gmail.com>

Author: github-actions[bot]

docs/lib/content/commands/npm-approve-scripts.md

@@ -19,6 +19,14 @@ In the current release, this field is advisory: install scripts still run
 by default, but installs print a list of packages whose scripts have not
 been reviewed. A future release will block unreviewed install scripts.
 
+This command only works inside a project that has a `package.json`. It does
+not apply to global installs (`npm install -g`) or one-off executions
+(`npm exec` / `npx`), which have no project `package.json` to write to and
+will fail with an `EGLOBAL` error. 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`.
+
 There are three modes:
 
 ```bash

lib/commands/rebuild.js

@@ -2,10 +2,12 @@ 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')
 
 class Rebuild extends ArboristWorkspaceCmd {
   static description = 'Rebuild a package'
@@ -73,10 +75,17 @@ class Rebuild extends ArboristWorkspaceCmd {
     if (unreviewed.length > 0) {
       const count = unreviewed.length
       const noun = count === 1 ? 'package has' : 'packages have'
+      // `npm approve-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)
+      const remediation = this.npm.global
+        ? `Run \`${configSetAllowScripts(names)}\` to allow their scripts.`
+        : 'Run `npm approve-scripts --allow-scripts-pending` to review.'
       log.warn(
         'rebuild',
         `${count} ${noun} install scripts not yet covered by allowScripts. ` +
-        'Run `npm approve-scripts --allow-scripts-pending` to review.'
+        remediation
       )
     }
 

lib/utils/allow-scripts-remediation.js

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

lib/utils/reify-output.js

@@ -16,6 +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 reifyOutput = (npm, arb, extras = {}) => {
   const { diff, actualTree } = arb
@@ -243,10 +244,12 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
   const pkg = count === 1 ? 'package has' : 'packages have'
   const header = `${count} ${pkg} install scripts not yet covered by allowScripts:`
 
+  const names = []
   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)
     const ver = version ? `@${version}` : ''
     const events = Object.entries(scripts)
       .map(([event, cmd]) => `${event}: ${cmd}`)
@@ -260,9 +263,28 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
       header,
       ...lines,
       '',
-      'Run `npm approve-scripts --allow-scripts-pending` to review, or `npm approve-scripts <pkg>` to allow.',
+      ...remediationLines(npm, names),
     ].join('\n')
   )
 }
 
+// `npm approve-scripts` writes to a project package.json, which doesn't
+// 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) => {
+  if (npm.global) {
+    const list = names.join(',')
+    return [
+      `Run \`npm install -g --allow-scripts=${list}\` to allow these scripts ` +
+      `once, or \`${configSetAllowScripts(names)}\` to allow them for ` +
+      'all global installs.',
+    ]
+  }
+  return [
+    'Run `npm approve-scripts --allow-scripts-pending` to review, ' +
+    'or `npm approve-scripts <pkg>` to allow.',
+  ]
+}
+
 module.exports = reifyOutput

lib/utils/strict-allow-scripts-preflight.js

@@ -1,4 +1,6 @@
 const checkAllowScripts = require('./check-allow-scripts.js')
+const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
+const { configSetAllowScripts } = 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
@@ -46,13 +48,26 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => {
     return `  ${label} (${events})`
   }).join('\n')
 
+  // `npm approve-scripts` / `npm deny-scripts` write 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)
+  const remediation = npm.global
+    ? 'Allow them with `--allow-scripts`, persist them with ' +
+      `\`${configSetAllowScripts(names)}\`, or bypass this ` +
+      'check with `--dangerously-allow-all-scripts`.'
+    : 'Approve them with `npm approve-scripts`, deny them with ' +
+      '`npm deny-scripts`, or bypass this check with ' +
+      '`--dangerously-allow-all-scripts`.'
+
   throw Object.assign(
     new Error(
       `--strict-allow-scripts: ${unreviewed.length} package(s) have install ` +
       `scripts not covered by allowScripts:\n${lines}\n` +
-      'Approve them with `npm approve-scripts`, deny them with ' +
-      '`npm deny-scripts`, or bypass this check with ' +
-      '`--dangerously-allow-all-scripts`.'
+      remediation
     ),
     { code: 'ESTRICTALLOWSCRIPTS' }
   )

test/lib/commands/rebuild.js

@@ -244,6 +244,31 @@ t.test('emits Phase 1 advisory warning for unreviewed install scripts', async t
   )
 })
 
+t.test('global advisory warning points at npm config set, not approve-scripts', async t => {
+  const { npm, logs } = await setupMockNpm(t, {
+    config: {
+      global: true,
+    },
+    globalPrefixDir: {
+      node_modules: {
+        canvas: {
+          'index.js': '',
+          'package.json': JSON.stringify({
+            name: 'canvas',
+            version: '1.0.0',
+            scripts: { install: 'echo install' },
+          }),
+        },
+      },
+    },
+  })
+  await npm.exec('rebuild', [])
+  const warn = logs.warn.byTitle('rebuild').join('\n')
+  t.match(warn, /install scripts not yet covered by allowScripts/)
+  t.match(warn, /npm config set allow-scripts=canvas/)
+  t.notMatch(warn, /approve-scripts/)
+})
+
 t.test('no advisory warning when allowScripts covers the package', async t => {
   const { npm, logs } = await setupMockNpm(t, {
     prefixDir: {

test/lib/utils/reify-output.js

@@ -487,6 +487,39 @@ t.test('prints unreviewed install scripts summary', async t => {
   t.match(warn, /npm approve-scripts --allow-scripts-pending/)
 })
 
+t.test('global install suggests --allow-scripts, not approve-scripts', async t => {
+  const mockReifyWithExtras = async (t, reify, extras, config = {}) => {
+    const mock = await mockNpm(t, { config })
+    reifyOutput(mock.npm, reify, extras)
+    mock.npm.finish()
+    return mock
+  }
+
+  const baseReify = {
+    actualTree: { name: 'host', inventory: { has: () => false } },
+    diff: { children: [] },
+  }
+
+  const unreviewedScripts = [
+    {
+      node: { packageName: 'canvas', name: 'canvas', version: '2.11.0', path: '/x/canvas' },
+      scripts: { install: 'node-gyp rebuild' },
+    },
+    {
+      node: { packageName: 'sharp', name: 'sharp', version: '0.33.2', path: '/x/sharp' },
+      scripts: { preinstall: 'pre', postinstall: 'post' },
+    },
+  ]
+
+  const mock = await mockReifyWithExtras(t, baseReify, { unreviewedScripts }, { global: true })
+  const warn = mock.logs.warn.byTitle('allow-scripts').join('\n')
+  t.match(warn, /2 packages have install scripts not yet covered/)
+  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.notMatch(warn, /approve-scripts/)
+})
+
 t.test('single unreviewed script uses singular wording', async t => {
   const mockReifyWithExtras = async (t, reify, extras) => {
     const mock = await mockNpm(t, {})

test/lib/utils/strict-allow-scripts-preflight.js

@@ -189,3 +189,33 @@ t.test('error label falls back to node.name when package.version is missing', as
     { message: /no-version-pkg \(install: node-gyp rebuild\)/ }
   )
 })
+
+t.test('project-scoped error suggests approve-scripts / deny-scripts', async t => {
+  const arb = makeArb({ ideal: tree([node({ name: 'canvas' })]) })
+  await t.rejects(
+    preflight({
+      arb,
+      npm: { flatOptions: { strictAllowScripts: true } },
+      idealTreeOpts: {},
+    }),
+    { message: /Approve them with `npm approve-scripts`/ }
+  )
+})
+
+t.test('global error points at --allow-scripts, not approve-scripts', async t => {
+  const arb = makeArb({ ideal: tree([node({ name: 'canvas' })]) })
+  await t.rejects(
+    preflight({
+      arb,
+      npm: { global: true, flatOptions: { strictAllowScripts: true } },
+      idealTreeOpts: {},
+    }),
+    (err) => {
+      t.equal(err.code, 'ESTRICTALLOWSCRIPTS')
+      t.match(err.message, /--allow-scripts/)
+      t.match(err.message, /npm config set allow-scripts=canvas/)
+      t.notMatch(err.message, /approve-scripts/)
+      return true
+    }
+  )
+})