fix: contain patch-remove deletions (#12341)

.changeset/fix-patch-remove-containment.md

@@ -0,0 +1,6 @@
+---
+"@pnpm/patching.commands": patch
+"pnpm": patch
+---
+
+Prevent `pnpm patch-remove` from removing files outside the configured patches directory.

patching/commands/src/isSubdirectory.ts

@@ -0,0 +1,17 @@
+import path from 'node: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)
+  )
+}

patching/commands/src/patchRemove.ts

@@ -1,3 +1,4 @@
+import type { Stats } from 'node:fs'
 import fs from 'node:fs/promises'
 import path from 'node:path'
 
@@ -9,6 +10,7 @@ import { install } from '@pnpm/installing.commands'
 import { pick } from 'ramda'
 import { renderHelp } from 'render-help'
 
+import { isSubdirectory } from './isSubdirectory.js'
 import { updatePatchedDependencies } from './updatePatchedDependencies.js'
 
 export function rcOptionsTypes (): Record<string, unknown> {
@@ -35,7 +37,7 @@ export type PatchRemoveCommandOptions = install.InstallCommandOptions & Pick<Con
 
 export async function handler (opts: PatchRemoveCommandOptions, params: string[]): Promise<void> {
   let patchesToRemove = params
-  const patchedDependencies = opts.patchedDependencies ?? {}
+  const patchedDependencies = { ...opts.patchedDependencies }
 
   if (!params.length) {
     const allPatches = Object.keys(patchedDependencies)
@@ -69,16 +71,21 @@ export async function handler (opts: PatchRemoveCommandOptions, params: string[]
     }
   }
 
-  const patchesDirs = new Set<string>()
-  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)
@@ -92,5 +99,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<PatchRemovalContext> {
+  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<PatchRemovalTarget> {
+  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<void> {
+  if (!targetExists) return
+
+  try {
+    await fs.unlink(targetPath)
+  } catch (err: unknown) {
+    if (isErrorWithCode(err, 'ENOENT')) return
+    throw err
+  }
+}
+
+async function lstatIfExists (targetPath: string): Promise<Stats | undefined> {
+  try {
+    return await fs.lstat(targetPath)
+  } catch (err: unknown) {
+    if (isErrorWithCode(err, 'ENOENT')) return undefined
+    throw err
+  }
+}
+
+async function realpathIfExists (targetPath: string): Promise<string | undefined> {
+  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
 }

patching/commands/test/isSubdirectory.test.ts

@@ -0,0 +1,21 @@
+import path from 'node:path'
+
+import { expect, test } from '@jest/globals'
+
+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)
+})

patching/commands/test/patchRemove.test.ts

@@ -0,0 +1,167 @@
+import fs from 'node:fs'
+import os from 'node:os'
+import path from 'node:path'
+
+import { afterEach, beforeEach, expect, jest, test } from '@jest/globals'
+
+import type { PatchRemoveCommandOptions } from '../src/patchRemove.js'
+
+jest.unstable_mockModule('@pnpm/installing.commands', () => ({
+  install: {
+    handler: jest.fn(),
+  },
+}))
+
+jest.unstable_mockModule('../src/updatePatchedDependencies.js', () => ({
+  updatePatchedDependencies: jest.fn(),
+}))
+
+const { install } = await import('@pnpm/installing.commands')
+const patchRemove = await import('../src/patchRemove.js')
+const { updatePatchedDependencies } = await import('../src/updatePatchedDependencies.js')
+
+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<string, string>
+): PatchRemoveCommandOptions {
+  return {
+    dir: projectDir,
+    lockfileDir: projectDir,
+    patchedDependencies,
+    rootProjectManifest: {},
+    rootProjectManifestDir: projectDir,
+  } as PatchRemoveCommandOptions
+}