From e0f12f7e57aca36f71c9615bd971f427ed23e91a Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 12 May 2026 09:05:33 -0700 Subject: [PATCH 1/3] feat: add allow-git/allow-file/allow-directory/allow-remote configs --- test/lib/commands/install.js | 77 ++++++++++++++++- .../arborist/lib/arborist/build-ideal-tree.js | 58 ++++++++++++- workspaces/arborist/lib/arborist/reify.js | 6 +- .../test/arborist/build-ideal-tree.js | 84 +++++++++++++++++++ 4 files changed, 218 insertions(+), 7 deletions(-) diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 584690b68b5c6..205e64ed3a75a 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -257,7 +257,7 @@ t.test('exec commands', async t => { t.test('allow-git=root refuses non-root git dependency', async t => { const { npm } = await loadMockNpm(t, { config: { - 'allow-git': 'none', + 'allow-git': 'root', }, prefixDir: { 'package.json': JSON.stringify({ name: '@npmcli/test-package', version: '1.0.0' }), @@ -268,7 +268,80 @@ t.test('exec commands', async t => { }) await t.rejects( npm.exec('install', ['./abbrev']), - /Fetching packages of type "git" have been disabled/ + /Fetching non-root packages of type "git" have been disabled/ + ) + }) + + t.test('allow-directory=none blocks default symlink install', async t => { + const { npm } = await loadMockNpm(t, { + config: { + 'allow-directory': 'none', + }, + prefixDir: { + 'package.json': JSON.stringify({ + name: '@npmcli/test-package', + version: '1.0.0', + dependencies: { 'dir-dep': 'file:./dir-dep' }, + }), + 'dir-dep': { + 'package.json': JSON.stringify({ name: 'dir-dep', version: '1.0.0' }), + }, + }, + }) + await t.rejects( + npm.exec('install', []), + { + code: 'EALLOWDIRECTORY', + message: 'Fetching packages of type "directory" have been disabled', + } + ) + }) + + t.test('allow-directory=root permits top-level directory dependency', async t => { + const { npm } = await loadMockNpm(t, { + config: { + 'allow-directory': 'root', + }, + prefixDir: { + 'package.json': JSON.stringify({ + name: '@npmcli/test-package', + version: '1.0.0', + dependencies: { 'dir-dep': 'file:./dir-dep' }, + }), + 'dir-dep': { + 'package.json': JSON.stringify({ name: 'dir-dep', version: '1.0.0' }), + }, + }, + }) + await npm.exec('install', []) + const installedPkg = require(path.join(npm.prefix, 'node_modules', 'dir-dep', 'package.json')) + t.equal(installedPkg.name, 'dir-dep', 'dir-dep is installed and readable through node_modules') + }) + + t.test('allow-git=root soft-skips transitive optional git dependency', async t => { + const { npm } = await loadMockNpm(t, { + config: { + 'allow-git': 'root', + }, + prefixDir: { + 'package.json': JSON.stringify({ name: '@npmcli/test-package', version: '1.0.0' }), + abbrev: { + 'package.json': JSON.stringify({ + name: 'abbrev', + version: '1.0.0', + optionalDependencies: { npm: 'npm/npm' }, + }), + }, + }, + }) + await npm.exec('install', ['./abbrev']) + t.ok( + fs.existsSync(path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')), + 'abbrev (the legitimate parent) is installed' + ) + t.notOk( + fs.existsSync(path.join(npm.prefix, 'node_modules', 'npm')), + 'optional transitive git dep is silently skipped' ) }) }) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 2648ddedc5a60..4e552502eceab 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -649,6 +649,37 @@ module.exports = cls => class IdealTreeBuilder extends cls { return vuln.range } + // Enforces the allow-git / allow-file / allow-directory / allow-remote configs at the arborist resolution layer, before any branching into the symlink (Link) path or the manifest-fetch path. + // Pacote also enforces these inside FetcherBase.get() as defense-in-depth, but the symlink branch never reaches pacote, and the manifest cache here would bypass pacote on a cached hit. + // Throws the same { code: EALLOW${TYPE} } shape pacote uses, so callers and downstream consumers stay consistent. + #checkAllow (spec, edge) { + const optionFor = { + git: 'allowGit', + remote: 'allowRemote', + file: 'allowFile', + directory: 'allowDirectory', + } + const optName = optionFor[spec.type] + if (!optName) { + return + } + const allow = this.options[optName] ?? 'all' + if (allow === 'all') { + return + } + const isRoot = !!(edge?.from?.isProjectRoot || edge?.from?.isWorkspace) + if (allow !== 'none' && isRoot) { + return + } + throw Object.assign( + new Error(`Fetching${allow === 'root' ? ' non-root' : ''} packages of type "${spec.type}" have been disabled`), + { + code: `EALLOW${spec.type.toUpperCase()}`, + package: spec.toString(), + } + ) + } + #queueNamedUpdates () { // ignore top nodes, since they are not loaded the same way, and // probably have their own project associated with them. @@ -1029,7 +1060,7 @@ This is a one-time fix-up, please be patient... // This can't be changed or removed till we figure out why // The test is named "tarball deps with transitive tarball deps" promises.push(() => - this.#fetchManifest(npa.resolve(e.name, e.spec, fromPath(placed, e)), parent) + this.#fetchManifest(npa.resolve(e.name, e.spec, fromPath(placed, e)), parent, e) .catch(() => null) ) } @@ -1215,12 +1246,14 @@ This is a one-time fix-up, please be patient... return problems } - async #fetchManifest (spec, parent) { + async #fetchManifest (spec, parent, edge) { + // Enforce allow-* gates before consulting the manifest cache so a cached entry from a different edge cannot bypass the policy. + this.#checkAllow(spec, edge) const options = { ...this.options, avoid: this.#avoidRange(spec.name), fullMetadata: true, - _isRoot: parent?.isProjectRoot || parent?.isWorkspace, + _isRoot: !!(edge?.from?.isProjectRoot || edge?.from?.isWorkspace), } // get the intended spec and stored metadata from yarn.lock file, // if available and valid. @@ -1237,6 +1270,23 @@ This is a one-time fix-up, please be patient... } async #nodeFromSpec (name, spec, parent, edge) { + // Enforce allow-git / allow-file / allow-directory / allow-remote before any branching, so the symlink (Link) path is enforced as well as the manifest-fetch path. + // Route the failure through #loadFailures so optional-dep semantics apply (e.g. a transitive optionalDependencies entry that resolves to a disallowed git URL is silently dropped rather than failing the install). + try { + this.#checkAllow(spec, edge) + } catch (error) { + error.requiredBy = edge?.from?.location || '.' + const n = new Node({ + name, + parent, + error, + installLinks: this.installLinks, + legacyPeerDeps: this.legacyPeerDeps, + }) + this.#loadFailures.add(n) + return n + } + // pacote will slap integrity on its options, so we have to clone the object so it doesn't get mutated. // Don't bother to load the manifest for link deps, because the target might be within another package that doesn't exist yet. const { installLinks, legacyPeerDeps } = this @@ -1291,7 +1341,7 @@ This is a one-time fix-up, please be patient... // spec isn't a directory, and either isn't a workspace or the workspace we have // doesn't satisfy the edge. try to fetch a manifest and build a node from that. - return this.#fetchManifest(spec, parent) + return this.#fetchManifest(spec, parent, edge) .then(pkg => new Node({ name, pkg, parent, installLinks, legacyPeerDeps }), error => { error.requiredBy = edge.from.location || '.' diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 2bd64212df043..d3c6ae4ade9dc 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -704,7 +704,11 @@ module.exports = cls => class Reifier extends cls { ...this.options, resolved: node.resolved, integrity: node.integrity, - _isRoot: node.parent?.isProjectRoot || node.parent?.isWorkspace, + // A node counts as "root" for allow-* enforcement if it satisfies at least one valid dependency edge declared by the project root or a workspace. + // node.parent is unsafe here: after hoisting, transitive packages can have the project root as their tree parent. + _isRoot: [...node.edgesIn].some(e => + e.valid && (e.from?.isProjectRoot || e.from?.isWorkspace) + ), }) // store nodes don't use Node class so node.package doesn't get updated if (node.isInStore) { diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index b4bf6bdbce8ee..143255a0ec9cd 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -4732,3 +4732,87 @@ t.test('overrides with bundledDependencies', async t => { t.notOk(tree.children.get('bar'), 'bar stays inside dep bundle') }) }) + +t.test('allow-directory=root permits a top-level directory dependency', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root-pkg', + version: '1.0.0', + dependencies: { 'dir-dep': 'file:./dir-dep' }, + }), + 'dir-dep': { + 'package.json': JSON.stringify({ name: 'dir-dep', version: '1.0.0' }), + }, + }) + const tree = await buildIdeal(path, { allowDirectory: 'root' }) + t.ok(tree.children.get('dir-dep'), 'dir-dep is in the ideal tree') + t.equal(tree.children.get('dir-dep').isLink, true, 'dir-dep is a Link node') +}) + +t.test('allow-directory=none blocks a top-level directory dependency before the symlink branch', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root-pkg', + version: '1.0.0', + dependencies: { 'dir-dep': 'file:./dir-dep' }, + }), + 'dir-dep': { + 'package.json': JSON.stringify({ name: 'dir-dep', version: '1.0.0' }), + }, + }) + await t.rejects( + buildIdeal(path, { allowDirectory: 'none' }), + { code: 'EALLOWDIRECTORY' }, + 'arborist refuses before reaching pacote or the Link branch' + ) +}) + +t.test('allow-directory=root blocks a transitive directory dependency', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root-pkg', + version: '1.0.0', + dependencies: { parent: 'file:./parent' }, + }), + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + dependencies: { child: 'file:./child' }, + }), + child: { + 'package.json': JSON.stringify({ name: 'child', version: '1.0.0' }), + }, + }, + }) + await t.rejects( + buildIdeal(path, { allowDirectory: 'root' }), + { code: 'EALLOWDIRECTORY' }, + 'transitive directory dep is refused because edge.from is not the project root' + ) +}) + +t.test('allow-directory=root soft-skips a transitive optional directory dependency', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'root-pkg', + version: '1.0.0', + dependencies: { parent: 'file:./parent' }, + }), + parent: { + 'package.json': JSON.stringify({ + name: 'parent', + version: '1.0.0', + optionalDependencies: { 'opt-child': 'file:./opt-child' }, + }), + 'opt-child': { + 'package.json': JSON.stringify({ name: 'opt-child', version: '1.0.0' }), + }, + }, + }) + const tree = await buildIdeal(path, { allowDirectory: 'root' }) + t.ok(tree.children.get('parent'), 'parent (root-edge) is in the tree') + const optChild = [...tree.inventory.values()].find(n => n.name === 'opt-child') + t.ok(optChild, 'blocked optional transitive is recorded in the tree') + t.equal(optChild.inert, true, 'blocked optional transitive is marked inert (will not be reified)') +}) From 1f17566ad90353b88e81fa1f8a4da5879d3ec7a3 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 12 May 2026 11:12:15 -0700 Subject: [PATCH 2/3] fix: allow-remote=none does not block registry tarballs --- test/lib/commands/install.js | 45 +++++++++++++++++++++++ workspaces/arborist/lib/arborist/reify.js | 21 +++++++++++ 2 files changed, 66 insertions(+) diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 205e64ed3a75a..16c947c397265 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -344,6 +344,51 @@ t.test('exec commands', async t => { 'optional transitive git dep is silently skipped' ) }) + + t.test('allow-remote=none does not block registry tarballs', async t => { + const { npm, registry } = await loadMockNpm(t, { + config: { + 'allow-remote': 'none', + audit: false, + }, + prefixDir: { + 'package.json': JSON.stringify({ + ...packageJson, + dependencies: { abbrev: '^1.0.0' }, + }), + abbrev, + }, + }) + const manifest = registry.manifest({ name: 'abbrev' }) + await registry.package({ manifest }) + await registry.tarball({ + manifest: manifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'abbrev'), + }) + await npm.exec('install', []) + const installed = require(path.join(npm.prefix, 'node_modules', 'abbrev', 'package.json')) + t.equal(installed.name, 'abbrev', 'registry dep is installed despite allow-remote=none') + }) + + t.test('allow-remote=none still blocks a user-supplied remote URL', async t => { + const { npm } = await loadMockNpm(t, { + config: { + 'allow-remote': 'none', + }, + prefixDir: { + 'package.json': JSON.stringify({ + name: '@npmcli/test-package', + version: '1.0.0', + dependencies: { abbrev: 'https://registry.npmjs.org/abbrev/-/abbrev-2.0.0.tgz' }, + }), + }, + }) + await t.rejects( + npm.exec('install', []), + { code: 'EALLOWREMOTE' }, + 'user-supplied remote URL is still blocked' + ) + }) }) t.test('completion', async t => { diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index d3c6ae4ade9dc..7c688740df6fd 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -4,6 +4,7 @@ const hgi = require('hosted-git-info') const npa = require('npm-package-arg') const packageContents = require('@npmcli/installed-package-contents') const pacote = require('pacote') +const { pickRegistry } = require('npm-registry-fetch') const promiseAllRejectLate = require('promise-all-reject-late') const runScript = require('@npmcli/run-script') const { callLimit: promiseCallLimit } = require('promise-call-limit') @@ -709,6 +710,9 @@ module.exports = cls => class Reifier extends cls { _isRoot: [...node.edgesIn].some(e => e.valid && (e.from?.isProjectRoot || e.from?.isWorkspace) ), + // pacote's npa re-parses our `name@URL` spec as type=remote, so allowRemote would mis-fire on registry tarballs. + // Override only when we can prove the URL is registry-mediated; see #isRegistryResolvedTarball. + ...(this.#isRegistryResolvedTarball(node) ? { allowRemote: 'all' } : {}), }) // store nodes don't use Node class so node.package doesn't get updated if (node.isInStore) { @@ -832,6 +836,23 @@ module.exports = cls => class Reifier extends cls { return wrapper } + // When extracting a registry-resolved package, the spec we hand to pacote is name@URL. + // pacote re-parses that with npa and gets spec.type === 'remote', so without an override the allow-remote gate would fire on every registry tarball (both =none and =root mis-fire). + // Returns true only when we are confident this is a registry-mediated install: the node's inbound edges must all be registry-typed (no exotic spec smuggled the URL in) AND the resolved URL's host must match the registry npm-registry-fetch selected for this spec, so a tampered lockfile pointing at an attacker host still hits the gate. + #isRegistryResolvedTarball (node) { + if (!node.resolved || !node.isRegistryDependency) { + return false + } + try { + const resolvedHost = new URL(node.resolved).hostname + // pickRegistry only consults spec.scope, so a bare-name (tag) parse is sufficient and avoids a node.version dependency. + const registryHost = new URL(pickRegistry(npa(node.name), this.options)).hostname + return resolvedHost === registryHost + } catch { + return false + } + } + #registryResolved (resolved) { // the default registry url is a magic value meaning "the currently // configured registry". From f550eb415de0aff83a5fa297850104f5390a6e30 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 13 May 2026 09:26:40 -0700 Subject: [PATCH 3/3] fix: refactor #failureNode, adjust tests and safety --- test/lib/commands/install.js | 6 ++ .../arborist/lib/arborist/build-ideal-tree.js | 62 +++++++++---------- workspaces/arborist/lib/arborist/reify.js | 5 +- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 16c947c397265..2aa56a3dcd175 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -242,6 +242,7 @@ t.test('exec commands', async t => { const { npm } = await loadMockNpm(t, { config: { 'allow-git': 'none', + audit: false, }, }) await t.rejects( @@ -258,6 +259,7 @@ t.test('exec commands', async t => { const { npm } = await loadMockNpm(t, { config: { 'allow-git': 'root', + audit: false, }, prefixDir: { 'package.json': JSON.stringify({ name: '@npmcli/test-package', version: '1.0.0' }), @@ -276,6 +278,7 @@ t.test('exec commands', async t => { const { npm } = await loadMockNpm(t, { config: { 'allow-directory': 'none', + audit: false, }, prefixDir: { 'package.json': JSON.stringify({ @@ -301,6 +304,7 @@ t.test('exec commands', async t => { const { npm } = await loadMockNpm(t, { config: { 'allow-directory': 'root', + audit: false, }, prefixDir: { 'package.json': JSON.stringify({ @@ -322,6 +326,7 @@ t.test('exec commands', async t => { const { npm } = await loadMockNpm(t, { config: { 'allow-git': 'root', + audit: false, }, prefixDir: { 'package.json': JSON.stringify({ name: '@npmcli/test-package', version: '1.0.0' }), @@ -374,6 +379,7 @@ t.test('exec commands', async t => { const { npm } = await loadMockNpm(t, { config: { 'allow-remote': 'none', + audit: false, }, prefixDir: { 'package.json': JSON.stringify({ diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 4e552502eceab..c9d9c308cbc9c 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -28,6 +28,15 @@ const Shrinkwrap = require('../shrinkwrap.js') const { defaultLockfileVersion } = Shrinkwrap const Node = require('../node.js') const Link = require('../link.js') + +// Maps a parsed spec.type to the corresponding allow-* arborist option name. +// Hoisted to module scope so #checkAllow doesn't re-allocate it per call. +const ALLOW_OPTION_FOR_TYPE = { + git: 'allowGit', + remote: 'allowRemote', + file: 'allowFile', + directory: 'allowDirectory', +} const addRmPkgDeps = require('../add-rm-pkg-deps.js') const optionalSet = require('../optional-set.js') const { checkEngine, checkPlatform } = require('npm-install-checks') @@ -653,13 +662,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { // Pacote also enforces these inside FetcherBase.get() as defense-in-depth, but the symlink branch never reaches pacote, and the manifest cache here would bypass pacote on a cached hit. // Throws the same { code: EALLOW${TYPE} } shape pacote uses, so callers and downstream consumers stay consistent. #checkAllow (spec, edge) { - const optionFor = { - git: 'allowGit', - remote: 'allowRemote', - file: 'allowFile', - directory: 'allowDirectory', - } - const optName = optionFor[spec.type] + const optName = ALLOW_OPTION_FOR_TYPE[spec.type] if (!optName) { return } @@ -680,6 +683,20 @@ module.exports = cls => class IdealTreeBuilder extends cls { ) } + // Builds a Node representing a spec we failed to load (allow-* gate, network failure, ENOTARGET, etc.) and records it in #loadFailures so #pruneFailedOptional can later decide whether the failure is fatal or silently dropped for optional deps. + #failureNode (name, parent, error, edge) { + error.requiredBy = edge?.from?.location || '.' + const n = new Node({ + name, + parent, + error, + installLinks: this.installLinks, + legacyPeerDeps: this.legacyPeerDeps, + }) + this.#loadFailures.add(n) + return n + } + #queueNamedUpdates () { // ignore top nodes, since they are not loaded the same way, and // probably have their own project associated with them. @@ -1275,16 +1292,7 @@ This is a one-time fix-up, please be patient... try { this.#checkAllow(spec, edge) } catch (error) { - error.requiredBy = edge?.from?.location || '.' - const n = new Node({ - name, - parent, - error, - installLinks: this.installLinks, - legacyPeerDeps: this.legacyPeerDeps, - }) - this.#loadFailures.add(n) - return n + return this.#failureNode(name, parent, error, edge) } // pacote will slap integrity on its options, so we have to clone the object so it doesn't get mutated. @@ -1342,22 +1350,10 @@ This is a one-time fix-up, please be patient... // spec isn't a directory, and either isn't a workspace or the workspace we have // doesn't satisfy the edge. try to fetch a manifest and build a node from that. return this.#fetchManifest(spec, parent, edge) - .then(pkg => new Node({ name, pkg, parent, installLinks, legacyPeerDeps }), error => { - error.requiredBy = edge.from.location || '.' - - // failed to load the spec, either because of enotarget or - // fetch failure of some other sort. save it so we can verify - // later that it's optional; otherwise, the error is fatal. - const n = new Node({ - name, - parent, - error, - installLinks, - legacyPeerDeps, - }) - this.#loadFailures.add(n) - return n - }) + .then( + pkg => new Node({ name, pkg, parent, installLinks, legacyPeerDeps }), + error => this.#failureNode(name, parent, error, edge) + ) } // load all peer deps and meta-peer deps into the node's parent diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 7c688740df6fd..0d3a36af6902c 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -844,9 +844,10 @@ module.exports = cls => class Reifier extends cls { return false } try { - const resolvedHost = new URL(node.resolved).hostname + // Hostnames are case-insensitive; lowercase both sides for safety even though WHATWG URL already normalizes. + const resolvedHost = new URL(node.resolved).hostname.toLowerCase() // pickRegistry only consults spec.scope, so a bare-name (tag) parse is sufficient and avoids a node.version dependency. - const registryHost = new URL(pickRegistry(npa(node.name), this.options)).hostname + const registryHost = new URL(pickRegistry(npa(node.name), this.options)).hostname.toLowerCase() return resolvedHost === registryHost } catch { return false