From bb916b2976e8c6e961c280efe74fb5b2d0f66d38 Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Wed, 27 May 2026 23:59:56 -0700 Subject: [PATCH] feat: phase 2 of allowScripts opt-in install-script policy --- .../content/commands/npm-approve-scripts.md | 7 +- docs/lib/content/commands/npm-deny-scripts.md | 8 +- lib/commands/rebuild.js | 18 +- lib/utils/allow-scripts-cmd.js | 27 +-- lib/utils/check-allow-scripts.js | 64 ++----- lib/utils/reify-output.js | 5 +- lib/utils/strict-allow-scripts-preflight.js | 23 +-- .../tap-snapshots/test/index.js.test.cjs | 2 +- tap-snapshots/test/lib/docs.js.test.cjs | 25 +-- test/lib/commands/approve-scripts.js | 20 ++- test/lib/commands/edit.js | 5 + test/lib/commands/rebuild.js | 98 +++++++++- test/lib/utils/check-allow-scripts.js | 51 +----- test/lib/utils/reify-output.js | 28 ++- .../arborist/lib/arborist/isolated-reifier.js | 5 +- workspaces/arborist/lib/arborist/rebuild.js | 30 ++-- workspaces/arborist/lib/isolated-classes.js | 4 + workspaces/arborist/lib/script-allowed.js | 24 +-- workspaces/arborist/lib/unreviewed-scripts.js | 89 +++++++++ workspaces/arborist/test/arborist/rebuild.js | 24 ++- workspaces/arborist/test/arborist/reify.js | 8 +- workspaces/arborist/test/isolated-mode.js | 4 +- workspaces/arborist/test/script-allowed.js | 47 ++++- .../arborist/test/unreviewed-scripts.js | 169 ++++++++++++++++++ .../config/lib/definitions/definitions.js | 19 +- workspaces/libnpmexec/lib/index.js | 19 +- .../lib/strict-allow-scripts-preflight.js | 40 +++++ .../test/strict-allow-scripts-preflight.js | 82 +++++++++ 28 files changed, 707 insertions(+), 238 deletions(-) create mode 100644 workspaces/arborist/lib/unreviewed-scripts.js create mode 100644 workspaces/arborist/test/unreviewed-scripts.js create mode 100644 workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js create mode 100644 workspaces/libnpmexec/test/strict-allow-scripts-preflight.js diff --git a/docs/lib/content/commands/npm-approve-scripts.md b/docs/lib/content/commands/npm-approve-scripts.md index 70abe153369d6..99e684a31607b 100644 --- a/docs/lib/content/commands/npm-approve-scripts.md +++ b/docs/lib/content/commands/npm-approve-scripts.md @@ -15,9 +15,10 @@ records which of your dependencies are permitted to run install scripts (`preinstall`, `install`, `postinstall`, and `prepare` for non-registry sources). This command is the recommended way to maintain that field. -In the current release, this field is advisory: install scripts still run -by default, but installs print a list of packages whose scripts have not -been reviewed. A future release will block unreviewed install scripts. +Dependency install scripts are blocked by default. Install commands +silently skip lifecycle scripts for any dependency that does not have a +matching entry in `allowScripts`, and end with a list of the packages +whose scripts were skipped so you can review them with this command. There are three modes: diff --git a/docs/lib/content/commands/npm-deny-scripts.md b/docs/lib/content/commands/npm-deny-scripts.md index d03028d6c2f51..4aee897891c1d 100644 --- a/docs/lib/content/commands/npm-deny-scripts.md +++ b/docs/lib/content/commands/npm-deny-scripts.md @@ -15,10 +15,10 @@ Writes `false` entries into the `allowScripts` field of your project's `package.json`, recording that a dependency must not run install scripts even if a future version would otherwise be eligible. -In the current release, install scripts still run by default, so `deny-scripts` -only affects how installs of denied packages are reported. A future release -will block unreviewed install scripts and respect deny entries at install -time. +Dependency install scripts are blocked by default. Adding a `false` +entry with `deny-scripts` makes the denial explicit (so it survives +`npm approve-scripts --all`) and excludes the package from any future +`--allow-scripts-pending` review prompts. ```bash npm deny-scripts [ ...] diff --git a/lib/commands/rebuild.js b/lib/commands/rebuild.js index 333a879026cbc..4dad32659bd20 100644 --- a/lib/commands/rebuild.js +++ b/lib/commands/rebuild.js @@ -56,7 +56,7 @@ class Rebuild extends ArboristWorkspaceCmd { return spec }) - const nodes = tree.inventory.filter(node => this.isNode(specs, node)) + const nodes = [...tree.inventory.filter(node => this.isNode(specs, node))] await strictAllowScriptsPreflight({ arb, npm: this.npm }) await arb.rebuild({ nodes }) @@ -66,16 +66,15 @@ class Rebuild extends ArboristWorkspaceCmd { await arb.rebuild() } - // Phase 1 advisory: list any packages whose install scripts ran (or - // would have run) and are not yet covered by allowScripts. Rebuild - // doesn't go through reifyFinish, so the walker is invoked here. + // Rebuild skips reifyFinish, so run the walker here to list any + // packages whose install scripts were blocked. const unreviewed = await checkAllowScripts({ arb, npm: this.npm }) if (unreviewed.length > 0) { const count = unreviewed.length - const noun = count === 1 ? 'package has' : 'packages have' + const noun = count === 1 ? 'package had' : 'packages had' log.warn( 'rebuild', - `${count} ${noun} install scripts not yet covered by allowScripts. ` + + `${count} ${noun} install scripts blocked because they are not covered by allowScripts. ` + 'Run `npm approve-scripts --allow-scripts-pending` to review.' ) } @@ -84,6 +83,13 @@ class Rebuild extends ArboristWorkspaceCmd { } isNode (specs, node) { + // Bundled dependencies are never selected by name. Their identity comes + // from the bundling parent's tarball (a bundled folder can call itself + // anything), so `npm rebuild bcrypt` must never target a bundled + // `node_modules/bcrypt`. Their install scripts never run regardless. + if (node.inBundle) { + return false + } return specs.some(spec => { if (spec.type === 'directory') { return node.path === spec.fetchSpec diff --git a/lib/utils/allow-scripts-cmd.js b/lib/utils/allow-scripts-cmd.js index c1ff242abeaa8..93a79c31c2649 100644 --- a/lib/utils/allow-scripts-cmd.js +++ b/lib/utils/allow-scripts-cmd.js @@ -84,7 +84,7 @@ class AllowScriptsCmd extends BaseCommand { const has = count === 1 ? 'has' : 'have' const pkg = count === 1 ? 'package' : 'packages' output.standard( - `${count} ${pkg} ${has} install scripts not yet covered by allowScripts:` + `${count} ${pkg} ${has} install scripts blocked because they are not covered by allowScripts:` ) for (const { node, scripts } of unreviewed) { const { name, version } = trustedDisplay(node) @@ -107,27 +107,10 @@ class AllowScriptsCmd extends BaseCommand { output.standard('No packages with unreviewed install scripts.') return } - // Bundled dependencies cannot be allowlisted in Phase 1 (RFC defers - // this to a follow-up because matching by name@version from the - // bundled tarball would reintroduce manifest confusion). Exclude - // them from `--all` so we don't silently write a policy entry under - // attacker-controlled identity. - const candidates = unreviewed.filter(({ node }) => !node.inBundle) - const skipped = unreviewed.length - candidates.length - if (skipped > 0) { - /* istanbul ignore next: plural variant covered separately */ - const noun = skipped === 1 ? 'dependency' : 'dependencies' - log.warn( - this.logTitle, - `Skipping ${skipped} bundled ${noun}; bundled deps with install ` + - 'scripts cannot be allowlisted in this release.' - ) - } - if (candidates.length === 0) { - output.standard('No packages eligible for approval.') - return - } - const groups = this.groupByPackage(candidates.map(({ node }) => node)) + // Bundled dependencies never appear in `unreviewed` (checkAllowScripts + // skips them because they never run their install scripts and cannot + // be allowlisted), so there is nothing extra to filter here. + const groups = this.groupByPackage(unreviewed.map(({ node }) => node)) await this.writePolicyChanges(groups) } diff --git a/lib/utils/check-allow-scripts.js b/lib/utils/check-allow-scripts.js index 5ef2bfb74cf15..14d59a8204ca6 100644 --- a/lib/utils/check-allow-scripts.js +++ b/lib/utils/check-allow-scripts.js @@ -1,54 +1,22 @@ -const isScriptAllowed = require('@npmcli/arborist/lib/script-allowed.js') -const getInstallScripts = require('@npmcli/arborist/lib/install-scripts.js') +const { collectUnreviewedScripts } = require('@npmcli/arborist/lib/unreviewed-scripts.js') -// Walks arb.actualTree.inventory and returns the list of dep nodes that -// have install-relevant lifecycle scripts and are not yet covered (or -// explicitly denied) by the allowScripts policy. +// Walks a tree's inventory and returns the list of dep nodes that have +// install-relevant lifecycle scripts and are not yet covered (or explicitly +// denied) by the allowScripts policy. +// +// Thin wrapper around arborist's shared `collectUnreviewedScripts`, mapping +// the CLI's `({ arb, npm, tree })` shape onto the shared walk. Defaults to +// `arb.actualTree` (post-reify) but accepts an explicit tree so callers can +// pre-flight against the idealTree before scripts run. // // Returns an array of `{ node, scripts }` entries. `scripts` is an object // describing the relevant lifecycle scripts that would run. - -const checkAllowScripts = async ({ arb, npm, tree }) => { - const ignoreScripts = !!arb.options?.ignoreScripts - const dangerouslyAllowAll = !!npm?.flatOptions?.dangerouslyAllowAllScripts - - if (ignoreScripts || dangerouslyAllowAll) { - return [] - } - - // Defaults to actualTree (post-reify) but accepts an explicit tree so - // callers can pre-flight against the idealTree before scripts run. - const targetTree = tree || arb.actualTree - if (!targetTree?.inventory) { - return [] - } - - const policy = arb.options?.allowScripts || null - - const unreviewed = [] - for (const node of targetTree.inventory.values()) { - if (node.isProjectRoot || node.isWorkspace) { - continue - } - if (node.isLink) { - // Linked workspace dependencies are managed by the workspace owner. - continue - } - - const verdict = isScriptAllowed(node, policy) - if (verdict === true || verdict === false) { - continue - } - - const scripts = await getInstallScripts(node) - if (Object.keys(scripts).length === 0) { - continue - } - - unreviewed.push({ node, scripts }) - } - - return unreviewed -} +const checkAllowScripts = async ({ arb, npm, tree }) => + collectUnreviewedScripts({ + tree: tree || arb.actualTree, + policy: arb.options?.allowScripts || null, + ignoreScripts: !!arb.options?.ignoreScripts, + dangerouslyAllowAllScripts: !!npm?.flatOptions?.dangerouslyAllowAllScripts, + }) module.exports = checkAllowScripts diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js index b1e1ffbcddd17..a5b02b17caf69 100644 --- a/lib/utils/reify-output.js +++ b/lib/utils/reify-output.js @@ -240,8 +240,9 @@ const unreviewedScriptsMessage = (npm, unreviewedScripts) => { // stdout is reserved for things the user explicitly asked to see // (npm ls, npm view). const count = unreviewedScripts.length - const pkg = count === 1 ? 'package has' : 'packages have' - const header = `${count} ${pkg} install scripts not yet covered by allowScripts:` + const pkg = count === 1 ? 'package had' : 'packages had' + const header = + `${count} ${pkg} install scripts blocked because they are not covered by allowScripts:` const lines = unreviewedScripts.map(({ node, scripts }) => { const { name, version } = trustedDisplay(node) diff --git a/lib/utils/strict-allow-scripts-preflight.js b/lib/utils/strict-allow-scripts-preflight.js index a3f83ea4b662b..4519b7c6356c2 100644 --- a/lib/utils/strict-allow-scripts-preflight.js +++ b/lib/utils/strict-allow-scripts-preflight.js @@ -1,4 +1,5 @@ const checkAllowScripts = require('./check-allow-scripts.js') +const { strictAllowScriptsError } = require('@npmcli/arborist/lib/unreviewed-scripts.js') // Pre-flight check for `--strict-allow-scripts`. Call after arborist has // been constructed but before `arb.reify()` runs, so that install scripts @@ -36,26 +37,12 @@ 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') - - throw Object.assign( - new Error( - `--strict-allow-scripts: ${unreviewed.length} package(s) have install ` + - `scripts not covered by allowScripts:\n${lines}\n` + + throw strictAllowScriptsError(unreviewed, { + remediation: 'Approve them with `npm approve-scripts`, deny them with ' + '`npm deny-scripts`, or bypass this check with ' + - '`--dangerously-allow-all-scripts`.' - ), - { code: 'ESTRICTALLOWSCRIPTS' } - ) + '`--dangerously-allow-all-scripts`.', + }) } module.exports = strictAllowScriptsPreflight diff --git a/smoke-tests/tap-snapshots/test/index.js.test.cjs b/smoke-tests/tap-snapshots/test/index.js.test.cjs index 451efdeaaf677..94ade88222232 100644 --- a/smoke-tests/tap-snapshots/test/index.js.test.cjs +++ b/smoke-tests/tap-snapshots/test/index.js.test.cjs @@ -107,7 +107,7 @@ npm error --allow-scripts npm error Comma-separated list of packages whose install-time lifecycle scripts npm error npm error --strict-allow-scripts -npm error If \`true\`, turn the install-script policy from a warning into a hard +npm error If \`true\`, turn the install-script policy from a silent skip into a npm error npm error --dangerously-allow-all-scripts npm error If \`true\`, bypass the \`allowScripts\` policy entirely and run every diff --git a/tap-snapshots/test/lib/docs.js.test.cjs b/tap-snapshots/test/lib/docs.js.test.cjs index f6dfe04f369d8..6152ffc514e09 100644 --- a/tap-snapshots/test/lib/docs.js.test.cjs +++ b/tap-snapshots/test/lib/docs.js.test.cjs @@ -1900,12 +1900,13 @@ this to work properly. * Default: false * Type: Boolean -If \`true\`, turn the install-script policy from a warning into a hard error: -any dependency with install scripts not covered by \`allowScripts\` will fail -the install instead of running with a notice. +If \`true\`, turn the install-script policy from a silent skip into a hard +error: any dependency with install scripts not covered by \`allowScripts\` +will fail the install instead of being silently skipped. -Dependencies explicitly denied with \`false\` in \`allowScripts\` are always -silently skipped; this setting only affects unreviewed entries. +By default, dependencies whose install scripts are not approved in +\`allowScripts\` are silently skipped; this setting promotes that silent skip +into a hard failure, which is the recommended posture for CI. \`--ignore-scripts\` and \`--dangerously-allow-all-scripts\` both override this setting. @@ -3271,7 +3272,7 @@ Options: Comma-separated list of packages whose install-time lifecycle scripts --strict-allow-scripts - If \`true\`, turn the install-script policy from a warning into a hard + If \`true\`, turn the install-script policy from a silent skip into a --dangerously-allow-all-scripts If \`true\`, bypass the \`allowScripts\` policy entirely and run every @@ -3833,7 +3834,7 @@ Options: Comma-separated list of packages whose install-time lifecycle scripts --strict-allow-scripts - If \`true\`, turn the install-script policy from a warning into a hard + If \`true\`, turn the install-script policy from a silent skip into a --dangerously-allow-all-scripts If \`true\`, bypass the \`allowScripts\` policy entirely and run every @@ -4281,7 +4282,7 @@ Options: Comma-separated list of packages whose install-time lifecycle scripts --strict-allow-scripts - If \`true\`, turn the install-script policy from a warning into a hard + If \`true\`, turn the install-script policy from a silent skip into a --dangerously-allow-all-scripts If \`true\`, bypass the \`allowScripts\` policy entirely and run every @@ -4431,7 +4432,7 @@ Options: Comma-separated list of packages whose install-time lifecycle scripts --strict-allow-scripts - If \`true\`, turn the install-script policy from a warning into a hard + If \`true\`, turn the install-script policy from a silent skip into a --dangerously-allow-all-scripts If \`true\`, bypass the \`allowScripts\` policy entirely and run every @@ -4577,7 +4578,7 @@ Options: Comma-separated list of packages whose install-time lifecycle scripts --strict-allow-scripts - If \`true\`, turn the install-script policy from a warning into a hard + If \`true\`, turn the install-script policy from a silent skip into a --dangerously-allow-all-scripts If \`true\`, bypass the \`allowScripts\` policy entirely and run every @@ -5562,7 +5563,7 @@ Options: Comma-separated list of packages whose install-time lifecycle scripts --strict-allow-scripts - If \`true\`, turn the install-script policy from a warning into a hard + If \`true\`, turn the install-script policy from a silent skip into a --dangerously-allow-all-scripts If \`true\`, bypass the \`allowScripts\` policy entirely and run every @@ -6380,7 +6381,7 @@ Options: Comma-separated list of packages whose install-time lifecycle scripts --strict-allow-scripts - If \`true\`, turn the install-script policy from a warning into a hard + If \`true\`, turn the install-script policy from a silent skip into a --dangerously-allow-all-scripts If \`true\`, bypass the \`allowScripts\` policy entirely and run every diff --git a/test/lib/commands/approve-scripts.js b/test/lib/commands/approve-scripts.js index dde7a358b12e2..cf1f719cda66a 100644 --- a/test/lib/commands/approve-scripts.js +++ b/test/lib/commands/approve-scripts.js @@ -55,7 +55,7 @@ t.test('approve-scripts --pending lists unreviewed packages', async t => { }) await npm.exec('approve-scripts', []) const out = joinedOutput() - t.match(out, /2 packages have install scripts not yet covered/) + t.match(out, /2 packages have install scripts blocked because they are not covered by allowScripts/) t.match(out, /canvas@1\.0\.0/) t.match(out, /sharp@1\.0\.0/) }) @@ -385,10 +385,10 @@ t.test('approve-scripts --pending lists packages that only have binding.gyp', as t.match(out, /install: node-gyp rebuild/, 'synthetic node-gyp install is named') }) -t.test('approve-scripts --all skips bundled deps with a notice', async t => { - // Bundled deps cannot be allowlisted in Phase 1 (RFC defers their - // allowlisting to a follow-up). --all must not silently write a key - // derived from the bundled tarball's self-claimed identity. +t.test('approve-scripts --all never approves bundled deps', async t => { + // Bundled deps never run their install scripts and cannot be + // allowlisted. They never reach the unreviewed list, so --all must not + // write a key derived from the bundled tarball's self-claimed identity. const { npm, logs, prefix } = await _mockNpm(t, { prefixDir: { 'package.json': JSON.stringify({ @@ -445,7 +445,8 @@ t.test('approve-scripts --all skips bundled deps with a notice', async t => { 'non-bundled parent gets approved') t.notOk(Object.keys(pkg.allowScripts).some(k => k.startsWith('inner')), 'bundled inner is not approved') - t.match(logs.warn.byTitle('approve-scripts'), [/Skipping 1 bundled dependency/]) + t.strictSame(logs.warn.byTitle('approve-scripts'), [], + 'no warning; bundled deps are excluded upstream') }) t.test('approve-scripts positional is ignored', async t => { @@ -505,7 +506,7 @@ t.test('approve-scripts positional is ignored', async t => { ) }) -t.test('approve-scripts --all with only bundled deps prints "no eligible" notice', async t => { +t.test('approve-scripts --all with only bundled deps has nothing to review', async t => { const { npm, logs, joinedOutput, prefix } = await _mockNpm(t, { prefixDir: { 'package.json': JSON.stringify({ @@ -554,8 +555,9 @@ t.test('approve-scripts --all with only bundled deps prints "no eligible" notice config: { all: true }, }) await npm.exec('approve-scripts', []) - t.match(joinedOutput(), /No packages eligible for approval/) - t.match(logs.warn.byTitle('approve-scripts'), [/Skipping 1 bundled dependency/]) + t.match(joinedOutput(), /No packages with unreviewed install scripts/) + t.strictSame(logs.warn.byTitle('approve-scripts'), [], + 'no warning; bundled deps are excluded upstream') // Ensure no policy entry was written. const pkg = JSON.parse(fs.readFileSync(resolve(prefix, 'package.json'), 'utf8')) t.notOk(pkg.allowScripts, 'no allowScripts written') diff --git a/test/lib/commands/edit.js b/test/lib/commands/edit.js index 915241c82f6da..167b77a674ce1 100644 --- a/test/lib/commands/edit.js +++ b/test/lib/commands/edit.js @@ -22,6 +22,11 @@ const spawk = tspawk(t) const npmConfig = { config: { 'ignore-scripts': false, + // Phase 2 gates dependency install scripts by default. `npm edit` + // rebuilds the edited package via `npm rebuild`, which honors the + // allowScripts gate, so opt every script in for these tests to exercise + // the editor -> rebuild -> install-script flow. + 'dangerously-allow-all-scripts': true, editor: 'testeditor', 'script-shell': process.platform === 'win32' ? process.env.COMSPEC : 'sh', }, diff --git a/test/lib/commands/rebuild.js b/test/lib/commands/rebuild.js index de91fd3471b4e..d3a026460bc5d 100644 --- a/test/lib/commands/rebuild.js +++ b/test/lib/commands/rebuild.js @@ -222,7 +222,7 @@ t.test('completion', async t => { t.type(res, Array) }) -t.test('emits Phase 1 advisory warning for unreviewed install scripts', async t => { +t.test('emits blocked warning for unreviewed install scripts', async t => { const { npm, logs } = await setupMockNpm(t, { prefixDir: { 'package.json': JSON.stringify({ name: 'host', version: '1.0.0' }), @@ -240,7 +240,7 @@ t.test('emits Phase 1 advisory warning for unreviewed install scripts', async t await npm.exec('rebuild', []) t.match( logs.warn.byTitle('rebuild'), - [/install scripts not yet covered by allowScripts/] + [/install scripts blocked because they are not covered by allowScripts/] ) }) @@ -281,3 +281,97 @@ t.test('no advisory warning when allowScripts covers the package', async t => { await npm.exec('rebuild', []) t.strictSame(logs.warn.byTitle('rebuild'), []) }) + +t.test('rebuild honors the gate for an unreviewed package', async t => { + const { npm, logs, prefix: path } = await setupMockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + dependencies: { canvas: '1.0.0' }, + }), + 'package-lock.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { name: 'host', version: '1.0.0', dependencies: { canvas: '1.0.0' } }, + 'node_modules/canvas': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/canvas/-/canvas-1.0.0.tgz', + hasInstallScript: true, + }, + }, + }), + node_modules: { + canvas: { + 'package.json': JSON.stringify({ + name: 'canvas', + version: '1.0.0', + scripts: { + install: "node -e \"require('fs').writeFileSync('ran', '')\"", + }, + }), + }, + }, + }, + }) + + const ranFile = resolve(path, 'node_modules/canvas/ran') + t.throws(() => fs.statSync(ranFile)) + + await npm.exec('rebuild', ['canvas']) + + t.throws(() => fs.statSync(ranFile), 'unreviewed install script must not run') + t.match( + logs.warn.byTitle('rebuild'), + [/install scripts blocked because they are not covered by allowScripts/] + ) +}) + +t.test('rebuild never targets a bundled dependency', async t => { + const { npm, prefix: path } = await setupMockNpm(t, { + prefixDir: { + 'package.json': JSON.stringify({ + name: 'host', + version: '1.0.0', + dependencies: { parent: '1.0.0' }, + }), + node_modules: { + parent: { + 'index.js': '', + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + bundleDependencies: ['bcrypt'], + dependencies: { bcrypt: '1.0.0' }, + }), + node_modules: { + bcrypt: { + 'index.js': '', + 'package.json': JSON.stringify({ + name: 'bcrypt', + version: '1.0.0', + bin: 'index.js', + scripts: { + install: "node -e \"require('fs').writeFileSync('ran', '')\"", + }, + }), + }, + }, + }, + }, + }, + }) + + const ranFile = resolve(path, 'node_modules/parent/node_modules/bcrypt/ran') + t.throws(() => fs.statSync(ranFile)) + + await npm.exec('rebuild', ['bcrypt']) + + t.throws( + () => fs.statSync(ranFile), + 'bundled bcrypt install script must not run' + ) +}) diff --git a/test/lib/utils/check-allow-scripts.js b/test/lib/utils/check-allow-scripts.js index 8dea9674375df..07a9c7cadd112 100644 --- a/test/lib/utils/check-allow-scripts.js +++ b/test/lib/utils/check-allow-scripts.js @@ -135,46 +135,6 @@ t.test('prepare counts for non-registry sources only', async t => { t.equal(result[0].node.name, 'git-pkg') }) -t.test('detects synthetic node-gyp via binding.gyp runtime check', async t => { - const checkAllowScripts = mockCheck(t, { - '@npmcli/arborist/lib/install-scripts.js': async (n) => { - if (n.path === '/has-bindings') { - return { install: 'node-gyp rebuild' } - } - return {} - }, - }) - - const result = await checkAllowScripts({ - arb: arb({ - nodes: [ - node({ name: 'native', path: '/has-bindings' }), - node({ name: 'pure-js', path: '/no-bindings' }), - ], - }), - npm: { flatOptions: {} }, - }) - t.equal(result.length, 1) - t.equal(result[0].node.name, 'native') - t.strictSame(result[0].scripts, { install: 'node-gyp rebuild' }) -}) - -t.test('skips node-gyp detection when gypfile is explicitly false', async t => { - // Mock returns no scripts to simulate the gypfile:false short-circuit - // inside getInstallScripts. - const checkAllowScripts = mockCheck(t, { - '@npmcli/arborist/lib/install-scripts.js': async () => ({}), - }) - - const result = await checkAllowScripts({ - arb: arb({ - nodes: [node({ name: 'opt-out', gypfile: false })], - }), - npm: { flatOptions: {} }, - }) - t.strictSame(result, []) -}) - t.test('skips approved nodes', async t => { const checkAllowScripts = mockCheck(t) const result = await checkAllowScripts({ @@ -238,7 +198,7 @@ t.test('survives missing actualTree', async t => { t.strictSame(result, []) }) -t.test('bundled dep with install scripts is reported as unreviewed regardless of policy', async t => { +t.test('bundled dep with install scripts is never reported (never runs, never pending)', async t => { const checkAllowScripts = mockCheck(t) const bundled = node({ name: 'bundled-pkg', @@ -251,13 +211,12 @@ t.test('bundled dep with install scripts is reported as unreviewed regardless of const result = await checkAllowScripts({ arb: arb({ nodes: [bundled], - // Policy explicitly allows the bundled name — the matcher should - // still return null and the walker should still flag the bundled - // dep as unreviewed. + // Even with an explicit allow entry, a bundled dep never runs its + // install scripts and is never counted as pending, so the walker + // must not flag it. allowScripts: { 'bundled-pkg': true }, }), npm: { flatOptions: {} }, }) - t.equal(result.length, 1, 'bundled dep flagged despite explicit allow entry') - t.equal(result[0].node, bundled) + t.strictSame(result, [], 'bundled dep never flagged') }) diff --git a/test/lib/utils/reify-output.js b/test/lib/utils/reify-output.js index b1bc92b1c77ae..29f573e6bcebb 100644 --- a/test/lib/utils/reify-output.js +++ b/test/lib/utils/reify-output.js @@ -481,7 +481,7 @@ t.test('prints unreviewed install scripts summary', async t => { const mock = await mockReifyWithExtras(t, baseReify, { unreviewedScripts }) const warn = mock.logs.warn.byTitle('allow-scripts').join('\n') - t.match(warn, /2 packages have install scripts not yet covered/) + 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, /sharp@0\.33\.2 \(preinstall: pre; postinstall: post\)/) t.match(warn, /npm approve-scripts --allow-scripts-pending/) @@ -505,7 +505,31 @@ t.test('single unreviewed script uses singular wording', async t => { }], } ) - t.match(mock.logs.warn.byTitle('allow-scripts').join('\n'), /1 package has install scripts/) + t.match(mock.logs.warn.byTitle('allow-scripts').join('\n'), /1 package had install scripts blocked/) +}) + +t.test('optional dep with blocked scripts appears in the summary', async t => { + const mock = await mockNpm(t, {}) + reifyOutput(mock.npm, { + actualTree: { inventory: { has: () => false } }, + diff: { children: [] }, + }, { + unreviewedScripts: [{ + node: { + packageName: 'opt', + name: 'opt', + version: '1.0.0', + path: '/x/opt', + optional: true, + devOptional: true, + }, + scripts: { install: 'cmd' }, + }], + }) + mock.npm.finish() + const warn = mock.logs.warn.byTitle('allow-scripts').join('\n') + t.match(warn, /1 package had install scripts blocked/) + t.match(warn, /opt@1\.0\.0 \(install: cmd\)/) }) t.test('json output includes unreviewedScripts', async t => { diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js index 68893349cc75a..fcf83c58565e5 100644 --- a/workspaces/arborist/lib/arborist/isolated-reifier.js +++ b/workspaces/arborist/lib/arborist/isolated-reifier.js @@ -38,9 +38,10 @@ module.exports = cls => class IsolatedReifier extends cls { #processedEdges = new Set() #workspaceProxies = new Map() - #generateChild (node, location, pkg, isInStore, root) { + #generateChild (node, location, pkg, isInStore, root, inBundle = false) { const newChild = new IsolatedNode({ isInStore, + inBundle, location, name: node.packageName || node.name, optional: node.optional, @@ -327,7 +328,7 @@ module.exports = cls => class IsolatedReifier extends cls { }) bundledTree.nodes.forEach(node => { - this.#generateChild(node, node.location, node.pkg, false, root) + this.#generateChild(node, node.location, node.pkg, false, root, true) }) bundledTree.edges.forEach(edge => { diff --git a/workspaces/arborist/lib/arborist/rebuild.js b/workspaces/arborist/lib/arborist/rebuild.js index e70a2186c2971..b4a66a0af7c09 100644 --- a/workspaces/arborist/lib/arborist/rebuild.js +++ b/workspaces/arborist/lib/arborist/rebuild.js @@ -199,10 +199,24 @@ module.exports = cls => class Builder extends cls { const { package: { bin, scripts = {} } } = node.target const { preinstall, install, postinstall, prepare } = scripts const tests = { bin, preinstall, install, postinstall, prepare } + // allowScripts gate (RFC npm/rfcs#868). `true` lets lifecycle + // scripts run; `false` and `null` (unreviewed) both block. + // --ignore-scripts in #build() still wins. Bypasses: + // --dangerously-allow-all-scripts, links and workspaces (the + // owner is responsible). Bin linking is not gated. + const scriptsAllowed = + this.options.dangerouslyAllowAllScripts || + node.isLink || + node.isWorkspace || + isScriptAllowed(node, this.options.allowScripts) === true for (const [key, has] of Object.entries(tests)) { - if (has) { - this.#queues[key].push(node) + if (!has) { + continue } + if (key !== 'bin' && !scriptsAllowed) { + continue + } + this.#queues[key].push(node) } } timeEnd() @@ -226,18 +240,6 @@ module.exports = cls => class Builder extends cls { return } - // Phase 1 allowScripts gate: a `false` verdict from the policy matcher - // means the user explicitly denied install scripts for this node, so skip - // it. `true` and `null` (unreviewed) both fall through to the existing - // detection logic — unreviewed nodes still run their scripts in Phase 1 - // and are surfaced via the post-reify advisory warning. The global - // --ignore-scripts kill switch in #build() still takes precedence, and - // --dangerously-allow-all-scripts bypasses this gate entirely. - if (!this.options.dangerouslyAllowAllScripts && - isScriptAllowed(node, this.options.allowScripts) === false) { - return - } - if (this.#oldMeta === null) { const { root: { meta } } = node this.#oldMeta = meta && meta.loadedFromDisk && diff --git a/workspaces/arborist/lib/isolated-classes.js b/workspaces/arborist/lib/isolated-classes.js index 113bb98ba19de..32bf19972d150 100644 --- a/workspaces/arborist/lib/isolated-classes.js +++ b/workspaces/arborist/lib/isolated-classes.js @@ -19,6 +19,7 @@ class IsolatedNode { integrity = null inventory = new IsolatedInventory() isInStore = false + inBundle = false linksIn = new Set() meta = { loadedFromDisk: false } optional = false @@ -46,6 +47,9 @@ class IsolatedNode { if (options.isInStore) { this.isInStore = true } + if (options.inBundle) { + this.inBundle = true + } if (options.optional) { this.optional = true } diff --git a/workspaces/arborist/lib/script-allowed.js b/workspaces/arborist/lib/script-allowed.js index 91734fa38c103..66104b274c7c5 100644 --- a/workspaces/arborist/lib/script-allowed.js +++ b/workspaces/arborist/lib/script-allowed.js @@ -24,13 +24,13 @@ const versionFromTgz = require('./version-from-tgz.js') // resolved committish const isScriptAllowed = (node, policy) => { - // Bundled dependencies cannot be allowlisted in Phase 1. The RFC defers - // allowlisting them to a follow-up RFC because matching by name@version - // from the bundled tarball would reintroduce manifest confusion (a - // bundled tarball can claim any name and version). Returning null here - // marks bundled deps as unreviewed regardless of any policy entries, so - // their install scripts surface in the Phase 1 advisory warning and - // (eventually) get blocked at the install-time gate. + // Bundled dependencies never run their install scripts and cannot be + // allowlisted. Matching by name@version from the bundled tarball would + // reintroduce manifest confusion (a bundled tarball can claim any name + // and version). Returning null marks them as not-allowed regardless of + // any policy entry, so their install scripts are blocked by the + // install-time gate. A package that needs a bundled dep's script must + // forward it as one of its own lifecycle scripts. if (node.inBundle) { return null } @@ -319,11 +319,11 @@ const isRegistryNode = (node) => { return /^https?:\/\/[^/]+\/.+\/-\/[^/]+-\d/.test(node.resolved) } -// Trusted display identity for human-facing output (`npm install` -// advisory, `npm approve-scripts --allow-scripts-pending`). Same idea as -// getTrustedRegistryIdentity, but for DISPLAY only — version falls back -// to node.version when the URL doesn't carry one. Must never be used -// for policy matching. +// Trusted display identity for human-facing output (the `npm install` +// blocked-scripts summary and `npm approve-scripts --allow-scripts-pending`). +// Same as getTrustedRegistryIdentity, but for display only: version +// falls back to node.version when the URL doesn't carry one. Do not +// use for policy matching. const trustedDisplay = (node) => { const trusted = getTrustedRegistryIdentity(node) /* istanbul ignore next: defensive fallbacks for nodes without name/version */ diff --git a/workspaces/arborist/lib/unreviewed-scripts.js b/workspaces/arborist/lib/unreviewed-scripts.js new file mode 100644 index 0000000000000..23bab401d6fe2 --- /dev/null +++ b/workspaces/arborist/lib/unreviewed-scripts.js @@ -0,0 +1,89 @@ +const isScriptAllowed = require('./script-allowed.js') +const getInstallScripts = require('./install-scripts.js') + +// Shared allowScripts walk used by both the npm CLI +// (lib/utils/check-allow-scripts.js, lib/utils/strict-allow-scripts-preflight.js) +// and libnpmexec (npm exec / npx). It lives in arborist because that is the +// only package both callers can import. +// +// Walks a tree's inventory and returns the dep nodes that have +// install-relevant lifecycle scripts and are not yet covered (or explicitly +// denied) by the allowScripts policy. +// +// Returns an array of `{ node, scripts }` entries. `scripts` is an object +// describing the relevant lifecycle scripts that would run. +const collectUnreviewedScripts = async ({ + tree, + policy, + ignoreScripts = false, + dangerouslyAllowAllScripts = false, +} = {}) => { + if (ignoreScripts || dangerouslyAllowAllScripts) { + return [] + } + + if (!tree?.inventory) { + return [] + } + + const resolvedPolicy = policy || null + + const unreviewed = [] + for (const node of tree.inventory.values()) { + if (node.isProjectRoot || node.isWorkspace) { + continue + } + if (node.isLink) { + // Linked workspace dependencies are managed by the workspace owner. + continue + } + if (node.inBundle) { + // Bundled dependencies never run their install scripts and cannot be + // allowlisted, so they are never "pending". Skipping them keeps them + // out of the advisory warning and out of strict-allow-scripts. A + // package that needs a bundled dep's script must forward it as one of + // its own lifecycle scripts. + continue + } + + const verdict = isScriptAllowed(node, resolvedPolicy) + if (verdict === true || verdict === false) { + continue + } + + const scripts = await getInstallScripts(node) + if (Object.keys(scripts).length === 0) { + continue + } + + unreviewed.push({ node, scripts }) + } + + return unreviewed +} + +// Builds the `ESTRICTALLOWSCRIPTS` error thrown by the strict-mode preflight +// from a list of `{ node, scripts }` entries. `remediation` is the +// caller-specific guidance appended after the package list (npm install vs +// npm exec have different remediation commands). +const strictAllowScriptsError = (unreviewed, { remediation } = {}) => { + 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') + + return Object.assign( + new Error( + `--strict-allow-scripts: ${unreviewed.length} package(s) have install ` + + `scripts not covered by allowScripts:\n${lines}\n${remediation}` + ), + { code: 'ESTRICTALLOWSCRIPTS' } + ) +} + +module.exports = { collectUnreviewedScripts, strictAllowScriptsError } diff --git a/workspaces/arborist/test/arborist/rebuild.js b/workspaces/arborist/test/arborist/rebuild.js index df2eac9eec25b..2eaf6bccabe69 100644 --- a/workspaces/arborist/test/arborist/rebuild.js +++ b/workspaces/arborist/test/arborist/rebuild.js @@ -20,7 +20,11 @@ new MockRegistry({ const isWindows = process.platform === 'win32' -const newArb = opt => new Arborist(opt) +// Most rebuild tests pre-date the allowScripts gate and assert that +// install scripts run. Bypass the default-deny in this suite by +// default; individual tests that want to assert the gate's behaviour +// override the option explicitly. +const newArb = opt => new Arborist({ dangerouslyAllowAllScripts: true, ...opt }) // track the logs that are emitted. returns a function that removes // the listener and provides the list of what it saw. @@ -121,7 +125,8 @@ t.test('allowScripts deny entry skips the build set entry for that node', async const path = fixture(t, 'testing-rebuild-script-env-flags') const arb = newArb({ path, - allowScripts: { devdep: false }, + allowScripts: { devdep: false, devopt: true }, + dangerouslyAllowAllScripts: false, }) await arb.rebuild() // devdep is denied — its postinstall must NOT have produced the `env` @@ -261,7 +266,7 @@ t.test('run scripts in foreground if foregroundScripts set', async t => { }, }) - const arb = new Arborist({ path, foregroundScripts: true }) + const arb = new Arborist({ path, foregroundScripts: true, dangerouslyAllowAllScripts: true }) await arb.rebuild() // add a sentinel to make sure we didn't get too many entries, since // t.match() will allow trailing/extra values in the test object. @@ -450,7 +455,7 @@ t.test('rebuild node-gyp dependencies lacking both preinstall and install script }, }), }) - const arb = new Arborist({ path }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) await arb.rebuild() t.match(RUNS, [ { @@ -496,7 +501,7 @@ t.test('do not rebuild node-gyp dependencies with gypfile:false', async t => { }, }), }) - const arb = new Arborist({ path }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) await arb.rebuild() }) @@ -534,7 +539,7 @@ t.test('do not run lifecycle scripts of linked deps twice', async t => { return require('@npmcli/run-script')(opts) }, }) - const arb = new Arborist({ path }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) await arb.rebuild() t.equal(RUNS.length, 1, 'should run postinstall script only once') t.match(RUNS, [ @@ -580,7 +585,7 @@ t.test('workspaces', async t => { return require('@npmcli/run-script')(opts) }, }) - const arb = new Arborist({ path }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) await arb.rebuild() t.equal(RUNS.length, 2, 'should run prepare script only once per ws') @@ -629,7 +634,7 @@ t.test('workspaces', async t => { return { code: 0, signal: null } }, }) - const arb = new Arborist({ path, binLinks: false }) + const arb = new Arborist({ path, binLinks: false, dangerouslyAllowAllScripts: true }) await arb.rebuild() t.equal(RUNS.length, 1, 'should run prepare script only once') @@ -677,7 +682,7 @@ t.test('workspaces', async t => { return { code: 0, signal: null } }, }) - const arb = new Arborist({ path }) + const arb = new Arborist({ path, dangerouslyAllowAllScripts: true }) await arb.rebuild() t.equal(RUNS.length, 1, 'should run prepare script only once') @@ -841,6 +846,7 @@ t.test('no workspaces', async t => { const arb = new Arborist({ path, workspacesEnabled: false, + dangerouslyAllowAllScripts: true, }) await arb.rebuild() diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index c483bbc54608c..63252a6a7b693 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -344,7 +344,7 @@ t.test('Bundles rebuilt as long as rebuildBundle not false', async t => { const a = resolve(path, 'node_modules/@isaacs/testing-rebuild-bundle-a') const dir = resolve(a, 'node_modules/@isaacs/testing-rebuild-bundle-b') const file = resolve(dir, 'cwd') - await reify(path) + await reify(path, { dangerouslyAllowAllScripts: true }) t.equal(fs.readFileSync(file, 'utf8'), dir) }) t.test('do not rebuild the bundle', async t => { @@ -610,13 +610,13 @@ t.test('optional dependency failures', async t => { await t.test(`${c} save=false`, async t => { createRegistry(t, true) await t.resolveMatchSnapshot(printReified(fixture(t, c), - { update: true, save: false })) + { update: true, save: false, dangerouslyAllowAllScripts: true })) }) // npm update --save await t.test(`${c} save=true`, async t => { createRegistry(t, true) await t.resolveMatchSnapshot(printReified(fixture(t, c), - { update: true, save: true })) + { update: true, save: true, dangerouslyAllowAllScripts: true })) }) } }) @@ -637,7 +637,7 @@ t.test('failing script means install failure, unless ignoreScripts', async t => for (const c of cases) { await t.test(c, async t => { createRegistry(t, true) - t.rejects(printReified(fixture(t, c))) + t.rejects(printReified(fixture(t, c), { dangerouslyAllowAllScripts: true })) }) await t.test(`${c} --ignore-scripts`, async t => { createRegistry(t, true) diff --git a/workspaces/arborist/test/isolated-mode.js b/workspaces/arborist/test/isolated-mode.js index 2be8561806b3f..38e0b9bca36c0 100644 --- a/workspaces/arborist/test/isolated-mode.js +++ b/workspaces/arborist/test/isolated-mode.js @@ -1383,7 +1383,7 @@ tap.test('postinstall scripts are run', async t => { // Note that we override this cache to prevent interference from other tests const cache = fs.mkdtempSync(`${getTempDir()}/test-`) - const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache, dangerouslyAllowAllScripts: true }) await arborist.reify({ installStrategy: 'linked' }) const postInstallRanWhich = pathExists(`${setupRequire(dir)('which')}/postInstallRanWhich`) @@ -1415,7 +1415,7 @@ tap.test('postinstall scripts run once for store packages', async t => { const { dir, registry } = await getRepo(graph) const cache = fs.mkdtempSync(`${getTempDir()}/test-`) - const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache }) + const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache, dangerouslyAllowAllScripts: true }) await arborist.reify({ installStrategy: 'linked' }) const whichDir = setupRequire(dir)('which') diff --git a/workspaces/arborist/test/script-allowed.js b/workspaces/arborist/test/script-allowed.js index c1ee8abc13b2e..4e20f987d9c87 100644 --- a/workspaces/arborist/test/script-allowed.js +++ b/workspaces/arborist/test/script-allowed.js @@ -314,11 +314,11 @@ t.test('isRegistryNode — arborist isRegistryDependency true accepts even unusu t.equal(isScriptAllowed(arboristNode, { 'trusted@1.0.0': true }), true) }) -t.test('bundled deps cannot be allowlisted (Phase 1 blocks outright)', async t => { +t.test('bundled deps cannot be allowlisted (never run)', async t => { // Bundled dependencies have inBundle=true and no independent resolved - // URL. The RFC explicitly forbids allowlisting them in Phase 1 because - // matching by name@version from the bundled tarball would reintroduce - // manifest confusion. They must always return null (unreviewed). + // URL. They can never be allowlisted because matching by name@version + // from the bundled tarball would reintroduce manifest confusion. They + // always return null, and their install scripts never run. const bundled = { name: 'bundled-pkg', @@ -341,8 +341,8 @@ t.test('bundled deps cannot be allowlisted (Phase 1 blocks outright)', async t = t.test('bundled deps: deny entry does not match either (returns null, not false)', async t => { // A deny entry doesn't apply to bundled deps because they're outside - // the policy scope entirely. Phase 1 blocks them via the walker, not - // via the policy. + // the policy scope entirely. They're blocked because they never run, + // not via a policy entry. const bundled = { name: 'bundled-pkg', packageName: 'bundled-pkg', @@ -378,6 +378,41 @@ t.test('inBundle: false does not affect normal matching', async t => { t.equal(isScriptAllowed(normal, { 'pkg@1.0.0': true }), true) }) +t.test('isolated mode (linked): bundled IsolatedNode is blocked', async t => { + // Regression guard: in isolated/linked mode the gate runs against + // IsolatedNode instances, not real Nodes. A bundled IsolatedNode must + // report inBundle so the gate blocks it even when its resolved URL + // looks like a registry identity that a name entry would otherwise + // match. Without inBundle on IsolatedNode the guard is silently + // skipped and the bundled install script runs. + const { IsolatedNode } = require('../lib/isolated-classes.js') + + const bundled = new IsolatedNode({ + inBundle: true, + location: 'node_modules/bundled-pkg', + name: 'bundled-pkg', + package: { name: 'bundled-pkg', version: '1.0.0' }, + path: '/project/node_modules/bundled-pkg', + resolved: 'https://registry.npmjs.org/bundled-pkg/-/bundled-pkg-1.0.0.tgz', + }) + + t.equal(bundled.inBundle, true, 'bundled IsolatedNode reports inBundle') + t.equal(isScriptAllowed(bundled, { 'bundled-pkg': true }), null) + t.equal(isScriptAllowed(bundled, { 'bundled-pkg@1.0.0': true }), null) + + const store = new IsolatedNode({ + isInStore: true, + location: 'node_modules/.store/store-pkg@1.0.0/node_modules/store-pkg', + name: 'store-pkg', + package: { name: 'store-pkg', version: '1.0.0' }, + path: '/project/node_modules/.store/store-pkg@1.0.0/node_modules/store-pkg', + resolved: 'https://registry.npmjs.org/store-pkg/-/store-pkg-1.0.0.tgz', + }) + + t.equal(store.inBundle, false, 'external store IsolatedNode is not bundled') + t.equal(isScriptAllowed(store, { 'store-pkg@1.0.0': true }), true) +}) + t.test('manifest confusion: malicious tarball self-name cannot bypass allow entry', async t => { // A package author publishes 'naughty' to the registry but inside the // tarball claims `package.json#name = "trusted"` and the matching diff --git a/workspaces/arborist/test/unreviewed-scripts.js b/workspaces/arborist/test/unreviewed-scripts.js new file mode 100644 index 0000000000000..9672f0dc228f7 --- /dev/null +++ b/workspaces/arborist/test/unreviewed-scripts.js @@ -0,0 +1,169 @@ +const t = require('tap') +const { + collectUnreviewedScripts, + strictAllowScriptsError, +} = require('../lib/unreviewed-scripts.js') + +// Loads a fresh copy of the shared module with `install-scripts.js` mocked, +// so script detection (including the synthetic node-gyp `binding.gyp` path) +// can be controlled without touching the filesystem. +const mockCollect = (t, getInstallScripts) => + t.mock('../lib/unreviewed-scripts.js', { + '../lib/install-scripts.js': getInstallScripts, + }).collectUnreviewedScripts + +// Minimal tree fixture for the walk. +const tree = (nodes) => ({ + inventory: new Map(nodes.map((n, i) => [`node_modules/${n.name || `n${i}`}`, n])), +}) + +// Registry-shaped node so the real script-allowed/install-scripts helpers +// behave deterministically (registry tarballs skip `prepare`, and the +// identity matcher keys off the resolved URL). +const node = ({ + name = 'pkg', + version = '1.0.0', + scripts = {}, + isProjectRoot = false, + isWorkspace = false, + isLink = false, + inBundle = false, + resolved, +} = {}) => ({ + name, + packageName: name, + version, + resolved: resolved ?? `https://registry.npmjs.org/${name}/-/${name}-${version}.tgz`, + location: `node_modules/${name}`, + isProjectRoot, + isWorkspace, + isLink, + inBundle, + isRegistryDependency: true, + package: { name, version, scripts }, +}) + +t.test('collectUnreviewedScripts', async t => { + t.test('returns [] when ignoreScripts is set', async t => { + const result = await collectUnreviewedScripts({ + tree: tree([node({ scripts: { install: 'x' } })]), + policy: null, + ignoreScripts: true, + }) + t.strictSame(result, []) + }) + + t.test('returns [] when dangerouslyAllowAllScripts is set', async t => { + const result = await collectUnreviewedScripts({ + tree: tree([node({ scripts: { install: 'x' } })]), + policy: null, + dangerouslyAllowAllScripts: true, + }) + t.strictSame(result, []) + }) + + t.test('returns [] when tree has no inventory', async t => { + t.strictSame(await collectUnreviewedScripts({ tree: undefined }), []) + t.strictSame(await collectUnreviewedScripts({ tree: {} }), []) + t.strictSame(await collectUnreviewedScripts({}), []) + t.strictSame(await collectUnreviewedScripts(), []) + }) + + t.test('skips project root, workspace, linked, and bundled nodes', async t => { + const result = await collectUnreviewedScripts({ + tree: tree([ + node({ name: 'root', scripts: { install: 'x' }, isProjectRoot: true }), + node({ name: 'ws', scripts: { install: 'x' }, isWorkspace: true }), + node({ name: 'linked', scripts: { install: 'x' }, isLink: true }), + node({ name: 'bundled', scripts: { install: 'x' }, inBundle: true }), + ]), + policy: null, + }) + t.strictSame(result, []) + }) + + t.test('skips nodes with no install-relevant scripts', async t => { + const result = await collectUnreviewedScripts({ + tree: tree([node({ scripts: { test: 'jest' } })]), + policy: null, + }) + t.strictSame(result, []) + }) + + t.test('collects unreviewed install scripts', async t => { + const result = await collectUnreviewedScripts({ + tree: tree([ + node({ name: 'a', scripts: { preinstall: 'pre' } }), + node({ name: 'b', scripts: { install: 'inst' } }), + node({ name: 'c', scripts: { postinstall: 'post' } }), + ]), + policy: null, + }) + t.equal(result.length, 3) + t.strictSame(result[0].scripts, { preinstall: 'pre' }) + t.strictSame(result[1].scripts, { install: 'inst' }) + t.strictSame(result[2].scripts, { postinstall: 'post' }) + }) + + t.test('skips nodes the policy allows or denies', async t => { + const result = await collectUnreviewedScripts({ + tree: tree([ + node({ name: 'allowed', version: '1.0.0', scripts: { install: 'x' } }), + node({ name: 'denied', version: '1.0.0', scripts: { install: 'x' } }), + node({ name: 'pending', version: '1.0.0', scripts: { install: 'x' } }), + ]), + policy: { allowed: true, denied: false }, + }) + t.equal(result.length, 1) + t.equal(result[0].node.name, 'pending') + }) + + t.test('detects synthetic node-gyp via binding.gyp runtime check', async t => { + const collect = mockCollect(t, async (n) => { + if (n.path === '/has-bindings') { + return { install: 'node-gyp rebuild' } + } + return {} + }) + const result = await collect({ + tree: tree([ + { ...node({ name: 'native' }), path: '/has-bindings' }, + { ...node({ name: 'pure-js' }), path: '/no-bindings' }, + ]), + policy: null, + }) + t.equal(result.length, 1) + t.equal(result[0].node.name, 'native') + t.strictSame(result[0].scripts, { install: 'node-gyp rebuild' }) + }) +}) + +t.test('strictAllowScriptsError', async t => { + const unreviewed = [ + { node: { package: { name: 'a', version: '1.0.0' } }, scripts: { install: 'do-a' } }, + { node: { package: { name: 'b', version: '2.0.0' } }, scripts: { preinstall: 'pre', postinstall: 'post' } }, + ] + + const err = strictAllowScriptsError(unreviewed, { remediation: 'FIX IT.' }) + t.equal(err.code, 'ESTRICTALLOWSCRIPTS') + t.match(err.message, /2 package\(s\) have install scripts not covered by allowScripts:/) + t.match(err.message, /a@1\.0\.0 \(install: do-a\)/) + t.match(err.message, /b@2\.0\.0 \(preinstall: pre; postinstall: post\)/) + t.match(err.message, /FIX IT\.$/) +}) + +t.test('strictAllowScriptsError falls back to node.name when no package', async t => { + const err = strictAllowScriptsError( + [{ node: { name: 'c' }, scripts: { install: 'x' } }], + { remediation: 'go.' } + ) + t.match(err.message, /\n {2}c \(install: x\)\n/) +}) + +t.test('strictAllowScriptsError defaults options when called without them', async t => { + const err = strictAllowScriptsError( + [{ node: { name: 'c' }, scripts: { install: 'x' } }] + ) + t.equal(err.code, 'ESTRICTALLOWSCRIPTS') + t.match(err.message, /\n {2}c \(install: x\)\n/) +}) diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index 1fd88e9351953..b3aea4f4172d5 100644 --- a/workspaces/config/lib/definitions/definitions.js +++ b/workspaces/config/lib/definitions/definitions.js @@ -2316,15 +2316,16 @@ const definitions = { default: false, type: Boolean, description: ` - If \`true\`, turn the install-script policy from a warning into a hard - error: any dependency with install scripts not covered by - \`allowScripts\` will fail the install instead of running with a - notice. - - Dependencies explicitly denied with \`false\` in \`allowScripts\` are - always silently skipped; this setting only affects unreviewed entries. - \`--ignore-scripts\` and \`--dangerously-allow-all-scripts\` both - override this setting. + If \`true\`, turn the install-script policy from a silent skip into a + hard error: any dependency with install scripts not covered by + \`allowScripts\` will fail the install instead of being silently + skipped. + + By default, dependencies whose install scripts are not approved in + \`allowScripts\` are silently skipped; this setting promotes that + silent skip into a hard failure, which is the recommended posture + for CI. \`--ignore-scripts\` and \`--dangerously-allow-all-scripts\` + both override this setting. `, flatten, }), diff --git a/workspaces/libnpmexec/lib/index.js b/workspaces/libnpmexec/lib/index.js index 3add22cd2edca..35452f796246b 100644 --- a/workspaces/libnpmexec/lib/index.js +++ b/workspaces/libnpmexec/lib/index.js @@ -4,6 +4,7 @@ const { dirname, join, resolve } = require('node:path') const crypto = require('node:crypto') const { mkdir } = require('node:fs/promises') const Arborist = require('@npmcli/arborist') +const strictAllowScriptsPreflight = require('./strict-allow-scripts-preflight.js') const ciInfo = require('ci-info') const { log, input } = require('proc-log') const npa = require('npm-package-arg') @@ -86,6 +87,9 @@ const missingFromTree = async ({ spec, tree, flatOptions, isNpxTree, shallow }) } } +// Strict-mode pre-flight for `npm exec` / `npx` lives in +// ./strict-allow-scripts-preflight.js + // see if the package.json at `path` has an entry that matches `cmd` // the path is a known-local directory, not a user-supplied dep, so // allow-directory must not gate this introspection @@ -301,11 +305,16 @@ const exec = async (opts) => { } } } - await withLock(lockPath, () => npxArb.reify({ - ...flatOptions, - save: true, - add, - })) + await withLock(lockPath, async () => { + // Hard-fail before reify if --strict-allow-scripts is set and + // any node has install scripts not covered by allowScripts. + await strictAllowScriptsPreflight(npxArb, { ...flatOptions, add }) + await npxArb.reify({ + ...flatOptions, + save: true, + add, + }) + }) } binPaths.push(resolve(installDir, 'node_modules/.bin')) const pkgJson = await PackageJson.load(installDir) diff --git a/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js b/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js new file mode 100644 index 0000000000000..579fd0c0fddf2 --- /dev/null +++ b/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js @@ -0,0 +1,40 @@ +const { collectUnreviewedScripts, strictAllowScriptsError } = require('@npmcli/arborist/lib/unreviewed-scripts.js') + +// Strict-mode pre-flight for `npm exec` / `npx`. When +// `--strict-allow-scripts` is set, build the npx-cache ideal tree and +// throw before reify if any node has install scripts not covered by +// the resolved `allowScripts` policy. The arborist gate already +// silently skips those scripts; this turns the silent skip into a +// hard failure for CI. Bypassed by `--ignore-scripts` and +// `--dangerously-allow-all-scripts`. +const strictAllowScriptsPreflight = async (arb, opts) => { + if (!opts.strictAllowScripts) { + return + } + if (opts.ignoreScripts || opts.dangerouslyAllowAllScripts) { + return + } + + if (!arb.idealTree) { + await arb.buildIdealTree(opts) + } + + const unreviewed = await collectUnreviewedScripts({ + tree: arb.idealTree, + policy: opts.allowScripts || null, + ignoreScripts: opts.ignoreScripts, + dangerouslyAllowAllScripts: opts.dangerouslyAllowAllScripts, + }) + + if (unreviewed.length === 0) { + return + } + + throw strictAllowScriptsError(unreviewed, { + remediation: + 'Pass --allow-scripts= for one-off approval, or bypass this ' + + 'check with --dangerously-allow-all-scripts.', + }) +} + +module.exports = strictAllowScriptsPreflight diff --git a/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js b/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js new file mode 100644 index 0000000000000..1d6795703ed31 --- /dev/null +++ b/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js @@ -0,0 +1,82 @@ +const t = require('tap') + +const strictAllowScriptsPreflight = require('../lib/strict-allow-scripts-preflight.js') + +// a node carrying install scripts that the policy has not reviewed +const unreviewedTree = { + inventory: new Map([ + ['node_modules/has-scripts', { + name: 'has-scripts', + location: 'node_modules/has-scripts', + package: { scripts: { install: 'node-gyp rebuild' } }, + }], + ]), +} + +const fakeArb = (idealTree) => { + const arb = { + idealTree, + buildIdealTreeCalled: false, + async buildIdealTree () { + arb.buildIdealTreeCalled = true + arb.idealTree = unreviewedTree + }, + } + return arb +} + +t.test('no-op when strictAllowScripts is not set', async t => { + const arb = fakeArb(unreviewedTree) + await t.resolves(strictAllowScriptsPreflight(arb, {})) + t.notOk(arb.buildIdealTreeCalled, 'does not build the ideal tree') +}) + +t.test('bypassed by ignoreScripts', async t => { + const arb = fakeArb(unreviewedTree) + await t.resolves(strictAllowScriptsPreflight(arb, + { strictAllowScripts: true, ignoreScripts: true })) + t.notOk(arb.buildIdealTreeCalled, 'does not build the ideal tree') +}) + +t.test('bypassed by dangerouslyAllowAllScripts', async t => { + const arb = fakeArb(unreviewedTree) + await t.resolves(strictAllowScriptsPreflight(arb, + { strictAllowScripts: true, dangerouslyAllowAllScripts: true })) + t.notOk(arb.buildIdealTreeCalled, 'does not build the ideal tree') +}) + +t.test('builds the ideal tree when missing', async t => { + const arb = fakeArb(null) + await t.rejects( + strictAllowScriptsPreflight(arb, { strictAllowScripts: true }), + /install scripts/i, + 'throws on unreviewed scripts' + ) + t.ok(arb.buildIdealTreeCalled, 'builds the ideal tree') +}) + +t.test('throws when unreviewed scripts are present', async t => { + const arb = fakeArb(unreviewedTree) + await t.rejects( + strictAllowScriptsPreflight(arb, { strictAllowScripts: true }), + { code: 'ESTRICTALLOWSCRIPTS' }, + 'throws with the strict-allow-scripts error' + ) +}) + +t.test('resolves when no unreviewed scripts are present', async t => { + const cleanTree = { + inventory: new Map([ + ['node_modules/no-scripts', { + name: 'no-scripts', + location: 'node_modules/no-scripts', + package: {}, + }], + ]), + } + const arb = fakeArb(cleanTree) + await t.resolves( + strictAllowScriptsPreflight(arb, { strictAllowScripts: true }), + 'no error when nothing is unreviewed' + ) +})