diff --git a/lib/utils/error-message.js b/lib/utils/error-message.js index 41e35bd78ac46..9e99cdd9ba8da 100644 --- a/lib/utils/error-message.js +++ b/lib/utils/error-message.js @@ -364,6 +364,20 @@ const errorMessage = (er, npm) => { break } + // Append a dependency "why is this here" explanation for resolution errors + // that carry structured explain() data from Arborist (ETARGET, + // EALLOWREMOTE/EALLOWGIT, tarball/network failures, EINCOMPLETEMANIFEST, + // ESTRICTALLOWSCRIPTS, etc.). ERESOLVE does its own richer formatting in the + // switch above, so it is excluded here. + const hasExplanation = er.explanation && + (Array.isArray(er.explanation) ? er.explanation.length > 0 : true) + if (hasExplanation && er.code !== 'ERESOLVE') { + const { report } = require('./explain-dep-error.js') + const { explanation, file } = report(er, npm.logChalk, npm.noColorChalk) + detail.push(['', explanation]) + files.push([`${String(er.code).toLowerCase()}-report.txt`, file]) + } + return { summary, detail, diff --git a/lib/utils/explain-dep-error.js b/lib/utils/explain-dep-error.js new file mode 100644 index 0000000000000..4f0fb1b0f8fb0 --- /dev/null +++ b/lib/utils/explain-dep-error.js @@ -0,0 +1,51 @@ +// Formats the structured dependency `explanation` attached to install errors +// (ETARGET, EALLOWREMOTE/EALLOWGIT, tarball/network, EINCOMPLETEMANIFEST, +// EBADENGINE, EBADPLATFORM, ESTRICTALLOWSCRIPTS) into a human-readable +// "why is this package here" report. Mirrors explain-eresolve.js and reuses +// the same `npm explain` formatters. +// +// Arborist attaches one of two shapes to `er.explanation`: +// - an edge-explanation object (from `edge.explain()`) describing the single +// dependency request that failed to resolve/fetch, or +// - a node-explanation object (or array of them, from `node.explain()`) for +// a package that resolved but is unusable/flagged (bad platform/engine, +// unreviewed install scripts). +const { explainEdge, explainNode } = require('./explain-dep.js') + +// Normalize `er.explanation` (single object, array, or nothing) to a list. +const toList = (expl) => Array.isArray(expl) ? expl : expl ? [expl] : [] + +// An edge-explanation carries a top-level `spec`; a node-explanation does not. +const explainOne = (expl, depth, chalk) => + 'spec' in expl + ? explainEdge(expl, depth, chalk) + : explainNode(expl, depth, chalk) + +const explain = (expl, chalk, depth) => + toList(expl).map(e => explainOne(e, depth, chalk)).join('\n\n') + +// Generate a depth-limited explanation for the terminal plus a full +// (depth=Infinity) report written to the logs folder, just like ERESOLVE. +const report = (er, chalk, noColorChalk) => { + const list = toList(er.explanation) + if (!list.length) { + return { explanation: '', file: '' } + } + // Edge-shaped explanations are genuine resolution failures; node-shaped ones + // describe a package that resolved but is unusable, where the actionable + // context is "why is it here" (removing that path avoids the error). + const heading = 'spec' in list[0] + ? 'Could not resolve dependency:' + : list.length > 1 + ? 'These packages are installed because:' + : 'This package is installed because:' + return { + explanation: `${heading}\n${explain(er.explanation, chalk, 4)}`, + file: `# npm ${er.code} error report\n\n${heading}\n${explain(er.explanation, noColorChalk, Infinity)}`, + } +} + +module.exports = { + explain, + report, +} diff --git a/lib/utils/strict-allow-scripts-preflight.js b/lib/utils/strict-allow-scripts-preflight.js index d3575289fa8ed..9f5fbb5d467ed 100644 --- a/lib/utils/strict-allow-scripts-preflight.js +++ b/lib/utils/strict-allow-scripts-preflight.js @@ -1,5 +1,6 @@ const checkAllowScripts = require('./check-allow-scripts.js') const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js') +const { strictAllowScriptsError } = require('@npmcli/arborist/lib/unreviewed-scripts.js') const { configSetAllowScripts } = require('./allow-scripts-remediation.js') // Pre-flight check for `--strict-allow-scripts`. Call after arborist has @@ -38,16 +39,6 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => { return } - const lines = unreviewed.map(({ node, scripts }) => { - const events = Object.entries(scripts) - .map(([event, body]) => `${event}: ${body}`) - .join('; ') - const name = node.package?.name || node.name - const version = node.package?.version || '' - const label = version ? `${name}@${version}` : name - return ` ${label} (${events})` - }).join('\n') - // `npm approve-scripts` / `npm deny-scripts` write 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`, @@ -63,14 +54,12 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => { '`npm deny-scripts`, or bypass this check with ' + '`--dangerously-allow-all-scripts`.' - throw Object.assign( - new Error( - `--strict-allow-scripts: ${unreviewed.length} package(s) have install ` + - `scripts not covered by allowScripts:\n${lines}\n` + - remediation - ), - { code: 'ESTRICTALLOWSCRIPTS' } - ) + // Build via the shared arborist helper so the thrown error carries the + // structured dependency `explanation` (one node.explain() per unreviewed + // package). This lets the CLI show *which path* pulled each package in — + // removing that path is how a user avoids the prompt. The message produced + // here is identical to the inline form this previously threw. + throw strictAllowScriptsError(unreviewed, { remediation }) } module.exports = strictAllowScriptsPreflight diff --git a/tap-snapshots/test/lib/utils/error-message.js.test.cjs b/tap-snapshots/test/lib/utils/error-message.js.test.cjs index a63412c96ea4a..e628ed2afc5fe 100644 --- a/tap-snapshots/test/lib/utils/error-message.js.test.cjs +++ b/tap-snapshots/test/lib/utils/error-message.js.test.cjs @@ -1109,6 +1109,33 @@ Object { } ` +exports[`test/lib/utils/error-message.js TAP explain dependency resolution errors > must match snapshot 1`] = ` +Object { + "detail": Array [ + Array [ + "notarget", + "In most cases you or one of your dependencies are requesting a package version that doesn't exist.", + ], + Array [ + "", + "explanation", + ], + ], + "files": Array [ + Array [ + "etarget-report.txt", + "report", + ], + ], + "summary": Array [ + Array [ + "notarget", + "no matching version found for foo@9.9.9", + ], + ], +} +` + exports[`test/lib/utils/error-message.js TAP just simple messages > must match snapshot 1`] = ` Object { "detail": Array [], diff --git a/tap-snapshots/test/lib/utils/explain-dep-error.js.test.cjs b/tap-snapshots/test/lib/utils/explain-dep-error.js.test.cjs new file mode 100644 index 0000000000000..80d9682c18236 --- /dev/null +++ b/tap-snapshots/test/lib/utils/explain-dep-error.js.test.cjs @@ -0,0 +1,172 @@ +/* IMPORTANT + * This snapshot file is auto-generated, but designed for humans. + * It should be checked into source control and tracked carefully. + * Re-generate by setting TAP_SNAPSHOT=1 and running tests. + * Make sure to inspect the output below. Do not ignore changes! + */ +'use strict' +exports[`test/lib/utils/explain-dep-error.js TAP explain depth > strict scripts array, full depth 1`] = ` +has-install-script@1.2.3 +node_modules/has-install-script + has-install-script@"^1.0.0" from the root project + +nested-install-script@4.5.6 +node_modules/parent/node_modules/nested-install-script + nested-install-script@"^4.0.0" from parent@7.0.0 + node_modules/parent + parent@"^7.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP explain depth > transitive, depth 2 1`] = ` +foo@"^9.9.9" from bar@1.0.0 +node_modules/bar + bar@"^1.0.0" from baz@2.0.0 + node_modules/baz +` + +exports[`test/lib/utils/explain-dep-error.js TAP explain depth > transitive, full depth 1`] = ` +foo@"^9.9.9" from bar@1.0.0 +node_modules/bar + bar@"^1.0.0" from baz@2.0.0 + node_modules/baz + dev baz@"^2.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report directEdge > report file (no color) 1`] = ` +# npm ETARGET error report + +Could not resolve dependency: +foo@"^9.9.9" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report directEdge > report with color 1`] = ` +Could not resolve dependency: +foo@"^9.9.9" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report directEdge > report with no color 1`] = ` +Could not resolve dependency: +foo@"^9.9.9" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report incompatibleNode > report file (no color) 1`] = ` +# npm EBADPLATFORM error report + +This package is installed because: +fsevents@2.3.3 +node_modules/fsevents + fsevents@"^2.3.0" from chokidar@3.6.0 + node_modules/chokidar + dev chokidar@"^3.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report incompatibleNode > report with color 1`] = ` +This package is installed because: +fsevents@2.3.3 +node_modules/fsevents + fsevents@"^2.3.0" from chokidar@3.6.0 + node_modules/chokidar + dev chokidar@"^3.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report incompatibleNode > report with no color 1`] = ` +This package is installed because: +fsevents@2.3.3 +node_modules/fsevents + fsevents@"^2.3.0" from chokidar@3.6.0 + node_modules/chokidar + dev chokidar@"^3.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report remoteEdge > report file (no color) 1`] = ` +# npm EALLOWREMOTE error report + +Could not resolve dependency: +sketchy@"github:foo/sketchy" from middle@3.1.4 +node_modules/middle + middle@"^3.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report remoteEdge > report with color 1`] = ` +Could not resolve dependency: +sketchy@"github:foo/sketchy" from middle@3.1.4 +node_modules/middle + middle@"^3.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report remoteEdge > report with no color 1`] = ` +Could not resolve dependency: +sketchy@"github:foo/sketchy" from middle@3.1.4 +node_modules/middle + middle@"^3.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report strictScripts > report file (no color) 1`] = ` +# npm ESTRICTALLOWSCRIPTS error report + +These packages are installed because: +has-install-script@1.2.3 +node_modules/has-install-script + has-install-script@"^1.0.0" from the root project + +nested-install-script@4.5.6 +node_modules/parent/node_modules/nested-install-script + nested-install-script@"^4.0.0" from parent@7.0.0 + node_modules/parent + parent@"^7.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report strictScripts > report with color 1`] = ` +These packages are installed because: +has-install-script@1.2.3 +node_modules/has-install-script + has-install-script@"^1.0.0" from the root project + +nested-install-script@4.5.6 +node_modules/parent/node_modules/nested-install-script + nested-install-script@"^4.0.0" from parent@7.0.0 + node_modules/parent + parent@"^7.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report strictScripts > report with no color 1`] = ` +These packages are installed because: +has-install-script@1.2.3 +node_modules/has-install-script + has-install-script@"^1.0.0" from the root project + +nested-install-script@4.5.6 +node_modules/parent/node_modules/nested-install-script + nested-install-script@"^4.0.0" from parent@7.0.0 + node_modules/parent + parent@"^7.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report transitiveEdge > report file (no color) 1`] = ` +# npm ETARGET error report + +Could not resolve dependency: +foo@"^9.9.9" from bar@1.0.0 +node_modules/bar + bar@"^1.0.0" from baz@2.0.0 + node_modules/baz + dev baz@"^2.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report transitiveEdge > report with color 1`] = ` +Could not resolve dependency: +foo@"^9.9.9" from bar@1.0.0 +node_modules/bar + bar@"^1.0.0" from baz@2.0.0 + node_modules/baz + dev baz@"^2.0.0" from the root project +` + +exports[`test/lib/utils/explain-dep-error.js TAP report transitiveEdge > report with no color 1`] = ` +Could not resolve dependency: +foo@"^9.9.9" from bar@1.0.0 +node_modules/bar + bar@"^1.0.0" from baz@2.0.0 + node_modules/baz + dev baz@"^2.0.0" from the root project +` diff --git a/test/fixtures/dep-error-explanations.js b/test/fixtures/dep-error-explanations.js new file mode 100644 index 0000000000000..5bde98df68a31 --- /dev/null +++ b/test/fixtures/dep-error-explanations.js @@ -0,0 +1,137 @@ +// Sample dependency-explanation objects attached to install resolution +// errors. Edge-shaped objects come from `edge.explain()` (ETARGET, +// EALLOWREMOTE/EALLOWGIT, tarball/network, EINCOMPLETEMANIFEST). The array of +// node-shaped objects comes from `node.explain()` (ESTRICTALLOWSCRIPTS). +module.exports = { + // a direct, top-level dependency request that failed to resolve + directEdge: { + type: 'prod', + name: 'foo', + spec: '^9.9.9', + from: { + location: '/some/project', + }, + }, + + // a transitive dependency request, requested via a chain of dependents + transitiveEdge: { + type: 'prod', + name: 'foo', + spec: '^9.9.9', + from: { + name: 'bar', + version: '1.0.0', + location: 'node_modules/bar', + dependents: [ + { + type: 'prod', + name: 'bar', + spec: '^1.0.0', + from: { + name: 'baz', + version: '2.0.0', + location: 'node_modules/baz', + dependents: [ + { + type: 'dev', + name: 'baz', + spec: '^2.0.0', + from: { location: '/some/project' }, + }, + ], + }, + }, + ], + }, + }, + + // a git/remote dependency blocked by an allow-* policy + remoteEdge: { + type: 'prod', + name: 'sketchy', + spec: 'github:foo/sketchy', + from: { + name: 'middle', + version: '3.1.4', + location: 'node_modules/middle', + dependents: [ + { + type: 'prod', + name: 'middle', + spec: '^3.0.0', + from: { location: '/some/project' }, + }, + ], + }, + }, + + // EBADPLATFORM/EBADENGINE: a single node explanation for a package that + // resolved fine but is incompatible with the current OS/CPU/engine + incompatibleNode: { + name: 'fsevents', + version: '2.3.3', + location: 'node_modules/fsevents', + dependents: [ + { + type: 'prod', + name: 'fsevents', + spec: '^2.3.0', + from: { + name: 'chokidar', + version: '3.6.0', + location: 'node_modules/chokidar', + dependents: [ + { + type: 'dev', + name: 'chokidar', + spec: '^3.0.0', + from: { location: '/some/project' }, + }, + ], + }, + }, + ], + }, + + // ESTRICTALLOWSCRIPTS: an array of node explanations, one per unreviewed pkg + strictScripts: [ + { + name: 'has-install-script', + version: '1.2.3', + location: 'node_modules/has-install-script', + dependents: [ + { + type: 'prod', + name: 'has-install-script', + spec: '^1.0.0', + from: { location: '/some/project' }, + }, + ], + }, + { + name: 'nested-install-script', + version: '4.5.6', + location: 'node_modules/parent/node_modules/nested-install-script', + dependents: [ + { + type: 'prod', + name: 'nested-install-script', + spec: '^4.0.0', + from: { + name: 'parent', + version: '7.0.0', + location: 'node_modules/parent', + dependents: [ + { + type: 'prod', + name: 'parent', + spec: '^7.0.0', + from: { location: '/some/project' }, + }, + ], + }, + }, + ], + }, + ], +} diff --git a/test/lib/utils/error-message.js b/test/lib/utils/error-message.js index 44a57eca645d7..6d5e6a5173650 100644 --- a/test/lib/utils/error-message.js +++ b/test/lib/utils/error-message.js @@ -442,3 +442,90 @@ t.test('explain ERESOLVE errors', async t => { t.not(EXPLAIN_CALLED[1].level, 0, 'color chalk level is not 0') t.equal(EXPLAIN_CALLED[2].level, 0, 'colorless chalk level is 0') }) + +t.test('explain dependency resolution errors', async t => { + const EXPLAIN_CALLED = [] + + const { errorMessage } = await loadMockNpm(t, { + errorMocks: { + '{LIB}/utils/explain-dep-error.js': { + report: (...args) => { + EXPLAIN_CALLED.push(...args) + return { explanation: 'explanation', file: 'report' } + }, + }, + }, + config: { + color: 'always', + }, + }) + + const er = Object.assign(new Error('no matching version found for foo@9.9.9'), { + code: 'ETARGET', + explanation: { type: 'prod', name: 'foo', spec: '9.9.9', from: { location: '/x' } }, + }) + + const res = errorMessage(er) + t.matchSnapshot(res) + t.equal(EXPLAIN_CALLED.length, 3, 'report() called once') + t.match(EXPLAIN_CALLED, [er, Function, Function]) + t.not(EXPLAIN_CALLED[1].level, 0, 'color chalk level is not 0') + t.equal(EXPLAIN_CALLED[2].level, 0, 'colorless chalk level is 0') + t.match(res.files, [['etarget-report.txt', 'report']], 'report file uses code-based name') + + // a non-empty array explanation (ESTRICTALLOWSCRIPTS) also renders + const arrEr = Object.assign(new Error('strict scripts'), { + code: 'ESTRICTALLOWSCRIPTS', + explanation: [{ name: 'a', version: '1.0.0' }], + }) + const arrRes = errorMessage(arrEr) + t.match(arrRes.detail, [['', 'explanation']], 'array explanation is rendered') + t.match(arrRes.files, [['estrictallowscripts-report.txt', 'report']], 'report file written') +}) + +t.test('ERESOLVE does not double-format via explain-dep-error', async t => { + const DEP_ERROR_CALLED = [] + + const { errorMessage } = await loadMockNpm(t, { + errorMocks: { + '{LIB}/utils/explain-eresolve.js': { + report: () => ({ explanation: 'eresolve', file: 'eresolve-report' }), + }, + '{LIB}/utils/explain-dep-error.js': { + report: (...args) => { + DEP_ERROR_CALLED.push(...args) + return { explanation: 'explanation', file: 'report' } + }, + }, + }, + }) + + // even if an ERESOLVE error somehow also carries an explanation, the generic + // block must not run for it (it has its own richer formatting) + const er = Object.assign(new Error('could not resolve'), { + code: 'ERESOLVE', + explanation: { type: 'prod', name: 'foo', spec: '1', from: { location: '/x' } }, + }) + + errorMessage(er) + t.equal(DEP_ERROR_CALLED.length, 0, 'explain-dep-error.report not called for ERESOLVE') +}) + +t.test('errors without an explanation are unaffected', async t => { + const DEP_ERROR_CALLED = [] + + const { errorMessage } = await loadMockNpm(t, { + errorMocks: { + '{LIB}/utils/explain-dep-error.js': { + report: (...args) => { + DEP_ERROR_CALLED.push(...args) + return { explanation: 'explanation', file: 'report' } + }, + }, + }, + }) + + errorMessage(Object.assign(new Error('nope'), { code: 'ETARGET' })) + errorMessage(Object.assign(new Error('empty'), { code: 'ESTRICTALLOWSCRIPTS', explanation: [] })) + t.equal(DEP_ERROR_CALLED.length, 0, 'no report when explanation missing or empty') +}) diff --git a/test/lib/utils/explain-dep-error.js b/test/lib/utils/explain-dep-error.js new file mode 100644 index 0000000000000..407ae67530b83 --- /dev/null +++ b/test/lib/utils/explain-dep-error.js @@ -0,0 +1,58 @@ +const t = require('tap') +const { explain, report } = require('../../../lib/utils/explain-dep-error.js') + +const cases = require('../../fixtures/dep-error-explanations.js') + +// map fixture name -> the error code that would carry that explanation +const codeFor = (name) => name === 'remoteEdge' ? 'EALLOWREMOTE' + : name === 'strictScripts' ? 'ESTRICTALLOWSCRIPTS' + : name === 'incompatibleNode' ? 'EBADPLATFORM' + : 'ETARGET' + +t.test('report', async t => { + const { Chalk } = await import('chalk') + const color = new Chalk({ level: 3 }) + const noColor = new Chalk({ level: 0 }) + + for (const [name, expl] of Object.entries(cases)) { + t.test(name, t => { + const er = { code: codeFor(name), explanation: expl } + + const colorReport = report(er, color, noColor) + t.matchSnapshot(colorReport.explanation, 'report with color') + t.matchSnapshot(colorReport.file, 'report file (no color)') + + const noColorReport = report(er, noColor, noColor) + t.matchSnapshot(noColorReport.explanation, 'report with no color') + t.equal(noColorReport.file, colorReport.file, 'same file written regardless of terminal color') + + t.match(colorReport.file, new RegExp(`# npm ${er.code} error report`), + 'file is headed with the error code') + + t.end() + }) + } +}) + +t.test('explain depth', async t => { + const { Chalk } = await import('chalk') + const noColor = new Chalk({ level: 0 }) + + t.matchSnapshot(explain(cases.transitiveEdge, noColor, 2), 'transitive, depth 2') + t.matchSnapshot(explain(cases.transitiveEdge, noColor, Infinity), 'transitive, full depth') + t.matchSnapshot(explain(cases.strictScripts, noColor, Infinity), 'strict scripts array, full depth') +}) + +t.test('empty explanation', async t => { + const { Chalk } = await import('chalk') + const noColor = new Chalk({ level: 0 }) + t.equal(explain(undefined, noColor, 4), '', 'undefined explanation -> empty string') + t.equal(explain([], noColor, 4), '', 'empty array -> empty string') + + // report() is exported and may be called directly; it must not throw on an + // absent/empty explanation (error-message.js already guards this path). + t.same(report({ code: 'ETARGET', explanation: [] }, noColor, noColor), + { explanation: '', file: '' }, 'empty array -> empty report') + t.same(report({ code: 'ETARGET' }, noColor, noColor), + { explanation: '', file: '' }, 'missing explanation -> empty report') +}) diff --git a/test/lib/utils/strict-allow-scripts-preflight.js b/test/lib/utils/strict-allow-scripts-preflight.js index e0c54105f8863..b9f692a9ecdc4 100644 --- a/test/lib/utils/strict-allow-scripts-preflight.js +++ b/test/lib/utils/strict-allow-scripts-preflight.js @@ -219,3 +219,45 @@ t.test('global error points at --allow-scripts, not approve-scripts', async t => } ) }) + +t.test('error carries a dependency explanation for each unreviewed package', async t => { + // Real arborist tree nodes have an explain() method; the error must carry + // one structured explanation per unreviewed package so the CLI can show + // which path pulled it in. + const explained = node({ name: 'fsevents', version: '2.3.3' }) + explained.explain = () => ({ + name: 'fsevents', + version: '2.3.3', + location: 'node_modules/fsevents', + dependents: [ + { type: 'prod', name: 'fsevents', spec: '^2.3.0', from: { location: '/proj' } }, + ], + }) + const arb = makeArb({ ideal: tree([explained]) }) + + const err = await preflight({ + arb, + npm: { flatOptions: { strictAllowScripts: true } }, + idealTreeOpts: {}, + }).then(() => null, e => e) + + t.equal(err?.code, 'ESTRICTALLOWSCRIPTS') + t.ok(Array.isArray(err.explanation) && err.explanation.length === 1, + 'one explanation per unreviewed package') + t.equal(err.explanation[0].name, 'fsevents', 'explanation names the package') + t.ok(err.explanation[0].dependents.length, 'explanation records the requiring path') +}) + +t.test('explanation is best-effort: nodes without explain() do not throw', async t => { + // The plain fixture nodes have no explain(); the error must still be thrown + // with its message intact and simply no explanation attached. + const arb = makeArb({ ideal: tree([node({ name: 'canvas' })]) }) + const err = await preflight({ + arb, + npm: { flatOptions: { strictAllowScripts: true } }, + idealTreeOpts: {}, + }).then(() => null, e => e) + t.equal(err?.code, 'ESTRICTALLOWSCRIPTS') + t.match(err.message, /canvas/, 'error message is intact') + t.equal(err.explanation, undefined, 'no explanation attached when explain() is unavailable') +}) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 1de07a37f8f55..66e8cf4c7b000 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -41,9 +41,15 @@ const ALLOW_OPTION_FOR_TYPE = { const addRmPkgDeps = require('../add-rm-pkg-deps.js') const optionalSet = require('../optional-set.js') const { checkEngine, checkPlatform } = require('npm-install-checks') +const { attachExplanation } = require('../attach-explanation.js') const relpath = require('../relpath.js') const resetDepFlags = require('../reset-dep-flags.js') +// Stashes the edge that required a failed load on its error, so #failureNode +// can defer the (potentially wasted) explain() until #pruneFailedOptional +// decides the failure is fatal. Optional failures are dropped without it. +const _failureEdge = Symbol('failureEdge') + // note: some of these symbols are shared so we can hit // them with unit tests and reuse them across mixins const _updateAll = Symbol.for('updateAll') @@ -201,7 +207,8 @@ module.exports = cls => class IdealTreeBuilder extends cls { } } catch (err) { if (engineStrict) { - throw err + // explain which path pulled in the incompatible package + throw attachExplanation(err, () => node.explain()) } log.warn(err.code, err.message, { package: err.pkgid, @@ -209,7 +216,11 @@ module.exports = cls => class IdealTreeBuilder extends cls { current: err.current, }) } - checkPlatform(node.package, this.options.force) + try { + checkPlatform(node.package, this.options.force) + } catch (err) { + throw attachExplanation(err, () => node.explain()) + } } if (node.optional && !node.inert) { // Mark any optional packages we can't install as inert. @@ -691,6 +702,9 @@ module.exports = cls => class IdealTreeBuilder extends cls { // Builds a Node representing a spec we failed to load (allow-* gate, network failure, ENOTARGET, etc.) and records it in #loadFailures so #pruneFailedOptional can later decide whether the failure is fatal or silently dropped for optional deps. #failureNode (name, parent, error, edge) { error.requiredBy = edge?.from?.location || '.' + // Stash the requiring edge; #pruneFailedOptional turns it into an + // explanation only if the failure is fatal (optional ones are dropped). + error[_failureEdge] = edge const n = new Node({ name, parent, @@ -1615,7 +1629,12 @@ This is a one-time fix-up, please be patient... #pruneFailedOptional () { for (const node of this.#loadFailures) { if (!node.optional) { - throw node.errors[0] + // Only now that the failure is fatal do we walk the graph for the + // "why is this here" explanation (see #failureNode). Failures that did + // not come through #failureNode have no stashed edge; attachExplanation + // swallows the resulting error and simply attaches nothing. + const err = node.errors[0] + throw attachExplanation(err, () => err[_failureEdge].explain()) } const set = optionalSet(node) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index e76a277ebbf92..94cd8372a3b14 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -26,6 +26,7 @@ const optionalSet = require('../optional-set.js') const relpath = require('../relpath.js') const retirePath = require('../retire-path.js') const treeCheck = require('../tree-check.js') +const { attachExplanation } = require('../attach-explanation.js') const { defaultLockfileVersion } = require('../shrinkwrap.js') const { saveTypeMap, hasSubKey } = require('../add-rm-pkg-deps.js') const { IsolatedNode, IsolatedLink } = require('../isolated-classes.js') @@ -701,20 +702,28 @@ module.exports = cls => class Reifier extends cls { }) } }) - await pacote.extract(res, node.path, { - ...this.options, - resolved: node.resolved, - integrity: node.integrity, - // A node counts as "root" for allow-* enforcement if it satisfies at least one valid dependency edge declared by the project root or a workspace. - // node.parent is unsafe here: after hoisting, transitive packages can have the project root as their tree parent. - // In the linked strategy the store node has no edgesIn, so isolated-reifier precomputes isRootDependency from the source node's edges. - _isRoot: node.isRootDependency || [...node.edgesIn].some(e => - e.valid && (e.from?.isProjectRoot || e.from?.isWorkspace) - ), - // pacote's npa re-parses our `name@URL` spec as type=remote, so allowRemote would mis-fire on registry tarballs. - // Override only when we can prove the URL is registry-mediated; see #isRegistryResolvedTarball. - ...(this.#isRegistryResolvedTarball(node) ? { allowRemote: 'all' } : {}), - }) + try { + await pacote.extract(res, node.path, { + ...this.options, + resolved: node.resolved, + integrity: node.integrity, + // A node counts as "root" for allow-* enforcement if it satisfies at least one valid dependency edge declared by the project root or a workspace. + // node.parent is unsafe here: after hoisting, transitive packages can have the project root as their tree parent. + // In the linked strategy the store node has no edgesIn, so isolated-reifier precomputes isRootDependency from the source node's edges. + _isRoot: node.isRootDependency || [...node.edgesIn].some(e => + e.valid && (e.from?.isProjectRoot || e.from?.isWorkspace) + ), + // pacote's npa re-parses our `name@URL` spec as type=remote, so allowRemote would mis-fire on registry tarballs. + // Override only when we can prove the URL is registry-mediated; see #isRegistryResolvedTarball. + ...(this.#isRegistryResolvedTarball(node) ? { allowRemote: 'all' } : {}), + }) + } catch (error) { + // Reify-time fetch/extract failures (EALLOWREMOTE/EALLOWGIT, tarball or + // network errors, integrity mismatches) come from pacote and never pass + // through the resolution-phase #failureNode, so explain here too. Hit + // when a node is reused from the lockfile and only fetched at reify. + throw attachExplanation(error, () => node.explain()) + } // store nodes don't use Node class so node.package doesn't get updated if (node.isInStore) { const { content: pkg } = await PackageJson.normalize(node.path) diff --git a/workspaces/arborist/lib/attach-explanation.js b/workspaces/arborist/lib/attach-explanation.js new file mode 100644 index 0000000000000..cc33dc989f915 --- /dev/null +++ b/workspaces/arborist/lib/attach-explanation.js @@ -0,0 +1,22 @@ +// Best-effort attachment of a dependency "explanation" to an error, so the CLI +// can show *why* a package is in the tree (mirroring how ERESOLVE errors carry +// explain() data). `compute` returns the explanation (an edge.explain() / +// node.explain() result, or an array of them) and is invoked lazily, so the +// potentially expensive graph walk only runs when a caller is actually failing. +// Never throws and never overwrites an existing explanation. +const attachExplanation = (err, compute) => { + if (!err || typeof err !== 'object' || err.explanation !== undefined) { + return err + } + try { + const explanation = compute() + if (explanation !== undefined) { + err.explanation = explanation + } + } catch { + // advisory only; never mask the original error + } + return err +} + +module.exports = { attachExplanation } diff --git a/workspaces/arborist/lib/unreviewed-scripts.js b/workspaces/arborist/lib/unreviewed-scripts.js index 587d06bf8fe56..a26bd1ddc7e8e 100644 --- a/workspaces/arborist/lib/unreviewed-scripts.js +++ b/workspaces/arborist/lib/unreviewed-scripts.js @@ -1,5 +1,6 @@ const isScriptAllowed = require('./script-allowed.js') const getInstallScripts = require('./install-scripts.js') +const { attachExplanation } = require('./attach-explanation.js') // Shared allowScripts walk used by both the npm CLI // (lib/utils/check-allow-scripts.js, lib/utils/strict-allow-scripts-preflight.js) @@ -82,13 +83,15 @@ const strictAllowScriptsError = (unreviewed, { remediation } = {}) => { return ` ${label} (${events})` }).join('\n') - return Object.assign( + const error = Object.assign( new Error( `--strict-allow-scripts: ${unreviewed.length} package(s) have install ` + `scripts not covered by allowScripts:\n${lines}\n${remediation}` ), { code: 'ESTRICTALLOWSCRIPTS' } ) + // explain *why* each unreviewed package is in the tree (one per package) + return attachExplanation(error, () => unreviewed.map(({ node }) => node.explain())) } module.exports = { collectUnreviewedScripts, strictAllowScriptsError } diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index 18fed1bcd65cf..f8801bf7d97b3 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -67,13 +67,14 @@ const printIdeal = (path, opt) => buildIdeal(path, opt).then(printTree) t.test('fail on mismatched engine when engineStrict is set', async t => { const path = resolve(fixtures, 'engine-specification') - await t.rejects(buildIdeal(path, { + const err = await buildIdeal(path, { nodeVersion: '12.18.4', engineStrict: true, - }), - { code: 'EBADENGINE' }, - 'should fail with EBADENGINE error' - ) + }).then(() => null, e => e) + t.equal(err?.code, 'EBADENGINE', 'should fail with EBADENGINE error') + // engine/platform errors carry a dependency explanation so the CLI can show + // which path pulled in the incompatible package (removing it avoids the error) + t.ok(err.explanation, 'EBADENGINE error carries a dependency explanation') }) t.test('fail on malformed package.json', async t => { @@ -109,9 +110,12 @@ t.test('warn on mismatched engine when engineStrict is false', t => { t.test('fail on mismatched platform', async t => { const path = resolve(fixtures, 'platform-specification') createRegistry(t, true) - await t.rejects(buildIdeal(path, { + const err = await buildIdeal(path, { nodeVersion: '4.0.0', - }), { code: 'EBADPLATFORM' }) + }).then(() => null, e => e) + t.equal(err?.code, 'EBADPLATFORM', 'should fail with EBADPLATFORM error') + t.ok(err.explanation, 'EBADPLATFORM error carries a dependency explanation') + t.ok(err.explanation.name, 'explanation names the incompatible package') }) t.test('ignore mismatched platform for optional dependencies', async t => { @@ -4848,11 +4852,13 @@ t.test('allow-directory=root blocks a transitive directory dependency', async t }, }, }) - await t.rejects( - buildIdeal(path, { allowDirectory: 'root' }), - { code: 'EALLOWDIRECTORY' }, - 'transitive directory dep is refused because edge.from is not the project root' - ) + const err = await buildIdeal(path, { allowDirectory: 'root' }).then(() => null, e => e) + t.equal(err?.code, 'EALLOWDIRECTORY', + 'transitive directory dep is refused because edge.from is not the project root') + // the explanation is computed lazily in #pruneFailedOptional only because + // this failure is fatal; it records the path that required the blocked dep + t.ok(err.explanation, 'fatal load failure carries a dependency explanation') + t.equal(err.explanation.name, 'child', 'explanation names the blocked dependency') }) t.test('allow-directory=root soft-skips a transitive optional directory dependency', async t => { diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index adb01394f2091..3f84813d4dac8 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -3854,7 +3854,15 @@ t.test('should preserve exact ranges, missing actual tree', async (t) => { allowRemote: 'none', }) - await t.rejects(arb.reify(), { code: 'EALLOWREMOTE' }, 'sibling path tarball is blocked') + const err = await arb.reify().then(() => null, e => e) + t.equal(err?.code, 'EALLOWREMOTE', 'sibling path tarball is blocked') + // reify-time fetch failures must carry a dependency explanation so the CLI + // can show *who required* the blocked package (npm/cli regression: this was + // missing because the block happens in pacote, not arborist resolution). + t.ok(err.explanation, 'EALLOWREMOTE error carries a dependency explanation') + t.equal(err.explanation.name, 'abbrev', 'explanation names the blocked package') + t.ok(Array.isArray(err.explanation.dependents) && err.explanation.dependents.length, + 'explanation records who required the package') }) t.test('allowRemote=none allows same-origin tarball for root registry path', async t => { diff --git a/workspaces/arborist/test/attach-explanation.js b/workspaces/arborist/test/attach-explanation.js new file mode 100644 index 0000000000000..225dfba5c0e51 --- /dev/null +++ b/workspaces/arborist/test/attach-explanation.js @@ -0,0 +1,57 @@ +const t = require('tap') +const { attachExplanation } = require('../lib/attach-explanation.js') + +t.test('attaches a computed explanation to an error object', t => { + const err = new Error('boom') + const ret = attachExplanation(err, () => ({ name: 'x' })) + t.equal(ret, err, 'returns the same error') + t.same(err.explanation, { name: 'x' }, 'explanation attached') + t.end() +}) + +t.test('does not overwrite an existing explanation', t => { + const err = Object.assign(new Error('boom'), { explanation: { name: 'kept' } }) + let called = false + attachExplanation(err, () => { + called = true + return { name: 'new' } + }) + t.notOk(called, 'compute not invoked') + t.same(err.explanation, { name: 'kept' }, 'original explanation kept') + t.end() +}) + +t.test('no-op on a non-object error (string throw)', t => { + let called = false + const ret = attachExplanation('just a string', () => { + called = true + return {} + }) + t.equal(ret, 'just a string', 'returns the value unchanged') + t.notOk(called, 'compute not invoked') + t.end() +}) + +t.test('no-op on a nullish error', t => { + t.equal(attachExplanation(null, () => ({})), null) + t.equal(attachExplanation(undefined, () => ({})), undefined) + t.end() +}) + +t.test('skips attachment when compute returns undefined', t => { + const err = new Error('boom') + attachExplanation(err, () => undefined) + t.notOk('explanation' in err, 'no explanation property added') + t.end() +}) + +t.test('best-effort: a throwing compute never masks the original error', t => { + const err = new Error('boom') + const ret = attachExplanation(err, () => { + throw new Error('explain failed') + }) + t.equal(ret, err, 'returns the original error') + t.notOk('explanation' in err, 'no explanation attached') + t.equal(err.message, 'boom', 'original error untouched') + t.end() +})