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

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) {

workspaces/arborist/test/script-allowed.js

@@ -415,18 +415,19 @@ t.test('isRegistryNode — arborist isRegistryDependency true accepts even unusu
   t.equal(isScriptAllowed(arboristNode, { 'trusted@1.0.0': true }), true)
 })
 
-t.test('bundled deps cannot be allowlisted (never run)', async t => {
-  // Bundled dependencies have inBundle=true and no independent resolved
-  // URL. They can never be allowlisted because matching by name@version
-  // from the bundled tarball would reintroduce manifest confusion. They
-  // always return null, and their install scripts never run.
+t.test('dep-bundled deps cannot be allowlisted (never run)', async t => {
+  // Dependencies bundled inside another package's tarball have
+  // inDepBundle=true and no independent resolved URL. They can never be
+  // allowlisted because matching by name@version from the bundled
+  // tarball would reintroduce manifest confusion. They always return
+  // null, and their install scripts never run.
 
   const bundled = {
     name: 'bundled-pkg',
     packageName: 'bundled-pkg',
     version: '1.0.0',
     resolved: undefined,
-    inBundle: true,
+    inDepBundle: true,
   }
 
   // Name-only allow: must NOT match a bundled dep.
@@ -440,33 +441,65 @@ t.test('bundled deps cannot be allowlisted (never run)', async t => {
   t.equal(isScriptAllowed(bundled, null), null)
 })
 
-t.test('bundled deps: deny entry does not match either (returns null, not false)', async t => {
-  // A deny entry doesn't apply to bundled deps because they're outside
-  // the policy scope entirely. They're blocked because they never run,
-  // not via a policy entry.
+t.test('dep-bundled deps: deny entry does not match either (returns null, not false)', async t => {
+  // A deny entry doesn't apply to dep-bundled deps because they're
+  // outside the policy scope entirely. They're blocked because they
+  // never run, not via a policy entry.
   const bundled = {
     name: 'bundled-pkg',
     packageName: 'bundled-pkg',
     version: '1.0.0',
     resolved: undefined,
-    inBundle: true,
+    inDepBundle: true,
   }
   t.equal(isScriptAllowed(bundled, { 'bundled-pkg': false }), null)
 })
 
-t.test('bundled dep with resolved field is still rejected', async t => {
-  // Defensive: even if a bundled dep somehow has a resolved URL, the
-  // inBundle flag wins over identity matching.
+t.test('dep-bundled dep with resolved field is still rejected', async t => {
+  // Defensive: even if a dep-bundled dep somehow has a resolved URL,
+  // the inDepBundle flag wins over identity matching.
   const bundledWithResolved = {
     name: 'pkg',
     packageName: 'pkg',
     version: '1.0.0',
     resolved: 'https://registry.npmjs.org/pkg/-/pkg-1.0.0.tgz',
-    inBundle: true,
+    inDepBundle: true,
   }
   t.equal(isScriptAllowed(bundledWithResolved, { 'pkg@1.0.0': true }), null)
 })
 
+t.test('root-bundled deps remain allowlistable (npm/cli#9679)', async t => {
+  // The root project's bundleDependencies list only controls what ships
+  // in the root's published tarball. At install time those deps are
+  // fetched from the registry like any other direct dep, so they:
+  //   - have a trusted resolved URL (no manifest confusion),
+  //   - actually run install scripts during reify, and
+  //   - must therefore be reviewable via allowScripts.
+  //
+  // Real Nodes report inBundle=true / inDepBundle=false for root-bundled
+  // deps. The gate must key off inDepBundle, not inBundle, or the user
+  // can never approve their own bundled deps' install scripts and the
+  // scripts are silently skipped.
+  const rootBundled = {
+    name: 'pkg',
+    packageName: 'pkg',
+    version: '1.0.0',
+    resolved: 'https://registry.npmjs.org/pkg/-/pkg-1.0.0.tgz',
+    inBundle: true,
+    inDepBundle: false,
+    isRegistryDependency: true,
+  }
+
+  t.equal(isScriptAllowed(rootBundled, { pkg: true }), true,
+    'name-only allow matches a root-bundled dep')
+  t.equal(isScriptAllowed(rootBundled, { 'pkg@1.0.0': true }), true,
+    'versioned allow matches a root-bundled dep')
+  t.equal(isScriptAllowed(rootBundled, { pkg: false }), false,
+    'deny entry blocks a root-bundled dep')
+  t.equal(isScriptAllowed(rootBundled, null), null,
+    'unreviewed root-bundled dep returns null (blocks scripts, surfaces in pending)')
+})
+
 t.test('inBundle: false does not affect normal matching', async t => {
   // Sanity check: explicit inBundle: false behaves identically to absent.
   const normal = {
@@ -482,9 +515,9 @@ t.test('inBundle: false does not affect normal matching', async t => {
 t.test('isolated mode (linked): bundled IsolatedNode is blocked', async t => {
   // Regression guard: in isolated/linked mode the gate runs against
   // IsolatedNode instances, not real Nodes. A bundled IsolatedNode must
-  // report inBundle so the gate blocks it even when its resolved URL
+  // report inDepBundle so the gate blocks it even when its resolved URL
   // looks like a registry identity that a name entry would otherwise
-  // match. Without inBundle on IsolatedNode the guard is silently
+  // match. Without inDepBundle on IsolatedNode the guard is silently
   // skipped and the bundled install script runs.
   const { IsolatedNode } = require('../lib/isolated-classes.js')
 
@@ -498,7 +531,10 @@ t.test('isolated mode (linked): bundled IsolatedNode is blocked', async t => {
   })
 
   t.equal(bundled.inBundle, true, 'bundled IsolatedNode reports inBundle')
-  t.equal(isScriptAllowed(bundled, { 'bundled-pkg': true }), null)
+  t.equal(bundled.inDepBundle, false,
+    'IsolatedNode inDepBundle is always false (class does not track it)')
+  t.equal(isScriptAllowed(bundled, { 'bundled-pkg': true }), null,
+    'isolated bundled node is still blocked via getBundler()===null fallback')
   t.equal(isScriptAllowed(bundled, { 'bundled-pkg@1.0.0': true }), null)
 
   const store = new IsolatedNode({
@@ -512,6 +548,7 @@ t.test('isolated mode (linked): bundled IsolatedNode is blocked', async t => {
   })
 
   t.equal(store.inBundle, false, 'external store IsolatedNode is not bundled')
+  t.equal(store.inDepBundle, false, 'external store IsolatedNode is not dep-bundled')
   t.equal(isScriptAllowed(store, { 'store-pkg@1.0.0': true }), true)
 })
 

workspaces/arborist/test/unreviewed-scripts.js

@@ -28,6 +28,7 @@ const node = ({
   isWorkspace = false,
   isLink = false,
   inBundle = false,
+  inDepBundle = false,
   inert = false,
   resolved,
 } = {}) => ({
@@ -40,6 +41,7 @@ const node = ({
   isWorkspace,
   isLink,
   inBundle,
+  inDepBundle,
   inert,
   isRegistryDependency: true,
   package: { name, version, scripts },
@@ -71,19 +73,41 @@ t.test('collectUnreviewedScripts', async t => {
     t.strictSame(await collectUnreviewedScripts(), [])
   })
 
-  t.test('skips project root, workspace, linked, and bundled nodes', async t => {
+  t.test('skips project root, workspace, linked, and dep-bundled nodes', async t => {
     const result = await collectUnreviewedScripts({
       tree: tree([
         node({ name: 'root', scripts: { install: 'x' }, isProjectRoot: true }),
         node({ name: 'ws', scripts: { install: 'x' }, isWorkspace: true }),
         node({ name: 'linked', scripts: { install: 'x' }, isLink: true }),
-        node({ name: 'bundled', scripts: { install: 'x' }, inBundle: true }),
+        node({ name: 'bundled', scripts: { install: 'x' }, inDepBundle: true }),
       ]),
       policy: null,
     })
     t.strictSame(result, [])
   })
 
+  t.test('root-bundled deps are still listed as pending (npm/cli#9679)', async t => {
+    // Deps listed in the root's bundleDependencies are fetched from the
+    // registry at install time, run their install scripts, and so must
+    // still surface in the unreviewed/pending list so the user can
+    // approve them. inBundle is true for these (any bundler), but
+    // inDepBundle is false (the bundler is the root project).
+    const result = await collectUnreviewedScripts({
+      tree: tree([
+        node({
+          name: 'root-bundled',
+          scripts: { install: 'build.js' },
+          inBundle: true,
+          inDepBundle: false,
+        }),
+      ]),
+      policy: null,
+    })
+    t.equal(result.length, 1, 'root-bundled dep with install script is pending')
+    t.equal(result[0].node.name, 'root-bundled')
+    t.match(result[0].scripts, { install: 'build.js' })
+  })
+
   t.test('skips inert (platform/engine-incompatible) optional nodes', async t => {
     const result = await collectUnreviewedScripts({
       tree: tree([node({ name: 'fsevents', scripts: { install: 'x' }, inert: true })]),