fix(arborist): gate allowScripts on inDepBundle, not inBundle (#9679)

lib/utils/allow-scripts-cmd.js

@@ -2,7 +2,7 @@ const { log, output } = require('proc-log')
 const npa = require('npm-package-arg')
 const semver = require('semver')
 const pkgJson = require('@npmcli/package-json')
-const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
+const { trustedDisplay, isBundledByDependency } = require('@npmcli/arborist/lib/script-allowed.js')
 const getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js')
 const checkAllowScripts = require('./check-allow-scripts.js')
 const resolveAllowScripts = require('./resolve-allow-scripts.js')
@@ -188,9 +188,10 @@ class AllowScriptsCmd extends BaseCommand {
       output.standard('No packages with unreviewed install scripts.')
       return
     }
-    // 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.
+    // Dep-bundled dependencies never appear in `unreviewed` (checkAllowScripts
+    // skips them because they never run their install scripts and cannot be
+    // allowlisted); root-bundled deps do appear and are approved like any other
+    // reviewable dep. Either way there is nothing extra to filter here.
     const groups = this.groupByPackage(unreviewed.map(({ node }) => node))
     await this.writePolicyChanges(groups)
   }
@@ -219,15 +220,19 @@ class AllowScriptsCmd extends BaseCommand {
     // Match positional args against each node's trusted name. Registry deps
     // use the URL-derived name; non-registry deps fall back to the dependency
     // edge name. A version or range on the arg narrows the match to installed
-    // versions that satisfy it. Bundled deps are excluded for the same reason
-    // as --all. Args that match nothing are returned in `unmatched`.
+    // versions that satisfy it. Dep-bundled deps are excluded for the same
+    // reason as --all (they never run their install scripts and can't be
+    // allowlisted); root-bundled deps are kept, matching the inDepBundle gate
+    // used by collectUnreviewedScripts so `approve <name>` can allow the same
+    // packages the pending list tells the user to approve (npm/cli#9679). Args
+    // that match nothing are returned in `unmatched`.
     const matched = []
     const unmatched = []
     for (const arg of args) {
       const { name: wantName, range } = parsePositional(arg)
       const found = []
       for (const node of arb.actualTree.inventory.values()) {
-        if (node.isProjectRoot || node.isWorkspace || node.inBundle) {
+        if (node.isProjectRoot || node.isWorkspace || isBundledByDependency(node)) {
           continue
         }
         const { name, version } = trustedDisplay(node)
@@ -256,7 +261,7 @@ class AllowScriptsCmd extends BaseCommand {
     const groups = {}
     for (const node of nodes) {
       const key = nameKeyFor(node)
-      /* istanbul ignore if: callers prefilter via inBundle and trustedDisplay so untrusted nodes don't reach here */
+      /* istanbul ignore if: callers prefilter via isBundledByDependency and trustedDisplay so untrusted nodes don't reach here */
       if (!key) {
         log.warn(
           this.logTitle,
@@ -355,10 +360,14 @@ class AllowScriptsCmd extends BaseCommand {
 
       // Candidate install nodes (mirrors collectUnreviewedScripts), tagged with
       // whether each has install scripts so the classifier can tell "gone" from
-      // "no longer has scripts".
+      // "no longer has scripts". Uses isBundledByDependency (not node.inBundle)
+      // so root-bundled deps stay in the candidate set: they are reviewable and
+      // may hold an allowScripts entry the user just approved, and pruning them
+      // out would silently delete that entry (npm/cli#9679).
       const nodes = []
       for (const node of arb.actualTree.inventory.values()) {
-        if (node.isProjectRoot || node.isWorkspace || node.isLink || node.inBundle || node.inert) {
+        if (node.isProjectRoot || node.isWorkspace || node.isLink ||
+            isBundledByDependency(node) || node.inert) {
           continue
         }
         const scripts = await getInstallScripts(node)

test/lib/commands/approve-scripts.js

@@ -859,3 +859,73 @@ t.test('approve-scripts --all with only bundled deps has nothing to review', asy
   const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8'))
   t.notOk(pkg.allowScripts, 'no allowScripts written')
 })
+
+// Root-bundled deps (listed in the ROOT project's bundleDependencies) are
+// fetched from the registry at install time, run their install scripts, and
+// are reviewable — unlike deps bundled inside another package's tarball. The
+// command layer must approve them by name, mirroring the inDepBundle gate the
+// pending list / preflight use (npm/cli#9679). Before the fix, findNodesForArgs
+// filtered on node.inBundle (true for root-bundled deps too), so
+// `approve-scripts <name>` threw ENOMATCH even though `--all` and the pending
+// list surfaced the dep.
+const rootBundledFixture = () => ({
+  'package.json': JSON.stringify({
+    name: 'host',
+    version: '1.0.0',
+    dependencies: { 'root-bundled-dep': '*' },
+    bundleDependencies: ['root-bundled-dep'],
+  }),
+  'package-lock.json': JSON.stringify({
+    name: 'host',
+    version: '1.0.0',
+    lockfileVersion: 3,
+    requires: true,
+    packages: {
+      '': {
+        name: 'host',
+        version: '1.0.0',
+        dependencies: { 'root-bundled-dep': '*' },
+        bundleDependencies: ['root-bundled-dep'],
+      },
+      'node_modules/root-bundled-dep': {
+        version: '1.0.0',
+        resolved: 'https://registry.npmjs.org/root-bundled-dep/-/root-bundled-dep-1.0.0.tgz',
+        hasInstallScript: true,
+      },
+    },
+  }),
+  node_modules: {
+    'root-bundled-dep': {
+      'package.json': JSON.stringify({
+        name: 'root-bundled-dep',
+        version: '1.0.0',
+        scripts: { install: 'echo install' },
+      }),
+    },
+  },
+})
+
+t.test('approve-scripts <root-bundled-dep> by name is approved (npm/cli#9679)', async t => {
+  const { npm, prefix } = await _mockNpm(t, {
+    prefixDir: rootBundledFixture(),
+  })
+  // The bug: this threw ENOMATCH because findNodesForArgs skipped the node
+  // on node.inBundle. It must now match and write the policy entry.
+  await npm.exec('approve-scripts', ['root-bundled-dep'])
+
+  const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8'))
+  t.strictSame(pkg.allowScripts, { 'root-bundled-dep@1.0.0': true },
+    'root-bundled dep is approved by name, not rejected with ENOMATCH')
+})
+
+t.test('approve-scripts --all approves a root-bundled dep (npm/cli#9679)', async t => {
+  const { npm, prefix } = await _mockNpm(t, {
+    prefixDir: rootBundledFixture(),
+    config: { all: true },
+  })
+  await npm.exec('approve-scripts', [])
+
+  const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8'))
+  t.strictSame(pkg.allowScripts, { 'root-bundled-dep@1.0.0': true },
+    'root-bundled dep surfaces in --all and is approved')
+})

workspaces/arborist/lib/script-allowed.js

@@ -23,15 +23,72 @@ const versionFromTgz = require('./version-from-tgz.js')
 //   - git: match on hosted.ssh() plus a short-SHA prefix of the
 //     resolved committish
 
+// True when the node was bundled by some dependency (not by the root
+// project). The script-approval gate keys off this rather than
+// node.inBundle so the user can review and allow scripts in deps the
+// root explicitly bundles, while still blocking install scripts from
+// tarballs nested inside another package (where manifest-confusion makes
+// identity matching unsafe).
+//
+// Real Node (workspaces/arborist/lib/node.js): inDepBundle is a
+// getBundler() walk that excludes the root project, so it is the
+// trusted bit and we use it directly.
+//
+// IsolatedNode (workspaces/arborist/lib/isolated-classes.js): linked /
+// isolated mode constructs bundled IsolatedNodes by walking from a
+// bundle entry point and the class does not track root-vs-dep bundling,
+// so its inDepBundle always returns false. To preserve the existing
+// security gate (no manifest confusion via isolated bundled nodes), we
+// treat any inBundle IsolatedNode as dep-bundled. The discriminator
+// versus a real-Node root-bundled dep is getBundler(): real Nodes walk
+// up to the root and return it; IsolatedNode.getBundler returns null.
+const isBundledByDependency = (node) => {
+  if (!node) {
+    return false
+  }
+  if (node.inDepBundle) {
+    return true
+  }
+  if (!node.inBundle) {
+    return false
+  }
+  // inBundle is true and inDepBundle is false. For real Nodes this means
+  // root-bundled (allow review). For IsolatedNode it means "bundled in
+  // isolated mode, root/dep distinction not tracked" (must block). The
+  // discriminator is whether getBundler() can identify a real bundling
+  // parent; IsolatedNode.getBundler returns null.
+  if (typeof node.getBundler !== 'function') {
+    return false
+  }
+  return node.getBundler() === null
+}
+
 const isScriptAllowed = (node, policy) => {
-  // 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) {
+  // Dependencies bundled inside another package's tarball 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.
+  //
+  // Real Nodes: inDepBundle is the trusted bit. inBundle is also true
+  // when the root project itself bundles a dep, but root-bundled deps
+  // are still fetched from the registry at install time, have a trusted
+  // resolved URL, run their install scripts, and so must be reviewable
+  // (npm/cli#9679). Keying off inBundle would silently block them and
+  // hide them from the pending list.
+  //
+  // IsolatedNodes (linked/isolated mode): the class can't tell root-
+  // bundled from dep-bundled, so its inDepBundle getter always returns
+  // false. Anything an IsolatedNode reports as inBundle came through
+  // the isolated reifier's bundled tree and must keep being blocked, or
+  // a bundled tarball's identity match would bypass the gate. The
+  // getBundler()===null discriminator below catches that case without
+  // collapsing the root-bundled / dep-bundled distinction elsewhere
+  // (diff/reify/edge all key off inDepBundle in real-Node semantics).
+  if (isBundledByDependency(node)) {
     return null
   }
 
@@ -382,3 +439,4 @@ module.exports.isExactVersionDisjunction = isExactVersionDisjunction
 module.exports.getTrustedRegistryIdentity = getTrustedRegistryIdentity
 module.exports.resolvedSourceSpecs = resolvedSourceSpecs
 module.exports.trustedDisplay = trustedDisplay
+module.exports.isBundledByDependency = isBundledByDependency

workspaces/arborist/lib/unreviewed-scripts.js

@@ -1,4 +1,4 @@
-const isScriptAllowed = require('./script-allowed.js')
+const { isScriptAllowed, isBundledByDependency } = require('./script-allowed.js')
 const getInstallScripts = require('./install-scripts.js')
 
 // Shared allowScripts walk used by both the npm CLI
@@ -42,12 +42,18 @@ const collectUnreviewedScripts = async ({
       // Linked workspace dependencies are managed by the workspace owner.
       continue
     }
-    if (node.inBundle) {
-      // Bundled dependencies never run their install scripts and cannot be
-      // allowlisted, so they are never "pending". Skipping them keeps them
-      // out of the advisory warning and out of strict-allow-scripts. A
-      // package that needs a bundled dep's script must forward it as one of
-      // its own lifecycle scripts.
+    if (isBundledByDependency(node)) {
+      // Dependencies bundled inside another package's tarball never run
+      // their install scripts and cannot be allowlisted, so they are never
+      // "pending". Skipping them keeps them out of the advisory warning
+      // and out of strict-allow-scripts. A package that needs a bundled
+      // dep's script must forward it as one of its own lifecycle scripts.
+      //
+      // Uses isBundledByDependency (not node.inBundle): the root
+      // project's bundleDependencies list only controls what ships in
+      // the root's published tarball; at install time those deps come
+      // from the registry like any other direct dep, so their install
+      // scripts must still surface as pending review (npm/cli#9679).
       continue
     }
     if (node.inert) {