From b480b41749eba1e6a93da554b8c25f5fd76a75aa Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 11 Jun 2026 08:41:14 -0700 Subject: [PATCH] fix: match dotted and versioned args in approve-scripts/deny-scripts (cherry picked from commit abf78b3c143a1825de910c0e401d01c0d3f5199b) --- lib/utils/allow-scripts-cmd.js | 72 +++++++++++--- test/lib/commands/approve-scripts.js | 139 +++++++++++++++++++++++++++ test/lib/commands/deny-scripts.js | 10 ++ 3 files changed, 208 insertions(+), 13 deletions(-) diff --git a/lib/utils/allow-scripts-cmd.js b/lib/utils/allow-scripts-cmd.js index 5ba997764251c..b6ec663124fe8 100644 --- a/lib/utils/allow-scripts-cmd.js +++ b/lib/utils/allow-scripts-cmd.js @@ -1,4 +1,6 @@ const { log, output } = require('proc-log') +const npa = require('npm-package-arg') +const semver = require('semver') const pkgJson = require('@npmcli/package-json') const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') const checkAllowScripts = require('./check-allow-scripts.js') @@ -10,6 +12,28 @@ const { } = require('./allow-scripts-writer.js') const BaseCommand = require('../base-cmd.js') +// Parse a positional arg into a name and an optional version range. A bare +// name matches every installed version; `pkg@1.2.3` or `pkg@^1` narrows by +// semver. npm-package-arg handles dotted and scoped names; fall back to the +// raw string as the name if it can't be parsed. +const parsePositional = (arg) => { + let parsed + try { + parsed = npa(arg) + } catch { + return { name: arg, range: null } + } + const name = parsed.name || arg + if (parsed.type === 'version' || parsed.type === 'range') { + const spec = parsed.fetchSpec + const range = (!spec || spec === '*' || parsed.rawSpec === '' || parsed.rawSpec === '*') + ? null + : spec + return { name, range } + } + return { name, range: null } +} + // Shared implementation for `npm approve-scripts` and `npm deny-scripts`. // Subclasses set `verb` to `'approve'` or `'deny'`. // @@ -144,8 +168,16 @@ class AllowScriptsCmd extends BaseCommand { } async runPositional (args, arb) { - const matched = this.findNodesForArgs(args, arb) + const { matched, unmatched } = this.findNodesForArgs(args, arb) + if (unmatched.length > 0) { + throw Object.assign( + new Error(`No installed packages match: ${unmatched.join(', ')}`), + { code: 'ENOMATCH' } + ) + } const groups = this.groupByPackage(matched) + /* istanbul ignore if: matched is non-empty here; groups only empties when a + matched node has no trusted key, which groupByPackage already warns on */ if (Object.keys(groups).length === 0) { throw Object.assign( new Error(`No installed packages match: ${args.join(', ')}`), @@ -156,22 +188,36 @@ class AllowScriptsCmd extends BaseCommand { } findNodesForArgs (args, arb) { - // Match positional args against each node's trusted name. Registry - // deps use the URL-derived name; non-registry deps fall back to the - // dependency edge name. Bundled deps are excluded for the same reason - // as --all. - const wanted = new Set(args) + // Match positional args against each node's trusted name. Registry deps + // use the URL-derived name; non-registry deps fall back to the dependency + // 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`. const matched = [] - for (const node of arb.actualTree.inventory.values()) { - if (node.isProjectRoot || node.isWorkspace || node.inBundle) { - continue + 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) { + continue + } + const { name, version } = trustedDisplay(node) + if (!name || name !== wantName) { + continue + } + if (range && (!version || !semver.satisfies(version, range, { loose: true }))) { + continue + } + found.push(node) } - const { name } = trustedDisplay(node) - if (name && wanted.has(name)) { - matched.push(node) + if (found.length === 0) { + unmatched.push(arg) + } else { + matched.push(...found) } } - return matched + return { matched, unmatched } } get logTitle () { diff --git a/test/lib/commands/approve-scripts.js b/test/lib/commands/approve-scripts.js index cf7cd8f0cb071..30325f41014c8 100644 --- a/test/lib/commands/approve-scripts.js +++ b/test/lib/commands/approve-scripts.js @@ -129,6 +129,125 @@ t.test('approve-scripts errors on unknown package', async t => { ) }) +t.test('approve-scripts approves a package whose name contains dots', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['cordova.plugins.diagnostic'] }), + }) + await npm.exec('approve-scripts', ['cordova.plugins.diagnostic']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'cordova.plugins.diagnostic@1.0.0': true }) +}) + +t.test('approve-scripts approves a dotted name with a version specifier', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['cordova.plugins.diagnostic'] }), + }) + await npm.exec('approve-scripts', ['cordova.plugins.diagnostic@1.0.0']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'cordova.plugins.diagnostic@1.0.0': true }) +}) + +t.test('approve-scripts <@scope/pkg@version> approves a scoped name with a version', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + dependencies: { '@scope/pkg': '*' }, + }), + 'package-lock.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { name: 'host', version: '1.0.0', dependencies: { '@scope/pkg': '*' } }, + 'node_modules/@scope/pkg': { + version: '2.0.0', + resolved: 'https://registry.npmjs.org/@scope/pkg/-/pkg-2.0.0.tgz', + hasInstallScript: true, + }, + }, + }), + node_modules: { + '@scope': { + pkg: { + 'package.json': JSON.stringify({ + name: '@scope/pkg', + version: '2.0.0', + scripts: { install: 'echo install' }, + }), + }, + }, + }, + }, + }) + await npm.exec('approve-scripts', ['@scope/pkg@2.0.0']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { '@scope/pkg@2.0.0': true }) +}) + +t.test('approve-scripts errors when no installed version matches', async t => { + const { npm } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + }) + await t.rejects( + npm.exec('approve-scripts', ['canvas@9.9.9']), + { code: 'ENOMATCH' } + ) +}) + +t.test('approve-scripts reports only the unmatched args and writes nothing', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + }) + const err = await npm.exec('approve-scripts', ['canvas', 'not-installed']) + .then(() => null, e => e) + t.equal(err.code, 'ENOMATCH') + t.match(err.message, /not-installed/) + t.notMatch(err.message, /canvas/) + // Nothing is written when any arg fails to match. + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk(pkg.allowScripts) +}) + +t.test('approve-scripts matches every installed version', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + }) + // A tag carries no version to filter on, so it behaves like a bare name. + await npm.exec('approve-scripts', ['canvas@latest']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'canvas@1.0.0': true }) +}) + +t.test('approve-scripts errors on an unparseable spec', async t => { + const { npm } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + }) + // npm-package-arg throws on this; the raw string is used as the name and + // matches nothing. + await t.rejects( + npm.exec('approve-scripts', ['foo@@bar']), + { code: 'ENOMATCH' } + ) +}) + +t.test('approve-scripts errors on a spec with no package name', async t => { + const { npm } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + }) + // A path spec parses but carries no name, so it matches no registry dep. + await t.rejects( + npm.exec('approve-scripts', ['./local-pkg']), + { code: 'ENOMATCH' } + ) +}) + t.test('approve-scripts respects existing deny entry', async t => { const { npm, prefix, logs } = await mockNpm(t, { prefixDir: setupProject({ @@ -364,6 +483,26 @@ t.test('approve-scripts groups multiple installed versions of the same package', }) }) +t.test('approve-scripts pins only the matching installed version', async t => { + const { npm, prefix } = await _mockNpm(t, { + prefixDir: twoVersionFixture, + }) + await npm.exec('approve-scripts', ['lodash@4.17.21']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'lodash@4.17.21': true }) +}) + +t.test('approve-scripts pins only versions satisfying the range', async t => { + const { npm, prefix } = await _mockNpm(t, { + prefixDir: twoVersionFixture, + }) + await npm.exec('approve-scripts', ['lodash@^4']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'lodash@4.17.21': true }) +}) + t.test('approve-scripts --pending handles node with no version', async t => { // Exercise the ternary's falsy branch in runPending: `node.version ? '@'... : ''` // when the node has no version field. diff --git a/test/lib/commands/deny-scripts.js b/test/lib/commands/deny-scripts.js index 23c9a8ad6a4b5..358b71d7da7bd 100644 --- a/test/lib/commands/deny-scripts.js +++ b/test/lib/commands/deny-scripts.js @@ -104,6 +104,16 @@ t.test('deny-scripts errors on unknown package', async t => { ) }) +t.test('deny-scripts denies a dotted name with a version specifier', async t => { + const { npm, prefix } = await _mockNpm(t, { + prefixDir: setupProject({ withScripts: ['cordova.plugins.diagnostic'] }), + }) + await npm.exec('deny-scripts', ['cordova.plugins.diagnostic@1.0.0']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'cordova.plugins.diagnostic': false }) +}) + t.test('deny-scripts requires positional args or --all', async t => { const { npm } = await _mockNpm(t, { prefixDir: setupProject({ withScripts: ['core-js'] }),