diff --git a/lib/commands/ci.js b/lib/commands/ci.js index 17307badede98..9f250e706f5ca 100644 --- a/lib/commands/ci.js +++ b/lib/commands/ci.js @@ -1,6 +1,7 @@ const reifyFinish = require('../utils/reify-finish.js') const resolveAllowScripts = require('../utils/resolve-allow-scripts.js') const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js') +const { warnUnreviewedScripts } = require('../utils/unreviewed-scripts-message.js') const runScript = require('@npmcli/run-script') const fs = require('node:fs/promises') const path = require('node:path') @@ -146,20 +147,26 @@ class CI extends ArboristWorkspaceCmd { await runRootScript('preinstall') } - await arb.reify(opts) + try { + await arb.reify(opts) - if (!ignoreScripts) { - const postReifyScripts = [ - 'install', - 'postinstall', - 'prepublish', // XXX should we remove this finally?? - 'preprepare', - 'prepare', - 'postprepare', - ] - for (const event of postReifyScripts) { - await runRootScript(event) + if (!ignoreScripts) { + const postReifyScripts = [ + 'install', + 'postinstall', + 'prepublish', // XXX should we remove this finally?? + 'preprepare', + 'prepare', + 'postprepare', + ] + for (const event of postReifyScripts) { + await runRootScript(event) + } } + } catch (err) { + // Both a failed reify and a failed root script exit before reifyFinish would have reported the blocked scripts, and a rolled back reify leaves nothing in actualTree, so report against the tree npm meant to build. + await warnUnreviewedScripts(this.npm, arb, arb.idealTree) + throw err } await reifyFinish(this.npm, arb) } diff --git a/lib/commands/install.js b/lib/commands/install.js index 2fd9bc8d5cd7a..bab43a350a3a7 100644 --- a/lib/commands/install.js +++ b/lib/commands/install.js @@ -7,6 +7,7 @@ const checks = require('npm-install-checks') const reifyFinish = require('../utils/reify-finish.js') const resolveAllowScripts = require('../utils/resolve-allow-scripts.js') const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js') +const { warnUnreviewedScripts } = require('../utils/unreviewed-scripts-message.js') const { patchRelaxOpts } = require('../utils/cli-only-flag.js') const ArboristWorkspaceCmd = require('../arborist-cmd.js') @@ -173,20 +174,26 @@ class Install extends ArboristWorkspaceCmd { const arb = new Arborist(opts) await strictAllowScriptsPreflight({ arb, npm: this.npm, idealTreeOpts: opts }) - await arb.reify(opts) - - if (runRootLifecycle) { - const postReifyScripts = [ - 'install', - 'postinstall', - 'prepublish', // XXX(npm9) should we remove this finally?? - 'preprepare', - 'prepare', - 'postprepare', - ] - for (const event of postReifyScripts) { - await runRootScript(event) + try { + await arb.reify(opts) + + if (runRootLifecycle) { + const postReifyScripts = [ + 'install', + 'postinstall', + 'prepublish', // XXX(npm9) should we remove this finally?? + 'preprepare', + 'prepare', + 'postprepare', + ] + for (const event of postReifyScripts) { + await runRootScript(event) + } } + } catch (err) { + // Both a failed reify and a failed root script exit before reifyFinish would have reported the blocked scripts, and a rolled back reify leaves nothing in actualTree, so report against the tree npm meant to build. + await warnUnreviewedScripts(this.npm, arb, arb.idealTree) + throw err } await reifyFinish(this.npm, arb) } diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js index 75129abfb153d..f6054b3a0b12d 100644 --- a/lib/utils/reify-output.js +++ b/lib/utils/reify-output.js @@ -16,7 +16,7 @@ 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 { unreviewedScriptsMessage } = require('./unreviewed-scripts-message.js') const reifyOutput = (npm, arb, extras = {}) => { const { diff, actualTree } = arb @@ -233,61 +233,4 @@ const packagesFundingMessage = (npm, { funding }) => { output.standard(' run `npm fund` for details') } -const unreviewedScriptsMessage = (npm, unreviewedScripts) => { - if (!unreviewedScripts.length) { - return - } - - // Goes through log.warn so it respects --loglevel / --silent and lands - // on stderr like every other "FYI, here's something to know" message. - // stdout is reserved for things the user explicitly asked to see - // (npm ls, npm view). - const count = unreviewedScripts.length - const pkg = count === 1 ? 'package had' : 'packages had' - const header = - `${count} ${pkg} install scripts blocked because they are not covered by allowScripts:` - - const names = [] - 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) - const ver = version ? `@${version}` : '' - const events = Object.entries(scripts) - .map(([event, cmd]) => `${event}: ${cmd}`) - .join('; ') - return ` ${display}${ver} (${events})` - }) - - log.warn( - 'install-scripts', - [ - header, - ...lines, - '', - ...remediationLines(npm, names), - ].join('\n') - ) -} - -// `npm install-scripts` writes to a project package.json, which doesn't -// 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) => { - if (npm.global) { - const list = names.join(',') - return [ - `Run \`npm install -g --allow-scripts=${list}\` to allow these scripts ` + - `once, or \`${configSetAllowScripts(names)}\` to allow them for ` + - 'all global installs.', - ] - } - return [ - 'Run `npm install-scripts ls` to review, ' + - 'or `npm install-scripts approve ` to allow.', - ] -} - module.exports = reifyOutput diff --git a/lib/utils/unreviewed-scripts-message.js b/lib/utils/unreviewed-scripts-message.js new file mode 100644 index 0000000000000..296d6eecb3da5 --- /dev/null +++ b/lib/utils/unreviewed-scripts-message.js @@ -0,0 +1,71 @@ +const { log } = require('proc-log') +const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') +const checkAllowScripts = require('./check-allow-scripts.js') +const { configSetAllowScripts } = require('./allow-scripts-remediation.js') + +// `unreviewedScriptsMessage` renders the install-time warning listing the deps +// whose scripts were blocked by the allowScripts policy. +// `warnUnreviewedScripts` is the check-and-warn variant, for callers that fail +// before `reifyFinish` gets the chance to report them. + +const unreviewedScriptsMessage = (npm, unreviewedScripts) => { + if (!unreviewedScripts.length) { + return + } + + // Goes through log.warn so it respects --loglevel / --silent and lands + // on stderr like every other "FYI, here's something to know" message. + // stdout is reserved for things the user explicitly asked to see + // (npm ls, npm view). + const count = unreviewedScripts.length + const pkg = count === 1 ? 'package had' : 'packages had' + const header = + `${count} ${pkg} install scripts blocked because they are not covered by allowScripts:` + + const names = [] + 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) + const ver = version ? `@${version}` : '' + const events = Object.entries(scripts) + .map(([event, cmd]) => `${event}: ${cmd}`) + .join('; ') + return ` ${display}${ver} (${events})` + }) + + log.warn( + 'install-scripts', + [ + header, + ...lines, + '', + ...remediationLines(npm, names), + ].join('\n') + ) +} + +// `npm install-scripts` writes to a project package.json, which doesn't +// 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) => { + if (npm.global) { + const list = names.join(',') + return [ + `Run \`npm install -g --allow-scripts=${list}\` to allow these scripts ` + + `once, or \`${configSetAllowScripts(names)}\` to allow them for ` + + 'all global installs.', + ] + } + return [ + 'Run `npm install-scripts ls` to review, ' + + 'or `npm install-scripts approve ` to allow.', + ] +} + +const warnUnreviewedScripts = async (npm, arb, tree) => + unreviewedScriptsMessage(npm, await checkAllowScripts({ arb, npm, tree })) + +module.exports = { unreviewedScriptsMessage, warnUnreviewedScripts } diff --git a/test/lib/commands/ci.js b/test/lib/commands/ci.js index a90ca4b0cffe6..43a36011e1b43 100644 --- a/test/lib/commands/ci.js +++ b/test/lib/commands/ci.js @@ -357,6 +357,61 @@ t.test('a failing preinstall prevents reify for npm ci', async t => { ) }) +// Regression test: symmetric to the install-side guarantee — the blocked-scripts warning is printed by reifyFinish, which a failing root post-reify script would otherwise skip past. +t.test('a failing root prepare still reports blocked install scripts for npm ci', async t => { + const { npm, registry, logs } = await loadMockNpm(t, { + config: { audit: false }, + prefixDir: { + abbrev: { + ...abbrev, + 'package.json': JSON.stringify({ + name: 'abbrev', + version: '1.0.0', + scripts: { postinstall: 'echo postinstall' }, + }), + }, + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + prepare: 'exit 1', + }, + }), + 'package-lock.json': JSON.stringify({ + ...packageLock, + packages: { + ...packageLock.packages, + 'node_modules/abbrev': { + ...packageLock.packages['node_modules/abbrev'], + hasInstallScript: true, + }, + }, + }), + }, + mocks: { + '@npmcli/run-script': async (opts) => { + if (opts.path === npm.prefix && opts.event === 'prepare') { + throw Object.assign(new Error('prepare failed'), { code: 'ELIFECYCLE' }) + } + }, + }, + }) + const manifest = registry.manifest({ + name: 'abbrev', + packuments: [{ version: '1.0.0', scripts: { postinstall: 'echo postinstall' } }], + }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + + await t.rejects(npm.exec('ci', []), /prepare failed/, 'ci rejects when prepare fails') + t.match( + logs.warn.byTitle('install-scripts').join('\n'), + /1 package had install scripts blocked/, + 'blocked install scripts are reported before the failure' + ) +}) + t.test('should throw if package-lock.json is missing', async t => { const { npm } = await loadMockNpm(t, { prefixDir: { diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 3961b7f02e0c1..b8afcc5bc7d4a 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -213,6 +213,79 @@ t.test('exec commands', async t => { ) }) + // Regression test: the blocked-scripts warning is printed by reifyFinish, which a failing root post-reify script would otherwise skip past, hiding the very thing that most likely broke the build. + await t.test('a failing root prepare still reports blocked install scripts', async t => { + const { npm, registry, logs } = await loadMockNpm(t, { + config: { audit: false }, + prefixDir: { + 'package.json': JSON.stringify({ + ...packageJson, + scripts: { + prepare: 'exit 1', + }, + }), + abbrev: { + ...abbrev, + 'package.json': JSON.stringify({ + name: 'abbrev', + version: '1.0.0', + scripts: { postinstall: 'echo postinstall' }, + }), + }, + }, + mocks: { + '@npmcli/run-script': async (opts) => { + if (opts.path === npm.prefix && opts.event === 'prepare') { + throw Object.assign(new Error('prepare failed'), { code: 'ELIFECYCLE' }) + } + }, + }, + }) + const manifest = registry.manifest({ + name: 'abbrev', + packuments: [{ version: '1.0.0', scripts: { postinstall: 'echo postinstall' } }], + }) + await registry.package({ manifest }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + + await t.rejects(npm.exec('install'), /prepare failed/, 'install rejects when prepare fails') + t.match( + logs.warn.byTitle('install-scripts').join('\n'), + /1 package had install scripts blocked/, + 'blocked install scripts are reported before the failure' + ) + }) + + // Regression test: a reify that throws rolls its tree back, so the blocked scripts have to be read off the ideal tree to still be reported. + await t.test('a failing reify still reports blocked install scripts', async t => { + const { npm, registry, logs } = await loadMockNpm(t, { + config: { audit: false }, + prefixDir: { + 'package.json': JSON.stringify(packageJson), + abbrev, + }, + mocks: { + '@npmcli/run-script': async () => {}, + }, + }) + const manifest = registry.manifest({ + name: 'abbrev', + packuments: [{ version: '1.0.0', scripts: { postinstall: 'echo postinstall' } }], + }) + await registry.package({ manifest }) + registry.nock.get('/abbrev/-/abbrev-1.0.0.tgz').reply(404) + + await t.rejects(npm.exec('install'), 'install rejects when a dependency cannot be fetched') + t.match( + logs.warn.byTitle('install-scripts').join('\n'), + /1 package had install scripts blocked/, + 'blocked install scripts are reported before the failure' + ) + }) + await t.test('should ignore scripts with --ignore-scripts', async t => { const { npm, registry } = await loadMockNpm(t, { config: {