fix: match dotted and versioned args in approve-scripts/deny-scripts

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 () {

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 <pkg@version> 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 <pkg@version> 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 <pkg@tag> 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 <pkg@version> 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 <pkg@range> 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.

test/lib/commands/deny-scripts.js

@@ -104,6 +104,16 @@ t.test('deny-scripts errors on unknown package', async t => {
   )
 })
 
+t.test('deny-scripts <pkg@version> 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'] }),