fix: contain patch-remove deletions by zkochan · Pull Request #12341 · pnpm/pnpm

zkochan · 2026-06-11T23:43:05Z

Summary

Contain pnpm patch-remove deletions to the configured patches directory.
Validate the configured patches directory and the full removal batch before unlinking any patch file.
Canonicalize existing parent directories, preserve missing-file no-ops, and unlink final symlinks without following their targets.
Add focused coverage for traversal, directory entries, nested symlink escapes, final symlinks, valid symlinked patch directories, and component-aware containment checks.
Add a patch changeset for @pnpm/patching.commands and pnpm.

Validation

PNPR_PREPARE_BIN=/Users/zoltan/.cargo_shared_target/debug/pnpr-prepare PNPR_BIN=/Users/zoltan/.cargo_shared_target/debug/pnpr pnpm --filter @pnpm/patching.commands test test/isSubdirectory.test.ts test/patchRemove.test.ts
PNPR_PREPARE_BIN=/Users/zoltan/.cargo_shared_target/debug/pnpr-prepare PNPR_BIN=/Users/zoltan/.cargo_shared_target/debug/pnpr pnpm --filter @pnpm/patching.commands test test/patch.test.ts -t patch-remove
pnpm --filter @pnpm/patching.commands run compile
git diff --check
Public pre-push hook passed while pushing vuln-030 to pnpm/pnpm; Rust checks were skipped because there are no pacquet/ or pnpr/ changes.

Pacquet

No Rust-side parity change is included because patch-remove is not in pacquet's current command surface.

Written by an agent (Codex, GPT-5).

Summary by CodeRabbit

Bug Fixes
- pnpm patch-remove now validates that patch file paths remain within the configured patches directory, preventing accidental deletion of files outside the patches scope.
Tests
- Added comprehensive test coverage for path validation and filesystem edge cases in patch removal.

coderabbitai · 2026-06-11T23:43:13Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 80bb1b97-bd1f-4b08-bb7b-c0e0cd28fae0

📥 Commits

Reviewing files that changed from the base of the PR and between 01b3d45 and b80e7ce.

📒 Files selected for processing (5)

.changeset/fix-patch-remove-containment.md
patching/commands/src/isSubdirectory.ts
patching/commands/src/patchRemove.ts
patching/commands/test/isSubdirectory.test.ts
patching/commands/test/patchRemove.test.ts

📜 Recent review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: ubuntu-latest / Node.js 24 / Test

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate

Files:

patching/commands/test/isSubdirectory.test.ts
patching/commands/src/isSubdirectory.ts
patching/commands/test/patchRemove.test.ts
patching/commands/src/patchRemove.ts

**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not use instanceof Error for checking if a caught error is an Error object in Jest tests; use util.types.isNativeError() instead to work across realms

Files:

patching/commands/test/isSubdirectory.test.ts
patching/commands/test/patchRemove.test.ts

🧠 Learnings (4)

📚 Learning: 2026-05-14T09:04:00.133Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.

Applied to files:

patching/commands/test/isSubdirectory.test.ts
patching/commands/src/isSubdirectory.ts
patching/commands/test/patchRemove.test.ts
patching/commands/src/patchRemove.ts

📚 Learning: 2026-06-05T13:47:05.929Z

Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/test/install/frozenStore.ts:2-17
Timestamp: 2026-06-05T13:47:05.929Z
Learning: In the pnpm/pnpm repository, the shared Jest preset keeps `injectGlobals` at its default (`true`), so `test` and `expect` are available as Jest globals. Therefore, reviewers should not flag (or treat as TypeScript/compilation errors) missing `import { test, expect } from 'jest/globals'` when a test file uses `test`/`expect` without importing them. Importing from `jest/globals` may still be used for consistency with sibling files, but it is not required for execution in this repo unless a Jest preset is explicitly configured with `injectGlobals: false`.

Applied to files:

patching/commands/test/isSubdirectory.test.ts
patching/commands/test/patchRemove.test.ts

📚 Learning: 2026-06-05T13:47:26.046Z

Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.

Applied to files:

patching/commands/test/isSubdirectory.test.ts
patching/commands/src/isSubdirectory.ts
patching/commands/test/patchRemove.test.ts
patching/commands/src/patchRemove.ts

📚 Learning: 2026-05-26T21:01:06.666Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11966
File: .changeset/require-tarball-integrity.md:6-6
Timestamp: 2026-05-26T21:01:06.666Z
Learning: In pnpm lockfile-related release notes/docs (especially changeset markdown), preserve URL hostnames exactly as they appear in pnpm-lock.yaml tarball resolution entries—keep hosts like `codeload.github.com`, `bitbucket.org`, and `gitlab.com` in lowercase. Do not “correct” them to title-case/preserve brand capitalization (e.g., LanguageTool rules like `GITHUB` capitalization) because these are literal URL fragments, not platform brand names.

Applied to files:

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

🔇 Additional comments (2)

.changeset/fix-patch-remove-containment.md (1)

1-6: LGTM!
patching/commands/src/patchRemove.ts (1)
122-129: Confirm patches-dir contract: if absolute paths are supported, fix re-rooting in getPatchRemovalContext

patching/commands/src/patchRemove.ts builds patchesDir as path.join(lockfileDir, path.normalize(patchesDirSetting)). If patches-dir is documented/expected to be relative to the project root, this is fine; if absolute patches-dir is intended to work, path.join() will re-root absolute inputs under lockfileDir, which can make the containment checks reject/mishandle valid configs.
Suggested fix if absolute `patches-dir` should be supported
-  const patchesDir = path.join(lockfileDir, path.normalize(patchesDirSetting))
+  const patchesDir = path.resolve(lockfileDir, patchesDirSetting)

📝 Walkthrough

Walkthrough

This PR hardens pnpm patch-remove against path-traversal attacks by introducing path containment validation. A new utility validates that patch file paths stay within configured directories using relative path resolution. The patch-remove handler is refactored to validate and resolve patch targets, unlink only existing files, and pass cleaned dependency maps to downstream installation.

Changes

Patch-remove security hardening

Layer / File(s)	Summary
Path containment utility and tests `patching/commands/src/isSubdirectory.ts`, `patching/commands/test/isSubdirectory.test.ts`	Exports `isSubdirectory()` to determine whether a child path remains within a parent directory by computing and validating relative paths. Rejects `..` traversal, drive-letter changes, and absolute results. Tested against valid descendants, traversal escapes, and Windows path edge cases.
Secured patch removal implementation and handler logic `patching/commands/src/patchRemove.ts`	Refactors handler to use `isSubdirectory` validation, introduces `PatchRemovalContext` and `PatchRemovalTarget` models, resolves and validates patch targets concurrently with realpath checks, unlinks existing patch files safely (ENOENT-suppressed), removes entries from `patchedDependencies` only after successful validation, and explicitly passes the updated dependencies to downstream `install.handler`.
Comprehensive patch removal test suite `patching/commands/test/patchRemove.test.ts`	Validates handler security with mocked `install` and `updatePatchedDependencies`: rejects path traversal and directory targets before any filesystem changes, rejects symlinks resolving outside patches (preserving dangling targets), safely unlinks internal symlinks without modifying targets, allows symlinked patches directories that resolve inside the project, and treats missing patch files as no-ops while still completing the update/install flow.
Release documentation `.changeset/fix-patch-remove-containment.md`	Documents patch releases for `@pnpm/patching.commands` and `pnpm` noting that `pnpm patch-remove` is prevented from removing files outside the configured patches directory.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit hops through patch-remove today,
No more escapes down the .. way!
Symlinks and traversals meet their match,
Safe containment guards each precious patch. 🛡️

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: contain patch-remove deletions' directly and concisely describes the main change: restricting pnpm patch-remove deletions to the configured patches directory to prevent out-of-scope file removal.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch vuln-030

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

qodo-free-for-open-source-projects · 2026-06-12T00:01:01Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

qodo-free-for-open-source-projects · 2026-06-12T00:02:11Z

PR Summary by Qodo

Contain pnpm patch-remove deletions to the configured patches directory
🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Walkthroughs

Description

• Reject patch removals that escape the configured patches directory (including via symlinks).
• Validate the full removal batch before unlinking any patch file.
• Add targeted tests for traversal, symlink cases, and missing-file no-ops.

Diagram

graph TD
A["pnpm patch-remove"] --> B["Get removal context"] --> C["Compute targets + validate"] --> D["Unlink patch files"] --> E["Prune empty patch dirs"] --> F["Update patchedDependencies"] --> G["install.handler (reinstall)"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Resolve realpath of the patch file and enforce containment on that

➕ Simpler mental model: compare real paths only
➕ Catches escapes through any symlink in the full path
➖ Would follow the final symlink, which changes semantics (unlinking symlink vs target)
➖ Fails for dangling symlinks (cannot realpath), weakening protections or requiring special cases

2. Restrict allowed patchFile values to normalized relative paths at write-time

➕ Prevents unsafe entries from being persisted into patchedDependencies
➕ Reduces runtime checks in patch-remove
➖ Does not protect against already-existing unsafe state in older lockfiles/configs
➖ Still needs runtime validation for symlinked directories and filesystem reality

Recommendation: Keep the PR’s approach: it validates both the configured patchesDir (including realpath when present) and each removal target via path-based containment plus realpath checks on parent directories, while using lstat+unlink to ensure final symlinks are removed without following targets. This balances security (blocking traversal/symlink escapes) with correct behavior for dangling links and missing files.

File Changes

Bug fix (2)

isSubdirectory.ts Add isSubdirectory() helper for safe path containment checks +17/-0
Add isSubdirectory() helper for safe path containment checks
• Adds a small utility that determines whether a path is within a parent directory using path.relative semantics. Supports injecting path utils to test Windows behaviors (drive letters/UNC).
patching/commands/src/isSubdirectory.ts

patchRemove.ts Harden patch-remove deletions with containment + symlink-aware validation +125/-9
Harden patch-remove deletions with containment + symlink-aware validation
• Refactors patch removal to precompute and validate all targets before unlinking any file, blocking traversal and symlink-escape scenarios. Validates the configured patchesDir (including realpath when present), rejects directory targets, preserves missing-file deletions as no-ops, and unlinks symlinks without following their targets; also passes an updated patchedDependencies object into install.handler.
patching/commands/src/patchRemove.ts

Tests (2)

isSubdirectory.test.ts Add unit tests for isSubdirectory() across POSIX and Windows semantics +21/-0
Add unit tests for isSubdirectory() across POSIX and Windows semantics
• Covers acceptance of in-directory paths and rejection of traversal/sibling-prefix escapes. Adds Windows-specific cases for drive-letter and UNC path escapes using path.win32.
patching/commands/test/isSubdirectory.test.ts

patchRemove.test.ts Add patch-remove security/regression tests for traversal and symlinks +167/-0
Add patch-remove security/regression tests for traversal and symlinks
• Adds focused tests ensuring the command rejects unsafe removals before deleting anything, including parent traversal, directory entries, and nested symlink escapes. Verifies correct behavior for final symlink unlinking (without touching targets), symlinked patchesDir that resolves inside the project, and missing patch files treated as no-ops.
patching/commands/test/patchRemove.test.ts

Documentation (1)

fix-patch-remove-containment.md Add changeset for patch-remove containment fix +6/-0
Add changeset for patch-remove containment fix
• Introduces a changeset bumping @pnpm/patching.commands and pnpm as patch releases. Documents the fix preventing patch-remove from deleting files outside the patches directory.
.changeset/fix-patch-remove-containment.md

…12504) Backports three merged security fixes from main to release/10: - Contain hoisted dependency aliases so a crafted lockfile alias cannot escape node_modules or overwrite pnpm-owned layout. The hoisted graph builder now validates each alias at the safeJoinModulesDir sink. (GHSA-fr4h-3cph-29xv, #12343) - Contain pnpm patch-remove deletions to the configured patches directory. (#12341) - Reject path-traversal config dependency names and versions from pnpm-workspace.yaml before they are used to build filesystem paths. (GHSA-qrv3-253h-g69c, #12470) Written by an agent (Claude Code, claude-opus-4-8).

fix: contain patch-remove deletions

b80e7ce

zkochan marked this pull request as ready for review June 12, 2026 00:00

coderabbitai Bot approved these changes Jun 12, 2026

View reviewed changes

zkochan merged commit 612a2e6 into main Jun 12, 2026
15 checks passed

zkochan deleted the vuln-030 branch June 12, 2026 06:41

zkochan mentioned this pull request Jun 18, 2026

fix(security): backport path-traversal and containment fixes to v10 #12504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: contain patch-remove deletions#12341

fix: contain patch-remove deletions#12341
zkochan merged 1 commit into
mainfrom
vuln-030

zkochan commented Jun 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 12, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

zkochan commented Jun 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

Pacquet

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Great, no issues found!

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 12, 2026

PR Summary by Qodo

Walkthroughs

File Changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

zkochan commented Jun 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 11, 2026 •

edited

Loading

qodo-free-for-open-source-projects Bot commented Jun 12, 2026 •

edited

Loading