diff --git a/docs/lib/content/commands/npm-install-scripts.md b/docs/lib/content/commands/npm-install-scripts.md index 6a003f5badf03..32f05a8577039 100644 --- a/docs/lib/content/commands/npm-install-scripts.md +++ b/docs/lib/content/commands/npm-install-scripts.md @@ -28,7 +28,7 @@ contexts, use the `--allow-scripts` flag at install time (for example `npm install -g --allow-scripts=canvas,sharp`) or persist the setting with `npm config set allow-scripts=canvas,sharp --location=user`. -There are three subcommands: +There are four subcommands: ```bash npm install-scripts approve [ ...] @@ -36,6 +36,7 @@ npm install-scripts approve --all npm install-scripts deny [ ...] npm install-scripts deny --all npm install-scripts ls +npm install-scripts prune ``` `approve` allows install scripts for the named packages. `` matches @@ -53,6 +54,14 @@ unreviewed install scripts. `ls` is read-only: it lists every package whose install scripts are not yet covered by `allowScripts`, without modifying `package.json`. +`prune` removes `allowScripts` entries that no longer match an installed +package with an install script, either because the package is no longer +installed (a transitive dependency changed, or a pinned `pkg@1.2.3` was +upgraded) or because it no longer has an install script. Both approvals +(`true`) and denials (`false`) are removed. It edits only the `allowScripts` +field in `package.json`, never `.npmrc` or `--allow-scripts`. Pass `--dry-run` +to preview without writing. Unparseable keys are left alone. + `approve` honours the asymmetric pin rule: if you re-approve a package whose installed version has changed, the existing pin is rewritten to track the new installed version. Multi-version statements (`pkg@1 || 2`) are left alone, @@ -78,6 +87,10 @@ npm install-scripts deny telemetry-pkg # Preview which packages still need review npm install-scripts ls + +# Preview stale allowScripts entries, then remove them +npm install-scripts prune --dry-run +npm install-scripts prune ``` ### Configuration diff --git a/lib/commands/install-scripts.js b/lib/commands/install-scripts.js index ab2eafc85b7b1..c6f91f87b0876 100644 --- a/lib/commands/install-scripts.js +++ b/lib/commands/install-scripts.js @@ -1,8 +1,9 @@ const AllowScriptsCmd = require('../utils/allow-scripts-cmd.js') -// Namespaced front-end for managing install-script approvals. -// `approve` and `deny` write the `allowScripts` policy; `ls` lists packages with unreviewed install scripts. -// The standalone `npm approve-scripts` and `npm deny-scripts` commands remain as aliases for `approve` and `deny`. +// Namespaced front-end for install-script approvals. +// `approve`/`deny` write the `allowScripts` policy, `ls` lists unreviewed packages, +// `prune` drops entries that no longer match an installed package with an install script. +// `npm approve-scripts` / `npm deny-scripts` are aliases for `approve` / `deny`. class InstallScripts extends AllowScriptsCmd { static description = 'Manage install-script approvals for dependencies' static name = 'install-scripts' @@ -12,13 +13,14 @@ class InstallScripts extends AllowScriptsCmd { 'deny [ ...]', 'deny --all', 'ls', + 'prune', ] - static params = ['all', 'allow-scripts-pin', 'json'] + static params = ['all', 'allow-scripts-pin', 'dry-run', 'json'] static async completion (opts) { const argv = opts.conf.argv.remain - const subcommands = ['approve', 'deny', 'ls'] + const subcommands = ['approve', 'deny', 'ls', 'prune'] if (argv.length === 2) { return subcommands } @@ -38,6 +40,8 @@ class InstallScripts extends AllowScriptsCmd { case 'ls': case 'list': return this.runMode('list', rest) + case 'prune': + return this.runMode('prune', rest) default: throw this.usageError( sub ? `\`${sub}\` is not a recognized subcommand.` : undefined diff --git a/lib/utils/allow-scripts-cmd.js b/lib/utils/allow-scripts-cmd.js index da557165c97dc..315b7b0b4fb9c 100644 --- a/lib/utils/allow-scripts-cmd.js +++ b/lib/utils/allow-scripts-cmd.js @@ -3,6 +3,7 @@ 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 getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js') const checkAllowScripts = require('./check-allow-scripts.js') const resolveAllowScripts = require('./resolve-allow-scripts.js') const { @@ -10,6 +11,7 @@ const { applyDenyForPackage, nameKeyFor, } = require('./allow-scripts-writer.js') +const { classifyUnusedEntries } = require('./allow-scripts-prune.js') const BaseCommand = require('../base-cmd.js') // Parse a positional arg into a name and an optional version range. A bare @@ -50,7 +52,7 @@ class AllowScriptsCmd extends BaseCommand { static ignoreImplicitWorkspace = false // Mode of the current run, set by runMode. - // One of 'approve', 'deny', or 'list'. + // One of 'approve', 'deny', 'list', or 'prune'. #mode = null // verb drives the writers and summaries, which only run in the two write modes, so it is never read while listing. @@ -73,6 +75,12 @@ class AllowScriptsCmd extends BaseCommand { ) } + // `prune` has its own flow: it reads the literal package.json#allowScripts + // map, not the resolved policy. + if (mode === 'prune') { + return this.runPrune(args) + } + // `--allow-scripts-pending` is only honored by commands that declare it; the namespace lists via `ls` instead. const pending = this.constructor.params.includes('allow-scripts-pending') && !!this.npm.config.get('allow-scripts-pending') @@ -318,6 +326,78 @@ class AllowScriptsCmd extends BaseCommand { output.standard(`Nothing to ${this.verb}; allowScripts unchanged.`) } } + + // `npm install-scripts prune`: drop package.json#allowScripts entries that no + // longer match an installed package with an install script. Edits only + // package.json (never `.npmrc`/CLI policy); `--dry-run` reports without writing. + async runPrune (args) { + const all = !!this.npm.config.get('all') + if (args.length > 0 || all) { + throw this.usageError( + '`npm install-scripts prune` cannot be combined with positional arguments or `--all`.' + ) + } + + const dryRun = !!this.npm.config.get('dry-run') + const pkg = await pkgJson.load(this.npm.prefix) + const existing = pkg.content.allowScripts && typeof pkg.content.allowScripts === 'object' + ? pkg.content.allowScripts + : {} + + let removed = [] + if (Object.keys(existing).length > 0) { + const Arborist = require('@npmcli/arborist') + const arb = new Arborist({ + ...this.npm.flatOptions, + path: this.npm.prefix, + }) + await arb.loadActual() + + // Candidate install nodes (mirrors collectUnreviewedScripts), tagged with + // whether each has install scripts so the classifier can tell "gone" from + // "no longer has scripts". + const nodes = [] + for (const node of arb.actualTree.inventory.values()) { + if (node.isProjectRoot || node.isWorkspace || node.isLink || node.inBundle || node.inert) { + continue + } + const scripts = await getInstallScripts(node) + nodes.push({ node, hasScripts: Object.keys(scripts).length > 0 }) + } + + const { remaining, removed: unused } = classifyUnusedEntries(existing, nodes) + removed = unused + + if (removed.length > 0 && !dryRun) { + // Drop the field entirely when nothing is left rather than leaving `{}`. + pkg.update({ + allowScripts: Object.keys(remaining).length > 0 ? remaining : undefined, + }) + await pkg.save() + } + } + + this.printPruneSummary({ removed, dryRun }) + } + + printPruneSummary ({ removed, dryRun }) { + if (this.npm.flatOptions.json) { + output.buffer({ allowScripts: { removed, dryRun } }) + return + } + if (removed.length === 0) { + output.standard('No unused allowScripts entries.') + return + } + const entry = removed.length === 1 ? 'entry' : 'entries' + output.standard( + `${dryRun ? 'Would remove' : 'Removed'} ${removed.length} unused allowScripts ${entry}:` + ) + for (const { key, reason } of removed) { + const text = reason === 'not-installed' ? 'package not installed' : 'no install scripts' + output.standard(` ${key} (${text})`) + } + } } module.exports = AllowScriptsCmd diff --git a/lib/utils/allow-scripts-prune.js b/lib/utils/allow-scripts-prune.js new file mode 100644 index 0000000000000..1111b45d9ca3f --- /dev/null +++ b/lib/utils/allow-scripts-prune.js @@ -0,0 +1,47 @@ +const npa = require('npm-package-arg') +const { matches } = require('@npmcli/arborist/lib/script-allowed.js') + +// Pure classifier behind `npm install-scripts prune`. +// +// Splits the `package.json#allowScripts` map into entries to keep (`remaining`) +// and unused ones to drop (`removed`). `nodes` is the install candidates as +// `{ node, hasScripts }`; the caller gathers them so this stays sync. +// +// An entry is unused (version-aware, by trusted identity) when: +// - no installed node matches the key -> reason 'not-installed' +// - it matches but none have scripts -> reason 'no-scripts' +// +// Unparseable keys are kept (prune never drops what it can't parse). Both +// `true` (approve) and `false` (deny) entries are pruned. +const classifyUnusedEntries = (allowScripts, nodes) => { + const remaining = {} + const removed = [] + + for (const [key, value] of Object.entries(allowScripts || {})) { + let parseable = true + try { + npa(key) + } catch { + parseable = false + } + if (!parseable) { + remaining[key] = value + continue + } + + const matching = nodes.filter(({ node }) => matches(node, key)) + if (matching.length === 0) { + removed.push({ key, value, reason: 'not-installed' }) + continue + } + if (!matching.some(({ hasScripts }) => hasScripts)) { + removed.push({ key, value, reason: 'no-scripts' }) + continue + } + remaining[key] = value + } + + return { remaining, removed } +} + +module.exports = { classifyUnusedEntries } diff --git a/tap-snapshots/test/lib/docs.js.test.cjs b/tap-snapshots/test/lib/docs.js.test.cjs index fda666dd49e98..9496bc714a10a 100644 --- a/tap-snapshots/test/lib/docs.js.test.cjs +++ b/tap-snapshots/test/lib/docs.js.test.cjs @@ -4597,9 +4597,10 @@ npm install-scripts approve --all npm install-scripts deny [ ...] npm install-scripts deny --all npm install-scripts ls +npm install-scripts prune Options: -[-a|--all] [--no-allow-scripts-pin] [--json] +[-a|--all] [--no-allow-scripts-pin] [--dry-run] [--json] -a|--all Show or act on all packages, not just the ones your project directly @@ -4607,6 +4608,9 @@ Options: --allow-scripts-pin Write pinned (\`pkg@version\`) entries when approving install scripts. + --dry-run + Indicates that you don't want npm to make any changes and that it should + --json Whether or not to output JSON data, rather than the normal output. @@ -4619,12 +4623,14 @@ npm install-scripts approve --all npm install-scripts deny [ ...] npm install-scripts deny --all npm install-scripts ls +npm install-scripts prune \`\`\` Note: This command is unaware of workspaces. #### \`all\` #### \`allow-scripts-pin\` +#### \`dry-run\` #### \`json\` ` diff --git a/test/lib/commands/install-scripts.js b/test/lib/commands/install-scripts.js index beb4b99c46700..523aba42bdd94 100644 --- a/test/lib/commands/install-scripts.js +++ b/test/lib/commands/install-scripts.js @@ -8,11 +8,11 @@ const mockNpm = async (t, opts = {}) => { return _mockNpm(t, opts) } -const setupProject = ({ allowScripts, withScripts = ['canvas'] } = {}) => { +const setupProject = ({ allowScripts, withScripts = ['canvas'], noScripts = [] } = {}) => { const pkg = { name: 'host', version: '1.0.0', - dependencies: Object.fromEntries(withScripts.map((n) => [n, '*'])), + dependencies: Object.fromEntries([...withScripts, ...noScripts].map((n) => [n, '*'])), } if (allowScripts !== undefined) { pkg.allowScripts = allowScripts @@ -34,6 +34,15 @@ const setupProject = ({ allowScripts, withScripts = ['canvas'] } = {}) => { resolved: `https://registry.npmjs.org/${name}/-/${name}-1.0.0.tgz`, } } + for (const name of noScripts) { + nodeModules[name] = { + 'package.json': JSON.stringify({ name, version: '1.0.0' }), + } + lockPackages[`node_modules/${name}`] = { + version: '1.0.0', + resolved: `https://registry.npmjs.org/${name}/-/${name}-1.0.0.tgz`, + } + } return { 'package.json': JSON.stringify(pkg, null, 2), @@ -52,10 +61,11 @@ t.test('completion', async t => { const comp = (argv) => InstallScripts.completion({ conf: { argv: { remain: argv } } }) - t.resolveMatch(comp(['npm', 'install-scripts']), ['approve', 'deny', 'ls']) + t.resolveMatch(comp(['npm', 'install-scripts']), ['approve', 'deny', 'ls', 'prune']) t.resolveMatch(comp(['npm', 'install-scripts', 'approve']), []) t.resolveMatch(comp(['npm', 'install-scripts', 'deny']), []) t.resolveMatch(comp(['npm', 'install-scripts', 'ls']), []) + t.resolveMatch(comp(['npm', 'install-scripts', 'prune']), []) await t.rejects(comp(['npm', 'install-scripts', 'frobnicate']), { message: 'frobnicate not recognized', }) @@ -188,3 +198,139 @@ t.test('install-scripts fails for global installs', async t => { { code: 'EGLOBAL' } ) }) + +t.test('install-scripts prune removes not-installed and no-script entries', async t => { + const { npm, prefix, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['canvas'], + noScripts: ['no-scripts-pkg'], + allowScripts: { + 'canvas@1.0.0': true, + 'no-scripts-pkg': true, + gone: true, + }, + }), + }) + await npm.exec('install-scripts', ['prune']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'canvas@1.0.0': true }) + + const out = joinedOutput() + t.match(out, /Removed 2 unused allowScripts entries:/) + t.match(out, /no-scripts-pkg \(no install scripts\)/) + t.match(out, /gone \(package not installed\)/) +}) + +t.test('install-scripts prune removes unused deny entries too', async t => { + const { npm, prefix } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['canvas'], + allowScripts: { 'canvas@1.0.0': true, 'denied-gone': false }, + }), + }) + await npm.exec('install-scripts', ['prune']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'canvas@1.0.0': true }) +}) + +t.test('install-scripts prune removes a stale version pin and drops the field', async t => { + const { npm, prefix, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['canvas'], + allowScripts: { 'canvas@9.9.9': true }, + }), + }) + await npm.exec('install-scripts', ['prune']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.notOk('allowScripts' in pkg, 'allowScripts field is removed when empty') + // Singular wording for a single entry. + t.match(joinedOutput(), /Removed 1 unused allowScripts entry:/) +}) + +t.test('install-scripts prune --dry-run reports without writing', async t => { + const allowScripts = { 'canvas@1.0.0': true, gone: true } + const { npm, prefix, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'], allowScripts }), + config: { 'dry-run': true }, + }) + await npm.exec('install-scripts', ['prune']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, allowScripts, 'package.json is unchanged') + t.match(joinedOutput(), /Would remove 1 unused allowScripts entry:/) +}) + +t.test('install-scripts prune --json emits a machine-readable summary', async t => { + const { npm, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['canvas'], + allowScripts: { 'canvas@1.0.0': true, gone: true }, + }), + config: { json: true }, + }) + await npm.exec('install-scripts', ['prune']) + + t.strictSame(JSON.parse(joinedOutput()), { + allowScripts: { + removed: [{ key: 'gone', value: true, reason: 'not-installed' }], + dryRun: false, + }, + }) +}) + +t.test('install-scripts prune with nothing unused says so', async t => { + const { npm, prefix, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ + withScripts: ['canvas'], + allowScripts: { 'canvas@1.0.0': true }, + }), + }) + await npm.exec('install-scripts', ['prune']) + + const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) + t.strictSame(pkg.allowScripts, { 'canvas@1.0.0': true }) + t.match(joinedOutput(), /No unused allowScripts entries\./) +}) + +t.test('install-scripts prune with no allowScripts field says so', async t => { + const { npm, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + }) + await npm.exec('install-scripts', ['prune']) + t.match(joinedOutput(), /No unused allowScripts entries\./) +}) + +t.test('install-scripts prune rejects positional args', async t => { + const { npm } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + }) + await t.rejects( + npm.exec('install-scripts', ['prune', 'canvas']), + /cannot be combined with positional arguments/ + ) +}) + +t.test('install-scripts prune rejects --all', async t => { + const { npm } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + config: { all: true }, + }) + await t.rejects( + npm.exec('install-scripts', ['prune']), + /cannot be combined with positional arguments or `--all`/ + ) +}) + +t.test('install-scripts prune fails for global installs', async t => { + const { npm } = await mockNpm(t, { + prefixDir: setupProject({ withScripts: ['canvas'] }), + config: { global: true }, + }) + await t.rejects( + npm.exec('install-scripts', ['prune']), + { code: 'EGLOBAL' } + ) +}) diff --git a/test/lib/utils/allow-scripts-prune.js b/test/lib/utils/allow-scripts-prune.js new file mode 100644 index 0000000000000..880b1dfe34371 --- /dev/null +++ b/test/lib/utils/allow-scripts-prune.js @@ -0,0 +1,90 @@ +const t = require('tap') +const { classifyUnusedEntries } = require('../../../lib/utils/allow-scripts-prune.js') + +// Minimal registry node: `matches` derives name/version from the resolved URL. +const node = ({ name = 'pkg', version = '1.0.0' } = {}) => ({ + name, + version, + isRegistryDependency: true, + resolved: `https://registry.npmjs.org/${name}/-/${name}-${version}.tgz`, +}) + +const withScripts = (overrides) => ({ node: node(overrides), hasScripts: true }) +const noScripts = (overrides) => ({ node: node(overrides), hasScripts: false }) + +t.test('empty / nullish policy', t => { + t.same(classifyUnusedEntries({}, []), { remaining: {}, removed: [] }) + t.same(classifyUnusedEntries(null, []), { remaining: {}, removed: [] }) + t.same(classifyUnusedEntries(undefined, [withScripts()]), { remaining: {}, removed: [] }) + t.end() +}) + +t.test('keeps entries that match an installed package with scripts', t => { + const { remaining, removed } = classifyUnusedEntries( + { canvas: true, 'esbuild@1.0.0': true }, + [withScripts({ name: 'canvas' }), withScripts({ name: 'esbuild', version: '1.0.0' })] + ) + t.same(remaining, { canvas: true, 'esbuild@1.0.0': true }) + t.same(removed, []) + t.end() +}) + +t.test('removes entries for packages no longer installed', t => { + const { remaining, removed } = classifyUnusedEntries( + { canvas: true, gone: true }, + [withScripts({ name: 'canvas' })] + ) + t.same(remaining, { canvas: true }) + t.same(removed, [{ key: 'gone', value: true, reason: 'not-installed' }]) + t.end() +}) + +t.test('removes entries whose package no longer has install scripts', t => { + const { remaining, removed } = classifyUnusedEntries( + { canvas: true }, + [noScripts({ name: 'canvas' })] + ) + t.same(remaining, {}) + t.same(removed, [{ key: 'canvas', value: true, reason: 'no-scripts' }]) + t.end() +}) + +t.test('a matching version with scripts keeps the entry even if another lacks them', t => { + const { remaining, removed } = classifyUnusedEntries( + { canvas: true }, + [noScripts({ name: 'canvas', version: '1.0.0' }), withScripts({ name: 'canvas', version: '2.0.0' })] + ) + t.same(remaining, { canvas: true }) + t.same(removed, []) + t.end() +}) + +t.test('version-pinned entry is unused when that exact version is not installed', t => { + const { remaining, removed } = classifyUnusedEntries( + { 'canvas@1.0.0': true }, + [withScripts({ name: 'canvas', version: '2.0.0' })] + ) + t.same(remaining, {}) + t.same(removed, [{ key: 'canvas@1.0.0', value: true, reason: 'not-installed' }]) + t.end() +}) + +t.test('prunes unused deny (false) entries the same way', t => { + const { remaining, removed } = classifyUnusedEntries( + { 'denied-gone': false, 'denied-here': false }, + [withScripts({ name: 'denied-here' })] + ) + t.same(remaining, { 'denied-here': false }) + t.same(removed, [{ key: 'denied-gone', value: false, reason: 'not-installed' }]) + t.end() +}) + +t.test('keeps unparseable keys untouched', t => { + const { remaining, removed } = classifyUnusedEntries( + { 'a b': true, gone: true }, + [withScripts({ name: 'canvas' })] + ) + t.same(remaining, { 'a b': true }) + t.same(removed, [{ key: 'gone', value: true, reason: 'not-installed' }]) + t.end() +}) diff --git a/workspaces/arborist/lib/script-allowed.js b/workspaces/arborist/lib/script-allowed.js index 89e24339791f1..aeba770e06e7c 100644 --- a/workspaces/arborist/lib/script-allowed.js +++ b/workspaces/arborist/lib/script-allowed.js @@ -366,6 +366,7 @@ const trustedDisplay = (node) => { module.exports = isScriptAllowed module.exports.isScriptAllowed = isScriptAllowed +module.exports.matches = matches module.exports.isExactVersionDisjunction = isExactVersionDisjunction module.exports.getTrustedRegistryIdentity = getTrustedRegistryIdentity module.exports.resolvedSourceSpecs = resolvedSourceSpecs