diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 584690b68b5c6..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( @@ -257,7 +258,8 @@ 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', + audit: false, }, prefixDir: { 'package.json': JSON.stringify({ name: '@npmcli/test-package', version: '1.0.0' }), @@ -268,7 +270,129 @@ 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', + audit: false, + }, + 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', + audit: false, + }, + 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', + audit: false, + }, + 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' + ) + }) + + 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', + audit: false, + }, + 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' ) }) }) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 2648ddedc5a60..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') @@ -649,6 +658,45 @@ 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 optName = ALLOW_OPTION_FOR_TYPE[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(), + } + ) + } + + // 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. @@ -1029,7 +1077,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 +1263,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 +1287,14 @@ 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) { + 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. // 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,23 +1349,11 @@ 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) - .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 - }) + return this.#fetchManifest(spec, parent, edge) + .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 2bd64212df043..0d3a36af6902c 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') @@ -704,7 +705,14 @@ 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) + ), + // 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) { @@ -828,6 +836,24 @@ 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 { + // 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.toLowerCase() + return resolvedHost === registryHost + } catch { + return false + } + } + #registryResolved (resolved) { // the default registry url is a magic value meaning "the currently // configured registry". 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)') +})