[pull] latest from npm:latest

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

@@ -15,9 +15,18 @@ records which of your dependencies are permitted to run install scripts
 (`preinstall`, `install`, `postinstall`, and `prepare` for non-registry
 sources). This command is the recommended way to maintain that field.
 
-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.
+Dependency install scripts are blocked by default. Install commands
+silently skip lifecycle scripts for any dependency that does not have a
+matching entry in `allowScripts`, and end with a list of the packages
+whose scripts were skipped so you can review them with this command.
+
+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:
 

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

@@ -15,10 +15,10 @@ Writes `false` entries into the `allowScripts` field of your project's
 `package.json`, recording that a dependency must not run install scripts
 even if a future version would otherwise be eligible.
 
-In the current release, install scripts still run by default, so `deny-scripts`
-only affects how installs of denied packages are reported. A future release
-will block unreviewed install scripts and respect deny entries at install
-time.
+Dependency install scripts are blocked by default. Adding a `false`
+entry with `deny-scripts` makes the denial explicit (so it survives
+`npm approve-scripts --all`) and excludes the package from any future
+`--allow-scripts-pending` review prompts.
 
 ```bash
 npm deny-scripts <pkg> [<pkg> ...]

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'
@@ -56,7 +58,7 @@ class Rebuild extends ArboristWorkspaceCmd {
 
         return spec
       })
-      const nodes = tree.inventory.filter(node => this.isNode(specs, node))
+      const nodes = [...tree.inventory.filter(node => this.isNode(specs, node))]
 
       await strictAllowScriptsPreflight({ arb, npm: this.npm })
       await arb.rebuild({ nodes })
@@ -66,24 +68,37 @@ class Rebuild extends ArboristWorkspaceCmd {
       await arb.rebuild()
     }
 
-    // Phase 1 advisory: list any packages whose install scripts ran (or
-    // would have run) and are not yet covered by allowScripts. Rebuild
-    // doesn't go through reifyFinish, so the walker is invoked here.
+    // Rebuild skips reifyFinish, so run the walker here to list any
+    // packages whose install scripts were blocked.
     const unreviewed = await checkAllowScripts({ arb, npm: this.npm })
     if (unreviewed.length > 0) {
       const count = unreviewed.length
-      const noun = count === 1 ? 'package has' : 'packages have'
+      const noun = count === 1 ? 'package had' : 'packages had'
+      // `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.'
+        `${count} ${noun} install scripts blocked because they are not covered by allowScripts. ` +
+        remediation
       )
     }
 
     output.standard('rebuilt dependencies successfully')
   }
 
   isNode (specs, node) {
+    // Bundled dependencies are never selected by name. Their identity comes
+    // from the bundling parent's tarball (a bundled folder can call itself
+    // anything), so `npm rebuild bcrypt` must never target a bundled
+    // `node_modules/bcrypt`. Their install scripts never run regardless.
+    if (node.inBundle) {
+      return false
+    }
     return specs.some(spec => {
       if (spec.type === 'directory') {
         return node.path === spec.fetchSpec

lib/utils/allow-scripts-cmd.js

@@ -62,7 +62,10 @@ class AllowScriptsCmd extends BaseCommand {
     })
     await arb.loadActual()
 
-    const unreviewed = await checkAllowScripts({ arb, npm: this.npm })
+    // Keep listing unreviewed packages even with ignore-scripts set, so
+    // you can move from a blanket ignore-scripts to an allowlist. This
+    // only lists; nothing runs.
+    const unreviewed = await checkAllowScripts({ arb, npm: this.npm, includeWhenIgnored: true })
 
     if (pending) {
       return this.runPending(unreviewed)
@@ -84,7 +87,7 @@ class AllowScriptsCmd extends BaseCommand {
     const has = count === 1 ? 'has' : 'have'
     const pkg = count === 1 ? 'package' : 'packages'
     output.standard(
-      `${count} ${pkg} ${has} install scripts not yet covered by allowScripts:`
+      `${count} ${pkg} ${has} install scripts blocked because they are not covered by allowScripts:`
     )
     for (const { node, scripts } of unreviewed) {
       const { name, version } = trustedDisplay(node)
@@ -107,27 +110,10 @@ class AllowScriptsCmd extends BaseCommand {
       output.standard('No packages with unreviewed install scripts.')
       return
     }
-    // Bundled dependencies cannot be allowlisted in Phase 1 (RFC defers
-    // this to a follow-up because matching by name@version from the
-    // bundled tarball would reintroduce manifest confusion). Exclude
-    // them from `--all` so we don't silently write a policy entry under
-    // attacker-controlled identity.
-    const candidates = unreviewed.filter(({ node }) => !node.inBundle)
-    const skipped = unreviewed.length - candidates.length
-    if (skipped > 0) {
-      /* istanbul ignore next: plural variant covered separately */
-      const noun = skipped === 1 ? 'dependency' : 'dependencies'
-      log.warn(
-        this.logTitle,
-        `Skipping ${skipped} bundled ${noun}; bundled deps with install ` +
-        'scripts cannot be allowlisted in this release.'
-      )
-    }
-    if (candidates.length === 0) {
-      output.standard('No packages eligible for approval.')
-      return
-    }
-    const groups = this.groupByPackage(candidates.map(({ node }) => node))
+    // Bundled dependencies never appear in `unreviewed` (checkAllowScripts
+    // skips them because they never run their install scripts and cannot
+    // be allowlisted), so there is nothing extra to filter here.
+    const groups = this.groupByPackage(unreviewed.map(({ node }) => node))
     await this.writePolicyChanges(groups)
   }
 

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/check-allow-scripts.js

@@ -1,54 +1,27 @@
-const isScriptAllowed = require('@npmcli/arborist/lib/script-allowed.js')
-const getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js')
+const { collectUnreviewedScripts } = require('@npmcli/arborist/lib/unreviewed-scripts.js')
 
-// Walks arb.actualTree.inventory and returns the list of dep nodes that
-// have install-relevant lifecycle scripts and are not yet covered (or
-// explicitly denied) by the allowScripts policy.
+// Walks a tree's inventory and returns the list of dep nodes that have
+// install-relevant lifecycle scripts and are not yet covered (or explicitly
+// denied) by the allowScripts policy.
+//
+// Thin wrapper around arborist's shared `collectUnreviewedScripts`, mapping
+// the CLI's `({ arb, npm, tree })` shape onto the shared walk. Defaults to
+// `arb.actualTree` (post-reify) but accepts an explicit tree so callers can
+// pre-flight against the idealTree before scripts run.
 //
 // Returns an array of `{ node, scripts }` entries. `scripts` is an object
 // describing the relevant lifecycle scripts that would run.
-
-const checkAllowScripts = async ({ arb, npm, tree }) => {
-  const ignoreScripts = !!arb.options?.ignoreScripts
-  const dangerouslyAllowAll = !!npm?.flatOptions?.dangerouslyAllowAllScripts
-
-  if (ignoreScripts || dangerouslyAllowAll) {
-    return []
-  }
-
-  // Defaults to actualTree (post-reify) but accepts an explicit tree so
-  // callers can pre-flight against the idealTree before scripts run.
-  const targetTree = tree || arb.actualTree
-  if (!targetTree?.inventory) {
-    return []
-  }
-
-  const policy = arb.options?.allowScripts || null
-
-  const unreviewed = []
-  for (const node of targetTree.inventory.values()) {
-    if (node.isProjectRoot || node.isWorkspace) {
-      continue
-    }
-    if (node.isLink) {
-      // Linked workspace dependencies are managed by the workspace owner.
-      continue
-    }
-
-    const verdict = isScriptAllowed(node, policy)
-    if (verdict === true || verdict === false) {
-      continue
-    }
-
-    const scripts = await getInstallScripts(node)
-    if (Object.keys(scripts).length === 0) {
-      continue
-    }
-
-    unreviewed.push({ node, scripts })
-  }
-
-  return unreviewed
-}
+//
+// `includeWhenIgnored` keeps listing unreviewed packages even when
+// ignore-scripts is set, so approve/deny can show what you'd move from a
+// blanket ignore-scripts to an allowlist. Execution callers leave it false.
+const checkAllowScripts = async ({ arb, npm, tree, includeWhenIgnored = false }) =>
+  collectUnreviewedScripts({
+    tree: tree || arb.actualTree,
+    policy: arb.options?.allowScripts || null,
+    ignoreScripts: !!arb.options?.ignoreScripts,
+    dangerouslyAllowAllScripts: !!npm?.flatOptions?.dangerouslyAllowAllScripts,
+    includeWhenIgnored,
+  })
 
 module.exports = checkAllowScripts

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
@@ -240,13 +241,16 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
   // stdout is reserved for things the user explicitly asked to see
   // (npm ls, npm view).
   const count = unreviewedScripts.length
-  const pkg = count === 1 ? 'package has' : 'packages have'
-  const header = `${count} ${pkg} install scripts not yet covered by allowScripts:`
+  const pkg = count === 1 ? 'package had' : 'packages had'
+  const header =
+    `${count} ${pkg} install scripts blocked because they are not 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 +264,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' }
   )

smoke-tests/tap-snapshots/test/index.js.test.cjs

@@ -107,7 +107,7 @@ npm error   --allow-scripts
 npm error     Comma-separated list of packages whose install-time lifecycle scripts
 npm error
 npm error   --strict-allow-scripts
-npm error     If \`true\`, turn the install-script policy from a warning into a hard
+npm error     If \`true\`, turn the install-script policy from a silent skip into a
 npm error
 npm error   --dangerously-allow-all-scripts
 npm error     If \`true\`, bypass the \`allowScripts\` policy entirely and run every