Skip to content

fix(arborist): skip extraneous orphans in strict-allow-scripts gate #9683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sanjays2402 wants to merge 3 commits into
base: latest
Choose a base branch
from
+47 −0
Open

fix(arborist): skip extraneous orphans in strict-allow-scripts gate #9683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a8821c
fix(arborist): skip extraneous orphans in strict-allow-scripts gate
Sanjays2402 Jun 28, 2026
735428d
fix(arborist): tidy review comments to single-line per feedback
Sanjays2402 Jul 2, 2026
5e1c6da
test: cover extraneous strict allow-scripts nodes
Sanjays2402 Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions test/lib/utils/strict-allow-scripts-preflight.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const node = ({
name = 'pkg',
version = '1.0.0',
scripts = { install: 'node-gyp rebuild' },
extraneous = false,
} = {}) => ({
name,
resolved: `https://registry.npmjs.org/${name}/-/${name}-${version}.tgz`,
Expand All @@ -17,6 +18,7 @@ const node = ({
isProjectRoot: false,
isWorkspace: false,
isLink: false,
extraneous,
package: { name, version, scripts },
})

Expand Down Expand Up @@ -89,6 +91,20 @@ t.test('passes when the only unreviewed node is inert (platform-incompatible opt
t.pass('no error thrown for inert node')
})

t.test('passes when the only unreviewed node is an extraneous orphan', async t => {
// Lockfile-only workspace orphans are pruned before scripts run and must not
// make the command-level strict preflight reject an install (npm/cli#9680).
const orphan = node({ name: 'core-js', extraneous: true })
orphan.isRegistryDependency = false
const arb = makeArb({ ideal: tree([orphan]) })
await preflight({
arb,
npm: { flatOptions: { strictAllowScripts: true } },
idealTreeOpts: {},
})
t.pass('no error thrown for extraneous orphan')
})

t.test('passes when all install-script nodes are explicitly approved', async t => {
const arb = makeArb({
ideal: tree([node({ name: 'canvas' })]),
Expand Down
5 changes: 5 additions & 0 deletions workspaces/arborist/lib/unreviewed-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ const collectUnreviewedScripts = async ({
// must not be flagged (npm/cli#9562).
continue
}
if (node.extraneous) {

@JamieMagee JamieMagee Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the reported install case, but this collector also walks the actual tree for npm install-scripts ls and strict npm rebuild.

I tested an extraneous package already present in node_modules. With this change, install-scripts ls reports nothing and strict rebuild succeeds instead of throwing ESTRICTALLOWSCRIPTS. The lower-level gate still blocks the script, but strict mode silently passes.

Could we limit this skip to reify preflights where pruning is enabled and the node will not be installed?

@JamieMagee JamieMagee Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also skips extraneous packages when unreviewedScripts() receives the installed actualTree. npm rebuild checks that tree before rebuilding, and extraneous packages are still rebuild candidates. For example, remove a package from package.json but leave it in node_modules, then run npm rebuild --strict-allow-scripts. The command exits successfully without rebuilding the package, and npm install-scripts ls does not report its script. Could we limit this skip to ideal-tree nodes that reify will prune?

// Extraneous nodes are orphans pruned before reify runs any install script, so their scripts never execute.
// buildIdealTree drops top-level orphans but can retain one nested in a workspace's node_modules, so this gate must skip them (npm/cli#9680).
continue
}

const verdict = isScriptAllowed(node, resolvedPolicy)
if (verdict === true || verdict === false) {
Expand Down
26 changes: 26 additions & 0 deletions workspaces/arborist/test/unreviewed-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const node = ({
isLink = false,
inBundle = false,
inert = false,
extraneous = false,
resolved,
} = {}) => ({
name,
Expand All @@ -41,6 +42,7 @@ const node = ({
isLink,
inBundle,
inert,
extraneous,
isRegistryDependency: true,
package: { name, version, scripts },
})
Expand Down Expand Up @@ -92,6 +94,30 @@ t.test('collectUnreviewedScripts', async t => {
t.strictSame(result, [])
})

t.test('skips extraneous (orphan) registry nodes', async t => {

@JamieMagee JamieMagee Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we cover this at the command level? We need one test for the lockfile-only orphan from #9680, plus a regression test confirming that an extraneous package on disk is still listed and still fails strict npm rebuild. These unit tests only verify the unconditional filter, so they miss the actual-tree behavior.

@Sanjays2402 Sanjays2402 Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the distinction. The unconditional collector skip is too broad: it fixes the lockfile-only ideal-tree orphan, but it also hides an extraneous package from the actual-tree install-scripts ls and strict npm rebuild paths.

I pushed 5e1c6da with a RED/GREEN preflight regression for the orphan, but that test does not cover the actual-tree behavior, so it does not close your full request. The fix needs to carry the prune/reify context into the strict install/ci preflight and only skip extraneous nodes there; ls and rebuild should continue to report and block them. I will rework the collector around that boundary rather than claim this version is complete.

// An extraneous orphan is pruned before reify runs any install script, so it must not gate the install even when the policy neither allows nor denies it (npm/cli#9680).
const result = await collectUnreviewedScripts({
tree: tree([
node({ name: 'orphan', scripts: { install: 'x' }, extraneous: true }),
]),
policy: null,
})
t.strictSame(result, [])
})

t.test('skips extraneous nodes even when policy denies by name', async t => {
// Same orphan, but with a name-only deny entry.
// An extraneous registry node usually has no resolved URL for the matcher to verify, so the deny misses and it falls through to "unreviewed".
// It still must not gate the install (npm/cli#9680).
const orphan = node({ name: 'esbuild', scripts: { install: 'x' }, extraneous: true })
orphan.isRegistryDependency = false
const result = await collectUnreviewedScripts({
tree: tree([orphan]),
policy: { esbuild: false },
})
t.strictSame(result, [])
})

t.test('skips nodes with no install-relevant scripts', async t => {
const result = await collectUnreviewedScripts({
tree: tree([node({ scripts: { test: 'jest' } })]),
Expand Down