feat: Phase 2 of allowScripts opt-in install-script policy

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

@@ -15,9 +15,10 @@ 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.
 
 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

@@ -56,7 +56,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,16 +66,15 @@ 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'
       log.warn(
         'rebuild',
-        `${count} ${noun} install scripts not yet covered by allowScripts. ` +
+        `${count} ${noun} install scripts blocked because they are not covered by allowScripts. ` +
         'Run `npm approve-scripts --allow-scripts-pending` to review.'
       )
     }
@@ -84,6 +83,13 @@ class Rebuild extends ArboristWorkspaceCmd {
   }
 
   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

@@ -84,7 +84,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 +107,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/check-allow-scripts.js

@@ -1,54 +1,22 @@
-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
-}
+const checkAllowScripts = async ({ arb, npm, tree }) =>
+  collectUnreviewedScripts({
+    tree: tree || arb.actualTree,
+    policy: arb.options?.allowScripts || null,
+    ignoreScripts: !!arb.options?.ignoreScripts,
+    dangerouslyAllowAllScripts: !!npm?.flatOptions?.dangerouslyAllowAllScripts,
+  })
 
 module.exports = checkAllowScripts

lib/utils/reify-output.js

@@ -240,8 +240,9 @@ 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 lines = unreviewedScripts.map(({ node, scripts }) => {
     const { name, version } = trustedDisplay(node)

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

@@ -1,4 +1,5 @@
 const checkAllowScripts = require('./check-allow-scripts.js')
+const { strictAllowScriptsError } = require('@npmcli/arborist/lib/unreviewed-scripts.js')
 
 // Pre-flight check for `--strict-allow-scripts`. Call after arborist has
 // been constructed but before `arb.reify()` runs, so that install scripts
@@ -36,26 +37,12 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => {
     return
   }
 
-  const lines = unreviewed.map(({ node, scripts }) => {
-    const events = Object.entries(scripts)
-      .map(([event, body]) => `${event}: ${body}`)
-      .join('; ')
-    const name = node.package?.name || node.name
-    const version = node.package?.version || ''
-    const label = version ? `${name}@${version}` : name
-    return `  ${label} (${events})`
-  }).join('\n')
-
-  throw Object.assign(
-    new Error(
-      `--strict-allow-scripts: ${unreviewed.length} package(s) have install ` +
-      `scripts not covered by allowScripts:\n${lines}\n` +
+  throw strictAllowScriptsError(unreviewed, {
+    remediation:
       'Approve them with `npm approve-scripts`, deny them with ' +
       '`npm deny-scripts`, or bypass this check with ' +
-      '`--dangerously-allow-all-scripts`.'
-    ),
-    { code: 'ESTRICTALLOWSCRIPTS' }
-  )
+      '`--dangerously-allow-all-scripts`.',
+  })
 }
 
 module.exports = strictAllowScriptsPreflight

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

tap-snapshots/test/lib/docs.js.test.cjs

@@ -1900,12 +1900,13 @@ this to work properly.
 * Default: false
 * Type: Boolean
 
-If \`true\`, turn the install-script policy from a warning into a hard error:
-any dependency with install scripts not covered by \`allowScripts\` will fail
-the install instead of running with a notice.
+If \`true\`, turn the install-script policy from a silent skip into a hard
+error: any dependency with install scripts not covered by \`allowScripts\`
+will fail the install instead of being silently skipped.
 
-Dependencies explicitly denied with \`false\` in \`allowScripts\` are always
-silently skipped; this setting only affects unreviewed entries.
+By default, dependencies whose install scripts are not approved in
+\`allowScripts\` are silently skipped; this setting promotes that silent skip
+into a hard failure, which is the recommended posture for CI.
 \`--ignore-scripts\` and \`--dangerously-allow-all-scripts\` both override this
 setting.
 
@@ -3271,7 +3272,7 @@ Options:
     Comma-separated list of packages whose install-time lifecycle scripts
 
   --strict-allow-scripts
-    If \`true\`, turn the install-script policy from a warning into a hard
+    If \`true\`, turn the install-script policy from a silent skip into a
 
   --dangerously-allow-all-scripts
     If \`true\`, bypass the \`allowScripts\` policy entirely and run every
@@ -3833,7 +3834,7 @@ Options:
     Comma-separated list of packages whose install-time lifecycle scripts
 
   --strict-allow-scripts
-    If \`true\`, turn the install-script policy from a warning into a hard
+    If \`true\`, turn the install-script policy from a silent skip into a
 
   --dangerously-allow-all-scripts
     If \`true\`, bypass the \`allowScripts\` policy entirely and run every
@@ -4281,7 +4282,7 @@ Options:
     Comma-separated list of packages whose install-time lifecycle scripts
 
   --strict-allow-scripts
-    If \`true\`, turn the install-script policy from a warning into a hard
+    If \`true\`, turn the install-script policy from a silent skip into a
 
   --dangerously-allow-all-scripts
     If \`true\`, bypass the \`allowScripts\` policy entirely and run every
@@ -4431,7 +4432,7 @@ Options:
     Comma-separated list of packages whose install-time lifecycle scripts
 
   --strict-allow-scripts
-    If \`true\`, turn the install-script policy from a warning into a hard
+    If \`true\`, turn the install-script policy from a silent skip into a
 
   --dangerously-allow-all-scripts
     If \`true\`, bypass the \`allowScripts\` policy entirely and run every
@@ -4577,7 +4578,7 @@ Options:
     Comma-separated list of packages whose install-time lifecycle scripts
 
   --strict-allow-scripts
-    If \`true\`, turn the install-script policy from a warning into a hard
+    If \`true\`, turn the install-script policy from a silent skip into a
 
   --dangerously-allow-all-scripts
     If \`true\`, bypass the \`allowScripts\` policy entirely and run every
@@ -5562,7 +5563,7 @@ Options:
     Comma-separated list of packages whose install-time lifecycle scripts
 
   --strict-allow-scripts
-    If \`true\`, turn the install-script policy from a warning into a hard
+    If \`true\`, turn the install-script policy from a silent skip into a
 
   --dangerously-allow-all-scripts
     If \`true\`, bypass the \`allowScripts\` policy entirely and run every
@@ -6380,7 +6381,7 @@ Options:
     Comma-separated list of packages whose install-time lifecycle scripts
 
   --strict-allow-scripts
-    If \`true\`, turn the install-script policy from a warning into a hard
+    If \`true\`, turn the install-script policy from a silent skip into a
 
   --dangerously-allow-all-scripts
     If \`true\`, bypass the \`allowScripts\` policy entirely and run every

test/lib/commands/approve-scripts.js

@@ -55,7 +55,7 @@ t.test('approve-scripts --pending lists unreviewed packages', async t => {
   })
   await npm.exec('approve-scripts', [])
   const out = joinedOutput()
-  t.match(out, /2 packages have install scripts not yet covered/)
+  t.match(out, /2 packages have install scripts blocked because they are not covered by allowScripts/)
   t.match(out, /canvas@1\.0\.0/)
   t.match(out, /sharp@1\.0\.0/)
 })
@@ -385,10 +385,10 @@ t.test('approve-scripts --pending lists packages that only have binding.gyp', as
   t.match(out, /install: node-gyp rebuild/, 'synthetic node-gyp install is named')
 })
 
-t.test('approve-scripts --all skips bundled deps with a notice', async t => {
-  // Bundled deps cannot be allowlisted in Phase 1 (RFC defers their
-  // allowlisting to a follow-up). --all must not silently write a key
-  // derived from the bundled tarball's self-claimed identity.
+t.test('approve-scripts --all never approves bundled deps', async t => {
+  // Bundled deps never run their install scripts and cannot be
+  // allowlisted. They never reach the unreviewed list, so --all must not
+  // write a key derived from the bundled tarball's self-claimed identity.
   const { npm, logs, prefix } = await _mockNpm(t, {
     prefixDir: {
       'package.json': JSON.stringify({
@@ -445,7 +445,8 @@ t.test('approve-scripts --all skips bundled deps with a notice', async t => {
     'non-bundled parent gets approved')
   t.notOk(Object.keys(pkg.allowScripts).some(k => k.startsWith('inner')),
     'bundled inner is not approved')
-  t.match(logs.warn.byTitle('approve-scripts'), [/Skipping 1 bundled dependency/])
+  t.strictSame(logs.warn.byTitle('approve-scripts'), [],
+    'no warning; bundled deps are excluded upstream')
 })
 
 t.test('approve-scripts <bundled-pkg> positional is ignored', async t => {
@@ -505,7 +506,7 @@ t.test('approve-scripts <bundled-pkg> positional is ignored', async t => {
   )
 })
 
-t.test('approve-scripts --all with only bundled deps prints "no eligible" notice', async t => {
+t.test('approve-scripts --all with only bundled deps has nothing to review', async t => {
   const { npm, logs, joinedOutput, prefix } = await _mockNpm(t, {
     prefixDir: {
       'package.json': JSON.stringify({
@@ -554,8 +555,9 @@ t.test('approve-scripts --all with only bundled deps prints "no eligible" notice
     config: { all: true },
   })
   await npm.exec('approve-scripts', [])
-  t.match(joinedOutput(), /No packages eligible for approval/)
-  t.match(logs.warn.byTitle('approve-scripts'), [/Skipping 1 bundled dependency/])
+  t.match(joinedOutput(), /No packages with unreviewed install scripts/)
+  t.strictSame(logs.warn.byTitle('approve-scripts'), [],
+    'no warning; bundled deps are excluded upstream')
   // Ensure no policy entry was written.
   const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8'))
   t.notOk(pkg.allowScripts, 'no allowScripts written')

test/lib/commands/edit.js

@@ -22,6 +22,11 @@ const spawk = tspawk(t)
 const npmConfig = {
   config: {
     'ignore-scripts': false,
+    // Phase 2 gates dependency install scripts by default. `npm edit`
+    // rebuilds the edited package via `npm rebuild`, which honors the
+    // allowScripts gate, so opt every script in for these tests to exercise
+    // the editor -> rebuild -> install-script flow.
+    'dangerously-allow-all-scripts': true,
     editor: 'testeditor',
     'script-shell': process.platform === 'win32' ? process.env.COMSPEC : 'sh',
   },