diff --git a/docs/lib/content/commands/npm-install-scripts.md b/docs/lib/content/commands/npm-install-scripts.md index 32f05a8577039..524c650e23d47 100644 --- a/docs/lib/content/commands/npm-install-scripts.md +++ b/docs/lib/content/commands/npm-install-scripts.md @@ -19,6 +19,9 @@ 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 here. +These warnings omit disconnected unused entries that installation cannot +build. Local link targets that can still be rebuilt remain subject to the +policy. This command only works inside a project that has a `package.json`. Running it with `--global` (`-g`) fails with an `EGLOBAL` error, since global @@ -53,6 +56,9 @@ unreviewed install scripts. `ls` is read-only: it lists every package whose install scripts are not yet covered by `allowScripts`, without modifying `package.json`. +This includes unreviewed extraneous packages, which are installed but not +required by any dependency. These packages can still cause +`npm rebuild --strict-allow-scripts` to fail. `prune` removes `allowScripts` entries that no longer match an installed package with an install script, either because the package is no longer diff --git a/lib/utils/check-allow-scripts.js b/lib/utils/check-allow-scripts.js index 47984803cde7b..352f8e7cb9bc2 100644 --- a/lib/utils/check-allow-scripts.js +++ b/lib/utils/check-allow-scripts.js @@ -15,13 +15,20 @@ const { collectUnreviewedScripts } = require('@npmcli/arborist/lib/unreviewed-sc // `includeWhenIgnored` keeps listing unreviewed packages even when // ignore-scripts is set, so approve/deny can show what you'd move from a // blanket ignore-scripts to an allowlist. Execution callers leave it false. -const checkAllowScripts = async ({ arb, npm, tree, includeWhenIgnored = false }) => +const checkAllowScripts = async ({ + arb, + npm, + tree, + includeWhenIgnored = false, + skipExtraneous = false, +}) => collectUnreviewedScripts({ tree: tree || arb.actualTree, policy: arb.options?.allowScripts || null, ignoreScripts: !!arb.options?.ignoreScripts, dangerouslyAllowAllScripts: !!npm?.flatOptions?.dangerouslyAllowAllScripts, includeWhenIgnored, + skipExtraneous, }) module.exports = checkAllowScripts diff --git a/lib/utils/reify-finish.js b/lib/utils/reify-finish.js index 1041c53fdb935..0278f9b591911 100644 --- a/lib/utils/reify-finish.js +++ b/lib/utils/reify-finish.js @@ -18,7 +18,7 @@ const reifyFinish = async (npm, arb) => { } } warnWorkspaceAllowScripts(arb.actualTree) - const unreviewedScripts = await checkAllowScripts({ arb, npm }) + const unreviewedScripts = await checkAllowScripts({ arb, npm, skipExtraneous: true }) reifyOutput(npm, arb, { unreviewedScripts }) } diff --git a/lib/utils/strict-allow-scripts-preflight.js b/lib/utils/strict-allow-scripts-preflight.js index 0c500018184c2..2bb5ceb6d9d45 100644 --- a/lib/utils/strict-allow-scripts-preflight.js +++ b/lib/utils/strict-allow-scripts-preflight.js @@ -33,7 +33,12 @@ const strictAllowScriptsPreflight = async ({ arb, npm, idealTreeOpts }) => { tree = arb.actualTree } - const unreviewed = await checkAllowScripts({ arb, npm, tree }) + const unreviewed = await checkAllowScripts({ + arb, + npm, + tree, + skipExtraneous: idealTreeOpts !== undefined, + }) if (unreviewed.length === 0) { return } diff --git a/tap-snapshots/test/lib/docs.js.test.cjs b/tap-snapshots/test/lib/docs.js.test.cjs index 3908dedbcf181..9f955ba602cc9 100644 --- a/tap-snapshots/test/lib/docs.js.test.cjs +++ b/tap-snapshots/test/lib/docs.js.test.cjs @@ -2091,6 +2091,11 @@ Optional dependencies that cannot be installed on the current platform or engine (a non-matching \`os\`, \`cpu\`, or \`libc\`) are not flagged, because their install scripts never run. +Installation checks skip unused entries with no package parent or incoming +links. Local link targets that installation can still rebuild remain subject +to this check. \`npm rebuild\` and \`npm install-scripts ls\` still check unused +packages on disk. + #### \`strict-npmrc\` diff --git a/test/fixtures/allow-scripts-orphan.js b/test/fixtures/allow-scripts-orphan.js new file mode 100644 index 0000000000000..9276c573f4ec3 --- /dev/null +++ b/test/fixtures/allow-scripts-orphan.js @@ -0,0 +1,32 @@ +const { workspaceMock } = require('./mock-npm.js') + +module.exports = (t, { workspace = true, allowScripts, requiredScripts = false } = {}) => { + const fixture = workspaceMock(t, { + workspaces: { app: { 'abbrev@1.0.0': {} } }, + }) + const pkg = JSON.parse(fixture['package.json']) + const lock = JSON.parse(fixture['package-lock.json']) + + if (!workspace) { + delete pkg.workspaces + pkg.dependencies = { app: 'file:app' } + lock.packages[''] = { ...pkg } + } + pkg.allowScripts = allowScripts + + // The missing intermediate directory leaves this orphan in the ideal-tree inventory. + lock.packages['app/node_modules/abbrev/node_modules/orphan'] = { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/orphan/-/orphan-1.0.0.tgz', + hasInstallScript: true, + extraneous: true, + } + if (requiredScripts) { + lock.packages['node_modules/abbrev'].hasInstallScript = true + } + + delete fixture.node_modules + fixture['package.json'] = JSON.stringify(pkg) + fixture['package-lock.json'] = JSON.stringify(lock) + return fixture +} diff --git a/test/lib/commands/ci.js b/test/lib/commands/ci.js index a90ca4b0cffe6..53d49fbb9e823 100644 --- a/test/lib/commands/ci.js +++ b/test/lib/commands/ci.js @@ -6,6 +6,8 @@ const { loadNpmWithRegistry: loadMockNpm, workspaceMock, } = require('../../fixtures/mock-npm') +const orphanFixture = require('../../fixtures/allow-scripts-orphan.js') +const tspawk = require('../../fixtures/tspawk.js') // t.cleanSnapshot = str => str.replace(/ in [0-9ms]+/g, ' in {TIME}') @@ -413,6 +415,69 @@ t.test('should throw error when ideal inventory mismatches virtual', async t => t.equal(fs.existsSync(nmTestFile), true, 'does not remove node_modules') }) +t.test('ci with lockfile-only orphans', async t => { + tspawk(t) + const testOrphan = async (t, { + workspace = true, + allowScripts, + strict = true, + json = false, + } = {}) => { + const prefixDir = orphanFixture(t, { workspace, allowScripts }) + const { npm, registry, assert, logs, clearOutput, joinedOutput } = await loadMockNpm(t, { + config: { audit: false, fund: false, 'strict-allow-scripts': strict, json }, + prefixDir, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'tarballs/abbrev@1.0.0'), + }) + + await npm.exec('ci', []) + + assert.packageInstalled('node_modules/abbrev@1.0.0') + assert.fileShouldNotExist('app/node_modules/abbrev/node_modules/orphan') + t.equal( + fs.readFileSync(path.join(npm.prefix, 'package-lock.json'), 'utf8'), + prefixDir['package-lock.json'], + 'does not change the lockfile' + ) + t.strictSame(logs.warn.byTitle('install-scripts'), [], 'no orphan warning') + if (json) { + t.notOk(Object.hasOwn(JSON.parse(joinedOutput()), 'unreviewedScripts'), 'no orphan entry') + } else { + clearOutput() + await npm.exec('install-scripts', ['ls']) + t.match(joinedOutput(), /No packages with unreviewed install scripts/) + } + } + + for (const workspace of [true, false]) { + for (const allowScripts of [undefined, { orphan: false }]) { + const source = workspace ? 'workspace' : 'file link' + const policy = allowScripts ? 'denied' : 'unreviewed' + for (const strict of [false, true]) { + await t.test(`${source}, ${policy} orphan, strict=${strict}`, + t => testOrphan(t, { workspace, allowScripts, strict })) + } + } + } + + await t.test('JSON excludes the orphan', t => testOrphan(t, { json: true })) + + await t.test('still rejects a required unreviewed package', async t => { + const { npm } = await loadMockNpm(t, { + config: { audit: false, 'strict-allow-scripts': true }, + prefixDir: orphanFixture(t, { requiredScripts: true }), + }) + await t.rejects(npm.exec('ci', []), { + code: 'ESTRICTALLOWSCRIPTS', + message: /1 package\(s\) have install scripts not covered by allowScripts:\n {2}abbrev@1\.0\.0/, + }) + }) +}) + t.test('should remove dirty node_modules with unhoisted workspace module', async t => { const { npm, registry, assert } = await loadMockNpm(t, { prefixDir: workspaceMock(t, { diff --git a/test/lib/commands/install-scripts.js b/test/lib/commands/install-scripts.js index fd7051c7e4419..9a7c20591445c 100644 --- a/test/lib/commands/install-scripts.js +++ b/test/lib/commands/install-scripts.js @@ -8,11 +8,17 @@ const mockNpm = async (t, opts = {}) => { return _mockNpm(t, opts) } -const setupProject = ({ allowScripts, withScripts = ['canvas'], noScripts = [] } = {}) => { +const setupProject = ({ + allowScripts, + withScripts = ['canvas'], + noScripts = [], + extraneous = false, +} = {}) => { const pkg = { name: 'host', version: '1.0.0', - dependencies: Object.fromEntries([...withScripts, ...noScripts].map((n) => [n, '*'])), + dependencies: extraneous ? {} + : Object.fromEntries([...withScripts, ...noScripts].map((n) => [n, '*'])), } if (allowScripts !== undefined) { pkg.allowScripts = allowScripts @@ -140,6 +146,29 @@ t.test('install-scripts ls lists unreviewed packages', async t => { t.match(out, /sharp@1\.0\.0/) }) +t.test('install-scripts ls lists extraneous packages on disk', async t => { + for (const json of [false, true]) { + await t.test(json ? 'json' : 'text', async t => { + const { npm, joinedOutput } = await mockNpm(t, { + prefixDir: setupProject({ extraneous: true }), + config: { json }, + }) + await npm.exec('install-scripts', ['ls']) + if (json) { + t.strictSame(JSON.parse(joinedOutput()), { + allowScripts: [{ + name: 'canvas', + changes: [{ key: 'canvas@1.0.0', change: 'pending' }], + }], + }) + } else { + t.match(joinedOutput(), /1 package has install scripts blocked/) + t.match(joinedOutput(), /canvas@1\.0\.0/) + } + }) + } +}) + t.test('install-scripts ls with no unreviewed says so', async t => { const { npm, joinedOutput } = await mockNpm(t, { prefixDir: setupProject({ allowScripts: { canvas: true }, withScripts: ['canvas'] }), diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 3961b7f02e0c1..f1d6a21efcd05 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -16,6 +16,7 @@ const { loadNpmWithRegistry: loadMockNpm, workspaceMock, } = require('../../fixtures/mock-npm') +const orphanFixture = require('../../fixtures/allow-scripts-orphan.js') // tspawk calls preventUnmatched which assures that no scripts run if we don't mock any const spawk = tspawk(t) @@ -597,6 +598,132 @@ t.test('completion', async t => { }) }) +t.test('install with an extraneous local link', async t => { + const setup = (t, config = {}) => loadMockNpm(t, { + config: { audit: false, fund: false, 'package-lock': false, ...config }, + prefixDir: { + 'package.json': JSON.stringify({ name: 'host', version: '1.0.0' }), + foo: { + 'package.json': JSON.stringify({ + name: 'foo', + version: '1.0.0', + scripts: { install: 'echo linked-install' }, + }), + }, + node_modules: { foo: t.fixture('symlink', '../foo') }, + }, + }) + + await t.test('strict mode rejects a retained link target', async t => { + const { npm, assert } = await setup(t, { 'strict-allow-scripts': true }) + await t.rejects(npm.exec('install', []), { + code: 'ESTRICTALLOWSCRIPTS', + message: /foo@1\.0\.0/, + }) + assert.fileShouldExist('node_modules/foo/package.json') + }) + + await t.test('normal mode keeps the blocked-script warning', async t => { + const { npm, logs } = await setup(t) + await npm.exec('install', []) + const warnings = logs.warn.byTitle('install-scripts').join('\n') + t.match(warnings, /1 package had install scripts blocked/) + t.match(warnings, /foo@1\.0\.0/) + }) + + await t.test('JSON keeps the blocked-script entry', async t => { + const { npm, joinedOutput } = await setup(t, { json: true }) + await npm.exec('install', []) + t.strictSame(JSON.parse(joinedOutput()).unreviewedScripts, [{ + name: 'foo', + version: '1.0.0', + path: path.join(npm.prefix, 'foo'), + scripts: { install: 'echo linked-install' }, + }]) + }) + + await t.test('the bypass still rebuilds the retained link', async t => { + const { npm } = await setup(t, { + 'strict-allow-scripts': true, + 'dangerously-allow-all-scripts': true, + }) + let calls = 0 + spawk.spawn(/.*/, args => { + calls++ + return args.includes('echo linked-install') + }) + await npm.exec('install', []) + t.equal(calls, 1, 'ran the linked package install script') + }) + + await t.test('pruning the link does not reject or warn about its detached target', async t => { + const { npm, logs, assert } = await setup(t, { + 'strict-allow-scripts': true, + 'package-lock': true, + }) + await npm.exec('install', []) + assert.fileShouldNotExist('node_modules/foo') + assert.fileShouldExist('foo/package.json') + t.strictSame(logs.warn.byTitle('install-scripts'), []) + }) +}) + +t.test('install with lockfile-only orphans', async t => { + const testOrphan = async (t, { + workspace = true, + allowScripts, + strict = true, + json = false, + } = {}) => { + const { npm, registry, assert, logs, clearOutput, joinedOutput } = await loadMockNpm(t, { + config: { audit: false, fund: false, 'strict-allow-scripts': strict, json }, + prefixDir: orphanFixture(t, { workspace, allowScripts }), + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'tarballs/abbrev@1.0.0'), + }) + + await npm.exec('install', []) + + assert.packageInstalled('node_modules/abbrev@1.0.0') + assert.fileShouldNotExist('app/node_modules/abbrev/node_modules/orphan') + t.strictSame(logs.warn.byTitle('install-scripts'), [], 'no orphan warning') + if (json) { + t.notOk(Object.hasOwn(JSON.parse(joinedOutput()), 'unreviewedScripts'), 'no orphan entry') + } else { + clearOutput() + await npm.exec('install-scripts', ['ls']) + t.match(joinedOutput(), /No packages with unreviewed install scripts/) + } + } + + for (const workspace of [true, false]) { + for (const allowScripts of [undefined, { orphan: false }]) { + const source = workspace ? 'workspace' : 'file link' + const policy = allowScripts ? 'denied' : 'unreviewed' + for (const strict of [false, true]) { + await t.test(`${source}, ${policy} orphan, strict=${strict}`, + t => testOrphan(t, { workspace, allowScripts, strict })) + } + } + } + + await t.test('JSON excludes the orphan', t => testOrphan(t, { json: true })) + + await t.test('still rejects a required unreviewed package', async t => { + const { npm } = await loadMockNpm(t, { + config: { audit: false, 'strict-allow-scripts': true }, + prefixDir: orphanFixture(t, { requiredScripts: true }), + }) + await t.rejects(npm.exec('install', []), { + code: 'ESTRICTALLOWSCRIPTS', + message: /1 package\(s\) have install scripts not covered by allowScripts:\n {2}abbrev@1\.0\.0/, + }) + }) +}) + t.test('should install in workspace with unhoisted module', async t => { const { npm, registry, assert } = await loadMockNpm(t, { prefixDir: workspaceMock(t, { diff --git a/test/lib/commands/rebuild.js b/test/lib/commands/rebuild.js index cfd2d2511e193..5bd2295f64775 100644 --- a/test/lib/commands/rebuild.js +++ b/test/lib/commands/rebuild.js @@ -244,6 +244,33 @@ t.test('emits blocked warning for unreviewed install scripts', async t => { ) }) +t.test('strict rebuild rejects extraneous packages on disk', async t => { + for (const args of [[], ['canvas']]) { + await t.test(args.length ? 'named package' : 'all packages', async t => { + const { npm, prefix } = await setupMockNpm(t, { + config: { 'strict-allow-scripts': true }, + prefixDir: { + 'package.json': JSON.stringify({ name: 'host', version: '1.0.0' }), + node_modules: { + canvas: { + 'package.json': JSON.stringify({ + name: 'canvas', + version: '1.0.0', + scripts: { install: "node -e \"require('fs').writeFileSync('ran', '')\"" }, + }), + }, + }, + }, + }) + await t.rejects(npm.exec('rebuild', args), { + code: 'ESTRICTALLOWSCRIPTS', + message: /canvas@1\.0\.0/, + }) + t.notOk(fs.existsSync(resolve(prefix, 'node_modules/canvas/ran')), 'script did not run') + }) + } +}) + t.test('global advisory warning points at npm config set, not approve-scripts', async t => { const { npm, logs } = await setupMockNpm(t, { config: { diff --git a/test/lib/utils/check-allow-scripts.js b/test/lib/utils/check-allow-scripts.js index c67ec2a7cd703..f7fe9c6367c1c 100644 --- a/test/lib/utils/check-allow-scripts.js +++ b/test/lib/utils/check-allow-scripts.js @@ -95,6 +95,29 @@ t.test('skips project root, workspace, and linked nodes', async t => { t.strictSame(result, []) }) +t.test('only skips extraneous nodes when requested', async t => { + const checkAllowScripts = mockCheck(t) + const orphan = { + ...node({ name: 'orphan', scripts: { install: 'x' }, isRegistryDependency: false }), + extraneous: true, + edgesIn: new Set(), + parent: null, + linksIn: new Set(), + } + for (const skipExtraneous of [undefined, false, true]) { + const result = await checkAllowScripts({ + arb: arb({ nodes: [orphan], allowScripts: { orphan: false } }), + npm: { flatOptions: {} }, + skipExtraneous, + }) + t.strictSame( + result.map(({ node: entry }) => entry), + skipExtraneous ? [] : [orphan], + `skipExtraneous=${skipExtraneous}` + ) + } +}) + t.test('skips nodes with no install-relevant scripts', async t => { const checkAllowScripts = mockCheck(t) const result = await checkAllowScripts({ diff --git a/test/lib/utils/reify-finish.js b/test/lib/utils/reify-finish.js index a1dd165034a46..56ef51d75abf7 100644 --- a/test/lib/utils/reify-finish.js +++ b/test/lib/utils/reify-finish.js @@ -23,8 +23,11 @@ const mockReififyFinish = async (t, { actualTree = {}, otherDirs = {}, ...config config, }) + let reportedScripts const reifyFinish = tmock(t, '{LIB}/utils/reify-finish.js', { - '{LIB}/utils/reify-output.js': () => {}, + '{LIB}/utils/reify-output.js': (_npm, _arb, { unreviewedScripts }) => { + reportedScripts = unreviewedScripts + }, }) await reifyFinish(mock.npm, { @@ -39,6 +42,7 @@ const mockReififyFinish = async (t, { actualTree = {}, otherDirs = {}, ...config return { builtinRc, + reportedScripts, ...mock, } } @@ -51,6 +55,31 @@ t.test('ok by default', async t => { t.strictSame(mock.builtinRc.data, { key: 'value' }) }) +t.test('only omits disconnected orphans from post-install reporting', async t => { + for (const strict of [false, true]) { + await t.test(`strict=${strict}`, async t => { + const candidate = (name, properties = {}) => ({ + name, + package: { name, version: '1.0.0', scripts: { install: `echo ${name}` } }, + extraneous: true, + parent: null, + linksIn: new Set(), + ...properties, + }) + const orphan = candidate('orphan') + const attached = candidate('attached', { parent: {} }) + const linked = candidate('linked', { linksIn: new Set([{}]) }) + const { reportedScripts } = await mockReififyFinish(t, { + 'strict-allow-scripts': strict, + actualTree: { + inventory: new Map([orphan, attached, linked].map(n => [n.name, n])), + }, + }) + t.strictSame(reportedScripts.map(({ node }) => node), [attached, linked]) + }) + } +}) + t.test('should not write if no global npm module', async t => { const mock = await mockReififyFinish(t, { global: true, diff --git a/test/lib/utils/strict-allow-scripts-preflight.js b/test/lib/utils/strict-allow-scripts-preflight.js index c67a6e4853a12..d8115857b64f3 100644 --- a/test/lib/utils/strict-allow-scripts-preflight.js +++ b/test/lib/utils/strict-allow-scripts-preflight.js @@ -1,4 +1,8 @@ const t = require('tap') +const { join } = require('node:path') +const Arborist = require('@npmcli/arborist') +const { loadNpmWithRegistry } = require('../../fixtures/mock-npm.js') +const orphanFixture = require('../../fixtures/allow-scripts-orphan.js') const preflight = require('../../../lib/utils/strict-allow-scripts-preflight.js') @@ -24,9 +28,18 @@ const tree = (nodes) => ({ inventory: new Map(nodes.map((n, i) => [`node_modules/${n.name}-${i}`, n])), }) -const makeArb = ({ ideal, actual, allowScripts = null } = {}) => { +const orphan = () => ({ + ...node({ name: 'orphan' }), + extraneous: true, + isRegistryDependency: false, + edgesIn: new Set(), + parent: null, + linksIn: new Set(), +}) + +const makeArb = ({ ideal, actual, allowScripts = null, options = {} } = {}) => { const arb = { - options: { allowScripts, ignoreScripts: false }, + options: { allowScripts, ignoreScripts: false, ...options }, idealTree: ideal ?? null, actualTree: actual ?? null, } @@ -89,6 +102,99 @@ t.test('passes when the only unreviewed node is inert (platform-incompatible opt t.pass('no error thrown for inert node') }) +t.test('skips disconnected orphans when pruning is disabled', async t => { + for (const installStrategy of ['hoisted', 'linked']) { + await t.test(installStrategy, async t => { + const { npm, prefix, registry, assert } = await loadNpmWithRegistry(t, { + config: { + audit: false, + 'strict-allow-scripts': true, + 'install-strategy': installStrategy, + }, + prefixDir: orphanFixture(t, { allowScripts: { orphan: false } }), + }) + const opts = { + ...npm.flatOptions, + path: prefix, + prune: false, + allowScripts: { orphan: false }, + } + const arb = new Arborist(opts) + await arb.buildIdealTree(opts) + const location = 'app/node_modules/abbrev/node_modules/orphan' + const unused = arb.idealTree.inventory.get(location) + t.match(unused, { extraneous: true, parent: null }) + t.equal(unused.linksIn.size, 0, 'the orphan has no incoming links') + + await preflight({ arb, npm, idealTreeOpts: opts }) + + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: join(prefix, 'tarballs/abbrev@1.0.0'), + }) + await arb.reify(opts) + const installedPath = installStrategy === 'linked' ? 'app/node_modules' : 'node_modules' + assert.packageInstalled(`${installedPath}/abbrev@1.0.0`) + assert.fileShouldNotExist(location) + t.equal(arb.scriptsRun.size, 0, 'no install scripts ran') + const actual = await new Arborist(opts).loadActual() + t.notOk([...actual.inventory.values()].some(n => n.name === 'orphan'), + 'the installed tree has no orphan') + }) + } +}) + +t.test('skips orphans in a newly built ideal tree', async t => { + const arb = makeArb() + arb.buildIdealTree = async () => { + arb.idealTree = tree([orphan()]) + } + await t.resolves(preflight({ + arb, + npm: { flatOptions: { strictAllowScripts: true } }, + idealTreeOpts: {}, + })) + t.ok(arb.idealTree, 'built the ideal tree') +}) + +t.test('does not skip an extraneous target with an incoming link', async t => { + const target = { ...orphan(), linksIn: new Set([{}]) } + const arb = makeArb({ + ideal: tree([target]), + allowScripts: { orphan: false }, + options: { prune: false }, + }) + await t.rejects(preflight({ + arb, + npm: { flatOptions: { strictAllowScripts: true } }, + idealTreeOpts: { prune: false }, + }), { code: 'ESTRICTALLOWSCRIPTS', message: /orphan@1\.0\.0/ }) +}) + +t.test('skipping an orphan does not skip a required unreviewed package', async t => { + const arb = makeArb({ ideal: tree([orphan(), node({ name: 'required' })]) }) + await t.rejects(preflight({ + arb, + npm: { flatOptions: { strictAllowScripts: true } }, + idealTreeOpts: {}, + }), { + code: 'ESTRICTALLOWSCRIPTS', + message: /1 package\(s\) have install scripts not covered by allowScripts:\n {2}required@1\.0\.0/, + }) +}) + +t.test('does not skip actual-tree orphans even with an ideal tree', async t => { + const arb = makeArb({ + ideal: tree([]), + actual: tree([orphan()]), + }) + await t.rejects(preflight({ + arb, + npm: { flatOptions: { strictAllowScripts: true } }, + }), { code: 'ESTRICTALLOWSCRIPTS', message: /orphan@1\.0\.0/ }) +}) + t.test('passes when all install-script nodes are explicitly approved', async t => { const arb = makeArb({ ideal: tree([node({ name: 'canvas' })]), diff --git a/workspaces/arborist/lib/unreviewed-scripts.js b/workspaces/arborist/lib/unreviewed-scripts.js index f7bed387845ec..e30a4540834f5 100644 --- a/workspaces/arborist/lib/unreviewed-scripts.js +++ b/workspaces/arborist/lib/unreviewed-scripts.js @@ -18,6 +18,7 @@ const collectUnreviewedScripts = async ({ ignoreScripts = false, dangerouslyAllowAllScripts = false, includeWhenIgnored = false, + skipExtraneous = false, } = {}) => { // With ignore-scripts set, no scripts run, so execution callers bail out // here. approve/deny pass includeWhenIgnored so they keep listing @@ -57,6 +58,10 @@ const collectUnreviewedScripts = async ({ // must not be flagged (npm/cli#9562). continue } + if (skipExtraneous && node.extraneous && !node.parent && node.linksIn?.size === 0) { + // Reify never installs disconnected extraneous nodes, so their scripts cannot run. + continue + } const verdict = isScriptAllowed(node, resolvedPolicy) if (verdict === true || verdict === false) { diff --git a/workspaces/arborist/test/unreviewed-scripts.js b/workspaces/arborist/test/unreviewed-scripts.js index 34e4878b4625a..12587aa06ab51 100644 --- a/workspaces/arborist/test/unreviewed-scripts.js +++ b/workspaces/arborist/test/unreviewed-scripts.js @@ -29,6 +29,7 @@ const node = ({ isLink = false, inBundle = false, inert = false, + extraneous = false, resolved, } = {}) => ({ name, @@ -41,6 +42,7 @@ const node = ({ isLink, inBundle, inert, + extraneous, isRegistryDependency: true, package: { name, version, scripts }, }) @@ -92,6 +94,44 @@ t.test('collectUnreviewedScripts', async t => { t.strictSame(result, []) }) + t.test('only skips disconnected extraneous nodes when requested', async t => { + const candidate = (name, properties = {}) => ({ + ...node({ name, scripts: { install: 'x' }, extraneous: true }), + isRegistryDependency: false, + edgesIn: new Set(), + parent: null, + linksIn: new Set(), + ...properties, + }) + const orphan = candidate('orphan') + const attached = candidate('attached', { parent: {} }) + const linked = candidate('linked', { linksIn: new Set([{}]) }) + const required = candidate('required', { extraneous: false }) + const unknown = candidate('unknown', { linksIn: undefined }) + const retained = [attached, linked, required, unknown] + + for (const skipExtraneous of [undefined, false, true]) { + for (const policy of [null, { orphan: false }]) { + const result = await collectUnreviewedScripts({ + tree: tree([orphan, ...retained]), + policy, + skipExtraneous, + }) + t.strictSame( + result.map(({ node }) => node), + skipExtraneous ? retained : [orphan, ...retained], + `skipExtraneous=${skipExtraneous}, policy=${JSON.stringify(policy)}` + ) + } + } + + linked.linksIn.clear() + t.strictSame(await collectUnreviewedScripts({ + tree: tree([linked]), + skipExtraneous: true, + }), [], 'skips the target after its incoming link is removed') + }) + t.test('skips nodes with no install-relevant scripts', async t => { const result = await collectUnreviewedScripts({ tree: tree([node({ scripts: { test: 'jest' } })]), diff --git a/workspaces/config/lib/definitions/definitions.js b/workspaces/config/lib/definitions/definitions.js index bd5c82363937c..08b30b232b379 100644 --- a/workspaces/config/lib/definitions/definitions.js +++ b/workspaces/config/lib/definitions/definitions.js @@ -2512,6 +2512,11 @@ const definitions = { Optional dependencies that cannot be installed on the current platform or engine (a non-matching \`os\`, \`cpu\`, or \`libc\`) are not flagged, because their install scripts never run. + + Installation checks skip unused entries with no package parent or + incoming links. Local link targets that installation can still rebuild + remain subject to this check. \`npm rebuild\` and \`npm install-scripts ls\` + still check unused packages on disk. `, flatten, }), diff --git a/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js b/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js index 579fd0c0fddf2..383521fab8fa9 100644 --- a/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js +++ b/workspaces/libnpmexec/lib/strict-allow-scripts-preflight.js @@ -24,6 +24,7 @@ const strictAllowScriptsPreflight = async (arb, opts) => { policy: opts.allowScripts || null, ignoreScripts: opts.ignoreScripts, dangerouslyAllowAllScripts: opts.dangerouslyAllowAllScripts, + skipExtraneous: true, }) if (unreviewed.length === 0) { diff --git a/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js b/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js index 5fc803c26c8d1..6322c6877f70d 100644 --- a/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js +++ b/workspaces/libnpmexec/test/strict-allow-scripts-preflight.js @@ -13,8 +13,25 @@ const unreviewedTree = { ]), } -const fakeArb = (idealTree) => { +const orphanTree = { + inventory: new Map([ + ['node_modules/orphan', { + name: 'orphan', + location: 'node_modules/orphan', + resolved: 'https://registry.npmjs.org/orphan/-/orphan-1.0.0.tgz', + extraneous: true, + isRegistryDependency: false, + edgesIn: new Set(), + parent: null, + linksIn: new Set(), + package: { scripts: { install: 'node-gyp rebuild' } }, + }], + ]), +} + +const fakeArb = (idealTree, options = {}) => { const arb = { + options, idealTree, buildIdealTreeCalled: false, async buildIdealTree () { @@ -64,6 +81,48 @@ t.test('throws when unreviewed scripts are present', async t => { ) }) +t.test('skips disconnected orphans when pruning is disabled', async t => { + const arb = fakeArb(orphanTree, { prune: false }) + await t.resolves(strictAllowScriptsPreflight(arb, { + strictAllowScripts: true, + allowScripts: { orphan: false }, + prune: false, + })) + t.notOk(arb.buildIdealTreeCalled, 'reuses the ideal tree') +}) + +t.test('skips orphans in a newly built ideal tree', async t => { + const arb = fakeArb(null) + arb.buildIdealTree = async () => { + arb.idealTree = orphanTree + } + await t.resolves(strictAllowScriptsPreflight(arb, { strictAllowScripts: true })) + t.equal(arb.idealTree, orphanTree, 'built the ideal tree') +}) + +t.test('does not skip an extraneous target with an incoming link', async t => { + const target = { + ...orphanTree.inventory.get('node_modules/orphan'), + linksIn: new Set([{}]), + } + const arb = fakeArb({ inventory: new Map([['node_modules/orphan', target]]) }, { prune: false }) + await t.rejects(strictAllowScriptsPreflight(arb, { + strictAllowScripts: true, + allowScripts: { orphan: false }, + prune: false, + }), { code: 'ESTRICTALLOWSCRIPTS', message: /orphan/ }) +}) + +t.test('skipping an orphan does not skip a required unreviewed package', async t => { + const arb = fakeArb({ + inventory: new Map([...orphanTree.inventory, ...unreviewedTree.inventory]), + }) + await t.rejects(strictAllowScriptsPreflight(arb, { strictAllowScripts: true }), { + code: 'ESTRICTALLOWSCRIPTS', + message: /1 package\(s\) have install scripts not covered by allowScripts:\n {2}has-scripts/, + }) +}) + t.test('resolves when the only unreviewed node is inert', async t => { // Inert deps (platform/engine-incompatible) are removed before any script // runs, so strict mode must not reject them (npm/cli#9562).