From 2995e490d1659133bd286238a3a2344f233f0d2c Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Wed, 24 Jun 2026 16:12:35 -0700 Subject: [PATCH 1/3] fix(arborist): fail-closed version-pinned allowScripts deny without resolved --- workspaces/arborist/lib/script-allowed.js | 21 ++++++--- workspaces/arborist/test/script-allowed.js | 51 ++++++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/script-allowed.js b/workspaces/arborist/lib/script-allowed.js index 89e24339791f1..ee89ce647cadb 100644 --- a/workspaces/arborist/lib/script-allowed.js +++ b/workspaces/arborist/lib/script-allowed.js @@ -43,7 +43,9 @@ const isScriptAllowed = (node, policy) => { let anyDeny = false for (const [key, value] of Object.entries(policy)) { - if (!matches(node, key)) { + // Pass deny intent so matchRegistry can fail closed on an unverifiable + // version: a deny still blocks, an allow stays refused. + if (!matches(node, key, value === false)) { continue } if (value === false) { @@ -66,7 +68,7 @@ const isScriptAllowed = (node, policy) => { return null } -const matches = (node, key) => { +const matches = (node, key, failClosed) => { let parsed try { parsed = npa(key) @@ -78,7 +80,7 @@ const matches = (node, key) => { case 'tag': case 'range': case 'version': - return matchRegistry(node, parsed) + return matchRegistry(node, parsed, failClosed) case 'git': return matchGit(node, parsed) case 'file': @@ -132,7 +134,7 @@ const resolvedSourceSpecs = (node) => { return specs } -const matchRegistry = (node, parsed) => { +const matchRegistry = (node, parsed, failClosed) => { // If this node is not a registry dep, refuse the match. A registry-style // key (`pkg`, `pkg@1`, `pkg@1 || 2`) must not match a tarball or git node // even if their names happen to coincide. @@ -168,9 +170,14 @@ const matchRegistry = (node, parsed) => { if (parsed.fetchSpec === '*' || parsed.rawSpec === '' || parsed.rawSpec === '*') { return true } - if (!trusted.version || !isExactVersionDisjunction(parsed.fetchSpec)) { + if (!isExactVersionDisjunction(parsed.fetchSpec)) { return false } + // Unverifiable version (omit-lockfile-registry-resolved): a deny blocks, + // an allow is refused. + if (!trusted.version) { + return failClosed + } return semver.satisfies(trusted.version, parsed.fetchSpec, { loose: true }) } @@ -178,6 +185,10 @@ const matchRegistry = (node, parsed) => { /* istanbul ignore else: parsed.type at this point is always 'version'; the istanbul-ignored fallback below handles the impossible case. */ if (parsed.type === 'version') { + // Unverifiable version: a deny blocks, an allow is refused. + if (!trusted.version) { + return failClosed + } return trusted.version === parsed.fetchSpec } diff --git a/workspaces/arborist/test/script-allowed.js b/workspaces/arborist/test/script-allowed.js index 615ffaba7140e..218ccf1e28888 100644 --- a/workspaces/arborist/test/script-allowed.js +++ b/workspaces/arborist/test/script-allowed.js @@ -229,6 +229,57 @@ t.test('omitLockfileRegistryResolved: name-only match via edges; version-pinned t.end() }) +t.test('omitLockfileRegistryResolved: version-pinned deny fails closed', t => { + // No resolved URL means no trusted version. A version-pinned deny must + // still block (fail closed); a matching allow stays refused. + const omitted = () => ({ + name: 'evilpkg', + packageName: 'evilpkg', + version: '1.0.0', + resolved: undefined, + location: 'node_modules/evilpkg', + edgesIn: new Set([{ name: 'evilpkg', spec: '^1.0.0' }]), + }) + + // Exact-version deny: blocked. + t.equal(isScriptAllowed(omitted(), { 'evilpkg@1.0.0': false }), false, + 'version-pinned deny blocks even without a trusted version') + // Exact-disjunction deny: blocked. + t.equal(isScriptAllowed(omitted(), { 'evilpkg@1.0.0 || 2.0.0': false }), false, + 'exact-disjunction deny blocks even without a trusted version') + // The exploit: name-only allow + version-pinned deny. Deny still wins. + t.equal(isScriptAllowed(omitted(), { evilpkg: true, 'evilpkg@1.0.0': false }), false, + 'deny wins over a name-only allow when the version is unverifiable') + + // Allow stays strict: an unverifiable version is never authorized. + t.equal(isScriptAllowed(omitted(), { 'evilpkg@1.0.0 || 2.0.0': true }), null, + 'exact-disjunction allow is refused without a trusted version') + + // Name mismatch: fail-closed must not over-match a different package. + t.equal(isScriptAllowed(omitted(), { 'otherpkg@1.0.0': false }), null, + 'a version-pinned deny for a different name does not match') + + t.end() +}) + +t.test('omitLockfileRegistryResolved + alias: version-pinned deny fails closed', t => { + // `"trusted": "npm:naughty@1.0.0"`, resolved omitted. A deny on the + // underlying name must block; the alias name authorizes nothing. + const aliasOmitted = { + name: 'trusted', + packageName: 'naughty', + version: '1.0.0', + resolved: undefined, + location: 'node_modules/trusted', + edgesIn: new Set([{ name: 'trusted', spec: 'npm:naughty@1.0.0' }]), + } + t.equal(isScriptAllowed(aliasOmitted, { 'naughty@1.0.0': false }), false, + 'underlying-name version deny blocks the aliased package') + t.equal(isScriptAllowed(aliasOmitted, { 'trusted@1.0.0': false }), null, + 'alias-name version deny does not match the underlying package') + t.end() +}) + t.test('omitLockfileRegistryResolved + alias: location is ignored; underlying name wins', t => { // Consumer's package.json has `"trusted": "npm:naughty@1.0.0"`. With // omitLockfileRegistryResolved, the resolved URL is absent. The install From 40ffcc022b11a605abec3e0bc45f6b367b145d69 Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Wed, 24 Jun 2026 16:12:45 -0700 Subject: [PATCH 2/3] chore(arborist): cover bundled-dep allowScripts gate --- workspaces/arborist/test/arborist/rebuild.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index ab433f2f97174..6c3062be89a00 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -118,6 +118,21 @@ t.test('do not run scripts if ignoreScripts=true', async t => { t.throws(() => fs.statSync(file), 'bundle build script not run') }) +t.test('bundled deps cannot be allowlisted: gate blocks their scripts', async t => { + // With the gate active, a bundled dep's script must not run even with an + // explicit allow: scripts only run when isScriptAllowed returns true, and + // bundled deps return null. + const path = fixture(t, 'testing-rebuild-bundle-reified') + const file = resolve(path, 'node_modules/@isaacs/testing-rebuild-bundle-a/node_modules/@isaacs/testing-rebuild-bundle-b/cwd') + t.throws(() => fs.statSync(file), 'file not there already (gut check)') + const arb = new Arborist({ + path, + allowScripts: { '@isaacs/testing-rebuild-bundle-b': true }, + }) + await arb.rebuild() + t.throws(() => fs.statSync(file), 'bundled dep postinstall did not run despite explicit allow') +}) + t.test('allowScripts deny entry skips the build set entry for that node', async t => { // Verifies the deny gate in #addToBuildSet: when `allowScripts` resolves // a node's policy to `false`, that node's scripts are skipped while From 83bdb35f4c862a3227ae35d98c39e612c99afa69 Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Wed, 24 Jun 2026 16:12:52 -0700 Subject: [PATCH 3/3] fix(link): gate npm link global install with allowScripts --- lib/commands/link.js | 15 +++++++-- test/lib/commands/link.js | 70 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/lib/commands/link.js b/lib/commands/link.js index b1169e0233972..cd6782a1f9836 100644 --- a/lib/commands/link.js +++ b/lib/commands/link.js @@ -5,6 +5,7 @@ const pkgJson = require('@npmcli/package-json') const semver = require('semver') const reifyFinish = require('../utils/reify-finish.js') const resolveAllowScripts = require('../utils/resolve-allow-scripts.js') +const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js') const { patchRelaxOpts } = require('../utils/cli-only-flag.js') const ArboristWorkspaceCmd = require('../arborist-cmd.js') @@ -69,6 +70,9 @@ class Link extends ArboristWorkspaceCmd { // load current packages from the global space, and then add symlinks installs locally const globalTop = resolve(this.npm.globalDir, '..') const Arborist = require('@npmcli/arborist') + // Resolve the policy up front so it also gates the global install of + // missing packages, not just the local link. + const { policy: allowScriptsPolicy } = await resolveAllowScripts(this.npm) const globalOpts = { ...this.npm.flatOptions, ...patchRelaxOpts(this.npm.config), @@ -76,6 +80,7 @@ class Link extends ArboristWorkspaceCmd { path: globalTop, global: true, prune: false, + allowScripts: allowScriptsPolicy, } const globalArb = new Arborist(globalOpts) @@ -88,10 +93,17 @@ class Link extends ArboristWorkspaceCmd { // any extra arg that is missing from the current global space should be reified there first const missing = this.missingArgsFromTree(globals, args) if (missing.length) { - await globalArb.reify({ + const globalReifyOpts = { ...globalOpts, add: missing, + } + // Gate the global install with the same preflight as `npm install`. + await strictAllowScriptsPreflight({ + arb: globalArb, + npm: this.npm, + idealTreeOpts: globalReifyOpts, }) + await globalArb.reify(globalReifyOpts) } // get a list of module names that should be linked in the local prefix @@ -118,7 +130,6 @@ class Link extends ArboristWorkspaceCmd { ) // create a new arborist instance for the local prefix and // reify all the pending names as symlinks there - const { policy: allowScriptsPolicy } = await resolveAllowScripts(this.npm) const localArb = new Arborist({ ...this.npm.flatOptions, ...patchRelaxOpts(this.npm.config), diff --git a/test/lib/commands/link.js b/test/lib/commands/link.js index 7fe72865fc52f..11ed182edd94b 100644 --- a/test/lib/commands/link.js +++ b/test/lib/commands/link.js @@ -592,3 +592,73 @@ t.test('link threads allowScripts policy through to arborist', async t => { t.strictSame(localOpts.allowScripts, { canvas: true }, 'local arborist opts.allowScripts populated from package.json') }) + +t.test('link threads allowScripts policy to the global install', async t => { + const capturedOpts = [] + const FakeArborist = function (opts) { + capturedOpts.push(opts) + this.options = opts + this.actualTree = { inventory: new Map() } + } + FakeArborist.prototype.loadActual = async () => ({ isLink: false, children: new Map() }) + FakeArborist.prototype.reify = async () => {} + + const mock = await mockNpm(t, { + command: 'link', + prefixDir: { + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + allowScripts: { canvas: true }, + }), + }, + mocks: { + '@npmcli/arborist': FakeArborist, + '{LIB}/utils/reify-finish.js': async () => {}, + }, + }) + await mock.npm.exec('link', ['canvas']) + // the global Arborist is constructed first; its missing-package install + // must carry the project policy. + t.strictSame(capturedOpts[0].allowScripts, { canvas: true }, + 'global arborist opts.allowScripts populated from package.json') +}) + +t.test('link runs the strict-allow-scripts preflight before the global install', async t => { + // The mocked preflight stands in for strict mode finding an uncovered + // script. It must run and throw before the global reify. + const calls = [] + const FakeArborist = function (opts) { + this.options = opts + this.actualTree = { inventory: new Map() } + } + FakeArborist.prototype.loadActual = async () => ({ isLink: false, children: new Map() }) + FakeArborist.prototype.reify = async () => { + calls.push('reify') + } + + const mock = await mockNpm(t, { + command: 'link', + prefixDir: { + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + }), + }, + mocks: { + '@npmcli/arborist': FakeArborist, + '{LIB}/utils/reify-finish.js': async () => {}, + '{LIB}/utils/strict-allow-scripts-preflight.js': async () => { + calls.push('preflight') + throw Object.assign(new Error('blocked'), { code: 'ESTRICTALLOWSCRIPTS' }) + }, + }, + }) + await t.rejects( + mock.npm.exec('link', ['canvas']), + { code: 'ESTRICTALLOWSCRIPTS' }, + 'the strict preflight blocks the link' + ) + t.strictSame(calls, ['preflight'], + 'preflight ran and the global reify never executed') +})