diff --git a/lib/utils/allow-scripts-writer.js b/lib/utils/allow-scripts-writer.js index 5f43bbebeedef..310e22412a4a4 100644 --- a/lib/utils/allow-scripts-writer.js +++ b/lib/utils/allow-scripts-writer.js @@ -1,6 +1,9 @@ const npa = require('npm-package-arg') const { log } = require('proc-log') -const { getTrustedRegistryIdentity } = require('@npmcli/arborist/lib/script-allowed.js') +const { + getTrustedRegistryIdentity, + resolvedSourceSpecs, +} = require('@npmcli/arborist/lib/script-allowed.js') // Pure helpers that implement the RFC's pin-mismatch table for // `npm approve-scripts` and `npm deny-scripts`. @@ -12,6 +15,8 @@ const { getTrustedRegistryIdentity } = require('@npmcli/arborist/lib/script-allo // Denying always writes `"": false`, regardless of `--allow-scripts-pin`, per the // RFC's asymmetric-pin rule. +const primaryResolvedSource = (node) => resolvedSourceSpecs(node)[0] || '' + // Convert an arborist Node into the spec string used for a versioned policy // entry. Returns `null` if the node cannot be represented as a versioned key // derived from trusted sources (lockfile URL for registry, hosted shortcut @@ -21,8 +26,7 @@ const versionedKeyFor = (node) => { if (!node) { return null } - /* istanbul ignore next: callers guarantee a string resolved */ - const resolved = typeof node.resolved === 'string' ? node.resolved : '' + const resolved = primaryResolvedSource(node) if (resolved.startsWith('git')) { try { const parsed = npa(resolved) @@ -69,8 +73,7 @@ const nameKeyFor = (node) => { if (!node) { return null } - /* istanbul ignore next: callers guarantee a string resolved */ - const resolved = typeof node.resolved === 'string' ? node.resolved : '' + const resolved = primaryResolvedSource(node) if (resolved.startsWith('git')) { try { const parsed = npa(resolved) @@ -164,7 +167,8 @@ const keyTargetsNode = (key, node) => { case 'git': { let resolvedParsed try { - resolvedParsed = node.resolved ? npa(node.resolved) : null + const resolved = primaryResolvedSource(node) + resolvedParsed = resolved ? npa(resolved) : null } catch { /* istanbul ignore next */ return false @@ -176,7 +180,8 @@ const keyTargetsNode = (key, node) => { case 'file': case 'directory': case 'remote': - return node.resolved === parsed.saveSpec || node.resolved === parsed.fetchSpec + return resolvedSourceSpecs(node) + .some(resolved => resolved === parsed.saveSpec || resolved === parsed.fetchSpec) default: return false } diff --git a/test/lib/utils/allow-scripts-writer.js b/test/lib/utils/allow-scripts-writer.js index 56314f8eb5a52..f72a84eef113b 100644 --- a/test/lib/utils/allow-scripts-writer.js +++ b/test/lib/utils/allow-scripts-writer.js @@ -45,6 +45,48 @@ t.test('nameKeyFor / versionedKeyFor — file', async t => { t.equal(versionedKeyFor(n), 'file:../local') }) +t.test('nameKeyFor / versionedKeyFor — local directory link target', async t => { + const targetPath = path.resolve('local') + const n = { + name: 'local', + packageName: 'local', + version: '1.0.0', + resolved: null, + path: targetPath, + realpath: targetPath, + linksIn: new Set([{ resolved: 'file:../local' }]), + } + + t.equal(nameKeyFor(n), 'file:../local') + t.equal(versionedKeyFor(n), 'file:../local') + + t.strictSame( + applyApprovalForPackage({}, [n], { pin: true }).allowScripts, + { 'file:../local': true } + ) + t.match( + applyApprovalForPackage({ 'file:local': false }, [n], { pin: true }).warning, + /denied|versioned deny/ + ) +}) + +t.test('nameKeyFor / versionedKeyFor — empty link target has no portable file key', async t => { + const targetPath = path.resolve('local') + const n = { + name: 'local', + packageName: 'local', + version: '1.0.0', + resolved: null, + path: targetPath, + realpath: targetPath, + linksIn: new Set(), + } + + t.equal(nameKeyFor(n), null) + t.equal(versionedKeyFor(n), null) + t.strictSame(applyApprovalForPackage({}, [n], { pin: true }).allowScripts, {}) +}) + t.test('isSingleVersionPin', async t => { t.ok(isSingleVersionPin('pkg@1.2.3')) t.notOk(isSingleVersionPin('pkg')) diff --git a/workspaces/arborist/lib/script-allowed.js b/workspaces/arborist/lib/script-allowed.js index 66104b274c7c5..89e24339791f1 100644 --- a/workspaces/arborist/lib/script-allowed.js +++ b/workspaces/arborist/lib/script-allowed.js @@ -97,6 +97,41 @@ const matches = (node, key) => { } } +const resolvedSourceSpecs = (node) => { + const specs = [] + const seen = new Set() + const add = (spec) => { + if (typeof spec !== 'string' || spec === '' || seen.has(spec)) { + return + } + seen.add(spec) + specs.push(spec) + } + + add(node?.resolved) + + if (!node?.resolved && node?.linksIn && typeof node.linksIn[Symbol.iterator] === 'function') { + let hasIncomingLink = false + for (const link of node.linksIn) { + hasIncomingLink = true + add(link.resolved) + } + + if (hasIncomingLink) { + // Link targets for local directory deps are separate inventory nodes + // whose own `resolved` is null. The incoming Link carries the saved spec + // (for example `file:../pkg`, relative to node_modules), while policy + // entries written by hand often use the dependency spec from package.json + // (for example `file:pkg`, resolved by npa to this target path). Include + // the real target paths so both forms can match the same local dep. + add(node.realpath) + add(node.path) + } + } + + return specs +} + const matchRegistry = (node, parsed) => { // 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 @@ -282,17 +317,13 @@ const matchGit = (node, parsed) => { } const matchFileOrDir = (node, parsed) => { - if (!node.resolved) { - return false - } - return node.resolved === parsed.saveSpec || node.resolved === parsed.fetchSpec + return resolvedSourceSpecs(node) + .some(resolved => resolved === parsed.saveSpec || resolved === parsed.fetchSpec) } const matchRemote = (node, parsed) => { - if (!node.resolved) { - return false - } - return node.resolved === parsed.fetchSpec || node.resolved === parsed.saveSpec + return resolvedSourceSpecs(node) + .some(resolved => resolved === parsed.fetchSpec || resolved === parsed.saveSpec) } const isRegistryNode = (node) => { @@ -337,4 +368,5 @@ module.exports = isScriptAllowed module.exports.isScriptAllowed = isScriptAllowed module.exports.isExactVersionDisjunction = isExactVersionDisjunction module.exports.getTrustedRegistryIdentity = getTrustedRegistryIdentity +module.exports.resolvedSourceSpecs = resolvedSourceSpecs module.exports.trustedDisplay = trustedDisplay diff --git a/workspaces/arborist/test/script-allowed.js b/workspaces/arborist/test/script-allowed.js index 4e20f987d9c87..e708074817402 100644 --- a/workspaces/arborist/test/script-allowed.js +++ b/workspaces/arborist/test/script-allowed.js @@ -118,6 +118,56 @@ t.test('file path — exact resolved match', t => { t.end() }) +t.test('file path — link target matches incoming link source', t => { + const targetPath = require('node:path').resolve('local-pkg') + const target = node({ + name: 'local-pkg', + packageName: 'local-pkg', + version: '1.0.0', + }) + target.resolved = null + target.path = targetPath + target.realpath = targetPath + target.linksIn = new Set([{ resolved: 'file:../local-pkg' }]) + + t.equal(isScriptAllowed(target, { 'file:../local-pkg': true }), true) + t.equal(isScriptAllowed(target, { 'file:local-pkg': true }), true) + t.equal(isScriptAllowed(target, { 'file:../local-pkg': false }), false) + t.equal(isScriptAllowed(target, { 'file:../other': true }), null) + t.end() +}) + +t.test('file path — registry nodes do not match by install path', t => { + const reg = node({ + name: 'sharp', + packageName: 'sharp', + version: '0.33.0', + path: 'node_modules/sharp', + realpath: require('node:path').resolve('node_modules/sharp'), + linksIn: new Set(), + }) + + t.equal(isScriptAllowed(reg, { 'file:node_modules/sharp': true }), null) + t.end() +}) + +t.test('file path — empty link sets do not add install paths', t => { + const targetPath = require('node:path').resolve('local-pkg') + const target = node({ + name: 'local-pkg', + packageName: 'local-pkg', + version: '1.0.0', + }) + target.resolved = null + target.path = targetPath + target.realpath = targetPath + target.linksIn = new Set() + + t.equal(isScriptAllowed(target, { 'file:local-pkg': true }), null) + t.equal(isScriptAllowed(target, { [targetPath]: true }), null) + t.end() +}) + t.test('directory key — npa parses absolute paths as type=directory', t => { // npa treats absolute paths as { type: 'directory' }, which the // matcher shares with the 'file' case. path.resolve produces a diff --git a/workspaces/arborist/test/unreviewed-scripts.js b/workspaces/arborist/test/unreviewed-scripts.js index 9672f0dc228f7..7f26f56db6875 100644 --- a/workspaces/arborist/test/unreviewed-scripts.js +++ b/workspaces/arborist/test/unreviewed-scripts.js @@ -118,6 +118,25 @@ t.test('collectUnreviewedScripts', async t => { t.equal(result[0].node.name, 'pending') }) + t.test('skips reviewed local directory link targets', async t => { + const target = node({ name: 'local', scripts: { install: 'x' } }) + target.resolved = null + target.isRegistryDependency = false + target.path = require('node:path').resolve('local') + target.realpath = target.path + target.linksIn = new Set([{ resolved: 'file:../local' }]) + + t.strictSame(await collectUnreviewedScripts({ + tree: tree([target]), + policy: { 'file:../local': false }, + }), []) + + t.strictSame(await collectUnreviewedScripts({ + tree: tree([target]), + policy: { 'file:local': true }, + }), []) + }) + t.test('detects synthetic node-gyp via binding.gyp runtime check', async t => { const collect = mockCollect(t, async (n) => { if (n.path === '/has-bindings') {