feat: allowScripts tooling and inBundle hardening (#9483)

Backport of #9480 to `release/v11`.

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

Author: github-actions[bot]

lib/commands/rebuild.js

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

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

@@ -1,59 +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, includeWhenIgnored = false }) => {
-  const ignoreScripts = !!arb.options?.ignoreScripts
-  const dangerouslyAllowAll = !!npm?.flatOptions?.dangerouslyAllowAllScripts
-
-  // With ignore-scripts set, no scripts run, so execution callers
-  // (install, rebuild, strict preflight) bail out here. approve/deny pass
-  // includeWhenIgnored so they keep listing unreviewed packages, which is
-  // what you need to move from a blanket ignore-scripts to an allowlist.
-  // Listing never runs anything.
-  if ((ignoreScripts && !includeWhenIgnored) || 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

test/lib/commands/approve-scripts.js

@@ -397,10 +397,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({
@@ -457,7 +457,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 => {
@@ -517,7 +518,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({
@@ -566,8 +567,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

@@ -306,3 +306,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

@@ -149,46 +149,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({
@@ -252,7 +212,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',
@@ -265,13 +225,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

@@ -40,9 +40,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,
@@ -372,7 +373,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

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

workspaces/arborist/lib/script-allowed.js

@@ -24,13 +24,13 @@ const versionFromTgz = require('./version-from-tgz.js')
 //     resolved committish
 
 const isScriptAllowed = (node, policy) => {
-  // Bundled dependencies cannot be allowlisted in Phase 1. The RFC defers
-  // allowlisting them to a follow-up RFC because matching by name@version
-  // from the bundled tarball would reintroduce manifest confusion (a
-  // bundled tarball can claim any name and version). Returning null here
-  // marks bundled deps as unreviewed regardless of any policy entries, so
-  // their install scripts surface in the Phase 1 advisory warning and
-  // (eventually) get blocked at the install-time gate.
+  // Bundled dependencies never run their install scripts and cannot be
+  // allowlisted. Matching by name@version from the bundled tarball would
+  // reintroduce manifest confusion (a bundled tarball can claim any name
+  // and version). Returning null marks them as not-allowed regardless of
+  // any policy entry, so their install scripts are blocked by the
+  // install-time gate. A package that needs a bundled dep's script must
+  // forward it as one of its own lifecycle scripts.
   if (node.inBundle) {
     return null
   }
@@ -319,11 +319,11 @@ const isRegistryNode = (node) => {
   return /^https?:\/\/[^/]+\/.+\/-\/[^/]+-\d/.test(node.resolved)
 }
 
-// Trusted display identity for human-facing output (`npm install`
-// advisory, `npm approve-scripts --allow-scripts-pending`). Same idea as
-// getTrustedRegistryIdentity, but for DISPLAY only — version falls back
-// to node.version when the URL doesn't carry one. Must never be used
-// for policy matching.
+// Trusted display identity for human-facing output (the `npm install`
+// blocked-scripts summary and `npm approve-scripts --allow-scripts-pending`).
+// Same as getTrustedRegistryIdentity, but for display only: version
+// falls back to node.version when the URL doesn't carry one. Do not
+// use for policy matching.
 const trustedDisplay = (node) => {
   const trusted = getTrustedRegistryIdentity(node)
   /* istanbul ignore next: defensive fallbacks for nodes without name/version */