fix(arborist): gate allowScripts on inDepBundle, not inBundle (#9679)#9684
Open
Sanjays2402 wants to merge 1 commit into
Open
fix(arborist): gate allowScripts on inDepBundle, not inBundle (#9679)#9684Sanjays2402 wants to merge 1 commit into
Sanjays2402 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #9679.
isScriptAllowedandcollectUnreviewedScriptswere keying offnode.inBundle. That bit is also true for deps the root project lists in its ownbundleDependencies, 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:nullfromisScriptAllowedfor a root-bundled dep, so the user could not add anallowScriptsentry that would actually match, andcollectUnreviewedScripts, so it never appeared in the "pending review" output or the--strict-allow-scriptspreflight.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:Node(workspaces/arborist/lib/node.js), it readsinDepBundledirectly. That getter walksgetBundler()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.IsolatedNode(linked / isolated mode),inDepBundleis hardcoded tofalsebecause the class does not track root-vs-dep bundling. To preserve the existing security gate (no allowlisting of bundledIsolatedNodes, since their identity comes from the bundling tarball), the predicate falls back toinBundlewhengetBundler()isnull. RealNode'sgetBundler()returns the bundling parent (the root, for root-bundled), so this discriminator does not affect real-Node semantics.inDepBundleis also used bydiff.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 inscript-allowed.jsinstead.lib/commands/rebuild.js'sinBundlecheck is left alone: that one gatesnpm 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:inDepBundle: truefixtures, since the dep-bundled case is what the security comment is actually about.root-bundled deps remain allowlistable (npm/cli#9679)test asserts that a node withinBundle: true, inDepBundle: falseand a registry resolved URL: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 blockednow also assertsinDepBundle === falseon the IsolatedNode and verifies thegetBundler()===nullfallback keeps the gate closed.workspaces/arborist/test/unreviewed-scripts.js:inDepBundle: true(the dep-bundled case).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
latestand pass with this patch. Rantest/script-allowed.js,test/unreviewed-scripts.js,test/node.js,test/isolated-mode.js, andtest/arborist/rebuild.jslocally with the patch applied: all green. (The pre-existingtest/arborist/reify.jsfailures inglobal style/global/global install ignores a per-call linked strategyreproduce on unpatchedlatest, so they are not from this change.) Coverage for the two touched lib files stays at 100% lines/branches/functions/statements.Lint clean (
eslinton 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.