From f648e9b7c4625fd7831701aa67a918dd6de4e526 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Fri, 12 Jun 2026 09:46:57 +0200 Subject: [PATCH] fix: contain hoisted dependency aliases (GHSA-fr4h-3cph-29xv) (#12343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: contain hoisted dependency aliases (GHSA-fr4h-3cph-29xv) The `nodeLinker: hoisted` install restores its dependency graph straight from the lockfile via `lockfileToHoistedDepGraph`, which joins each dependency alias under a `node_modules` directory and imports the package files there. On a frozen / up-to-date lockfile, resolution is skipped entirely, so the alias validation added for the resolution path never runs. A crafted lockfile alias such as `../../../escape` could therefore escape the install root, and reserved aliases such as `.bin`, `.pnpm`, or `node_modules` could overwrite pnpm-owned layout. Validate every alias at the hoisted-graph directory sink. The shared `safeJoinModulesDir` helper now rejects aliases that are not valid npm package names (path-traversal, absolute, and reserved names) in addition to its containment check, and the hoisted graph routes its `dep.name` sink through it. Pacquet mirrors the boundary: `safe_join_modules_dir` validates the hoister's `dep.0.name` before adding the graph node or recursing, reusing the same dependency-name rule it already applies to direct-dependency aliases. Both stacks surface `ERR_PNPM_INVALID_DEPENDENCY_NAME`. --- Written by an agent (Claude Code, claude-fable-5). * fix: reject invalid dependency aliases at the lockfile verification gate Add an always-on, policy-independent structural check to verifyLockfileResolutions that rejects any importer or package-snapshot dependency alias that is not a valid npm package name. A dependency alias becomes a `node_modules/` directory at link time, so an alias with path-traversal segments or a reserved name (`.bin`, `.pnpm`, `node_modules`) could escape the install root or overwrite pnpm-owned layout. This complements the linker-sink guards: the verifier runs before any fetch or filesystem work and covers every node linker at once, while the sink guards still protect the `trustLockfile` path the verifier skips. The check runs before the cache lookup so a record written by a version that predates the rule cannot fast-path around it, and before the `packages` guard so a tampered importer alias is caught even when nothing is installed. `isValidDependencyAlias` is now exported from `@pnpm/installing.deps-resolver` and reused here. Pacquet mirrors the gate in its lockfile-verification crate with a matching `ERR_PNPM_INVALID_DEPENDENCY_NAME` verdict. --- Written by an agent (Claude Code, claude-fable-5). * docs(package-manager): drop redundant explicit intra-doc link target `is_valid_dependency_alias` is in scope via `use`, so the bare intra-doc link resolves on its own. The explicit path target tripped `rustdoc::redundant-explicit-links` under the CI Doc job's `cargo doc --document-private-items` (the local pre-push hook runs `cargo doc` without that flag, so it didn't surface). --- Written by an agent (Claude Code, claude-fable-5). * refactor(lockfile-verification): fold the alias check into the single candidate pass The dependency-alias check ran as its own full traversal of the lockfile in addition to collectCandidates' existing pass over every package snapshot. Fold it into that pass instead: collectCandidates now also validates each importer and snapshot dependency alias and returns the invalid ones alongside the resolution-shape violations, so the lockfile is walked once per verification rather than twice. Because collectCandidates runs after the verification-cache lookup, the alias check is now covered by the cache the same way the resolution-shape check is: a new dependencyAliasCheck cache identity makes a record written before this rule existed fail canTrustPastCheck, forcing a re-verification. The shared helper is renamed withOfflineCheckCacheIdentities and appends both offline-structural-check identities. No behavior change for valid lockfiles; the same ERR_PNPM_INVALID_DEPENDENCY_NAME is thrown for invalid aliases. Mirrored in pacquet's lockfile-verification crate. --- Written by an agent (Claude Code, claude-fable-5). * refactor: declare pushInvalidAliases after its caller, trim duplicated comments Move `pushInvalidAliases` below `collectCandidates`, following the repo's declare-after-use convention. Collapse the repeated "an alias becomes a node_modules directory, so a traversal/reserved name escapes or overwrites layout" explanation that was copied across the verifier, the hoisted-graph error, and the pacquet mirror down to a single reference each — the full rationale lives once in the validating sink (`safeJoinModulesDir` / `safe_join_modules_dir`) and the user-facing error hints. --- Written by an agent (Claude Code, claude-fable-5). --- .../contain-hoisted-dependency-aliases.md | 14 +++ Cargo.lock | 1 + fs/symlink-dependency/package.json | 6 +- fs/symlink-dependency/src/index.ts | 1 + .../src/safeJoinModulesDir.ts | 36 ++++-- .../test/safeJoinModulesDir.test.ts | 25 +++- .../src/install/recordLockfileVerified.ts | 4 +- .../src/install/verifyLockfileResolutions.ts | 78 ++++++++++-- .../test/install/verifyLockfileResolutions.ts | 54 +++++++++ installing/deps-resolver/src/index.ts | 1 + .../src/lockfileToHoistedDepGraph.ts | 3 +- .../test/lockfileToHoistedDepGraph.test.ts | 83 +++++++++++++ .../crates/lockfile-verification/Cargo.toml | 9 +- .../lockfile-verification/src/errors.rs | 44 +++++++ .../src/record_lockfile_verified.rs | 4 +- .../src/verify_lockfile_resolutions.rs | 114 ++++++++++++++---- .../src/verify_lockfile_resolutions/tests.rs | 96 +++++++++++++++ .../package-manager/src/hoisted_dep_graph.rs | 8 +- .../src/hoisted_dep_graph/tests.rs | 33 +++++ pacquet/crates/package-manager/src/lib.rs | 1 + .../src/safe_join_modules_dir.rs | 47 ++++++++ .../src/safe_join_modules_dir/tests.rs | 34 ++++++ pnpm-lock.yaml | 6 + 23 files changed, 643 insertions(+), 59 deletions(-) create mode 100644 .changeset/contain-hoisted-dependency-aliases.md create mode 100644 installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts create mode 100644 pacquet/crates/package-manager/src/safe_join_modules_dir.rs create mode 100644 pacquet/crates/package-manager/src/safe_join_modules_dir/tests.rs diff --git a/.changeset/contain-hoisted-dependency-aliases.md b/.changeset/contain-hoisted-dependency-aliases.md new file mode 100644 index 0000000000..035e156d63 --- /dev/null +++ b/.changeset/contain-hoisted-dependency-aliases.md @@ -0,0 +1,14 @@ +--- +"@pnpm/fs.symlink-dependency": patch +"@pnpm/installing.deps-resolver": patch +"@pnpm/installing.deps-installer": patch +"@pnpm/installing.deps-restorer": patch +"pnpm": patch +--- + +Reject path-traversal and reserved dependency aliases (such as `../../../escape`, `.bin`, `.pnpm`, or `node_modules`) that come from a lockfile rather than a freshly resolved manifest. A crafted lockfile alias could otherwise be joined directly under a hoisted `node_modules` directory, letting package files be written outside the intended install root or overwrite pnpm-owned layout. + +The fix adds two layers: + +- The `nodeLinker: hoisted` graph builder now validates each alias at the directory sink (`safeJoinModulesDir`), matching the validation pnpm already performs when resolving aliases from manifests. +- The lockfile verification gate (`verifyLockfileResolutions`) now runs an always-on, policy-independent check that rejects any importer or snapshot dependency alias that is not a valid package name, failing the install early — before any fetch or filesystem work — for every node linker at once. diff --git a/Cargo.lock b/Cargo.lock index 3fa728b72c..6ca533b433 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3904,6 +3904,7 @@ dependencies = [ "pacquet-diagnostics", "pacquet-lockfile", "pacquet-reporter", + "pacquet-resolving-parse-wanted-dependency", "pacquet-resolving-resolver-base", "pretty_assertions", "serde", diff --git a/fs/symlink-dependency/package.json b/fs/symlink-dependency/package.json index 86aeaec056..a37c0bd971 100644 --- a/fs/symlink-dependency/package.json +++ b/fs/symlink-dependency/package.json @@ -37,7 +37,8 @@ "dependencies": { "@pnpm/core-loggers": "workspace:*", "@pnpm/types": "workspace:*", - "symlink-dir": "catalog:" + "symlink-dir": "catalog:", + "validate-npm-package-name": "catalog:" }, "peerDependencies": { "@pnpm/logger": "catalog:" @@ -46,7 +47,8 @@ "@jest/globals": "catalog:", "@pnpm/fs.symlink-dependency": "workspace:*", "@pnpm/logger": "workspace:*", - "@pnpm/prepare": "workspace:*" + "@pnpm/prepare": "workspace:*", + "@types/validate-npm-package-name": "catalog:" }, "engines": { "node": ">=22.13" diff --git a/fs/symlink-dependency/src/index.ts b/fs/symlink-dependency/src/index.ts index 5f80c69a58..8e619ce762 100644 --- a/fs/symlink-dependency/src/index.ts +++ b/fs/symlink-dependency/src/index.ts @@ -3,6 +3,7 @@ import { symlinkDir, symlinkDirSync } from 'symlink-dir' import { safeJoinModulesDir } from './safeJoinModulesDir.js' +export { safeJoinModulesDir } from './safeJoinModulesDir.js' export { symlinkDirectRootDependency } from './symlinkDirectRootDependency.js' export async function symlinkDependency ( diff --git a/fs/symlink-dependency/src/safeJoinModulesDir.ts b/fs/symlink-dependency/src/safeJoinModulesDir.ts index 5017b363bc..4c2684790b 100644 --- a/fs/symlink-dependency/src/safeJoinModulesDir.ts +++ b/fs/symlink-dependency/src/safeJoinModulesDir.ts @@ -1,19 +1,37 @@ import path from 'node:path' -// `path.join(modulesDir, alias)` paired with a containment check, so a -// caller can't accidentally use the joined path without verifying that -// it lives inside `modulesDir`. Earlier passes reject path-traversal -// aliases at manifest-read time, but this layer also runs for paths -// reconstructed from lockfiles and snapshots, so the check stays here -// as a final guarantee. +import validateNpmPackageName from 'validate-npm-package-name' + +// Joins `modulesDir` with a dependency alias and guarantees the result +// stays a direct child of `modulesDir`. The alias becomes a directory +// name inside `node_modules`, so it must be a valid npm package name: a +// single `name` or `@scope/name` of URL-friendly characters with no +// leading `.` or `_`, and not a reserved name. That rejects +// path-traversal (`../x`), absolute, and pnpm-owned aliases (`.bin`, +// `.pnpm`, `node_modules`) before they can escape `modulesDir` or +// overwrite pnpm's own layout. The containment check is kept as a +// belt-and-suspenders guard for any platform-specific join behavior the +// name check might not anticipate. +// +// Earlier passes reject such aliases at manifest-read and resolution +// time, but this layer also runs for paths reconstructed from lockfiles +// and snapshots, so the check stays here as a final guarantee. export function safeJoinModulesDir (modulesDir: string, alias: string): string { + if (!validateNpmPackageName(alias).validForOldPackages) { + throw invalidDependencyNameError(modulesDir, alias) + } const link = path.join(modulesDir, alias) const resolvedDir = path.resolve(modulesDir) const resolvedLink = path.resolve(link) if (resolvedLink === resolvedDir || !resolvedLink.startsWith(resolvedDir + path.sep)) { - const error = new Error(`Refusing to symlink dependency outside ${modulesDir}: alias ${JSON.stringify(alias)} resolves to ${resolvedLink}`) as Error & { code: string } - error.code = 'ERR_PNPM_INVALID_DEPENDENCY_NAME' - throw error + throw invalidDependencyNameError(modulesDir, alias, resolvedLink) } return link } + +function invalidDependencyNameError (modulesDir: string, alias: string, resolvedLink?: string): Error & { code: string } { + const detail = resolvedLink ? ` (it resolves to ${resolvedLink})` : '' + const error = new Error(`Refusing to place a dependency under ${modulesDir} with the invalid alias ${JSON.stringify(alias)}${detail}`) as Error & { code: string } + error.code = 'ERR_PNPM_INVALID_DEPENDENCY_NAME' + return error +} diff --git a/fs/symlink-dependency/test/safeJoinModulesDir.test.ts b/fs/symlink-dependency/test/safeJoinModulesDir.test.ts index edbda76131..773df61741 100644 --- a/fs/symlink-dependency/test/safeJoinModulesDir.test.ts +++ b/fs/symlink-dependency/test/safeJoinModulesDir.test.ts @@ -5,7 +5,18 @@ import { expect, test } from '@jest/globals' import { symlinkDependency, symlinkDependencySync, symlinkDirectRootDependency } from '@pnpm/fs.symlink-dependency' import { tempDir } from '@pnpm/prepare' -const escapeAliases = ['@x/../../../etc', '../sibling', '', '.'] +const escapeAliases = [ + '@x/../../../etc', + '../sibling', + '', + '.', + // Reserved names that resolve *inside* `node_modules` but would + // overwrite pnpm-owned layout, so the containment check alone can't + // catch them. + '.bin', + '.pnpm', + 'node_modules', +] test.each(escapeAliases)('symlinkDependency refuses alias %p', async (alias) => { const tmp = tempDir(false) @@ -34,3 +45,15 @@ test.each(escapeAliases)('symlinkDirectRootDependency refuses alias %p', async ( prefix: '', })).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) }) + +const validAliases = ['foo', '@scope/name', 'foo.bar'] + +test.each(validAliases)('symlinkDependency accepts valid alias %p', async (alias) => { + const tmp = tempDir(false) + const destModulesDir = path.join(tmp, 'node_modules') + fs.mkdirSync(destModulesDir) + const dep = path.join(tmp, 'dep') + fs.mkdirSync(dep) + await expect(symlinkDependency(dep, destModulesDir, alias)).resolves.toBeDefined() + expect(fs.existsSync(path.join(destModulesDir, alias))).toBe(true) +}) diff --git a/installing/deps-installer/src/install/recordLockfileVerified.ts b/installing/deps-installer/src/install/recordLockfileVerified.ts index f76d5840e5..dd057e1d88 100644 --- a/installing/deps-installer/src/install/recordLockfileVerified.ts +++ b/installing/deps-installer/src/install/recordLockfileVerified.ts @@ -2,7 +2,7 @@ import { hashObject } from '@pnpm/crypto.object-hasher' import type { LockfileObject } from '@pnpm/lockfile.fs' import type { ResolutionVerifier } from '@pnpm/resolving.resolver-base' -import { withResolutionShapeCacheIdentity } from './verifyLockfileResolutions.js' +import { withOfflineCheckCacheIdentities } from './verifyLockfileResolutions.js' import { recordVerification } from './verifyLockfileResolutionsCache.js' export interface RecordLockfileVerifiedOptions { @@ -31,7 +31,7 @@ export function recordLockfileVerified (opts: RecordLockfileVerifiedOptions): vo if (!opts.lockfile.packages) return recordVerification(opts.cacheDir, { lockfilePath: opts.lockfilePath, - verifiers: withResolutionShapeCacheIdentity(opts.resolutionVerifiers), + verifiers: withOfflineCheckCacheIdentities(opts.resolutionVerifiers), hashLockfile: () => hashObject(opts.lockfile), }) } diff --git a/installing/deps-installer/src/install/verifyLockfileResolutions.ts b/installing/deps-installer/src/install/verifyLockfileResolutions.ts index 70f5297d53..066f9f2b00 100644 --- a/installing/deps-installer/src/install/verifyLockfileResolutions.ts +++ b/installing/deps-installer/src/install/verifyLockfileResolutions.ts @@ -1,6 +1,7 @@ import { lockfileVerificationLogger } from '@pnpm/core-loggers' import { hashObject } from '@pnpm/crypto.object-hasher' import { PnpmError } from '@pnpm/error' +import { isValidDependencyAlias } from '@pnpm/installing.deps-resolver' import type { LockfileObject } from '@pnpm/lockfile.fs' import { isGitHostedTarballUrl, nameVerFromPkgSnapshot } from '@pnpm/lockfile.utils' import type { @@ -34,21 +35,32 @@ const DEFAULT_CONCURRENCY = 64 export const RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE = 'RESOLUTION_SHAPE_MISMATCH' +// Same code the sink-level guards (`safeJoinModulesDir`) throw. +export const INVALID_DEPENDENCY_ALIAS_CODE = 'INVALID_DEPENDENCY_NAME' + const RESOLUTION_SHAPE_CACHE_IDENTITY: VerifierCacheIdentity = { policy: { resolutionShapeCheck: true }, canTrustPastCheck: (cached) => cached.resolutionShapeCheck === true, } +const DEPENDENCY_ALIAS_CACHE_IDENTITY: VerifierCacheIdentity = { + policy: { dependencyAliasCheck: true }, + canTrustPastCheck: (cached) => cached.dependencyAliasCheck === true, +} + /** * Every verifier list that flows into the verification cache must carry - * the resolution-shape identity, so records written before the shape rule - * existed cannot stat-fast-path around it. Used by the gate itself and by - * {@link recordLockfileVerified}, whose freshly-resolved lockfile satisfies - * the shape invariant by construction (the writer derives every key from - * the resolution it just produced). + * the always-on offline structural checks' identities, so a record + * written before one of those rules existed cannot stat-fast-path around + * it — its missing flag fails `canTrustPastCheck`, forcing a + * re-verification that runs the new check. Used by the gate itself and by + * {@link recordLockfileVerified}, whose freshly-resolved lockfile + * satisfies these invariants by construction (the resolver validates + * aliases at manifest-read time and derives every resolution key from the + * resolution it just produced). */ -export function withResolutionShapeCacheIdentity (verifiers: readonly VerifierCacheIdentity[]): VerifierCacheIdentity[] { - return [...verifiers, RESOLUTION_SHAPE_CACHE_IDENTITY] +export function withOfflineCheckCacheIdentities (verifiers: readonly VerifierCacheIdentity[]): VerifierCacheIdentity[] { + return [...verifiers, RESOLUTION_SHAPE_CACHE_IDENTITY, DEPENDENCY_ALIAS_CACHE_IDENTITY] } export interface VerifyLockfileResolutionsOptions { @@ -108,7 +120,7 @@ export async function verifyLockfileResolutions ( ? { cacheDir: options.cacheDir, lockfilePath: options.lockfilePath } : undefined - const cacheVerifiers = withResolutionShapeCacheIdentity(verifiers) + const cacheVerifiers = withOfflineCheckCacheIdentities(verifiers) // Cache lookup runs before any registry I/O — the fast path is a // single stat() of the lockfile when the previous install already @@ -155,7 +167,10 @@ export async function verifyLockfileResolutions ( // A degenerate lockfile where every snapshot fails the // name/version extraction (so candidates is empty) skips emission // entirely — no work, no noise. - const { candidates, shapeViolations } = collectCandidates(lockfile) + const { candidates, shapeViolations, invalidAliases } = collectCandidates(lockfile) + if (invalidAliases.length > 0) { + throw buildInvalidAliasError(invalidAliases) + } if (shapeViolations.length > 0) { throw buildVerificationError(shapeViolations) } @@ -208,6 +223,24 @@ export async function verifyLockfileResolutions ( } } +function buildInvalidAliasError (aliases: string[]): PnpmError { + const sorted = [...aliases].sort() + const visible = sorted.slice(0, MAX_VIOLATIONS_TO_PRINT) + const omitted = sorted.length - visible.length + const breakdown = visible.map((alias) => ` ${JSON.stringify(alias)}`).join('\n') + const details = omitted > 0 ? `${breakdown}\n …and ${omitted} more` : breakdown + const plural = aliases.length === 1 ? 'alias' : 'aliases' + return new PnpmError( + INVALID_DEPENDENCY_ALIAS_CODE, + `The lockfile contains ${aliases.length} dependency ${plural} that are not valid package names:\n${details}`, + { + hint: 'A dependency alias becomes a directory under node_modules, so it must be a valid npm package name — a single `name` or `@scope/name` with no leading `.` or `_`, and not a reserved name such as `node_modules`. ' + + 'An alias containing path-traversal segments or a reserved name such as `.bin` or `.pnpm` could make an install write outside the intended directory or overwrite pnpm-owned layout. ' + + 'This usually means the lockfile was tampered with — inspect recent changes to pnpm-lock.yaml before trusting it.', + } + ) +} + function buildVerificationError (violations: ResolutionPolicyViolation[]): PnpmError { // Stable order so the error output is deterministic. violations.sort((a, b) => `${a.name}@${a.version}`.localeCompare(`${b.name}@${b.version}`)) @@ -329,10 +362,20 @@ interface Candidate { // skips the tarball/policy checks, so if the wrong shape wins the dedup the // surviving entry is verified under the wrong rules and the real one is // never checked. -function collectCandidates (lockfile: LockfileObject): { candidates: Map, shapeViolations: ResolutionPolicyViolation[] } { +function collectCandidates (lockfile: LockfileObject): { candidates: Map, shapeViolations: ResolutionPolicyViolation[], invalidAliases: string[] } { const candidates = new Map() const shapeViolations: ResolutionPolicyViolation[] = [] + // The importer alias maps are the one source not reached by the + // package loop below, so they're scanned here. + const invalidAliases = new Set() + for (const importer of Object.values(lockfile.importers ?? {})) { + pushInvalidAliases(importer.dependencies, invalidAliases) + pushInvalidAliases(importer.devDependencies, invalidAliases) + pushInvalidAliases(importer.optionalDependencies, invalidAliases) + } for (const [depPath, snapshot] of Object.entries(lockfile.packages ?? {})) { + pushInvalidAliases(snapshot.dependencies, invalidAliases) + pushInvalidAliases(snapshot.optionalDependencies, invalidAliases) const { name, version, nonSemverVersion } = nameVerFromPkgSnapshot(depPath as DepPath, snapshot) if (!name || !version) continue // A registry-style depPath (name@semver) must be backed by a @@ -357,7 +400,20 @@ function collectCandidates (lockfile: LockfileObject): { candidates: Map` + * directories — not `overrides` (`foo>bar` selectors) or + * `patchedDependencies` (`name@version` keys). + */ +function pushInvalidAliases (deps: Record | undefined, invalid: Set): void { + if (deps == null) return + for (const alias of Object.keys(deps)) { + if (!isValidDependencyAlias(alias)) invalid.add(alias) + } } async function iterateLockfileViolations ( diff --git a/installing/deps-installer/test/install/verifyLockfileResolutions.ts b/installing/deps-installer/test/install/verifyLockfileResolutions.ts index 4ab07628dc..cf891071b2 100644 --- a/installing/deps-installer/test/install/verifyLockfileResolutions.ts +++ b/installing/deps-installer/test/install/verifyLockfileResolutions.ts @@ -480,3 +480,57 @@ test('rejects a registry-style depPath whose git-host tarball varies the host ca code: 'ERR_PNPM_RESOLUTION_SHAPE_MISMATCH', }) }) + +test.each([ + '../../../escape', + '@scope/../../escape', + '.bin', + '.pnpm', + 'node_modules', +])('rejects an importer dependency alias %p, even with no verifiers', async (alias) => { + const lockfile = { + lockfileVersion: '9.0', + importers: { + '.': { + specifiers: { [alias]: '1.0.0' }, + dependencies: { [alias]: '1.0.0' }, + }, + }, + packages: { + 'real@1.0.0': { resolution: tarballResolution() }, + }, + } as unknown as LockfileObject + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME', + message: expect.stringMatching(/not valid package names/), + }) +}) + +test('rejects an invalid alias nested in a package snapshot, even with no verifiers', async () => { + const lockfile = makeLockfile({ + 'real@1.0.0': { + resolution: tarballResolution(), + dependencies: { '../../../escape': '1.0.0' }, + } as never, + }) + await expect(verifyLockfileResolutions(lockfile, [])).rejects.toMatchObject({ + code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME', + }) +}) + +test('accepts valid scoped and unscoped dependency aliases', async () => { + const lockfile = { + lockfileVersion: '9.0', + importers: { + '.': { + specifiers: { foo: '1.0.0', '@scope/bar': '1.0.0' }, + dependencies: { foo: '1.0.0', '@scope/bar': '1.0.0' }, + }, + }, + packages: { + 'foo@1.0.0': { resolution: tarballResolution() }, + '@scope/bar@1.0.0': { resolution: tarballResolution() }, + }, + } as unknown as LockfileObject + await expect(verifyLockfileResolutions(lockfile, [])).resolves.toBeUndefined() +}) diff --git a/installing/deps-resolver/src/index.ts b/installing/deps-resolver/src/index.ts index 79f2dd1e81..c51f31cf0a 100644 --- a/installing/deps-resolver/src/index.ts +++ b/installing/deps-resolver/src/index.ts @@ -70,6 +70,7 @@ export { type UpdateMatchingFunction, type WantedDependency, } +export { assertValidDependencyAliases, isValidDependencyAlias } from './validateDependencyAlias.js' interface ProjectToLink { binsDir: string diff --git a/installing/deps-restorer/src/lockfileToHoistedDepGraph.ts b/installing/deps-restorer/src/lockfileToHoistedDepGraph.ts index f538c806b0..2e1a806565 100644 --- a/installing/deps-restorer/src/lockfileToHoistedDepGraph.ts +++ b/installing/deps-restorer/src/lockfileToHoistedDepGraph.ts @@ -8,6 +8,7 @@ import type { LockfileToDepGraphResult, } from '@pnpm/deps.graph-builder' import * as dp from '@pnpm/deps.path' +import { safeJoinModulesDir } from '@pnpm/fs.symlink-dependency' import { hoist, type HoisterResult, type HoistingLimits } from '@pnpm/installing.linking.real-hoist' import type { IncludedDependencies } from '@pnpm/installing.modules-yaml' import type { @@ -218,7 +219,7 @@ async function fetchDeps ( return } - const dir = path.join(modules, dep.name) + const dir = safeJoinModulesDir(modules, dep.name) const depLocation = path.relative(opts.lockfileDir, dir) const resolution = pkgSnapshotToResolution(depPath, pkgSnapshot, opts.registries) let fetchResponse!: ReturnType diff --git a/installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts b/installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts new file mode 100644 index 0000000000..b829ecc551 --- /dev/null +++ b/installing/deps-restorer/test/lockfileToHoistedDepGraph.test.ts @@ -0,0 +1,83 @@ +/// +import fs from 'node:fs' +import path from 'node:path' + +import { expect, test } from '@jest/globals' +import type { LockfileObject } from '@pnpm/lockfile.fs' +import { tempDir } from '@pnpm/prepare' + +import { lockfileToHoistedDepGraph } from '../src/lockfileToHoistedDepGraph.js' + +// A crafted lockfile whose dependency *alias* (the key pnpm turns into a +// `node_modules/` directory) is a path-traversal or reserved name, +// pointing at an otherwise ordinary package snapshot. The `nodeLinker: +// hoisted` restore path reads aliases straight from the lockfile, so this +// is the shape an attacker who can ship a lockfile would use to escape +// `node_modules` or overwrite pnpm-owned layout (`.bin` / `.pnpm`). +function craftedLockfile (alias: string): LockfileObject { + return { + lockfileVersion: '9.0', + importers: { + '.': { + dependencies: { [alias]: '1.0.0' }, + specifiers: { [alias]: '1.0.0' }, + }, + }, + packages: { + [`${alias}@1.0.0`]: { + resolution: { integrity: 'sha512-deadbeef' }, + }, + }, + } as unknown as LockfileObject +} + +// `force: true` skips the installability check so the walk reaches the +// alias sink directly; the store controller throws if touched, proving +// the alias is rejected before any fetch or filesystem work. +function hoistedOpts (lockfileDir: string): Parameters[2] { + const unreachable = (name: string) => () => { + throw new Error(`${name} must not be reached for a rejected alias`) + } + return { + autoInstallPeers: false, + engineStrict: false, + force: true, + importerIds: ['.'], + include: { dependencies: true, devDependencies: true, optionalDependencies: true }, + ignoreScripts: false, + lockfileDir, + nodeVersion: process.version, + pnpmVersion: '0.0.0', + registries: { default: 'http://localhost/' }, + sideEffectsCacheRead: false, + skipped: new Set(), + storeController: { + fetchPackage: unreachable('fetchPackage'), + getFilesIndexFilePath: unreachable('getFilesIndexFilePath'), + }, + storeDir: path.join(lockfileDir, 'store'), + virtualStoreDir: path.join(lockfileDir, 'node_modules', '.pnpm'), + } as unknown as Parameters[2] +} + +test.each([ + '../../../escape', + '@scope/../../escape', + '.bin', + '.pnpm', + 'node_modules', +])('lockfileToHoistedDepGraph rejects hoisted alias %p', async (alias) => { + const dir = tempDir(false) + await expect( + lockfileToHoistedDepGraph(craftedLockfile(alias), null, hoistedOpts(dir)) + ).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) +}) + +test('lockfileToHoistedDepGraph does not create a file outside node_modules for a traversal alias', async () => { + const dir = tempDir(false) + const escaped = path.join(dir, 'node_modules', '..', '..', '..', 'escape') + await expect( + lockfileToHoistedDepGraph(craftedLockfile('../../../escape'), null, hoistedOpts(dir)) + ).rejects.toThrow(expect.objectContaining({ code: 'ERR_PNPM_INVALID_DEPENDENCY_NAME' })) + expect(fs.existsSync(escaped)).toBe(false) +}) diff --git a/pacquet/crates/lockfile-verification/Cargo.toml b/pacquet/crates/lockfile-verification/Cargo.toml index 76115e7489..66a240f665 100644 --- a/pacquet/crates/lockfile-verification/Cargo.toml +++ b/pacquet/crates/lockfile-verification/Cargo.toml @@ -11,10 +11,11 @@ license.workspace = true repository.workspace = true [dependencies] -pacquet-diagnostics = { workspace = true } -pacquet-lockfile = { workspace = true } -pacquet-reporter = { workspace = true } -pacquet-resolving-resolver-base = { workspace = true } +pacquet-diagnostics = { workspace = true } +pacquet-lockfile = { workspace = true } +pacquet-reporter = { workspace = true } +pacquet-resolving-parse-wanted-dependency = { workspace = true } +pacquet-resolving-resolver-base = { workspace = true } chrono = { workspace = true } derive_more = { workspace = true } diff --git a/pacquet/crates/lockfile-verification/src/errors.rs b/pacquet/crates/lockfile-verification/src/errors.rs index 415fc498d6..b31d5b654c 100644 --- a/pacquet/crates/lockfile-verification/src/errors.rs +++ b/pacquet/crates/lockfile-verification/src/errors.rs @@ -31,6 +31,13 @@ run \"pnpm clean --lockfile\" and then \"pnpm install\" to rebuild from \ a fresh resolution. Alternatively, relax the policy that flagged \ them."; +const INVALID_ALIAS_HINT: &str = "A dependency alias becomes a directory under node_modules, \ +so it must be a valid npm package name — a single `name` or `@scope/name` with no leading \ +`.` or `_`, and not a reserved name such as `node_modules`. An alias containing path-traversal \ +segments or a reserved name such as `.bin` or `.pnpm` could make an install write outside the \ +intended directory or overwrite pnpm-owned layout. This usually means the lockfile was tampered \ +with — inspect recent changes to pnpm-lock.yaml before trusting it."; + /// One verifier rejection rendered for the error breakdown. /// Internal-only data shape — the runner builds these from /// `ResolutionPolicyViolation` after sorting. @@ -91,9 +98,46 @@ pub enum VerifyError { count: usize, breakdown: String, }, + + /// One or more dependency aliases in the lockfile are not valid npm + /// package names. Surfaces `ERR_PNPM_INVALID_DEPENDENCY_NAME`, the + /// same code the sink-level guards raise. + #[display("{count} dependency {plural} in the lockfile {verb} not valid package names:\n{breakdown}", plural = if *count == 1 { "alias" } else { "aliases" }, verb = if *count == 1 { "is" } else { "are" })] + #[diagnostic(code(ERR_PNPM_INVALID_DEPENDENCY_NAME), help("{INVALID_ALIAS_HINT}"))] + InvalidDependencyAlias { + #[error(not(source))] + count: usize, + breakdown: String, + }, } impl VerifyError { + /// Build the [`VerifyError::InvalidDependencyAlias`] variant from a + /// list of offending aliases. Sorts for determinism and caps the + /// printed breakdown at `MAX_VIOLATIONS_TO_PRINT`. Empty input is + /// a logic error — callers must check before constructing. + #[must_use] + pub fn invalid_dependency_aliases(aliases: &[String]) -> Self { + debug_assert!(!aliases.is_empty(), "no invalid aliases → no error"); + let mut sorted: Vec<&String> = aliases.iter().collect(); + sorted.sort(); + let count = sorted.len(); + let visible_count = count.min(MAX_VIOLATIONS_TO_PRINT); + let omitted = count.saturating_sub(visible_count); + + let mut breakdown = String::new(); + for alias in sorted.iter().take(visible_count) { + writeln!(breakdown, " {alias:?}").unwrap(); + } + if omitted > 0 { + write!(breakdown, " …and {omitted} more").unwrap(); + } else if breakdown.ends_with('\n') { + breakdown.pop(); + } + + VerifyError::InvalidDependencyAlias { count, breakdown } + } + /// Build the appropriate variant from a list of rendered /// violations. The list is **already sorted** by `name@version` /// (the runner sorts before calling). Empty input is a logic diff --git a/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs b/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs index a934e6553f..5cd6c9eb4e 100644 --- a/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs +++ b/pacquet/crates/lockfile-verification/src/record_lockfile_verified.rs @@ -22,7 +22,7 @@ use pacquet_resolving_resolver_base::ResolutionVerifier; use crate::{ cache::record_verification, hash_lockfile, - verify_lockfile_resolutions::with_resolution_shape_cache_identity, + verify_lockfile_resolutions::with_offline_check_cache_identities, }; /// Persist the post-resolution lockfile as already-verified. @@ -49,7 +49,7 @@ pub fn record_lockfile_verified( record_verification( cache_dir, lockfile_path, - &with_resolution_shape_cache_identity(verifiers), + &with_offline_check_cache_identities(verifiers), || hash_lockfile(lockfile), crate::cache::CachePrecomputed::default(), ); diff --git a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs index f395f85a0e..d485ffc566 100644 --- a/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs +++ b/pacquet/crates/lockfile-verification/src/verify_lockfile_resolutions.rs @@ -23,6 +23,7 @@ use pacquet_lockfile::{Lockfile, LockfileResolution, PkgName, is_git_hosted_tarb use pacquet_reporter::{ LockfileVerificationLog, LockfileVerificationMessage, LogEvent, LogLevel, Reporter, }; +use pacquet_resolving_parse_wanted_dependency::is_valid_old_npm_package_name; use pacquet_resolving_resolver_base::{ ResolutionPolicyViolation, ResolutionVerification, ResolutionVerifier, VerifyCtx, VerifyFuture, }; @@ -95,7 +96,7 @@ pub async fn verify_lockfile_resolutions( // logic via the same code path). let cache_inputs = opts.cache_dir.zip(opts.lockfile_path); - let cache_verifiers = with_resolution_shape_cache_identity(verifiers); + let cache_verifiers = with_offline_check_cache_identities(verifiers); // Memoised content hash. Used by both the lookup (when the // stat-shortcut doesn't apply) and the recorder (after the @@ -140,7 +141,10 @@ pub async fn verify_lockfile_resolutions( cache_precomputed = result.precomputed; } - let (candidates, shape_violations) = collect_candidates(lockfile); + let (candidates, shape_violations, invalid_aliases) = collect_candidates(lockfile); + if !invalid_aliases.is_empty() { + return Err(VerifyError::invalid_dependency_aliases(&invalid_aliases)); + } if !shape_violations.is_empty() { return Err(build_verification_error(shape_violations)); } @@ -212,44 +216,59 @@ pub async fn collect_resolution_policy_violations( if verifiers.is_empty() || lockfile.packages.is_none() { return Vec::new(); } - // Shape violations are deliberately not collected here: they are - // hard tampering failures, not policy picks a caller may - // auto-exclude. - let (candidates, _shape_violations) = collect_candidates(lockfile); + // Shape violations and invalid aliases are deliberately not + // collected here: they are hard tampering failures, not policy + // picks a caller may auto-exclude. + let (candidates, _shape_violations, _invalid_aliases) = collect_candidates(lockfile); run_fan_out(candidates, verifiers, concurrency).await } pub const RESOLUTION_SHAPE_MISMATCH_VIOLATION_CODE: &str = "RESOLUTION_SHAPE_MISMATCH"; -/// Cache-key participant for the offline resolution-shape pass: a -/// record written before the rule existed lacks the flag, so -/// `can_trust_past_check` rejects it and forces a re-verification. -/// `verify` is never invoked — the identity is appended only to the -/// verifier lists handed to the cache lookup and recorder. -struct ResolutionShapeCacheIdentity { +/// Cache-key participant for an always-on offline structural check +/// (resolution-shape, dependency-alias): a record written before the +/// check's rule existed lacks its `flag`, so `can_trust_past_check` +/// rejects it and forces a re-verification. `verify` is never invoked — +/// the identity is appended only to the verifier lists handed to the +/// cache lookup and recorder. +struct OfflineCheckCacheIdentity { policy: serde_json::Map, + flag: &'static str, } fn resolution_shape_cache_identity() -> Arc { let mut policy = serde_json::Map::new(); policy.insert("resolutionShapeCheck".to_string(), serde_json::Value::Bool(true)); - Arc::new(ResolutionShapeCacheIdentity { policy }) + Arc::new(OfflineCheckCacheIdentity { policy, flag: "resolutionShapeCheck" }) +} + +fn dependency_alias_cache_identity() -> Arc { + let mut policy = serde_json::Map::new(); + policy.insert("dependencyAliasCheck".to_string(), serde_json::Value::Bool(true)); + Arc::new(OfflineCheckCacheIdentity { policy, flag: "dependencyAliasCheck" }) } /// Every verifier list that flows into the verification cache must -/// carry the resolution-shape identity, so records written before the -/// shape rule existed cannot stat-fast-path around it. Used by the -/// gate itself and by [`crate::record_lockfile_verified()`], whose -/// freshly-resolved lockfile satisfies the shape invariant by -/// construction (the writer derives every key from the resolution it -/// just produced). -pub(crate) fn with_resolution_shape_cache_identity( +/// carry the always-on offline structural checks' identities, so a +/// record written before one of those rules existed cannot +/// stat-fast-path around it — its missing flag fails +/// `can_trust_past_check`, forcing a re-verification that runs the new +/// check. Used by the gate itself and by +/// [`crate::record_lockfile_verified()`], whose freshly-resolved +/// lockfile satisfies these invariants by construction (the resolver +/// validates aliases at manifest-read time and derives every resolution +/// key from the resolution it just produced). +pub(crate) fn with_offline_check_cache_identities( verifiers: &[Arc], ) -> Vec> { - verifiers.iter().cloned().chain(std::iter::once(resolution_shape_cache_identity())).collect() + verifiers + .iter() + .cloned() + .chain([resolution_shape_cache_identity(), dependency_alias_cache_identity()]) + .collect() } -impl ResolutionVerifier for ResolutionShapeCacheIdentity { +impl ResolutionVerifier for OfflineCheckCacheIdentity { fn verify<'a>( &'a self, _resolution: &'a LockfileResolution, @@ -263,7 +282,7 @@ impl ResolutionVerifier for ResolutionShapeCacheIdentity { } fn can_trust_past_check(&self, cached: &serde_json::Map) -> bool { - cached.get("resolutionShapeCheck") == Some(&serde_json::Value::Bool(true)) + cached.get(self.flag) == Some(&serde_json::Value::Bool(true)) } } @@ -304,6 +323,23 @@ fn is_http_tarball_url(url: &str) -> bool { lower.starts_with("https://") || lower.starts_with("http://") } +/// Add every alias in `aliases` that fails +/// `is_valid_old_npm_package_name` (the `validForOldPackages` rule +/// pnpm's `isValidDependencyAlias` applies) to `invalid`. Only pass maps +/// whose keys become `node_modules/` directories — not +/// `overrides`, `patched_dependencies`, or peer dependencies. +fn push_invalid_aliases<'alias>( + aliases: impl Iterator, + invalid: &mut std::collections::BTreeSet, +) { + for alias in aliases { + let alias = alias.to_string(); + if !is_valid_old_npm_package_name(&alias) { + invalid.insert(alias); + } + } +} + /// One `(name, version, resolution)` tuple deduplicated from /// `lockfile.packages`. Mirrors upstream's inline `Candidate` /// interface. @@ -323,10 +359,36 @@ struct Candidate { /// `BTreeMap` over a serialized key gives deterministic iteration /// order for tests; the fan-out runs across the value iter so order /// doesn't affect correctness, only the reproducibility of failures. -fn collect_candidates(lockfile: &Lockfile) -> (Vec, Vec) { +fn collect_candidates( + lockfile: &Lockfile, +) -> (Vec, Vec, Vec) { let Some(packages) = lockfile.packages.as_ref() else { - return (Vec::new(), Vec::new()); + return (Vec::new(), Vec::new(), Vec::new()); }; + // Pacquet keeps the alias-bearing maps in `importers` / `snapshots`, + // separate from the `packages` metadata the loop below walks, so + // they're scanned here in the same pass. + let mut invalid_aliases: std::collections::BTreeSet = std::collections::BTreeSet::new(); + for importer in lockfile.importers.values() { + for deps in + [&importer.dependencies, &importer.dev_dependencies, &importer.optional_dependencies] + { + push_invalid_aliases( + deps.iter().flatten().map(|(alias, _)| alias), + &mut invalid_aliases, + ); + } + } + if let Some(snapshots) = lockfile.snapshots.as_ref() { + for snapshot in snapshots.values() { + for deps in [&snapshot.dependencies, &snapshot.optional_dependencies] { + push_invalid_aliases( + deps.iter().flatten().map(|(alias, _)| alias), + &mut invalid_aliases, + ); + } + } + } let mut deduped: BTreeMap = BTreeMap::new(); let mut shape_violations = Vec::new(); for (key, metadata) in packages { @@ -366,7 +428,7 @@ fn collect_candidates(lockfile: &Lockfile) -> (Vec, Vec( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("invalid importer alias must be rejected"); + let VerifyError::InvalidDependencyAlias { count, breakdown } = err else { + panic!("expected InvalidDependencyAlias for {alias:?}, got {err:?}"); + }; + assert_eq!(count, 1); + assert!(breakdown.contains(alias), "breakdown {breakdown:?} should mention {alias:?}"); + } +} + +#[tokio::test] +async fn rejects_invalid_alias_nested_in_snapshot() { + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + real: + specifier: ^1.0.0 + version: 1.0.0 + +packages: + + real@1.0.0: + resolution: {integrity: sha512-deadbeef} + +snapshots: + + real@1.0.0: + dependencies: + '../../../escape': 1.0.0 +", + ); + let err = verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect_err("invalid alias nested in a snapshot must be rejected"); + assert!(matches!(err, VerifyError::InvalidDependencyAlias { .. }), "got {err:?}"); +} + +#[tokio::test] +async fn accepts_valid_scoped_and_unscoped_aliases() { + let lockfile = parse( + "lockfileVersion: '9.0' + +importers: + + .: + dependencies: + foo: + specifier: ^1.0.0 + version: 1.0.0 + '@scope/bar': + specifier: ^1.0.0 + version: 1.0.0 + +packages: + + foo@1.0.0: + resolution: {integrity: sha512-deadbeef} + + '@scope/bar@1.0.0': + resolution: {integrity: sha512-deadbeef} + +snapshots: + + foo@1.0.0: {} + '@scope/bar@1.0.0': {} +", + ); + verify_lockfile_resolutions::( + &lockfile, + &[], + &VerifyLockfileResolutionsOptions::default(), + ) + .await + .expect("valid scoped and unscoped aliases pass"); +} diff --git a/pacquet/crates/package-manager/src/hoisted_dep_graph.rs b/pacquet/crates/package-manager/src/hoisted_dep_graph.rs index f4ad0890f0..1b4ed45a0e 100644 --- a/pacquet/crates/package-manager/src/hoisted_dep_graph.rs +++ b/pacquet/crates/package-manager/src/hoisted_dep_graph.rs @@ -31,6 +31,7 @@ //! to nest. Hoisting decisions are made at directory granularity, //! not depPath granularity. +use crate::safe_join_modules_dir::{InvalidDependencyAliasError, safe_join_modules_dir}; use derive_more::{Display, Error, From}; use indexmap::IndexSet; use miette::Diagnostic; @@ -325,6 +326,11 @@ pub enum HoistedDepGraphError { #[display("{_0}")] #[diagnostic(transparent)] Installability(#[error(source)] Box), + /// A hoisted node's alias was rejected by `safe_join_modules_dir` + /// before the join. Surfaces `ERR_PNPM_INVALID_DEPENDENCY_NAME`. + #[display("{_0}")] + #[diagnostic(transparent)] + InvalidDependencyAlias(#[error(source)] InvalidDependencyAliasError), } /// Build a directory-keyed [`LockfileToDepGraphResult`] from a @@ -690,7 +696,7 @@ fn walk_deps( } } - let dir = modules.join(&dep.0.name); + let dir = safe_join_modules_dir(modules, &dep.0.name)?; let dep_location = path_relative_to_lockfile_dir(&dir, state.lockfile_dir); // Insert *before* recursing — mirrors upstream's diff --git a/pacquet/crates/package-manager/src/hoisted_dep_graph/tests.rs b/pacquet/crates/package-manager/src/hoisted_dep_graph/tests.rs index e8efb33efb..a79c7f14d0 100644 --- a/pacquet/crates/package-manager/src/hoisted_dep_graph/tests.rs +++ b/pacquet/crates/package-manager/src/hoisted_dep_graph/tests.rs @@ -1061,3 +1061,36 @@ fn walker_forwards_external_dependencies_to_hoister() { "root direct deps drop the externalised alias", ); } + +/// A crafted lockfile whose dependency alias is a path-traversal +/// (`../../../escape`) or a reserved name (`.bin`, `.pnpm`, +/// `node_modules`) is rejected at the hoisted graph sink before the +/// node is inserted or the walker recurses. `PkgName::parse` is +/// permissive enough to carry such an alias straight out of a +/// deserialized lockfile, so this is the boundary that stops it. +/// Mirrors pnpm's `ERR_PNPM_INVALID_DEPENDENCY_NAME`; `force: true` +/// skips installability so the walk reaches the alias sink directly. +#[test] +fn walker_rejects_invalid_hoisted_alias() { + for alias in ["../../../escape", "@scope/../../escape", ".bin", ".pnpm", "node_modules"] { + let mut root_deps = ResolvedDependencyMap::new(); + root_deps.insert(pkg_name(alias), resolved_dep("1.0.0")); + + let mut packages = HashMap::new(); + packages.insert(dep_key(alias, "1.0.0"), metadata_stub()); + + let mut snapshots = HashMap::new(); + snapshots.insert(dep_key(alias, "1.0.0"), SnapshotEntry::default()); + + let lockfile = lockfile_with(root_deps, packages, snapshots); + let opts = LockfileToHoistedDepGraphOptions { force: true, ..host_aware_opts() }; + let err = lockfile_to_hoisted_dep_graph(&lockfile, None, &opts) + .expect_err("invalid alias must be rejected"); + match err { + HoistedDepGraphError::InvalidDependencyAlias(inner) => { + assert_eq!(inner.alias, alias); + } + other => panic!("expected InvalidDependencyAlias error for {alias:?}, got {other:?}"), + } + } +} diff --git a/pacquet/crates/package-manager/src/lib.rs b/pacquet/crates/package-manager/src/lib.rs index 3db6f95b57..f8a4478f5c 100644 --- a/pacquet/crates/package-manager/src/lib.rs +++ b/pacquet/crates/package-manager/src/lib.rs @@ -31,6 +31,7 @@ mod prefetching_resolver; mod remove; mod resolution_observer; mod retry_config; +mod safe_join_modules_dir; mod store_init; mod symlink_direct_dependencies; mod symlink_package; diff --git a/pacquet/crates/package-manager/src/safe_join_modules_dir.rs b/pacquet/crates/package-manager/src/safe_join_modules_dir.rs new file mode 100644 index 0000000000..afb0a2f2d8 --- /dev/null +++ b/pacquet/crates/package-manager/src/safe_join_modules_dir.rs @@ -0,0 +1,47 @@ +//! Join a `node_modules` directory with a dependency alias and reject +//! aliases that aren't valid npm package names before the join. The +//! alias becomes a directory name inside `node_modules`, so a +//! path-traversal alias (`../../../escape`) would escape the directory +//! and a reserved alias (`.bin`, `.pnpm`, `node_modules`) would +//! overwrite pnpm-owned layout. Mirrors pnpm's +//! [`safeJoinModulesDir`](https://github.com/pnpm/pnpm/blob/main/fs/symlink-dependency/src/safeJoinModulesDir.ts) +//! and routes through the same [`is_valid_dependency_alias`] +//! check pacquet applies to direct-dependency aliases at resolution +//! time, so the hoisted restore path enforces the boundary the +//! resolution path already enforces. + +use derive_more::{Display, Error}; +use miette::Diagnostic; +use pacquet_resolving_deps_resolver::is_valid_dependency_alias; +use std::path::{Path, PathBuf}; + +/// A dependency alias that would escape `modules` or collide with +/// pnpm's own `node_modules` layout. Surfaces pnpm's +/// `ERR_PNPM_INVALID_DEPENDENCY_NAME`. +#[derive(Debug, Display, Error, Diagnostic)] +#[display("Refusing to place a dependency under {} with the invalid alias {alias:?}", modules.display())] +#[diagnostic(code(INVALID_DEPENDENCY_NAME))] +pub struct InvalidDependencyAliasError { + pub modules: PathBuf, + #[error(not(source))] + pub alias: String, +} + +/// `modules.join(alias)` guarded by a package-name validity check. +/// Returns [`InvalidDependencyAliasError`] when `alias` is not a valid +/// npm package name (path-traversal, absolute, or reserved). +pub fn safe_join_modules_dir( + modules: &Path, + alias: &str, +) -> Result { + if !is_valid_dependency_alias(alias) { + return Err(InvalidDependencyAliasError { + modules: modules.to_path_buf(), + alias: alias.to_owned(), + }); + } + Ok(modules.join(alias)) +} + +#[cfg(test)] +mod tests; diff --git a/pacquet/crates/package-manager/src/safe_join_modules_dir/tests.rs b/pacquet/crates/package-manager/src/safe_join_modules_dir/tests.rs new file mode 100644 index 0000000000..f9f5d351c1 --- /dev/null +++ b/pacquet/crates/package-manager/src/safe_join_modules_dir/tests.rs @@ -0,0 +1,34 @@ +use super::safe_join_modules_dir; +use std::path::Path; + +#[test] +fn accepts_valid_aliases() { + let modules = Path::new("/project/node_modules"); + for alias in ["foo", "@scope/name", "foo.bar"] { + let joined = + safe_join_modules_dir(modules, alias).expect("valid alias should join cleanly"); + assert_eq!(joined, modules.join(alias)); + } +} + +#[test] +fn rejects_traversal_aliases() { + let modules = Path::new("/project/node_modules"); + for alias in ["../../../escape", "@scope/../../escape"] { + let err = safe_join_modules_dir(modules, alias) + .expect_err("traversal alias must be rejected before the join"); + assert_eq!(err.alias, alias); + } +} + +#[test] +fn rejects_reserved_aliases() { + let modules = Path::new("/project/node_modules"); + // These resolve *inside* `node_modules` but collide with pnpm-owned + // layout, so a containment check alone could not catch them. + for alias in [".bin", ".pnpm", "node_modules"] { + let err = safe_join_modules_dir(modules, alias) + .expect_err("reserved alias must be rejected before the join"); + assert_eq!(err.alias, alias); + } +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index e457f1ffe3..8442b546ee 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4936,6 +4936,9 @@ importers: symlink-dir: specifier: 'catalog:' version: 10.0.1 + validate-npm-package-name: + specifier: 'catalog:' + version: 7.0.2 devDependencies: '@jest/globals': specifier: 'catalog:' @@ -4949,6 +4952,9 @@ importers: '@pnpm/prepare': specifier: workspace:* version: link:../../__utils__/prepare + '@types/validate-npm-package-name': + specifier: 'catalog:' + version: 4.0.2 global/commands: dependencies: