From aef7ada415a46004afbd5da93be8dd463aca252c Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Tue, 23 Jun 2026 13:53:35 -0500 Subject: [PATCH] fix(approve-scripts): approve deps with no resolved URL by name (#9606) Fixes #9558. Registry deps with no `resolved` URL in `package-lock.json` have no trustable version, so `approve-scripts` cannot pin them. It was silently writing nothing and leaving them pending; now it approves them by name (`pkg: true`) and warns why. (cherry picked from commit 971500fce94c91b1a78ab8b88664a03070c571dd) --- .../content/commands/npm-approve-scripts.md | 8 ++ lib/utils/allow-scripts-writer.js | 36 +++++++- test/lib/commands/approve-scripts.js | 56 ++++++++++++- test/lib/utils/allow-scripts-writer.js | 84 +++++++++++++++++++ 4 files changed, 180 insertions(+), 4 deletions(-) diff --git a/docs/lib/content/commands/npm-approve-scripts.md b/docs/lib/content/commands/npm-approve-scripts.md index ce39e65082c17..160d2e37914a0 100644 --- a/docs/lib/content/commands/npm-approve-scripts.md +++ b/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 diff --git a/lib/utils/allow-scripts-writer.js b/lib/utils/allow-scripts-writer.js index 310e22412a4a4..d5c6dfa21f38b 100644 --- a/lib/utils/allow-scripts-writer.js +++ b/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 ( diff --git a/test/lib/commands/approve-scripts.js b/test/lib/commands/approve-scripts.js index 30325f41014c8..4b2e7e5a73e1f 100644 --- a/test/lib/commands/approve-scripts.js +++ b/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 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'] }), diff --git a/test/lib/utils/allow-scripts-writer.js b/test/lib/utils/allow-scripts-writer.js index f72a84eef113b..e4e933e2f7c0d 100644 --- a/test/lib/utils/allow-scripts-writer.js +++ b/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( {},