From 1608f4b42cb9590e1a76e4445b24b86a8a1682aa Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Wed, 24 Jun 2026 19:52:03 +0530 Subject: [PATCH] fix(arborist): audit the non-isolated tree under the linked strategy (#9625) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In continuation of our exploration of using `install-strategy=linked` in the [Gutenberg monorepo](https://github.com/WordPress/gutenberg/pull/75814), which Under `install-strategy=linked`, `npm install --audit` reported `found 0 vulnerabilities` even with a known-vulnerable package installed, while standalone `npm audit` reported it correctly. Only the install-time audit was affected. ## Why A linked reify swaps `idealTree` for the isolated tree (`createIsolatedTree()`) before the quick audit runs, so `_submitQuickAudit()` audited the isolated tree. That tree cannot be audited: its inventory had a stub `query()` that always returned `[]`, and its edges route through symlink `Link`s instead of real package nodes. So `AuditReport.prepareBulkData()` produced an empty bulk request and the registry was never asked about any installed version. Standalone `npm audit` was unaffected because it audits the regular tree loaded from the lockfile. ## How `reify.js` stashes the original non-isolated ideal tree in `#linkedIdealForAudit` during the linked swap, and `_submitQuickAudit()` now audits `this.#linkedIdealForAudit || this.idealTree` — the same tree standalone `npm audit` uses, with a queryable inventory and real package nodes. The `_diffTrees()`/`#reifyPackages()`/orphan-sweep block is wrapped in `try/finally` that restores `idealTree` and clears the stashed references even if reify throws, so a reused Arborist never audits or diffs a stale isolated tree. `isolated-classes.js` drops the now-unused `IsolatedInventory` class (its only caller was the rerouted audit path) in favor of a plain `Map`; the `query()` stub returning `[]` was the silent-empty behavior behind this bug. ## References Fixes #9609 Part of #9608 (cherry picked from commit 989f571fe9064d170f2a4d7c4b1ed238be3851a9) --- workspaces/arborist/lib/arborist/reify.js | 36 ++++++++++++++------- workspaces/arborist/lib/isolated-classes.js | 9 +----- workspaces/arborist/test/arborist/reify.js | 15 +++++++++ 3 files changed, 40 insertions(+), 20 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 15d499e7fb5d9..960510a86531e 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -77,6 +77,9 @@ module.exports = cls => class Reifier extends cls { #sparseTreeDirs = new Set() #sparseTreeRoots = new Set() #linkedActualForDiff = null + // Under the linked strategy the audit runs against this non-isolated ideal tree. + // The isolated tree's inventory has no queryable indexes and its edges route through symlinks, so auditing it reports no vulnerabilities. + #linkedIdealForAudit = null constructor (options) { super(options) @@ -123,21 +126,29 @@ module.exports = cls => class Reifier extends cls { this.idealTree, this.actualTree ) } + // Keep the non-isolated tree so the quick audit can run against it. + this.#linkedIdealForAudit = oldTree } - await this[_diffTrees]() - await this.#reifyPackages() - if (linked) { - // The sweep mutates node_modules on disk, so skip it for dry runs and lockfile-only installs (those modes also short-circuit #reifyPackages). - // The sweep itself scopes to in-filter workspaces when a filter is active, so it's safe to run for filtered installs too. - if (!this.options.dryRun && !this.options.packageLockOnly) { - await this.#cleanOrphanedStoreEntries() + try { + await this[_diffTrees]() + await this.#reifyPackages() + if (linked) { + // The sweep mutates node_modules on disk, so skip it for dry runs and lockfile-only installs (those modes also short-circuit #reifyPackages). + // The sweep itself scopes to in-filter workspaces when a filter is active, so it's safe to run for filtered installs too. + if (!this.options.dryRun && !this.options.packageLockOnly) { + await this.#cleanOrphanedStoreEntries() + } + } + } finally { + // Restore the non-isolated tree so the lockfile is preserved and a reused Arborist never sees the isolated tree, even if reify throws. + if (linked) { + this.idealTree = oldTree } - // swap back in the idealTree - // so that the lockfile is preserved - this.idealTree = oldTree + // The quick audit has captured its tree synchronously by now, so drop the stashed references even on throw. + this.#linkedIdealForAudit = null + this.#linkedActualForDiff = null } await this[_saveIdealTree](options) - this.#linkedActualForDiff = null // clean inert for (const node of this.idealTree.inventory.values()) { if (node.inert) { @@ -1152,7 +1163,8 @@ module.exports = cls => class Reifier extends cls { // with the reification, and be resolved at a later time. const timeEnd = time.start('reify:audit') const options = { ...this.options } - const tree = this.idealTree + // Under the linked strategy idealTree is the isolated tree, which the audit cannot traverse; audit the non-isolated tree instead. + const tree = this.#linkedIdealForAudit || this.idealTree // if we're operating on a workspace, only audit the workspace deps if (this.options.workspaces.length) { diff --git a/workspaces/arborist/lib/isolated-classes.js b/workspaces/arborist/lib/isolated-classes.js index 3a2ade64bbb3e..fc119119664d9 100644 --- a/workspaces/arborist/lib/isolated-classes.js +++ b/workspaces/arborist/lib/isolated-classes.js @@ -2,13 +2,6 @@ const CaseInsensitiveMap = require('./case-insensitive-map.js') const { resolve } = require('node:path') -// fake lib/inventory.js -class IsolatedInventory extends Map { - query () { - return [] - } -} - // fake lib/node.js class IsolatedNode { binPaths = [] @@ -18,7 +11,7 @@ class IsolatedNode { fsChildren = new Set() hasShrinkwrap = false integrity = null - inventory = new IsolatedInventory() + inventory = new Map() isInStore = false inBundle = false isRegistryDependency = false diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index f77a8b17974d9..becde0b6fa4c2 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -221,6 +221,21 @@ t.test('packageLockOnly with linked strategy in workspaces', async t => { t.throws(() => fs.statSync(path + '/node_modules'), { code: 'ENOENT' }) }) +t.test('linked strategy audits the non-isolated tree', async t => { + // The isolated tree has no queryable inventory, so auditing it reports nothing. The install-time audit must run against the non-isolated tree, matching standalone npm audit. https://github.com/npm/cli/issues/9609 + const src = resolve(fixtures, 'audit-one-vuln') + // Copy into a throwaway dir since packageLockOnly rewrites the lockfile. + const path = t.testdir({ + 'package.json': fs.readFileSync(join(src, 'package.json'), 'utf8'), + 'package-lock.json': fs.readFileSync(join(src, 'package-lock.json'), 'utf8'), + }) + const registry = createRegistry(t, true) + registry.audit({ convert: true, results: require(join(src, 'audit.json')) }) + const arb = newArb({ path, audit: true, packageLockOnly: true, installStrategy: 'linked' }) + await arb.reify() + t.ok(arb.auditReport.has('minimist'), 'vulnerable package reported under linked strategy') +}) + t.test('malformed package.json should not be overwritten', async t => { t.plan(2)