feat: allowScripts tooling and inBundle hardening (backportable)

Behavior-neutral additive tooling split out of #9424 so it can land on
v11 without the v12 default-deny flip:

- arborist: add collectUnreviewedScripts() + strictAllowScriptsError
  (ESTRICTALLOWSCRIPTS) helpers in unreviewed-scripts.js
- arborist: isScriptAllowed() returns null for bundled deps; propagate
  inBundle through isolated reifier / isolated-classes
- libnpmexec: opt-in strict-allow-scripts preflight (only under
  --strict-allow-scripts); no default behavior change
- cli: rewrite check-allow-scripts as a wrapper over
  collectUnreviewedScripts; exclude bundled deps from rebuild/runAll
- tests for all of the above

No default install-script behavior changes; the default-deny gate stays
in #9424 for v12.

Author: JamieMagee

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 })
@@ -84,6 +84,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

@@ -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/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

test/lib/commands/approve-scripts.js

@@ -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/rebuild.js

@@ -281,3 +281,49 @@ t.test('no advisory warning when allowScripts covers the package', async t => {
   await npm.exec('rebuild', [])
   t.strictSame(logs.warn.byTitle('rebuild'), [])
 })
+
+t.test('rebuild <name> never targets a bundled dependency', async t => {
+  const { npm, prefix: path } = await setupMockNpm(t, {
+    prefixDir: {
+      'package.json': JSON.stringify({
+        name: 'host',
+        version: '1.0.0',
+        dependencies: { parent: '1.0.0' },
+      }),
+      node_modules: {
+        parent: {
+          'index.js': '',
+          'package.json': JSON.stringify({
+            name: 'parent',
+            version: '1.0.0',
+            bundleDependencies: ['bcrypt'],
+            dependencies: { bcrypt: '1.0.0' },
+          }),
+          node_modules: {
+            bcrypt: {
+              'index.js': '',
+              'package.json': JSON.stringify({
+                name: 'bcrypt',
+                version: '1.0.0',
+                bin: 'index.js',
+                scripts: {
+                  install: "node -e \"require('fs').writeFileSync('ran', '')\"",
+                },
+              }),
+            },
+          },
+        },
+      },
+    },
+  })
+
+  const ranFile = resolve(path, 'node_modules/parent/node_modules/bcrypt/ran')
+  t.throws(() => fs.statSync(ranFile))
+
+  await npm.exec('rebuild', ['bcrypt'])
+
+  t.throws(
+    () => fs.statSync(ranFile),
+    'bundled bcrypt install script must not run'
+  )
+})

test/lib/utils/check-allow-scripts.js

@@ -135,46 +135,6 @@ t.test('prepare counts for non-registry sources only', async t => {
   t.equal(result[0].node.name, 'git-pkg')
 })
 
-t.test('detects synthetic node-gyp via binding.gyp runtime check', async t => {
-  const checkAllowScripts = mockCheck(t, {
-    '@npmcli/arborist/lib/install-scripts.js': async (n) => {
-      if (n.path === '/has-bindings') {
-        return { install: 'node-gyp rebuild' }
-      }
-      return {}
-    },
-  })
-
-  const result = await checkAllowScripts({
-    arb: arb({
-      nodes: [
-        node({ name: 'native', path: '/has-bindings' }),
-        node({ name: 'pure-js', path: '/no-bindings' }),
-      ],
-    }),
-    npm: { flatOptions: {} },
-  })
-  t.equal(result.length, 1)
-  t.equal(result[0].node.name, 'native')
-  t.strictSame(result[0].scripts, { install: 'node-gyp rebuild' })
-})
-
-t.test('skips node-gyp detection when gypfile is explicitly false', async t => {
-  // Mock returns no scripts to simulate the gypfile:false short-circuit
-  // inside getInstallScripts.
-  const checkAllowScripts = mockCheck(t, {
-    '@npmcli/arborist/lib/install-scripts.js': async () => ({}),
-  })
-
-  const result = await checkAllowScripts({
-    arb: arb({
-      nodes: [node({ name: 'opt-out', gypfile: false })],
-    }),
-    npm: { flatOptions: {} },
-  })
-  t.strictSame(result, [])
-})
-
 t.test('skips approved nodes', async t => {
   const checkAllowScripts = mockCheck(t)
   const result = await checkAllowScripts({
@@ -238,7 +198,7 @@ t.test('survives missing actualTree', async t => {
   t.strictSame(result, [])
 })
 
-t.test('bundled dep with install scripts is reported as unreviewed regardless of policy', async t => {
+t.test('bundled dep with install scripts is never reported (never runs, never pending)', async t => {
   const checkAllowScripts = mockCheck(t)
   const bundled = node({
     name: 'bundled-pkg',
@@ -251,13 +211,12 @@ t.test('bundled dep with install scripts is reported as unreviewed regardless of
   const result = await checkAllowScripts({
     arb: arb({
       nodes: [bundled],
-      // Policy explicitly allows the bundled name — the matcher should
-      // still return null and the walker should still flag the bundled
-      // dep as unreviewed.
+      // Even with an explicit allow entry, a bundled dep never runs its
+      // install scripts and is never counted as pending, so the walker
+      // must not flag it.
       allowScripts: { 'bundled-pkg': true },
     }),
     npm: { flatOptions: {} },
   })
-  t.equal(result.length, 1, 'bundled dep flagged despite explicit allow entry')
-  t.equal(result[0].node, bundled)
+  t.strictSame(result, [], 'bundled dep never flagged')
 })

workspaces/arborist/lib/arborist/isolated-reifier.js

@@ -38,9 +38,10 @@ module.exports = cls => class IsolatedReifier extends cls {
   #processedEdges = new Set()
   #workspaceProxies = new Map()
 
-  #generateChild (node, location, pkg, isInStore, root) {
+  #generateChild (node, location, pkg, isInStore, root, inBundle = false) {
     const newChild = new IsolatedNode({
       isInStore,
+      inBundle,
       location,
       name: node.packageName || node.name,
       optional: node.optional,
@@ -327,7 +328,7 @@ module.exports = cls => class IsolatedReifier extends cls {
     })
 
     bundledTree.nodes.forEach(node => {
-      this.#generateChild(node, node.location, node.pkg, false, root)
+      this.#generateChild(node, node.location, node.pkg, false, root, true)
     })
 
     bundledTree.edges.forEach(edge => {

workspaces/arborist/lib/isolated-classes.js

@@ -19,6 +19,7 @@ class IsolatedNode {
   integrity = null
   inventory = new IsolatedInventory()
   isInStore = false
+  inBundle = false
   linksIn = new Set()
   meta = { loadedFromDisk: false }
   optional = false
@@ -46,6 +47,9 @@ class IsolatedNode {
     if (options.isInStore) {
       this.isInStore = true
     }
+    if (options.inBundle) {
+      this.inBundle = true
+    }
     if (options.optional) {
       this.optional = true
     }