diff --git a/docs/lib/content/commands/npm-approve-scripts.md b/docs/lib/content/commands/npm-approve-scripts.md index 55c892fbf96b1..d73c052286e8d 100644 --- a/docs/lib/content/commands/npm-approve-scripts.md +++ b/docs/lib/content/commands/npm-approve-scripts.md @@ -25,8 +25,8 @@ it with `--global` (`-g`) fails with an `EGLOBAL` error, since global installs (`npm install -g`) and one-off executions (`npm exec` / `npx`) have no project `package.json` to write to. To allow install scripts in those 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`. +`npm install -g canvas sharp --allow-scripts=canvas,sharp`) or persist the +setting with `npm config set allow-scripts=canvas,sharp --location=user`. There are three modes: diff --git a/docs/lib/content/commands/npm-install-scripts.md b/docs/lib/content/commands/npm-install-scripts.md index 32f05a8577039..e83a72025f821 100644 --- a/docs/lib/content/commands/npm-install-scripts.md +++ b/docs/lib/content/commands/npm-install-scripts.md @@ -25,8 +25,8 @@ it with `--global` (`-g`) fails with an `EGLOBAL` error, since global installs (`npm install -g`) and one-off executions (`npm exec` / `npx`) have no project `package.json` to write to. To allow install scripts in those 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`. +`npm install -g canvas sharp --allow-scripts=canvas,sharp`) or persist the +setting with `npm config set allow-scripts=canvas,sharp --location=user`. There are four subcommands: diff --git a/lib/commands/rebuild.js b/lib/commands/rebuild.js index 0f98b41e8c168..bcb188fd8f502 100644 --- a/lib/commands/rebuild.js +++ b/lib/commands/rebuild.js @@ -2,12 +2,11 @@ const { resolve } = require('node:path') const { log, output } = require('proc-log') const npa = require('npm-package-arg') const semver = require('semver') -const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') const ArboristWorkspaceCmd = require('../arborist-cmd.js') const checkAllowScripts = require('../utils/check-allow-scripts.js') const resolveAllowScripts = require('../utils/resolve-allow-scripts.js') const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js') -const { configSetAllowScripts } = require('../utils/allow-scripts-remediation.js') +const { configSetAllowScripts, policyKeyFor } = require('../utils/allow-scripts-remediation.js') class Rebuild extends ArboristWorkspaceCmd { static description = 'Rebuild a package' @@ -77,9 +76,12 @@ class Rebuild extends ArboristWorkspaceCmd { // `npm install-scripts` writes to a project package.json, which doesn't // exist for global rebuilds. Point global users at `npm config set`, // which writes the `allow-scripts` setting to their user .npmrc. - const names = unreviewed.map(({ node }) => trustedDisplay(node).name) + // Use the policy identity, not the display name: only registry deps + // are matched by name, so a name-based suggestion would leave git, + // file and tarball deps blocked. + const keys = unreviewed.map(({ node }) => policyKeyFor(node)) const remediation = this.npm.global - ? `Run \`${configSetAllowScripts(names)}\` to allow their scripts.` + ? `Run \`${configSetAllowScripts(keys)}\` to allow their scripts.` : 'Run `npm install-scripts ls` to review.' log.warn( 'rebuild', diff --git a/lib/utils/allow-scripts-remediation.js b/lib/utils/allow-scripts-remediation.js index ff8c9b75a81fe..1db719963d3e0 100644 --- a/lib/utils/allow-scripts-remediation.js +++ b/lib/utils/allow-scripts-remediation.js @@ -1,9 +1,52 @@ +const { + getTrustedRegistryIdentity, + matches, + resolvedSourceSpecs, + trustedDisplay, +} = require('@npmcli/arborist/lib/script-allowed.js') + +// Policy keys come straight from resolved sources, which carry characters +// the shell acts on: `#` in a git committish starts a comment, and `&`, +// `?` or spaces in a tarball URL break the command apart. Quote whenever +// the value is not plainly safe, so the suggestion can be pasted as-is. +const SHELL_SAFE = /^[\w@,./:-]+$/ + +const shellQuote = (value) => + SHELL_SAFE.test(value) ? value : `'${value.replace(/'/g, `'\\''`)}'` + +// The blocked-scripts summary shows a human-readable name, but the +// allowScripts policy only matches registry deps by name. git, file, remote +// and tarball deps are matched by their resolved source, so a suggestion +// built from display names would leave their scripts blocked. Verify each +// candidate against the node with the real matcher, so the key we hand the +// user is one the policy will actually accept. +const policyKeyFor = (node) => { + const trusted = getTrustedRegistryIdentity(node) + const candidates = [trusted && trusted.name, node.resolved, ...resolvedSourceSpecs(node)] + for (const candidate of candidates) { + if (typeof candidate === 'string' && candidate !== '' && matches(node, candidate, false)) { + return candidate + } + } + /* istanbul ignore next: defensive fallback for nodes without name */ + return trustedDisplay(node).name || '' +} + // Builds the `npm config set allow-scripts` command suggested to global // users, who have no project package.json for `npm approve-scripts` to // write to. `--location=user` keeps the setting in the user .npmrc instead // of trying (and, for global installs, failing) to write it to the local // project config. -const configSetAllowScripts = (names) => - `npm config set allow-scripts=${names.join(',')} --location=user` +const configSetAllowScripts = (keys) => + `npm config set allow-scripts=${shellQuote(keys.join(','))} --location=user` + +// Builds the `--allow-scripts=` flag global users add to the install +// they just ran. Deliberately not a whole command: npm.argv holds +// positionals only, so a reconstructed `npm install -g ` would drop +// flags like --registry and retry against the default registry while +// allowing that package's scripts to run. A spec-less +// `npm install -g --allow-scripts=` is no better: it installs the +// current directory and fails with ENOENT reading package.json. +const allowScriptsFlag = (keys) => `--allow-scripts=${shellQuote(keys.join(','))}` -module.exports = { configSetAllowScripts } +module.exports = { allowScriptsFlag, configSetAllowScripts, policyKeyFor } diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js index e50d4ac72d967..9ddfb70cd85c2 100644 --- a/lib/utils/reify-output.js +++ b/lib/utils/reify-output.js @@ -16,7 +16,11 @@ const npmAuditReport = require('npm-audit-report') const { readTree: getFundingInfo } = require('libnpmfund') const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') const auditError = require('./audit-error.js') -const { configSetAllowScripts } = require('./allow-scripts-remediation.js') +const { + allowScriptsFlag, + configSetAllowScripts, + policyKeyFor, +} = require('./allow-scripts-remediation.js') const reifyOutput = (npm, arb, extras = {}) => { const { diff, actualTree } = arb @@ -246,12 +250,12 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => { const header = `${count} ${pkg} install scripts blocked because they are not covered by allowScripts:` - const names = [] + const nodes = [] const lines = unreviewedScripts.map(({ node, scripts }) => { const { name, version } = trustedDisplay(node) /* istanbul ignore next: every test node has a name */ const display = name || '' - names.push(display) + nodes.push(node) const ver = version ? `@${version}` : '' const events = Object.entries(scripts) .map(([event, cmd]) => `${event}: ${cmd}`) @@ -265,7 +269,7 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => { header, ...lines, '', - ...remediationLines(npm, names), + ...remediationLines(npm, nodes), ].join('\n') ) } @@ -274,13 +278,13 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => { // exist for global installs (it throws EGLOBAL). For those, point users at // the mechanism that does work globally: the `--allow-scripts` flag for a // one-off, or `npm config set allow-scripts` to persist it. -const remediationLines = (npm, names) => { +const remediationLines = (npm, nodes) => { if (npm.global) { - const list = names.join(',') + const keys = nodes.map(policyKeyFor) return [ - `Run \`npm install -g --allow-scripts=${list}\` to allow these scripts ` + - `once, or \`${configSetAllowScripts(names)}\` to allow them for ` + - 'all global installs.', + `Re-run your install with \`${allowScriptsFlag(keys)}\` to allow these ` + + `scripts once, or run \`${configSetAllowScripts(keys)}\` to allow them ` + + 'for all global installs.', ] } return [ diff --git a/lib/utils/strict-allow-scripts-preflight.js b/lib/utils/strict-allow-scripts-preflight.js index 0c500018184c2..786f32834e86b 100644 --- a/lib/utils/strict-allow-scripts-preflight.js +++ b/lib/utils/strict-allow-scripts-preflight.js @@ -1,6 +1,5 @@ const checkAllowScripts = require('./check-allow-scripts.js') -const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') -const { configSetAllowScripts } = require('./allow-scripts-remediation.js') +const { configSetAllowScripts, policyKeyFor } = require('./allow-scripts-remediation.js') // Pre-flight check for `--strict-allow-scripts`. Call after arborist has // been constructed but before `arb.reify()` runs, so that install scripts @@ -51,13 +50,14 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => { // `npm install-scripts` writes to a project package.json, which doesn't // exist for global installs. Point global users at the `--allow-scripts` // flag and `npm config set allow-scripts`, which both work for global - // installs. Use the trusted display identity so the suggested `npm config - // set` value matches what the policy matches on, not the tarball's - // self-reported name. - const names = unreviewed.map(({ node }) => trustedDisplay(node).name) + // installs. Use the policy identity so the suggested `npm config set` + // value is a key the matcher accepts: only registry deps are matched by + // name, so a name-based suggestion would leave git, file and tarball deps + // blocked, and the name itself is the tarball's self-reported one. + const keys = unreviewed.map(({ node }) => policyKeyFor(node)) const remediation = npm.global ? 'Allow them with `--allow-scripts`, persist them with ' + - `\`${configSetAllowScripts(names)}\`, or bypass this ` + + `\`${configSetAllowScripts(keys)}\`, or bypass this ` + 'check with `--dangerously-allow-all-scripts`.' : 'Approve them with `npm install-scripts approve`, deny them with ' + '`npm install-scripts deny`, or bypass this check with ' + diff --git a/test/lib/utils/allow-scripts-remediation.js b/test/lib/utils/allow-scripts-remediation.js new file mode 100644 index 0000000000000..18521bfc18797 --- /dev/null +++ b/test/lib/utils/allow-scripts-remediation.js @@ -0,0 +1,81 @@ +const t = require('tap') + +const { + allowScriptsFlag, + configSetAllowScripts, + policyKeyFor, +} = require('../../../lib/utils/allow-scripts-remediation.js') + +t.test('registry deps are keyed by their trusted name', async t => { + const node = { + name: 'canvas', + version: '2.11.0', + resolved: 'https://registry.npmjs.org/canvas/-/canvas-2.11.0.tgz', + } + t.equal(policyKeyFor(node), 'canvas') +}) + +// An alias installs `naughty` at `node_modules/trusted`. The policy matches +// on the registered name, so the suggestion has to name it too. +t.test('aliased registry deps are keyed by the registered name', async t => { + const node = { + name: 'trusted', + version: '1.0.0', + resolved: 'https://registry.npmjs.org/naughty/-/naughty-1.0.0.tgz', + } + t.equal(policyKeyFor(node), 'naughty') +}) + +// Non-registry deps are matched by their resolved source. Keying them by +// name would produce a suggestion the matcher rejects, leaving the scripts +// blocked after the user followed the advice. +t.test('tarball deps are keyed by their resolved URL', async t => { + const node = { name: 'tool', version: '1.0.0', resolved: 'https://example.com/tool.tgz' } + t.equal(policyKeyFor(node), 'https://example.com/tool.tgz') +}) + +t.test('file deps are keyed by their resolved path', async t => { + const node = { name: 'local', version: '1.0.0', resolved: 'file:../local' } + t.equal(policyKeyFor(node), 'file:../local') +}) + +t.test('git deps are keyed by their resolved git URL', async t => { + const resolved = `git+ssh://git@github.com/o/r.git#${'a'.repeat(40)}` + const node = { name: 'forked', version: '1.0.0', resolved } + t.equal(policyKeyFor(node), resolved) +}) + +// Bundled deps can never be allowlisted, so no candidate matches. Fall back +// to the display name rather than emitting nothing. +t.test('falls back to the display name when nothing matches', async t => { + const node = { + name: 'bundled', + version: '1.0.0', + inBundle: true, + resolved: 'https://registry.npmjs.org/bundled/-/bundled-1.0.0.tgz', + } + t.equal(policyKeyFor(node), 'bundled') +}) + +t.test('plain keys are left unquoted', async t => { + t.equal( + configSetAllowScripts(['canvas', 'sharp']), + 'npm config set allow-scripts=canvas,sharp --location=user' + ) + t.equal(allowScriptsFlag(['canvas', 'sharp']), '--allow-scripts=canvas,sharp') +}) + +// `#` starts a shell comment, which would silently truncate the committish +// off a pasted suggestion. +t.test('shell-unsafe keys are quoted', async t => { + const key = `git+ssh://git@github.com/o/r.git#${'a'.repeat(40)}` + t.equal( + configSetAllowScripts([key]), + `npm config set allow-scripts='${key}' --location=user` + ) + t.equal(allowScriptsFlag([key]), `--allow-scripts='${key}'`) +}) + +t.test('single quotes in a key are escaped', async t => { + t.equal(allowScriptsFlag(["file:../it's"]), `--allow-scripts='file:../it'\\''s'`) +}) diff --git a/test/lib/utils/reify-output.js b/test/lib/utils/reify-output.js index 0678e9cabeb28..2fd05df4b81c3 100644 --- a/test/lib/utils/reify-output.js +++ b/test/lib/utils/reify-output.js @@ -534,11 +534,103 @@ t.test('global install suggests --allow-scripts, not approve-scripts', async t = const warn = mock.logs.warn.byTitle('install-scripts').join('\n') t.match(warn, /2 packages had install scripts blocked because they are not covered by allowScripts/) t.match(warn, /canvas@2\.11\.0 \(install: node-gyp rebuild\)/) - t.match(warn, /npm install -g --allow-scripts=canvas,sharp/) - t.match(warn, /npm config set allow-scripts=canvas,sharp/) + t.match(warn, /Re-run your install with `--allow-scripts=canvas,sharp`/) + t.match(warn, /npm config set allow-scripts=canvas,sharp --location=user/) t.notMatch(warn, /approve-scripts/) }) +// A copy-pasteable `npm install -g --allow-scripts=` has no specs, so +// it installs the current directory and fails with ENOENT reading +// package.json. The command cannot be reconstructed safely either: npm.argv +// carries positionals only, so flags like --registry would be silently +// dropped from a suggestion that also allows scripts to run. +t.test('global remediation never suggests a spec-less install command', async t => { + const mock = await mockNpm(t, { + command: 'install', + argv: ['esbuild'], + config: { global: true, registry: 'https://internal.example.com/' }, + }) + + reifyOutput(mock.npm, { + actualTree: { name: 'host', inventory: { has: () => false } }, + diff: { children: [] }, + }, { + unreviewedScripts: [{ + node: { packageName: 'esbuild', name: 'esbuild', version: '0.28.1', path: '/x/esbuild' }, + scripts: { postinstall: 'node install.js' }, + }], + }) + mock.npm.finish() + + const warn = mock.logs.warn.byTitle('install-scripts').join('\n') + t.notMatch(warn, /npm install -g --allow-scripts=/) + t.notMatch(warn, /npm install -g esbuild/) + t.match(warn, /Re-run your install with `--allow-scripts=esbuild`/) +}) + +// Package names are only valid policy keys for registry deps. git, file and +// remote deps are matched by their resolved source, so a suggestion built +// from the display name would leave their scripts blocked. +t.test('global remediation uses resolved sources as policy keys', async t => { + const mock = await mockNpm(t, { command: 'install', config: { global: true } }) + + reifyOutput(mock.npm, { + actualTree: { name: 'host', inventory: { has: () => false } }, + diff: { children: [] }, + }, { + unreviewedScripts: [ + { + node: { + name: 'tool', + version: '1.0.0', + path: '/x/tool', + resolved: 'https://example.com/tool.tgz', + }, + scripts: { postinstall: 'node install.js' }, + }, + { + node: { + name: 'local', + version: '2.0.0', + path: '/x/local', + resolved: 'file:../local', + }, + scripts: { install: 'make' }, + }, + ], + }) + mock.npm.finish() + + const warn = mock.logs.warn.byTitle('install-scripts').join('\n') + t.match(warn, /allow-scripts=https:\/\/example\.com\/tool\.tgz,file:\.\.\/local/) +}) + +// Resolved sources contain characters the shell treats specially — `#` in a +// git committish starts a comment — so the suggested value has to be quoted. +t.test('global remediation quotes shell-unsafe policy keys', async t => { + const mock = await mockNpm(t, { command: 'install', config: { global: true } }) + const sha = 'a'.repeat(40) + + reifyOutput(mock.npm, { + actualTree: { name: 'host', inventory: { has: () => false } }, + diff: { children: [] }, + }, { + unreviewedScripts: [{ + node: { + name: 'forked', + version: '1.0.0', + path: '/x/forked', + resolved: `git+ssh://git@github.com/o/r.git#${sha}`, + }, + scripts: { postinstall: 'node install.js' }, + }], + }) + mock.npm.finish() + + const warn = mock.logs.warn.byTitle('install-scripts').join('\n') + t.match(warn, new RegExp(`allow-scripts='git\\+ssh://git@github\\.com/o/r\\.git#${sha}'`)) +}) + t.test('single unreviewed script uses singular wording', async t => { const mockReifyWithExtras = async (t, reify, extras) => { const mock = await mockNpm(t, {}) diff --git a/test/lib/utils/strict-allow-scripts-preflight.js b/test/lib/utils/strict-allow-scripts-preflight.js index c67a6e4853a12..fdf4bbe2ee577 100644 --- a/test/lib/utils/strict-allow-scripts-preflight.js +++ b/test/lib/utils/strict-allow-scripts-preflight.js @@ -232,3 +232,22 @@ t.test('global error points at --allow-scripts, not approve-scripts', async t => } ) }) + +// A bare name is only a valid policy key for a registry dep. A tarball dep +// is matched by its resolved URL, so suggesting its name would leave the +// scripts blocked after the user followed the advice. +t.test('global error suggests the resolved source for a tarball dep', async t => { + const tarball = { + ...node({ name: 'tool' }), + resolved: 'https://example.com/tool.tgz', + } + const arb = makeArb({ ideal: tree([tarball]) }) + await t.rejects( + preflight({ + arb, + npm: { global: true, flatOptions: { strictAllowScripts: true } }, + idealTreeOpts: {}, + }), + { message: /npm config set allow-scripts=https:\/\/example\.com\/tool\.tgz/ } + ) +})