diff --git a/.changeset/config-deps-path-traversal.md b/.changeset/config-deps-path-traversal.md new file mode 100644 index 0000000000..fcaa1ef33e --- /dev/null +++ b/.changeset/config-deps-path-traversal.md @@ -0,0 +1,6 @@ +--- +"@pnpm/config.deps-installer": patch +"pnpm": patch +--- + +Security: validate config dependency names and versions before using them to build filesystem paths. A `pnpm-workspace.yaml` with a traversal-shaped `configDependencies` name (such as `../../PWNED`) or version (such as `../../../PWNED`) could previously cause `pnpm install` to create symlinks or write package files outside `node_modules/.pnpm-config` and the store. Names must now be valid npm package names and versions must be exact semver versions. See [GHSA-qrv3-253h-g69c](https://github.com/pnpm/pnpm/security/advisories/GHSA-qrv3-253h-g69c). diff --git a/.changeset/contain-hoisted-dependency-aliases.md b/.changeset/contain-hoisted-dependency-aliases.md new file mode 100644 index 0000000000..2d6052045f --- /dev/null +++ b/.changeset/contain-hoisted-dependency-aliases.md @@ -0,0 +1,9 @@ +--- +"@pnpm/symlink-dependency": patch +"@pnpm/headless": 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 `nodeLinker: hoisted` graph builder now validates each alias at the directory sink (`safeJoinModulesDir`), matching the validation pnpm already performs when resolving aliases from manifests. See [GHSA-fr4h-3cph-29xv](https://github.com/pnpm/pnpm/security/advisories/GHSA-fr4h-3cph-29xv). diff --git a/.changeset/fix-patch-remove-containment.md b/.changeset/fix-patch-remove-containment.md new file mode 100644 index 0000000000..f71cdf7008 --- /dev/null +++ b/.changeset/fix-patch-remove-containment.md @@ -0,0 +1,6 @@ +--- +"@pnpm/plugin-commands-patching": patch +"pnpm": patch +--- + +Prevent `pnpm patch-remove` from removing files outside the configured patches directory. diff --git a/config/deps-installer/package.json b/config/deps-installer/package.json index 1c6657eb0d..76b1adc7b1 100644 --- a/config/deps-installer/package.json +++ b/config/deps-installer/package.json @@ -47,7 +47,9 @@ "@pnpm/read-package-json": "workspace:*", "@pnpm/types": "workspace:*", "@zkochan/rimraf": "catalog:", - "get-npm-tarball-url": "catalog:" + "get-npm-tarball-url": "catalog:", + "semver": "catalog:", + "validate-npm-package-name": "catalog:" }, "peerDependencies": { "@pnpm/logger": "catalog:" @@ -57,6 +59,8 @@ "@pnpm/prepare": "workspace:*", "@pnpm/registry-mock": "catalog:", "@pnpm/testing.temp-store": "workspace:*", + "@types/semver": "catalog:", + "@types/validate-npm-package-name": "catalog:", "load-json-file": "catalog:", "read-yaml-file": "catalog:" }, diff --git a/config/deps-installer/src/assertValidConfigDepName.ts b/config/deps-installer/src/assertValidConfigDepName.ts new file mode 100644 index 0000000000..639d984bca --- /dev/null +++ b/config/deps-installer/src/assertValidConfigDepName.ts @@ -0,0 +1,20 @@ +import { PnpmError } from '@pnpm/error' +import validateNpmPackageName from 'validate-npm-package-name' + +// A config-dependency name becomes a directory pnpm creates during install +// (`node_modules/.pnpm-config/`) and a store path segment, so it must be +// a valid npm package name. A traversal-shaped name (`../../PWNED`), a reserved +// name (`.bin`, `.pnpm`, `node_modules`), or `__proto__` is rejected before any +// path is built from it. Matches the `validForOldPackages` check pnpm applies +// to dependency aliases read from a manifest. +export function assertValidConfigDepName (name: string): void { + if (!validateNpmPackageName(name).validForOldPackages) { + throw new PnpmError( + 'INVALID_DEPENDENCY_NAME', + `The configDependencies in pnpm-workspace.yaml contains a dependency with an invalid name: ${JSON.stringify(name)}`, + { + hint: 'A dependency name must be a valid npm package name — a single `name` or `@scope/name` consisting of URL-friendly characters, with no leading `.` or `_`, and not equal to reserved names such as `node_modules`.', + } + ) + } +} diff --git a/config/deps-installer/src/assertValidConfigDepVersion.ts b/config/deps-installer/src/assertValidConfigDepVersion.ts new file mode 100644 index 0000000000..00418711dd --- /dev/null +++ b/config/deps-installer/src/assertValidConfigDepVersion.ts @@ -0,0 +1,15 @@ +import { PnpmError } from '@pnpm/error' +import semver from 'semver' + +// A config-dep version becomes a store path segment (`//`), +// so reject non-semver values to keep a traversal-shaped version from escaping +// the store root. +export function assertValidConfigDepVersion (name: string, version: string): void { + if (semver.valid(version) == null) { + throw new PnpmError( + 'INVALID_CONFIG_DEP_VERSION', + `The config dependency "${name}" has an invalid version "${version}"`, + { hint: 'A config dependency version must be an exact semver version.' } + ) + } +} diff --git a/config/deps-installer/src/normalizeConfigDeps.ts b/config/deps-installer/src/normalizeConfigDeps.ts index f519afc4e5..c1264a4ef4 100644 --- a/config/deps-installer/src/normalizeConfigDeps.ts +++ b/config/deps-installer/src/normalizeConfigDeps.ts @@ -2,6 +2,8 @@ import getNpmTarballUrl from 'get-npm-tarball-url' import { PnpmError } from '@pnpm/error' import { pickRegistryForPackage } from '@pnpm/pick-registry-for-package' import { type ConfigDependencies, type Registries } from '@pnpm/types' +import { assertValidConfigDepName } from './assertValidConfigDepName.js' +import { assertValidConfigDepVersion } from './assertValidConfigDepVersion.js' interface NormalizeConfigDepsOpts { registries: Registries @@ -18,10 +20,14 @@ type NormalizedConfigDeps = Record { prepareEmpty() const { storeController } = createTempStore() @@ -102,3 +119,62 @@ test('installation fails if the config dependency does not have a checksum', asy store: storeController, })).rejects.toThrow("doesn't have an integrity checksum") }) + +test('a config dependency with a path-traversal name is rejected', async () => { + prepareEmpty() + const { storeController, storeDir } = createTempStore() + + const configDeps: Record = { + '../../PWNED': `100.0.0+${getIntegrity('@pnpm.e2e/foo', '100.0.0')}`, + } + await expect(installConfigDeps(configDeps, { + registries: { + default: registry, + }, + rootDir: process.cwd(), + store: storeController, + })).rejects.toThrow('invalid name') + + expect(containsEntryNamed(process.cwd(), 'PWNED')).toBe(false) + expect(containsEntryNamed(storeDir, 'PWNED')).toBe(false) +}) + +test('a config dependency named __proto__ is rejected', async () => { + prepareEmpty() + const { storeController, storeDir } = createTempStore() + + // JSON.parse makes `__proto__` an own enumerable key (as on-disk parsing + // does); a plain object literal would set the prototype and hide it. + const configDeps: Record = JSON.parse( + `{"__proto__":"100.0.0+${getIntegrity('@pnpm.e2e/foo', '100.0.0')}"}` + ) + await expect(installConfigDeps(configDeps, { + registries: { + default: registry, + }, + rootDir: process.cwd(), + store: storeController, + })).rejects.toThrow('invalid name') + + expect(containsEntryNamed(process.cwd(), '__proto__')).toBe(false) + expect(containsEntryNamed(storeDir, '__proto__')).toBe(false) +}) + +test('a config dependency with a path-traversal version is rejected', async () => { + prepareEmpty() + const { storeController, storeDir } = createTempStore() + + const configDeps: Record = { + '@pnpm.e2e/foo': `../../../PWNED+${getIntegrity('@pnpm.e2e/foo', '100.0.0')}`, + } + await expect(installConfigDeps(configDeps, { + registries: { + default: registry, + }, + rootDir: process.cwd(), + store: storeController, + })).rejects.toThrow('invalid version') + + expect(containsEntryNamed(process.cwd(), 'PWNED')).toBe(false) + expect(containsEntryNamed(storeDir, 'PWNED')).toBe(false) +}) diff --git a/fs/symlink-dependency/package.json b/fs/symlink-dependency/package.json index 91f316f48f..c78bb67815 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:" @@ -45,7 +46,8 @@ "devDependencies": { "@pnpm/logger": "workspace:*", "@pnpm/prepare": "workspace:*", - "@pnpm/symlink-dependency": "workspace:*" + "@pnpm/symlink-dependency": "workspace:*", + "@types/validate-npm-package-name": "catalog:" }, "engines": { "node": ">=18.12" diff --git a/fs/symlink-dependency/src/index.ts b/fs/symlink-dependency/src/index.ts index 5c1da0ae94..842774ffba 100644 --- a/fs/symlink-dependency/src/index.ts +++ b/fs/symlink-dependency/src/index.ts @@ -3,6 +3,7 @@ import symlinkDir 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 a7bf6d7808..cdca5b4c28 100644 --- a/fs/symlink-dependency/src/safeJoinModulesDir.ts +++ b/fs/symlink-dependency/src/safeJoinModulesDir.ts @@ -1,19 +1,37 @@ import path from '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 35cd63be9a..a2c10c6283 100644 --- a/fs/symlink-dependency/test/safeJoinModulesDir.test.ts +++ b/fs/symlink-dependency/test/safeJoinModulesDir.test.ts @@ -4,7 +4,18 @@ import path from 'path' import { tempDir } from '@pnpm/prepare' import { symlinkDependency, symlinkDependencySync, symlinkDirectRootDependency } from '@pnpm/symlink-dependency' -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) @@ -33,3 +44,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/patching/plugin-commands-patching/src/isSubdirectory.ts b/patching/plugin-commands-patching/src/isSubdirectory.ts new file mode 100644 index 0000000000..a1818c6f02 --- /dev/null +++ b/patching/plugin-commands-patching/src/isSubdirectory.ts @@ -0,0 +1,17 @@ +import path from 'path' + +interface PathUtils { + isAbsolute: (path: string) => boolean + relative: (from: string, to: string) => string + sep: string +} + +export function isSubdirectory (parentDir: string, childPath: string, pathUtils: PathUtils = path): boolean { + const relativePath = pathUtils.relative(parentDir, childPath) + + return relativePath === '' || ( + relativePath !== '..' && + !relativePath.startsWith(`..${pathUtils.sep}`) && + !pathUtils.isAbsolute(relativePath) + ) +} diff --git a/patching/plugin-commands-patching/src/patchRemove.ts b/patching/plugin-commands-patching/src/patchRemove.ts index 5efb301b3f..433888465b 100644 --- a/patching/plugin-commands-patching/src/patchRemove.ts +++ b/patching/plugin-commands-patching/src/patchRemove.ts @@ -1,5 +1,6 @@ -import path from 'path' +import { type Stats } from 'fs' import fs from 'fs/promises' +import path from 'path' import { docsUrl } from '@pnpm/cli-utils' import { install } from '@pnpm/plugin-commands-installation' import { type Config, types as allTypes } from '@pnpm/config' @@ -7,6 +8,7 @@ import { PnpmError } from '@pnpm/error' import renderHelp from 'render-help' import { prompt } from 'enquirer' import pick from 'ramda/src/pick' +import { isSubdirectory } from './isSubdirectory.js' import { updatePatchedDependencies } from './updatePatchedDependencies.js' export function rcOptionsTypes (): Record { @@ -31,7 +33,7 @@ export type PatchRemoveCommandOptions = install.InstallCommandOptions & Pick { let patchesToRemove = params - const patchedDependencies = opts.patchedDependencies ?? {} + const patchedDependencies = { ...opts.patchedDependencies } if (!params.length) { const allPatches = Object.keys(patchedDependencies) @@ -60,16 +62,21 @@ export async function handler (opts: PatchRemoveCommandOptions, params: string[] } } - const patchesDirs = new Set() - await Promise.all(patchesToRemove.map(async (patch) => { - if (Object.hasOwn(patchedDependencies, patch)) { - const patchFile = patchedDependencies[patch] - patchesDirs.add(path.dirname(patchFile)) - await fs.rm(patchFile, { force: true }) - delete patchedDependencies![patch] + const patchRemovalContext = await getPatchRemovalContext(opts) + const patchesToRemoveTargets = await Promise.all(patchesToRemove.map(async (patch) => { + const patchFile = patchedDependencies[patch] + if (patchFile == null) { + throw new PnpmError('PATCH_NOT_FOUND', `Patch "${patch}" not found in patched dependencies`) } + return getPatchRemovalTarget(patch, patchFile, patchRemovalContext) })) + await Promise.all(patchesToRemoveTargets.map(unlinkPatchIfExists)) + for (const { patch } of patchesToRemoveTargets) { + delete patchedDependencies[patch] + } + + const patchesDirs = new Set(patchesToRemoveTargets.map(({ parentDir }) => parentDir)) await Promise.all(Array.from(patchesDirs).map(async (dir) => { try { const files = await fs.readdir(dir) @@ -83,5 +90,114 @@ export async function handler (opts: PatchRemoveCommandOptions, params: string[] workspaceDir: opts.workspaceDir ?? opts.rootProjectManifestDir, }) - return install.handler(opts) + return install.handler({ + ...opts, + patchedDependencies, + }) +} + +interface PatchRemovalContext { + lockfileDir: string + patchesDir: string + realPatchesDir?: string +} + +interface PatchRemovalTarget { + patch: string + patchFile: string + parentDir: string + targetPath: string + targetExists: boolean +} + +async function getPatchRemovalContext (opts: PatchRemoveCommandOptions): Promise { + const lockfileDir = path.resolve(opts.lockfileDir ?? opts.dir ?? process.cwd()) + const realLockfileDir = await fs.realpath(lockfileDir) + const patchesDirSetting = opts.patchesDir ?? 'patches' + const patchesDir = path.join(lockfileDir, path.normalize(patchesDirSetting)) + + if (!isSubdirectory(lockfileDir, patchesDir)) { + throw new PnpmError('PATCHES_DIR_OUTSIDE_PROJECT', `The configured patches directory is outside the project: ${patchesDirSetting}`) + } + + const realPatchesDir = await realpathIfExists(patchesDir) + if (realPatchesDir != null && !isSubdirectory(realLockfileDir, realPatchesDir)) { + throw new PnpmError('PATCHES_DIR_OUTSIDE_PROJECT', `The configured patches directory is outside the project: ${patchesDirSetting}`) + } + + return { + lockfileDir, + patchesDir, + realPatchesDir, + } +} + +async function getPatchRemovalTarget ( + patch: string, + patchFile: string, + ctx: PatchRemovalContext +): Promise { + const targetPath = path.resolve(ctx.lockfileDir, patchFile) + if ( + targetPath === ctx.patchesDir || + !isSubdirectory(ctx.patchesDir, targetPath) + ) { + throw new PnpmError('PATCH_FILE_OUTSIDE_PATCHES_DIR', `Patch file "${patchFile}" is outside the configured patches directory`) + } + + const parentDir = path.dirname(targetPath) + const targetStats = await lstatIfExists(targetPath) + const realParentDir = await realpathIfExists(parentDir) + const realPatchesDir = ctx.realPatchesDir ?? (await realpathIfExists(ctx.patchesDir)) + if ( + realParentDir != null && + realPatchesDir != null && + !isSubdirectory(realPatchesDir, realParentDir) + ) { + throw new PnpmError('PATCH_FILE_OUTSIDE_PATCHES_DIR', `Patch file "${patchFile}" is outside the configured patches directory`) + } + if (targetStats?.isDirectory()) { + throw new PnpmError('PATCH_FILE_IS_DIRECTORY', `Patch file "${patchFile}" is a directory`) + } + + return { + patch, + patchFile, + parentDir, + targetPath, + targetExists: targetStats != null, + } +} + +async function unlinkPatchIfExists ({ targetExists, targetPath }: PatchRemovalTarget): Promise { + if (!targetExists) return + + try { + await fs.unlink(targetPath) + } catch (err: unknown) { + if (isErrorWithCode(err, 'ENOENT')) return + throw err + } +} + +async function lstatIfExists (targetPath: string): Promise { + try { + return await fs.lstat(targetPath) + } catch (err: unknown) { + if (isErrorWithCode(err, 'ENOENT')) return undefined + throw err + } +} + +async function realpathIfExists (targetPath: string): Promise { + try { + return await fs.realpath(targetPath) + } catch (err: unknown) { + if (isErrorWithCode(err, 'ENOENT')) return undefined + throw err + } +} + +function isErrorWithCode (err: unknown, code: string): err is NodeJS.ErrnoException { + return typeof err === 'object' && err !== null && 'code' in err && err.code === code } diff --git a/patching/plugin-commands-patching/test/isSubdirectory.test.ts b/patching/plugin-commands-patching/test/isSubdirectory.test.ts new file mode 100644 index 0000000000..f7f6ef037d --- /dev/null +++ b/patching/plugin-commands-patching/test/isSubdirectory.test.ts @@ -0,0 +1,19 @@ +import path from 'path' + +import { isSubdirectory } from '../src/isSubdirectory.js' + +test('isSubdirectory() accepts paths inside the parent directory', () => { + expect(isSubdirectory('/project/patches', '/project/patches/pkg.patch')).toBe(true) + expect(isSubdirectory('/project/patches', '/project/patches/..pkg/pkg.patch')).toBe(true) +}) + +test('isSubdirectory() rejects parent traversal and sibling prefixes', () => { + expect(isSubdirectory('/project/patches', '/project/pkg.patch')).toBe(false) + expect(isSubdirectory('/project/patches', '/project/patches-other/pkg.patch')).toBe(false) +}) + +test('isSubdirectory() rejects Windows drive and UNC escapes', () => { + expect(isSubdirectory('C:\\project\\patches', 'D:\\pkg.patch', path.win32)).toBe(false) + expect(isSubdirectory('C:\\project\\patches', '\\\\server\\share\\pkg.patch', path.win32)).toBe(false) + expect(isSubdirectory('C:\\project\\patches', 'C:\\project\\patches\\..pkg\\pkg.patch', path.win32)).toBe(true) +}) diff --git a/patching/plugin-commands-patching/test/patchRemove.test.ts b/patching/plugin-commands-patching/test/patchRemove.test.ts new file mode 100644 index 0000000000..f1dad504cb --- /dev/null +++ b/patching/plugin-commands-patching/test/patchRemove.test.ts @@ -0,0 +1,166 @@ +import fs from 'fs' +import os from 'os' +import path from 'path' + +import { afterEach, beforeEach, expect, jest, test } from '@jest/globals' +import { install } from '@pnpm/plugin-commands-installation' + +import * as patchRemove from '../src/patchRemove.js' +import { updatePatchedDependencies } from '../src/updatePatchedDependencies.js' +import type { PatchRemoveCommandOptions } from '../src/patchRemove.js' + +jest.mock('@pnpm/plugin-commands-installation', () => ({ + install: { + handler: jest.fn(), + }, +})) + +jest.mock('../src/updatePatchedDependencies.js', () => ({ + updatePatchedDependencies: jest.fn(), +})) + +const installHandler = jest.mocked(install.handler) +const updatePatchedDependenciesMock = jest.mocked(updatePatchedDependencies) +const testOnNonWindows = process.platform === 'win32' ? test.skip : test + +let tempRoot: string + +beforeEach(() => { + tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'pnpm-patch-remove-')) + installHandler.mockResolvedValue(undefined) + updatePatchedDependenciesMock.mockResolvedValue(undefined) +}) + +afterEach(() => { + installHandler.mockReset() + updatePatchedDependenciesMock.mockReset() + fs.rmSync(tempRoot, { force: true, recursive: true }) +}) + +test('patch-remove rejects traversal outside the patches directory before deleting any patch', async () => { + const projectDir = path.join(tempRoot, 'project') + const outsideFile = path.join(tempRoot, 'outside.patch') + const goodPatch = path.join(projectDir, 'patches/good.patch') + fs.mkdirSync(path.dirname(goodPatch), { recursive: true }) + fs.writeFileSync(goodPatch, 'good patch', 'utf8') + fs.writeFileSync(outsideFile, 'outside patch', 'utf8') + + await expect(patchRemove.handler(createOptions(projectDir, { + good: 'patches/good.patch', + bad: '../outside.patch', + }), ['good', 'bad'])).rejects.toMatchObject({ + code: 'ERR_PNPM_PATCH_FILE_OUTSIDE_PATCHES_DIR', + }) + + expect(fs.existsSync(goodPatch)).toBe(true) + expect(fs.existsSync(outsideFile)).toBe(true) + expect(updatePatchedDependenciesMock).not.toHaveBeenCalled() + expect(installHandler).not.toHaveBeenCalled() +}) + +test('patch-remove rejects directory entries before deleting any patch', async () => { + const projectDir = path.join(tempRoot, 'project') + const goodPatch = path.join(projectDir, 'patches/good.patch') + const patchDir = path.join(projectDir, 'patches/not-a-file.patch') + fs.mkdirSync(patchDir, { recursive: true }) + fs.writeFileSync(goodPatch, 'good patch', 'utf8') + + await expect(patchRemove.handler(createOptions(projectDir, { + good: 'patches/good.patch', + bad: 'patches/not-a-file.patch', + }), ['good', 'bad'])).rejects.toMatchObject({ + code: 'ERR_PNPM_PATCH_FILE_IS_DIRECTORY', + }) + + expect(fs.existsSync(goodPatch)).toBe(true) + expect(updatePatchedDependenciesMock).not.toHaveBeenCalled() + expect(installHandler).not.toHaveBeenCalled() +}) + +testOnNonWindows('patch-remove rejects a nested parent symlink outside the patches directory before unlinking a dangling target', async () => { + const projectDir = path.join(tempRoot, 'project') + const patchesDir = path.join(projectDir, 'patches') + const outsideDir = path.join(tempRoot, 'outside') + const outsideDanglingLink = path.join(outsideDir, 'dangling.patch') + fs.mkdirSync(patchesDir, { recursive: true }) + fs.mkdirSync(outsideDir, { recursive: true }) + fs.symlinkSync(outsideDir, path.join(patchesDir, 'linked-dir'), 'dir') + fs.symlinkSync(path.join(tempRoot, 'missing-target.patch'), outsideDanglingLink) + + await expect(patchRemove.handler(createOptions(projectDir, { + bad: 'patches/linked-dir/dangling.patch', + }), ['bad'])).rejects.toMatchObject({ + code: 'ERR_PNPM_PATCH_FILE_OUTSIDE_PATCHES_DIR', + }) + + expect(fs.lstatSync(outsideDanglingLink).isSymbolicLink()).toBe(true) + expect(updatePatchedDependenciesMock).not.toHaveBeenCalled() + expect(installHandler).not.toHaveBeenCalled() +}) + +testOnNonWindows('patch-remove unlinks a final symlink inside the patches directory without touching its target', async () => { + const projectDir = path.join(tempRoot, 'project') + const patchesDir = path.join(projectDir, 'patches') + const outsideTarget = path.join(tempRoot, 'outside-target.patch') + const patchLink = path.join(patchesDir, 'linked.patch') + fs.mkdirSync(patchesDir, { recursive: true }) + fs.writeFileSync(outsideTarget, 'outside target', 'utf8') + fs.symlinkSync(outsideTarget, patchLink) + + await patchRemove.handler(createOptions(projectDir, { + pkg: 'patches/linked.patch', + }), ['pkg']) + + expect(fs.existsSync(patchLink)).toBe(false) + expect(fs.readFileSync(outsideTarget, 'utf8')).toBe('outside target') + expect(updatePatchedDependenciesMock).toHaveBeenCalledWith({}, expect.any(Object)) + expect(installHandler).toHaveBeenCalledWith(expect.objectContaining({ + patchedDependencies: {}, + })) +}) + +test('patch-remove allows a symlinked patches directory that resolves inside the project', async () => { + const projectDir = path.join(tempRoot, 'project') + const realPatchesDir = path.join(projectDir, 'real-patches') + const patchFile = path.join(realPatchesDir, 'pkg.patch') + fs.mkdirSync(realPatchesDir, { recursive: true }) + fs.symlinkSync(realPatchesDir, path.join(projectDir, 'patches'), process.platform === 'win32' ? 'junction' : 'dir') + fs.writeFileSync(patchFile, 'patch', 'utf8') + + await patchRemove.handler(createOptions(projectDir, { + pkg: 'patches/pkg.patch', + }), ['pkg']) + + expect(fs.existsSync(patchFile)).toBe(false) + expect(updatePatchedDependenciesMock).toHaveBeenCalledWith({}, expect.any(Object)) + expect(installHandler).toHaveBeenCalledWith(expect.objectContaining({ + patchedDependencies: {}, + })) +}) + +test('patch-remove keeps missing patch files as no-ops', async () => { + const projectDir = path.join(tempRoot, 'project') + fs.mkdirSync(path.join(projectDir, 'patches'), { recursive: true }) + + await patchRemove.handler(createOptions(projectDir, { + pkg: 'patches/missing.patch', + }), ['pkg']) + + expect(updatePatchedDependenciesMock).toHaveBeenCalledWith({}, expect.any(Object)) + expect(installHandler).toHaveBeenCalledWith(expect.objectContaining({ + patchedDependencies: {}, + })) +}) + +function createOptions ( + projectDir: string, + patchedDependencies: Record +): PatchRemoveCommandOptions { + return { + dir: projectDir, + lockfileDir: projectDir, + patchedDependencies, + rootProjectManifest: {}, + rootProjectManifestDir: projectDir, + } as PatchRemoveCommandOptions +} diff --git a/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts b/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts index 13f573ce2a..4daf3950a2 100644 --- a/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts +++ b/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts @@ -15,6 +15,7 @@ import { type IncludedDependencies } from '@pnpm/modules-yaml' import { packageIsInstallable } from '@pnpm/package-is-installable' import { type PatchGroupRecord, getPatchInfo } from '@pnpm/patching.config' import { safeReadPackageJsonFromDir } from '@pnpm/read-package-json' +import { safeJoinModulesDir } from '@pnpm/symlink-dependency' import { type DepPath, type SupportedArchitectures, type ProjectId, type Registries, type AllowBuild } from '@pnpm/types' import { type FetchPackageToStoreFunction, @@ -216,7 +217,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/pkg-manager/headless/test/lockfileToHoistedDepGraph.test.ts b/pkg-manager/headless/test/lockfileToHoistedDepGraph.test.ts new file mode 100644 index 0000000000..b4392a551d --- /dev/null +++ b/pkg-manager/headless/test/lockfileToHoistedDepGraph.test.ts @@ -0,0 +1,86 @@ +/// +import fs from 'fs' +import path from 'path' + +import { type LockfileObject } from '@pnpm/lockfile.fs' +import { tempDir } from '@pnpm/prepare' + +// Imported from the built output rather than `../src` on purpose: the source +// pulls in `@yarnpkg/nm`'s `HoisterResult` type, which ts-jest cannot resolve +// transitively under `moduleResolution: node`. The `.d.ts` is a lib file, so +// `skipLibCheck` skips it, and the test exercises the shipped graph builder. +import { lockfileToHoistedDepGraph } from '../lib/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/pnpm-lock.yaml b/pnpm-lock.yaml index 70a1745d8e..905cf5ff12 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1823,6 +1823,12 @@ importers: get-npm-tarball-url: specifier: 'catalog:' version: 2.1.0 + semver: + specifier: 'catalog:' + version: 7.7.4 + validate-npm-package-name: + specifier: 'catalog:' + version: 5.0.0 devDependencies: '@pnpm/config.deps-installer': specifier: workspace:* @@ -1836,6 +1842,12 @@ importers: '@pnpm/testing.temp-store': specifier: workspace:* version: link:../../testing/temp-store + '@types/semver': + specifier: 'catalog:' + version: 7.5.3 + '@types/validate-npm-package-name': + specifier: 'catalog:' + version: 4.0.2 load-json-file: specifier: 'catalog:' version: 6.2.0 @@ -3498,6 +3510,9 @@ importers: symlink-dir: specifier: 'catalog:' version: 6.0.5 + validate-npm-package-name: + specifier: 'catalog:' + version: 5.0.0 devDependencies: '@pnpm/logger': specifier: workspace:* @@ -3508,6 +3523,9 @@ importers: '@pnpm/symlink-dependency': specifier: workspace:* version: 'link:' + '@types/validate-npm-package-name': + specifier: 'catalog:' + version: 4.0.2 hooks/pnpmfile: dependencies: @@ -19237,7 +19255,7 @@ snapshots: '@yarnpkg/core@4.2.0(typanion@3.14.0)': dependencies: '@arcanis/slice-ansi': 1.1.1 - '@types/semver': 7.5.3 + '@types/semver': 7.7.1 '@types/treeify': 1.0.3 '@yarnpkg/fslib': 3.1.5 '@yarnpkg/libzip': 3.2.2(@yarnpkg/fslib@3.1.5)