fix: skip disconnected orphans in install script checks

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

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

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 })
 }
 

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
   }

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\`

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
+}

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, {

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'] }),

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, {