fix(approve-scripts): approve deps with no resolved URL by name

docs/lib/content/commands/npm-approve-scripts.md

@@ -53,6 +53,14 @@ the command cannot infer. Existing `false` entries always win;
 `approve-scripts` will not silently re-allow a package you previously
 denied.
 
+If a registry dependency has no `resolved` URL in your `package-lock.json`
+(for example, an older lockfile or one written with
+`omit-lockfile-registry-resolved`), npm cannot verify a trusted version for
+it and cannot pin it: a `pkg@1.2.3` entry never matches, so the package
+keeps appearing under `--allow-scripts-pending`. `approve-scripts` approves
+these by name (`pkg: true`) and warns when it does. To restore pinning,
+refresh the lockfile with `npm install`.
+
 ### Examples
 
 ```bash

lib/utils/allow-scripts-writer.js

@@ -250,7 +250,41 @@ const applyApprovalForPackage = (existing, nodes, { pin = true } = {}) => {
   // package are removed. Per the RFC's pin-mismatch table, an existing
   // name-only entry (`pkg: true`) is replaced by `pkg@x.y.z: true` once
   // every installed version has a pin.
-  const installedKeys = new Set(nodes.map(versionedKeyFor).filter(Boolean))
+  const versionedKeys = nodes.map(versionedKeyFor)
+  const installedKeys = new Set(versionedKeys.filter(Boolean))
+
+  // A registry dep with no `resolved` URL in the lockfile has no trustable
+  // version (getTrustedRegistryIdentity won't trust the tarball's
+  // node.version), so versionedKeyFor returns null and a `pkg@x.y.z` pin can
+  // never match it (npm/cli#9558). When any installed version can't be
+  // pinned, approve the whole package by name and drop now-redundant pins.
+  if (name && versionedKeys.some(key => !key)) {
+    for (const key of Object.keys(allowScripts)) {
+      if (
+        keyTargetsNode(key, sample) &&
+        key !== name &&
+        isSingleVersionPin(key) &&
+        allowScripts[key] === true
+      ) {
+        delete allowScripts[key]
+        changes.push({ key, change: 'removed-pinned-allow' })
+      }
+    }
+    if (allowScripts[name] !== true) {
+      allowScripts[name] = true
+      changes.push({ key: name, change: 'added' })
+    }
+    return {
+      allowScripts,
+      changes,
+      warning: changes.length
+        ? `${name}: approved by name (all versions) because its ` +
+        `package-lock.json entry has no "resolved" URL, so npm can't pin a ` +
+        `specific version. Run \`npm install\` to refresh the lockfile and ` +
+        `enable pinning.`
+        : undefined,
+    }
+  }
 
   for (const key of Object.keys(allowScripts)) {
     if (

test/lib/commands/approve-scripts.js

@@ -7,7 +7,7 @@ const mockNpm = async (t, opts = {}) => {
   return _mockNpm(t, opts)
 }
 
-const setupProject = ({ allowScripts, withScripts = ['canvas'] } = {}) => {
+const setupProject = ({ allowScripts, withScripts = ['canvas'], noResolved = [] } = {}) => {
   const pkg = {
     name: 'host',
     version: '1.0.0',
@@ -28,11 +28,16 @@ const setupProject = ({ allowScripts, withScripts = ['canvas'] } = {}) => {
         scripts: { install: 'echo install' },
       }),
     }
-    lockPackages[`node_modules/${name}`] = {
+    const lockEntry = {
       version: '1.0.0',
-      resolved: tarUrl,
       hasInstallScript: true,
     }
+    // Some lockfiles omit `resolved` for registry deps. Those nodes have no
+    // trustable version, so they can only be approved by name.
+    if (!noResolved.includes(name)) {
+      lockEntry.resolved = tarUrl
+    }
+    lockPackages[`node_modules/${name}`] = lockEntry
   }
 
   return {
@@ -119,6 +124,51 @@ t.test('approve-scripts --all approves every unreviewed package', async t => {
   })
 })
 
+t.test('approve-scripts --all approves a dep without a resolved URL by name', async t => {
+  // Regression for npm/cli#9558: a dep with no `resolved` URL can't be
+  // pinned, but must still be approved by name, not silently skipped.
+  const { npm, prefix, logs } = await mockNpm(t, {
+    prefixDir: setupProject({ withScripts: ['canvas'], noResolved: ['canvas'] }),
+    config: { all: true },
+  })
+  await npm.exec('approve-scripts', [])
+
+  const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8'))
+  t.strictSame(pkg.allowScripts, { canvas: true }, 'approved by name, not skipped')
+  t.match(
+    logs.warn.byTitle('approve-scripts').join('\n'),
+    /no "resolved" URL/,
+    'warns that a version pin could not be written'
+  )
+})
+
+t.test('approve-scripts <pkg> approves a dep without a resolved URL by name', async t => {
+  const { npm, prefix } = await mockNpm(t, {
+    prefixDir: setupProject({ withScripts: ['canvas'], noResolved: ['canvas'] }),
+  })
+  await npm.exec('approve-scripts', ['canvas'])
+
+  const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8'))
+  t.strictSame(pkg.allowScripts, { canvas: true })
+})
+
+t.test('approve-scripts --pending is empty after a no-resolved dep is approved by name', async t => {
+  const { npm, joinedOutput } = await mockNpm(t, {
+    prefixDir: setupProject({
+      allowScripts: { canvas: true },
+      withScripts: ['canvas'],
+      noResolved: ['canvas'],
+    }),
+    config: { 'allow-scripts-pending': true },
+  })
+  await npm.exec('approve-scripts', [])
+  t.match(
+    joinedOutput(),
+    /No packages with unreviewed install scripts/,
+    'name-only entry covers the dep even without a resolved URL'
+  )
+})
+
 t.test('approve-scripts errors on unknown package', async t => {
   const { npm } = await mockNpm(t, {
     prefixDir: setupProject({ withScripts: ['canvas'] }),

test/lib/utils/allow-scripts-writer.js

@@ -24,6 +24,23 @@ const node = (overrides = {}) => {
   }
 }
 
+// A registry node with no `resolved` URL in the lockfile. Its trusted name
+// comes from a dependency edge, but its version isn't trustable, so
+// versionedKeyFor returns null (npm/cli#9558).
+const noResolvedNode = (overrides = {}) => {
+  const name = overrides.name ?? 'pkg'
+  const version = overrides.version ?? '1.0.0'
+  return {
+    name,
+    packageName: overrides.packageName ?? name,
+    version,
+    resolved: undefined,
+    location: `node_modules/${name}`,
+    isRegistryDependency: true,
+    edgesIn: new Set([{ name, spec: overrides.spec ?? `^${version}` }]),
+  }
+}
+
 t.test('nameKeyFor / versionedKeyFor — registry', async t => {
   const n = node({ name: 'canvas', version: '2.11.0' })
   t.equal(nameKeyFor(n), 'canvas')
@@ -214,6 +231,73 @@ t.test('applyApprovalForPackage — keeps existing pin matching one installed, a
   t.strictSame(allowScripts, { 'lodash@3.10.1': true, 'lodash@4.17.21': true })
 })
 
+t.test('versionedKeyFor — registry dep without a resolved URL has no trustable version', async t => {
+  const n = noResolvedNode({ name: 'esbuild', version: '0.25.0' })
+  t.equal(nameKeyFor(n), 'esbuild', 'name still recoverable from the dependency edge')
+  t.equal(versionedKeyFor(n), null, 'version cannot be trusted without a resolved URL')
+})
+
+t.test('applyApprovalForPackage — pin mode falls back to name-only when version is not trustable', async t => {
+  const { allowScripts, changes, warning } = applyApprovalForPackage(
+    {},
+    [noResolvedNode({ name: 'esbuild', version: '0.25.0' })],
+    { pin: true }
+  )
+  t.strictSame(allowScripts, { esbuild: true }, 'approved by name instead of silently skipped')
+  t.strictSame(changes, [{ key: 'esbuild', change: 'added' }])
+  t.match(warning, /no "resolved" URL/, 'explains why a pin was not written')
+})
+
+t.test('applyApprovalForPackage — mixed resolved/no-resolved collapses to one name-only entry', async t => {
+  const { allowScripts, warning } = applyApprovalForPackage(
+    {},
+    [
+      node({ name: 'esbuild', version: '0.21.5' }),
+      noResolvedNode({ name: 'esbuild', version: '0.25.0' }),
+    ],
+    { pin: true }
+  )
+  t.strictSame(allowScripts, { esbuild: true }, 'no redundant pin alongside the name-only entry')
+  t.match(warning, /no "resolved" URL/)
+})
+
+t.test('applyApprovalForPackage — existing pin collapses into name-only when a sibling cannot be pinned', async t => {
+  const { allowScripts, changes } = applyApprovalForPackage(
+    { 'esbuild@0.21.5': true },
+    [
+      node({ name: 'esbuild', version: '0.21.5' }),
+      noResolvedNode({ name: 'esbuild', version: '0.25.0' }),
+    ],
+    { pin: true }
+  )
+  t.strictSame(allowScripts, { esbuild: true })
+  t.match(changes, [
+    { key: 'esbuild@0.21.5', change: 'removed-pinned-allow' },
+    { key: 'esbuild', change: 'added' },
+  ])
+})
+
+t.test('applyApprovalForPackage — no-resolved dep already approved by name is a no-op', async t => {
+  const { allowScripts, changes } = applyApprovalForPackage(
+    { esbuild: true },
+    [noResolvedNode({ name: 'esbuild', version: '0.25.0' })],
+    { pin: true }
+  )
+  t.strictSame(allowScripts, { esbuild: true })
+  t.strictSame(changes, [])
+})
+
+t.test('applyApprovalForPackage — no-resolved dep with name-only deny still loses to the deny', async t => {
+  const { allowScripts, changes, warning } = applyApprovalForPackage(
+    { esbuild: false },
+    [noResolvedNode({ name: 'esbuild', version: '0.25.0' })],
+    { pin: true }
+  )
+  t.strictSame(allowScripts, { esbuild: false }, 'deny wins; nothing approved')
+  t.strictSame(changes, [])
+  t.match(warning, /esbuild is denied/)
+})
+
 t.test('applyDenyForPackage — empty allowScripts adds name-only false', async t => {
   const { allowScripts, changes } = applyDenyForPackage(
     {},