Skip to content

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

Open
Sanjays2402 wants to merge 1 commit into
npm:latestfrom
Sanjays2402:fix/issue-9679
Open

fix(arborist): gate allowScripts on inDepBundle, not inBundle (#9679)#9684
Sanjays2402 wants to merge 1 commit into
npm:latestfrom
Sanjays2402:fix/issue-9679

Conversation

@Sanjays2402

Copy link
Copy Markdown

Description

Fixes #9679.

isScriptAllowed and collectUnreviewedScripts were keying off node.inBundle. That bit is also true for deps the root project lists in its own bundleDependencies, but those deps are still fetched from the registry at install time, run their install scripts during reify, and have a trusted resolved URL (no manifest confusion). The gate was therefore:

  • always returning null from isScriptAllowed for a root-bundled dep, so the user could not add an allowScripts entry that would actually match, and
  • skipping the same node in collectUnreviewedScripts, so it never appeared in the "pending review" output or the --strict-allow-scripts preflight.

The net effect, as described in the issue, was that install scripts were silently dropped for root-bundled deps with no way to surface or approve them.

Changes

Both call sites now use a new isBundledByDependency(node) predicate:

  • For real Node (workspaces/arborist/lib/node.js), it reads inDepBundle directly. That getter walks getBundler() and only returns true when the bundler is not the root, which is exactly the "bundled inside another package's tarball" case the security comment was trying to describe.
  • For IsolatedNode (linked / isolated mode), inDepBundle is hardcoded to false because the class does not track root-vs-dep bundling. To preserve the existing security gate (no allowlisting of bundled IsolatedNodes, since their identity comes from the bundling tarball), the predicate falls back to inBundle when getBundler() is null. Real Node's getBundler() returns the bundling parent (the root, for root-bundled), so this discriminator does not affect real-Node semantics.

inDepBundle is also used by diff.js, edge.js, reify.js, etc. in real-Node code paths, so changing the IsolatedNode getter itself would have regressed isolated-mode reify; that's why the discriminator lives in script-allowed.js instead.

lib/commands/rebuild.js's inBundle check is left alone: that one gates npm rebuild <name> from targeting a bundled directory by name, which is a separate concern from script approval.

Test Coverage

workspaces/arborist/test/script-allowed.js:

  • Existing "bundled deps cannot be allowlisted" tests are renamed to "dep-bundled deps..." and switched to inDepBundle: true fixtures, since the dep-bundled case is what the security comment is actually about.
  • New root-bundled deps remain allowlistable (npm/cli#9679) test asserts that a node with inBundle: true, inDepBundle: false and a registry resolved URL:
    • matches a name-only allow entry,
    • matches a versioned allow entry,
    • is blocked by a deny entry, and
    • still returns null (unreviewed) when no policy entry covers it, so its scripts stay blocked until the user explicitly approves them.
  • isolated mode (linked): bundled IsolatedNode is blocked now also asserts inDepBundle === false on the IsolatedNode and verifies the getBundler()===null fallback keeps the gate closed.

workspaces/arborist/test/unreviewed-scripts.js:

  • The "skips ... bundled nodes" fixture is updated to inDepBundle: true (the dep-bundled case).
  • New root-bundled deps are still listed as pending (npm/cli#9679) test asserts a root-bundled dep with an install script shows up in the unreviewed list with the right script content.

Both regression tests were verified to fail on latest and pass with this patch. Ran test/script-allowed.js, test/unreviewed-scripts.js, test/node.js, test/isolated-mode.js, and test/arborist/rebuild.js locally with the patch applied: all green. (The pre-existing test/arborist/reify.js failures in global style / global / global install ignores a per-call linked strategy reproduce on unpatched latest, so they are not from this change.) Coverage for the two touched lib files stays at 100% lines/branches/functions/statements.

Lint clean (eslint on the four changed files).

AI Disclosure

This change was written with the assistance of Claude. I have reviewed the diff and the tests and take responsibility for the code.

Deps listed in the root project's bundleDependencies are still fetched
from the registry at install time, run their install scripts, and have
a trusted resolved URL (no manifest confusion). The script-approval
gate was keying off node.inBundle, which is also true for root-bundled
deps, so isScriptAllowed always returned null and collectUnreviewedScripts
skipped them: the user could not approve the scripts, and they were
silently blocked without surfacing in the pending list.

Switch both call sites to a new isBundledByDependency predicate. For
real Node it reads inDepBundle directly (already the right bit). For
IsolatedNode (linked / isolated mode), which doesn't track root-vs-dep
bundling and hardcodes inDepBundle=false, fall back to inBundle when
getBundler() returns null so the existing isolated-mode security gate
(no allowlisting of bundled IsolatedNodes) is preserved.

Regression tests cover both directions and run against both real Node
and IsolatedNode fixtures.
@Sanjays2402 Sanjays2402 requested review from a team as code owners June 28, 2026 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] InBundle vs InDepBundle in approve scripts functionality

1 participant