diff --git a/lib/utils/allow-scripts-cmd.js b/lib/utils/allow-scripts-cmd.js index f07dc8d1dc504..cbedf649b2f7c 100644 --- a/lib/utils/allow-scripts-cmd.js +++ b/lib/utils/allow-scripts-cmd.js @@ -221,13 +221,14 @@ class AllowScriptsCmd extends BaseCommand { // edge name. A version or range on the arg narrows the match to installed // versions that satisfy it. Bundled deps are excluded for the same reason // as --all. Args that match nothing are returned in `unmatched`. + // Links are skipped like collectUnreviewedScripts and prune do: scripts only run on the target, and keying policy off a Link's resolved spec writes store paths under install-strategy=linked (npm/cli#9939). const matched = [] const unmatched = [] for (const arg of args) { const { name: wantName, range } = parsePositional(arg) const found = [] for (const node of arb.actualTree.inventory.values()) { - if (node.isProjectRoot || node.isWorkspace || node.inBundle) { + if (node.isProjectRoot || node.isWorkspace || node.isLink || node.inBundle) { continue } const { name, version } = trustedDisplay(node) diff --git a/test/lib/commands/approve-scripts.js b/test/lib/commands/approve-scripts.js index 449382d19e987..cda9f98681bf3 100644 --- a/test/lib/commands/approve-scripts.js +++ b/test/lib/commands/approve-scripts.js @@ -859,3 +859,131 @@ t.test('approve-scripts --all with only bundled deps has nothing to review', asy const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) t.notOk(pkg.allowScripts, 'no allowScripts written') }) + +// Layout produced by install-strategy=linked: the real package lives in the store, node_modules holds a symlink to it, and the hidden lockfile records that isolated layout (npm/cli#9939). +const setupLinkedProject = (t, { allowScripts, noResolved = false } = {}) => { + const storeDir = 'canvas@1.0.0-c2FsdHNhbHRzYWx0c2FsdA' + const storeLoc = `node_modules/.store/${storeDir}/node_modules/canvas` + const tarUrl = noResolved + ? undefined + : 'https://registry.npmjs.org/canvas/-/canvas-1.0.0.tgz' + const pkg = { + name: 'host', + version: '1.0.0', + dependencies: { canvas: '^1.0.0' }, + } + if (allowScripts !== undefined) { + pkg.allowScripts = allowScripts + } + return { + 'package.json': JSON.stringify(pkg, null, 2), + 'package-lock.json': JSON.stringify({ + name: pkg.name, + version: pkg.version, + lockfileVersion: 3, + requires: true, + packages: { + '': pkg, + 'node_modules/canvas': { + version: '1.0.0', + resolved: tarUrl, + hasInstallScript: true, + }, + }, + }), + node_modules: { + '.package-lock.json': JSON.stringify({ + lockfileVersion: 3, + requires: true, + packages: { + [`node_modules/.store/${storeDir}`]: {}, + [storeLoc]: { + version: '1.0.0', + resolved: tarUrl, + hasInstallScript: true, + }, + 'node_modules/canvas': { + resolved: storeLoc, + link: true, + }, + }, + }), + '.store': { + [storeDir]: { + node_modules: { + canvas: { + 'package.json': JSON.stringify({ + name: 'canvas', + version: '1.0.0', + scripts: { install: 'echo install' }, + }), + }, + }, + }, + }, + canvas: t.fixture('symlink', `.store/${storeDir}/node_modules/canvas`), + }, + } +} + +// loadActual only trusts the hidden lockfile when it is newer than every dir under node_modules (assertNoNewer allows 10ms), and fixture creation order makes that racy, so bump its mtime past the fixture writes. +const trustHiddenLockfile = (prefix) => { + const future = new Date(Date.now() + 60_000) + fs.utimesSync(resolve(prefix, 'node_modules', '.package-lock.json'), future, future) +} + +t.test('approve-scripts under linked strategy writes a registry pin, not store paths', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupLinkedProject(t), + }) + trustHiddenLockfile(prefix) + await npm.exec('approve-scripts', ['canvas']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'canvas@1.0.0': true }) +}) + +t.test('approve-scripts --all under linked strategy writes a registry pin, not store paths', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupLinkedProject(t), + config: { all: true }, + }) + trustHiddenLockfile(prefix) + await npm.exec('approve-scripts', []) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'canvas@1.0.0': true }) +}) + +t.test('approve-scripts --pending is empty when the registry pin covers a linked store package', async t => { + const { npm, prefix, joinedOutput } = await mockNpm(t, { + prefixDir: setupLinkedProject(t, { allowScripts: { 'canvas@1.0.0': true } }), + config: { 'allow-scripts-pending': true }, + }) + trustHiddenLockfile(prefix) + await npm.exec('approve-scripts', []) + t.match(joinedOutput(), /No packages with unreviewed install scripts/) +}) + +t.test('deny-scripts under linked strategy writes a name-only deny', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupLinkedProject(t), + }) + trustHiddenLockfile(prefix) + await npm.exec('deny-scripts', ['canvas']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { canvas: false }) +}) + +t.test('approve-scripts under linked strategy without resolved URLs approves by name', async t => { + // omit-lockfile-registry-resolved: the store package has no resolved URL, so it cannot be pinned but must still be approved by name via the incoming Link's edge. + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupLinkedProject(t, { noResolved: true }), + }) + trustHiddenLockfile(prefix) + await npm.exec('approve-scripts', ['canvas']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { canvas: true }) +}) diff --git a/test/lib/commands/install-scripts.js b/test/lib/commands/install-scripts.js index cf7063b595715..fd7051c7e4419 100644 --- a/test/lib/commands/install-scripts.js +++ b/test/lib/commands/install-scripts.js @@ -334,3 +334,64 @@ t.test('install-scripts prune fails for global installs', async t => { { code: 'EGLOBAL' } ) }) + +t.test('install-scripts prune under linked strategy keeps the registry pin, drops store paths', async t => { + // Regression for npm/cli#9939: the buggy writer produced file:.store keys; prune must drop those and keep the valid registry pin covering the store package. + const storeDir = 'canvas@1.0.0-c2FsdHNhbHRzYWx0c2FsdA' + const storeLoc = `node_modules/.store/${storeDir}/node_modules/canvas` + const tarUrl = 'https://registry.npmjs.org/canvas/-/canvas-1.0.0.tgz' + const pkg = { + name: 'host', + version: '1.0.0', + dependencies: { canvas: '^1.0.0' }, + allowScripts: { + 'canvas@1.0.0': true, + [`file:.store/${storeDir}/node_modules/canvas`]: true, + }, + } + const { npm, prefix, joinedOutput } = await mockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify(pkg, null, 2), + node_modules: { + '.package-lock.json': JSON.stringify({ + lockfileVersion: 3, + requires: true, + packages: { + [`node_modules/.store/${storeDir}`]: {}, + [storeLoc]: { + version: '1.0.0', + resolved: tarUrl, + hasInstallScript: true, + }, + 'node_modules/canvas': { + resolved: storeLoc, + link: true, + }, + }, + }), + '.store': { + [storeDir]: { + node_modules: { + canvas: { + 'package.json': JSON.stringify({ + name: 'canvas', + version: '1.0.0', + scripts: { install: 'echo install' }, + }), + }, + }, + }, + }, + canvas: t.fixture('symlink', `.store/${storeDir}/node_modules/canvas`), + }, + }, + }) + // loadActual only trusts the hidden lockfile when it is newer than every dir under node_modules (assertNoNewer allows 10ms), and fixture creation order makes that racy, so bump its mtime past the fixture writes. + const future = new Date(Date.now() + 60_000) + fs.utimesSync(resolve(prefix, 'node_modules', '.package-lock.json'), future, future) + await npm.exec('install-scripts', ['prune']) + + const updated = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(updated.allowScripts, { 'canvas@1.0.0': true }) + t.match(joinedOutput(), /file:\.store.*\(package not installed\)/) +}) diff --git a/workspaces/arborist/lib/script-allowed.js b/workspaces/arborist/lib/script-allowed.js index 629625f8e4f8c..4599a6163a98e 100644 --- a/workspaces/arborist/lib/script-allowed.js +++ b/workspaces/arborist/lib/script-allowed.js @@ -1,3 +1,4 @@ +const { sep } = require('node:path') const npa = require('npm-package-arg') const semver = require('semver') const versionFromTgz = require('./version-from-tgz.js') @@ -99,6 +100,15 @@ const matches = (node, key, failClosed) => { } } +// True when the node lives in the linked install strategy's `node_modules/.store` directory; such registry-managed packages must never derive identity from the store's internal `file:` link specs. +const isStoreBacked = (node) => { + if (node?.isInStore) { + return true + } + const real = node?.realpath || node?.path + return typeof real === 'string' && real.includes(`${sep}node_modules${sep}.store${sep}`) +} + const resolvedSourceSpecs = (node) => { const specs = [] const seen = new Set() @@ -112,7 +122,8 @@ const resolvedSourceSpecs = (node) => { add(node?.resolved) - if (!node?.resolved && node?.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') { + if (!node?.resolved && !isStoreBacked(node) && + node?.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') { let hasIncomingLink = false for (const link of node.linksIn) { hasIncomingLink = true @@ -235,10 +246,32 @@ const getTrustedRegistryIdentity = (node) => { } const nameFromEdges = (node) => { - if (!node.edgesIn || typeof node.edgesIn[Symbol.iterator] !== 'function') { + const name = nameFromEdgeSet(node?.edgesIn) + if (name) { + return name + } + // A link target carries no edges of its own; they land on the incoming Links (e.g. the linked strategy's store packages), so consult their edges too, failing closed when the Links disagree on the registry name rather than trusting insertion order. + let linkName = null + if (node?.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') { + for (const link of node.linksIn) { + const name = nameFromEdgeSet(link.edgesIn) + if (!name) { + continue + } + if (linkName && linkName !== name) { + return null + } + linkName = name + } + } + return linkName +} + +const nameFromEdgeSet = (edgesIn) => { + if (!edgesIn || typeof edgesIn[Symbol.iterator] !== 'function') { return null } - for (const edge of node.edgesIn) { + for (const edge of edgesIn) { let parsed try { parsed = npa.resolve(edge.name, edge.spec) @@ -350,7 +383,14 @@ const isRegistryNode = (node) => { // edge resolves to a registry spec, which is much harder to spoof than // the URL. if (typeof node.isRegistryDependency === 'boolean') { - return node.isRegistryDependency + if (node.isRegistryDependency) { + return true + } + // A link target carries no edges of its own; they land on the incoming Links (e.g. the linked strategy's store packages, npm/cli#9939), so delegate the edge-based check to them. + if (node.edgesIn?.size === 0 && node.linksIn?.size > 0) { + return [...node.linksIn].every(link => link.isRegistryDependency) + } + return false } // Fall back to URL parsing for nodes without the arborist getter // (e.g. test fixtures, lockfiles with omit-lockfile-registry-resolved). diff --git a/workspaces/arborist/test/script-allowed.js b/workspaces/arborist/test/script-allowed.js index dccc674c7a79b..378716b56e2af 100644 --- a/workspaces/arborist/test/script-allowed.js +++ b/workspaces/arborist/test/script-allowed.js @@ -865,3 +865,127 @@ t.test('trustedDisplay falls back to node.name/version when URL has no identity' t.strictSame(trustedDisplay(n), { name: 'bar', version: '1.2.3' }) t.end() }) + +t.test('registry — link target delegates edge check to incoming links (linked strategy)', t => { + // Under install-strategy=linked, the store package carries no edges of its own; they land on the incoming Link nodes (npm/cli#9939). + const target = node({ + name: 'canvas', + version: '1.0.0', + location: 'node_modules/.store/canvas@1.0.0-hash/node_modules/canvas', + }) + target.isRegistryDependency = false + target.edgesIn = new Set() + target.linksIn = new Set([{ isRegistryDependency: true }]) + + t.equal(isScriptAllowed(target, { 'canvas@1.0.0': true }), true) + t.equal(isScriptAllowed(target, { canvas: true }), true) + t.equal(isScriptAllowed(target, { canvas: false }), false) + t.equal(isScriptAllowed(target, { 'canvas@2.0.0': true }), null) + t.end() +}) + +t.test('registry — link target refuses when an incoming link is not a registry dep', t => { + const target = node({ name: 'canvas', version: '1.0.0' }) + target.isRegistryDependency = false + target.edgesIn = new Set() + target.linksIn = new Set([{ isRegistryDependency: true }, { isRegistryDependency: false }]) + + t.equal(isScriptAllowed(target, { 'canvas@1.0.0': true }), null) + t.end() +}) + +t.test('registry — direct non-registry edges refuse regardless of links', t => { + const target = node({ name: 'canvas', version: '1.0.0' }) + target.isRegistryDependency = false + target.edgesIn = new Set([{ spec: 'file:../canvas' }]) + target.linksIn = new Set([{ isRegistryDependency: true }]) + + t.equal(isScriptAllowed(target, { 'canvas@1.0.0': true }), null) + t.end() +}) + +t.test('store-backed link target never matches store file specs', t => { + // A store package whose resolved is unknown (fs-scan fallback) must not take identity from the store's internal file: link specs (npm/cli#9939). + const real = require('node:path').resolve('node_modules/.store/canvas@1.0.0-hash/node_modules/canvas') + const target = node({ name: 'canvas', version: '1.0.0' }) + target.resolved = null + target.path = real + target.realpath = real + target.linksIn = new Set([{ resolved: 'file:.store/canvas@1.0.0-hash/node_modules/canvas' }]) + + t.equal(isScriptAllowed(target, { 'file:.store/canvas@1.0.0-hash/node_modules/canvas': true }), null) + t.end() +}) + +t.test('isInStore link target never matches store file specs', t => { + const target = node({ name: 'canvas', version: '1.0.0' }) + target.resolved = null + target.isInStore = true + target.linksIn = new Set([{ resolved: 'file:.store/canvas@1.0.0-hash/node_modules/canvas' }]) + + t.equal(isScriptAllowed(target, { 'file:.store/canvas@1.0.0-hash/node_modules/canvas': true }), null) + t.end() +}) + +t.test('registry — link target without resolved takes trusted name from link edges', t => { + // omit-lockfile-registry-resolved under install-strategy=linked: the store package has no resolved URL and no edges; the name comes from the incoming Link's consumer-written edge. + const target = node({ name: 'canvas', version: '1.0.0' }) + target.resolved = null + target.isRegistryDependency = false + target.edgesIn = new Set() + target.linksIn = new Set([{ + isRegistryDependency: true, + edgesIn: new Set([{ name: 'canvas', spec: '^1.0.0' }]), + }]) + + t.equal(isScriptAllowed(target, { canvas: true }), true) + t.equal(isScriptAllowed(target, { canvas: false }), false) + t.equal(isScriptAllowed(target, { other: true }), null) + // With no verifiable version, an exact pin is refused but a deny still blocks. + t.equal(isScriptAllowed(target, { 'canvas@1.0.0': true }), null) + t.equal(isScriptAllowed(target, { 'canvas@1.0.0': false }), false) + t.end() +}) + +t.test('registry — link target with no registry name on any link edge stays untrusted', t => { + const target = node({ name: 'canvas', version: '1.0.0' }) + target.resolved = null + target.isRegistryDependency = false + target.edgesIn = new Set() + target.linksIn = new Set([{ + isRegistryDependency: true, + edgesIn: new Set([{ name: 'canvas', spec: 'file:../canvas' }]), + }]) + + t.equal(isScriptAllowed(target, { canvas: true }), null) + t.end() +}) + +t.test('registry — link target with agreeing links keeps the trusted name', t => { + const target = node({ name: 'canvas', version: '1.0.0' }) + target.resolved = null + target.isRegistryDependency = false + target.edgesIn = new Set() + target.linksIn = new Set([ + { isRegistryDependency: true, edgesIn: new Set([{ name: 'canvas', spec: '^1.0.0' }]) }, + { isRegistryDependency: true, edgesIn: new Set([{ name: 'canvas', spec: '~1.0.0' }]) }, + ]) + + t.equal(isScriptAllowed(target, { canvas: true }), true) + t.end() +}) + +t.test('registry — link target with disagreeing link names fails closed', t => { + const target = node({ name: 'canvas', version: '1.0.0' }) + target.resolved = null + target.isRegistryDependency = false + target.edgesIn = new Set() + target.linksIn = new Set([ + { isRegistryDependency: true, edgesIn: new Set([{ name: 'canvas', spec: '^1.0.0' }]) }, + { isRegistryDependency: true, edgesIn: new Set([{ name: 'sharp', spec: '^1.0.0' }]) }, + ]) + + t.equal(isScriptAllowed(target, { canvas: true }), null) + t.equal(isScriptAllowed(target, { sharp: true }), null) + t.end() +})