fix(arborist): gate allowScripts on inDepBundle, not inBundle (#9679)#9684
fix(arborist): gate allowScripts on inDepBundle, not inBundle (#9679)#9684Sanjays2402 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.
|
The predicate and both call sites are sound. I built real The problem is the change stops at the listing. |
| // 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 = { |
There was a problem hiding this comment.
This fixture is a plain object with no getBundler, so it only exercises the typeof getBundler !== 'function' fallback, not the real-Node getBundler() === root path that actually runs in production. I checked the real path by hand and it's fine, but nothing here would catch a regression on that branch. Could you add a case built from an actual Node with the root listing it in bundleDependencies? The IsolatedNode test already uses a real instance, so this is the last uncovered branch.
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.