fix(arborist): harden .npm-extension validation, staleness probe, and workspace warning

Author: manzoorwanijk

workspaces/arborist/lib/arborist/build-ideal-tree.js

@@ -28,6 +28,7 @@ const { isReleaseAgeExcluded, trustedSpecName } = require('../release-age-exclud
 const { resolvePatchedDependencies } = require('../patched-dependencies.js')
 const PackageExtensions = require('../package-extensions.js')
 const NpmExtension = require('../npm-extension.js')
+const { hasExtensionFile } = require('../npm-extension.js')
 const Shrinkwrap = require('../shrinkwrap.js')
 const { defaultLockfileVersion } = Shrinkwrap
 const Node = require('../node.js')
@@ -193,6 +194,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
         rm: options.rm || [],
       })
       this.#warnWorkspacePackageExtensions()
+      this.#warnWorkspaceNpmExtension()
     } finally {
       timeEnd()
       this.finishTracker('idealTree')
@@ -285,8 +287,9 @@ module.exports = cls => class IdealTreeBuilder extends cls {
   // The selected file's hash is stashed on the lockfile meta so commit() can persist it and npm ci can detect stale extension state.
   async #loadNpmExtension () {
     const lockedHash = this.idealTree.meta.npmExtensionHash
+    // ignore-extension (and, via flatten, ignore-scripts) disables discovery and execution.
+    // Leave any locked extension state untouched so npm ci reifies the locked graph as-is and the lockfile stays internally consistent.
     if (this.options.ignoreExtension) {
-      this.idealTree.meta.npmExtensionHash = null
       return
     }
     const ext = new NpmExtension({
@@ -295,20 +298,22 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     })
     this.#npmExtension = ext
     this.idealTree.meta.npmExtensionHash = ext.hash
-    if (!ext.present) {
-      return
+    if (ext.present) {
+      await ext.load()
     }
-    await ext.load()
 
     // When the extension file changed since the lockfile was written, locked manifests may no longer reflect its output.
-    // Arbitrary code has no selector to predict which packages it affects, so re-resolve any node that carried old provenance or that the new file now transforms.
+    // This also covers removal: a now-absent file hashes to null, which differs from the locked hash and reverts the previously transformed nodes.
+    // Arbitrary code has no selector to predict which packages it affects, so re-resolve any node that carried old provenance or that the current file now transforms.
     if (this.idealTree.meta.loadedFromDisk && lockedHash !== ext.hash) {
       for (const node of [...this.idealTree.inventory.values()]) {
         if (node.isProjectRoot || node.isWorkspace || node.isTop || node.isLink) {
           continue
         }
-        // a node with old provenance carries an already-transformed manifest, so refetch; otherwise its locked manifest is the raw baseline the new file can be tried against
-        const affected = node.npmExtensionApplied || ext.apply(node.package)
+        // A node with old provenance carries an already-transformed manifest, so always refetch it.
+        // Otherwise probe the locked manifest (without caching, so the authoritative full-manifest fetch is not pre-seeded).
+        // The locked manifest carries every resolution-relevant field (name, version, dependencies, peer metadata); a transform that newly targets a node by a field not persisted in the lockfile is the documented edge that may need a manual re-lock.
+        const affected = node.npmExtensionApplied || ext.apply(node.package, { memoize: false })
         if (affected) {
           for (const edge of node.edgesIn) {
             this.#depsQueue.push(edge.from)
@@ -348,6 +353,20 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     }
   }
 
+  // Warn when a non-root workspace package contains a .npm-extension file; only the workspace root's file is honored.
+  #warnWorkspaceNpmExtension () {
+    for (const node of this.idealTree.inventory.values()) {
+      // a workspace is in the inventory as both a Link and its target node; warn once by skipping the link
+      if (!node.isWorkspace || node.isLink || node.isProjectRoot) {
+        continue
+      }
+      if (hasExtensionFile(node.realpath)) {
+        log.warn('npm-extension',
+          `".npm-extension" in workspace ${node.name} is ignored; it is only honored at the workspace root`)
+      }
+    }
+  }
+
   #parseSettings (options) {
     const update = options.update === true ? { all: true }
       : Array.isArray(options.update) ? { names: options.update }

workspaces/arborist/lib/arborist/load-actual.js

@@ -175,8 +175,9 @@ module.exports = cls => class ActualLoader extends cls {
       await Promise.all(promises)
     }
 
-    this.#applyPackageExtensions()
+    // .npm-extension runs before packageExtensions, matching the ideal-tree resolution order
     await this.#applyNpmExtension()
+    this.#applyPackageExtensions()
 
     if (!ignoreMissing) {
       await this.#findMissingEdges()

workspaces/arborist/lib/npm-extension.js

@@ -3,7 +3,7 @@
 // This module discovers and hashes the root extension file, loads its `transformManifest` export, and applies it to a deeply isolated manifest copy, returning an extended manifest plus minimal provenance.
 // It never mutates the input manifest or any shared cache object.
 const { resolve, sep } = require('node:path')
-const { readFileSync } = require('node:fs')
+const { readFileSync, existsSync } = require('node:fs')
 const { pathToFileURL } = require('node:url')
 const { isDeepStrictEqual } = require('node:util')
 const { log } = require('proc-log')
@@ -102,7 +102,7 @@ class NpmExtension {
     if (this.format === 'mjs') {
       mod = await import(`${pathToFileURL(this.path).href}?h=${this.hash}`)
     } else {
-      delete require.cache[this.path]
+      delete require.cache[require.resolve(this.path)]
       mod = require(this.path)
     }
     const transform = mod?.transformManifest ?? mod?.default?.transformManifest
@@ -114,16 +114,22 @@ class NpmExtension {
 
   // Apply transformManifest to a candidate manifest, returning { pkg, applied } or null when nothing changed.
   // Results are cached once per resolved package identity; consumers get a deeply isolated copy so they cannot mutate the cached effective manifest.
-  apply (pkg) {
+  // Pass { memoize: false } to run without caching, e.g. a staleness probe over partial lockfile manifests that must not seed the cache used by full-manifest fetches.
+  apply (pkg, { memoize = true } = {}) {
     if (!this.#transform || !pkg?.name) {
       return null
     }
     const key = this.#identity(pkg)
-    if (!this.#cache.has(key)) {
-      this.#cache.set(key, this.#run(pkg))
+    let result
+    if (this.#cache.has(key)) {
+      result = this.#cache.get(key)
+    } else {
+      result = this.#run(pkg)
+      if (memoize) {
+        this.#cache.set(key, result)
+      }
     }
-    const result = this.#cache.get(key)
-    return result && { pkg: structuredClone(result.pkg), applied: result.applied }
+    return result && { pkg: structuredClone(result.pkg), applied: structuredClone(result.applied) }
   }
 
   // Identity key for the transform cache: package integrity when available, otherwise resolved source plus name and version.
@@ -157,32 +163,49 @@ class NpmExtension {
         `.npm-extension transformManifest must return a manifest object for ${pkg.name}@${pkg.version}`,
         'ENPMEXTENSIONRETURN', { pkgid: `${pkg.name}@${pkg.version}` })
     }
-    // Only dependency and peer fields may change; any other altered field is rejected so manifest contents stay authoritative.
-    for (const k of new Set([...Object.keys(pkg), ...Object.keys(returned)])) {
+    // Only dependency and peer fields may change; any other field the returned manifest explicitly alters is rejected.
+    // Fields the returned object omits are left untouched, so a handler may return a new object with only the fields it repairs.
+    for (const k of Object.keys(returned)) {
       if (!EXTENSION_FIELDS.includes(k) && !isDeepStrictEqual(returned[k], pkg[k])) {
         throw err(
           `.npm-extension transformManifest changed unsupported field "${k}" on ${pkg.name}@${pkg.version}; only ${EXTENSION_FIELDS.join(', ')} may change`,
           'ENPMEXTENSIONFIELD', { pkgid: `${pkg.name}@${pkg.version}`, field: k })
       }
     }
-    // Build the effective manifest from the original baseline plus the returned allowlisted fields, honoring deletion when a field is omitted.
+    // Build the effective manifest from the normalized baseline plus the returned allowlisted fields.
+    // A field the handler omits is left as the baseline; delete individual entries by returning a field object without them.
     const next = { ...pkg }
     for (const field of EXTENSION_FIELDS) {
       if (returned[field] === undefined) {
-        delete next[field]
         continue
       }
-      if (typeof returned[field] !== 'object' || Array.isArray(returned[field])) {
-        throw err(
-          `.npm-extension transformManifest set ${field} to a non-object on ${pkg.name}@${pkg.version}`,
-          'ENPMEXTENSIONVALUE', { pkgid: `${pkg.name}@${pkg.version}`, field })
-      }
+      this.#validateField(field, returned[field], pkg)
       next[field] = returned[field]
     }
     const applied = this.#provenance(pkg, next)
     return applied && { pkg: next, applied }
   }
 
+  // Validate a returned allowlisted field and its entries, so invalid output fails with .npm-extension and package context.
+  // Dependency maps hold version strings; peerDependenciesMeta holds metadata objects.
+  #validateField (field, value, pkg) {
+    const fail = (suffix) => err(
+      `.npm-extension transformManifest set ${suffix} to an invalid value on ${pkg.name}@${pkg.version}`,
+      'ENPMEXTENSIONVALUE', { pkgid: `${pkg.name}@${pkg.version}`, field })
+    if (value === null || typeof value !== 'object' || Array.isArray(value)) {
+      throw fail(field)
+    }
+    for (const [name, entry] of Object.entries(value)) {
+      if (field === 'peerDependenciesMeta') {
+        if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) {
+          throw fail(`${field}.${name}`)
+        }
+      } else if (typeof entry !== 'string') {
+        throw fail(`${field}.${name}`)
+      }
+    }
+  }
+
   // Minimal provenance: the extension point plus, for each changed allowlisted field, a sorted array of affected dependency names.
   // Returns null when nothing changed.
   #provenance (before, after) {
@@ -203,8 +226,12 @@ class NpmExtension {
   }
 }
 
+// Whether a directory contains any default .npm-extension file; a non-throwing existence check used for non-root workspace warnings.
+const hasExtensionFile = dir => FORMATS.some(f => existsSync(resolve(dir, f.file)))
+
 module.exports = NpmExtension
 module.exports.NpmExtension = NpmExtension
 module.exports.discover = discover
+module.exports.hasExtensionFile = hasExtensionFile
 module.exports.hashFile = hashFile
 module.exports.EXTENSION_POINT = EXTENSION_POINT

workspaces/arborist/test/arborist/reify-npm-extension.js

@@ -179,6 +179,27 @@ t.test('ignore-extension disables the transform and records no state', async t =
   t.notOk(lock.packages['node_modules/foo'].dependencies, 'foo has no extension-added dependency')
 })
 
+t.test('warns when a non-root workspace contains a .npm-extension file', async t => {
+  const warnings = []
+  const onlog = (...m) => m[0] === 'warn' && m[1] === 'npm-extension' && warnings.push(m[2])
+  process.on('log', onlog)
+  t.teardown(() => process.removeListener('log', onlog))
+
+  const dir = t.testdir({
+    'package.json': JSON.stringify({ name: 'root', workspaces: ['packages/*'] }),
+    packages: {
+      a: {
+        'package.json': JSON.stringify({ name: 'a', version: '1.0.0' }),
+        '.npm-extension.cjs': 'module.exports = { transformManifest (p) { return p } }\n',
+      },
+      b: { 'package.json': JSON.stringify({ name: 'b', version: '1.0.0' }) },
+    },
+  })
+  await newArb(dir).buildIdealTree()
+  t.match(warnings.join('\n'), /"\.npm-extension" in workspace a is ignored/, 'warns for the workspace with the file')
+  t.notMatch(warnings.join('\n'), /workspace b/, 'does not warn for the workspace without one')
+})
+
 t.test('a project with no .npm-extension installs normally and records no state', async t => {
   const dir = t.testdir({
     'package.json': JSON.stringify({ name: 'root', dependencies: { foo: '1.0.0' } }),
@@ -242,3 +263,18 @@ t.test('changing the extension file re-resolves affected packages', async t => {
   t.notOk(lock.packages['node_modules/bar'], 'bar removed after the extension stopped adding it')
   t.notOk(lock.packages['node_modules/foo']?.npmExtensionApplied, 'provenance cleared')
 })
+
+t.test('removing the extension file reverts the locked graph', async t => {
+  const dir = await setup(t)
+  await newArb(dir).reify()
+  t.ok(readLock(dir).packages['node_modules/bar'], 'bar installed by the extension')
+
+  // delete the extension file entirely, then reinstall; the transform-added edge and state must be reverted
+  fs.rmSync(join(dir, '.npm-extension.cjs'))
+  await register(t, dir, { withBar: false })
+  await newArb(dir).reify()
+  const lock = readLock(dir)
+  t.notOk(lock.packages['node_modules/bar'], 'bar removed once the extension file is gone')
+  t.notOk(lock.packages[''].npmExtensionHash, 'root hash cleared')
+  t.notOk(lock.packages['node_modules/foo']?.npmExtensionApplied, 'foo provenance cleared')
+})

workspaces/arborist/test/npm-extension.js

@@ -139,19 +139,17 @@ t.test('apply: no-op transform returns null', async t => {
 })
 
 t.test('apply: caches per identity and isolates consumers', async t => {
-  let calls = 0
   const dir = t.testdir({
     '.npm-extension.cjs': cjs(`pkg.dependencies = { ...pkg.dependencies, added: '1.0.0' }; return pkg`),
   })
   const ext = new NpmExtension({ root: dir })
   await ext.load()
-  // count executions by wrapping log via a side-effect file is overkill; instead rely on identity cache returning copies
+  // two consumers of the same cached identity must each get a distinct, deeply isolated object
   const a = ext.apply({ name: 'foo', version: '1.0.0', _integrity: 'sha512-shared' })
   const b = ext.apply({ name: 'foo', version: '1.0.0', _integrity: 'sha512-shared' })
   t.not(a.pkg, b.pkg, 'each consumer gets a distinct object')
   a.pkg.dependencies.added = 'mutated'
   t.equal(b.pkg.dependencies.added, '1.0.0', 'mutating one copy does not affect another')
-  void calls
 })
 
 t.test('apply: rejects invalid transform output', async t => {
@@ -161,7 +159,11 @@ t.test('apply: rejects invalid transform output', async t => {
     ['return []', 'ENPMEXTENSIONRETURN'],
     ['return Promise.resolve(pkg)', 'ENPMEXTENSIONRETURN'],
     [`pkg.scripts = { build: 'x' }; return pkg`, 'ENPMEXTENSIONFIELD'],
+    [`return { name: pkg.name, version: pkg.version, scripts: { build: 'x' } }`, 'ENPMEXTENSIONFIELD'],
     [`pkg.dependencies = 'nope'; return pkg`, 'ENPMEXTENSIONVALUE'],
+    [`pkg.dependencies = null; return pkg`, 'ENPMEXTENSIONVALUE'],
+    [`pkg.dependencies = { x: null }; return pkg`, 'ENPMEXTENSIONVALUE'],
+    [`pkg.peerDependencies = { p: '*' }; pkg.peerDependenciesMeta = { p: null }; return pkg`, 'ENPMEXTENSIONVALUE'],
     [`throw new Error('boom')`, 'ENPMEXTENSIONTHROW'],
   ]
   // a unique dir per case so require/import cache never serves a previous module
@@ -173,6 +175,27 @@ t.test('apply: rejects invalid transform output', async t => {
   }
 })
 
+t.test('apply: a handler may return a new object with only repaired fields', async t => {
+  const dir = t.testdir({
+    '.npm-extension.cjs': cjs(`return { name: pkg.name, version: pkg.version, dependencies: { ...pkg.dependencies, bar: '^2.0.0' } }`),
+  })
+  const ext = new NpmExtension({ root: dir })
+  await ext.load()
+  const res = ext.apply({ name: 'foo', version: '1.0.0', dependencies: { keep: '1' }, scripts: { build: 'x' }, _integrity: 'sha512-z' })
+  t.same(res.pkg.dependencies, { keep: '1', bar: '^2.0.0' }, 'returned dependencies overlaid on the baseline')
+  t.same(res.pkg.scripts, { build: 'x' }, 'omitted non-allowlisted field preserved from the baseline')
+})
+
+t.test('apply: isolates cached provenance between consumers', async t => {
+  const dir = t.testdir({ '.npm-extension.cjs': cjs(`pkg.dependencies = { ...pkg.dependencies, bar: '1' }; return pkg`) })
+  const ext = new NpmExtension({ root: dir })
+  await ext.load()
+  const a = ext.apply({ name: 'foo', version: '1.0.0', _integrity: 'sha512-prov' })
+  const b = ext.apply({ name: 'foo', version: '1.0.0', _integrity: 'sha512-prov' })
+  a.applied.dependencies.push('mutated')
+  t.same(b.applied.dependencies, ['bar'], 'mutating one consumer\'s provenance does not affect another')
+})
+
 t.test('apply: does not mutate the input manifest', async t => {
   const dir = t.testdir({ '.npm-extension.cjs': cjs(`pkg.dependencies = { bar: '1' }; return pkg`) })
   const ext = new NpmExtension({ root: dir })