patch: refuse symlink and hard link escapes when applying patches

[robobun]

Problem

• PatchFile::apply resolved every target path (openat, unlinkat, renameat, fchmodat, and the read of the file being patched) relative to the package directory and let the kernel follow whatever it found. is_safe_patch_path is lexical (empty / absolute / .. / NUL), so a symlink, junction, or hard link already present in the directory redirected the operation, and the patch's bytes were written to (or the unlink/rename/chmod applied to) the link target outside the directory. Reproduction with the internal testing API:

  import { patchInternals } from "bun:internal-for-testing";
  fs.symlinkSync("/tmp/outside.txt", "pkg/link");
  patchInternals.apply(
    "diff --git a/link b/link\nnew file mode 100644\n--- /dev/null\n+++ b/link\n@@ -0,0 +1,1 @@\n+PWNED\n",
    "pkg",
  );
  // /tmp/outside.txt now contains "PWNED"

• Scope, to be precise about what this does and does not close: the only production caller is patch_install.rs, which copies the package into a fresh temporary directory (InstallMethod::Copyfile, patch_install.rs:497) and applies the patch there. That copy never contains symlinks or extra hard links, so none of this is reachable from bun install today. This PR makes the applier enforce containment itself instead of relying on that caller property, so the applier's contract holds for any directory it is pointed at (the testing API, or a future caller that applies in place).

• Separately, a pre-existing bug this PR also fixes: FileCreation used the new file's mode from the patch header as the mode for any parent directories it had to create, so new file mode 100644 produced a 0644 directory and the file creation inside it failed with EACCES for a non-root user (newdir/x.txt in a patch reproduces this on main). It is in this PR because the rewrite replaces that line; it is listed here, and tested in describe("creation"), as a behavior change rather than as containment.

Fix

• resolve_target validates the path and walks its parent directory one component at a time from the package directory fd, opening each component O_RDONLY | O_DIRECTORY | O_NOFOLLOW (creating it with mkdirat first when the operation is allowed to create directories). It returns the verified parent fd plus the final component, and every operation in apply is now an *at call against exactly that pair, so no path is ever re-resolved by the kernel after it has been checked. All five operations (delete, rename, create, patch, chmod) go through it; apply attaches the patch path to whatever error comes back, so a failing multi-file patch names the file.
• open_target_file opens the final component O_NOFOLLOW (plus O_NONBLOCK on POSIX so a FIFO cannot block the open) and then requires the opened inode to be a regular file with st_nlink == 1. A second link means the inode is reachable from outside the directory, so it is refused before anything is truncated; the file being patched is read and written back through this one fd, and creation defers truncation until after the check.
• Lexical additions: : is rejected on Windows, because the Windows openat strips a drive prefix from a relative component, turning C:.. into ... (NUL rejection already landed on main in Robustness pass across install, css, ffi, crypto, spawn, shell, and node compat #36165.) Created parent directories are 0755.
• Windows: openat with O_NOFOLLOW opens with FILE_OPEN_REPARSE_POINT since sys(windows): keep FILE_SYNCHRONOUS_IO_NONALERT when opening with O::NOFOLLOW #36193, so the walk and the final open pass O_NOFOLLOW on every platform. On Windows that opens the reparse point itself rather than failing, and fstat on that handle reports a regular file, so reject_reparse_point additionally checks each component's own attributes (NtQueryAttributesFile via the new bun_sys::get_file_attributes_at) before opening it. That check is by name rather than by handle, so on Windows containment is not atomic against a concurrent swap; on POSIX it is.
• Error codes: a symlink or junction anywhere in the path is ELOOP on every platform (Linux reports ENOTDIR, Darwin ELOOP, FreeBSD EMLINK for the refused open; symlink_component_err normalizes them after confirming with lstat); a hard-linked target is EMLINK; a non-regular target or a rejected path is EINVAL.
• Verification: test/js/bun/patch/patch.test.ts adds suites for symlinks (final and intermediate component, all five operations), hard links, Windows junctions, the NUL .. chain, the Windows drive-relative component, FIFO targets, and the directory mode; each rejection test fails on the unfixed applier. bun-install-patch.test.ts and patch-bounds-check.test.ts snapshots are updated for the error messages, which now include the file name. byte-search and dead-code-escapes source lints pass.

Background

• openat-family syscalls take a directory fd and resolve the given path relative to it. With a single-component name and O_NOFOLLOW, the kernel refuses to go through a symlink at that name, and since the directory fd already pins the parent, the combination is the standard way to operate on "this entry, in this directory, and nothing else"; walking a multi-component path this way is what makes intermediate symlinks unreachable too.
• st_nlink is the number of directory entries pointing at an inode. A freshly copied package has exactly one for every file, so a count above one means a second name for the same data exists somewhere, and writing through either name writes the other.
• On Windows, symlinks and junctions are both "reparse points". FILE_OPEN_REPARSE_POINT makes an open return the reparse point itself instead of its target, which is why a successful open there has to be followed by an attribute check rather than treated as proof the entry is a plain file or directory.

---

no test proof · iteration 15 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-patch.test.ts test/js/bun/patch/patch.test.ts

[robobun]

^{Updated 3:19 PM PT - Aug 17th, 2026}

✅ @robobun, your commit 0a35b56d5daf5274627f2507a68324f9626213cd passed in Build #100196! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 32677

That installs a local version of the PR into your bun-32677 executable, so you can run:

bun-32677 --bun

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

src/patch/lib.rs now uses verified parent and target file descriptors for patch operations, adds path and target-type rejection checks, and updates patch tests for escape cases. Documentation examples and the web APIs table were reformatted.

Changes

Patch symlink hardening

Layer / File(s)	Summary
Verified apply operations src/patch/lib.rs	PatchFile::apply now performs deletion, rename, creation, and mode-change operations through verified parent directories and relative file descriptors, and apply_patch uses the same parent-walking target open flow.
Safety checks and write-back src/patch/lib.rs	apply_patch updates splice bounds error mapping and the write-back phase truncates through the verified fd, resets the file offset on Windows, and uses new helpers for symlink, reparse-point, hard-link, and non-regular target rejection; is_safe_patch_path also rejects interior NUL bytes.
Escape regression tests test/js/bun/patch/patch.test.ts, test/cli/install/bun-install-patch.test.ts, test/internal/dead-code-escape-limits.json	The patch test suite adds fixture helpers and coverage for symlink, hard-link, junction, NUL-byte, and non-regular target rejection, alongside assertions that in-directory patching still succeeds; related snapshot normalization and dead-code config updates are included here.

Documentation formatting

Layer / File(s)	Summary
Docs formatting updates docs/guides/util/base64.mdx, docs/runtime/web-apis.mdx	Reformats the base64 warning example as a fenced TypeScript block and reflows the web APIs table header and body layout without changing the listed content.

Suggested reviewers

• alii

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly and concisely identifies the main change: preventing symlink and hard-link escapes during patch application.
Description check	✅ Passed	The description explains the problem, implementation, platform behavior, verification coverage, and related behavior change in sufficient detail.

---

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

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/patch/lib.rs (1)

270-286: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Reject intermediate symlinks before reading the source file.

lstatat() only avoids following the final component; read_file_alloc() can still follow an intermediate directory symlink and read a file outside patch_dir before open_beneath() later rejects the write. Run the same parent containment check, or open/read through a verified open_beneath(..., RDONLY, ..., false, ...) fd, before stat/read.

As per coding guidelines, “Security checks fail closed and cover every path to the protected effect.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/patch/lib.rs` around lines 270 - 286, Reject intermediate symlinks before
the file is read in the patch loading flow. The current `lstatat()` check in
`src/patch/lib.rs` only protects the final path component, while
`read_file_alloc()` can still traverse a symlink in a parent directory and read
outside `patch_dir` before later containment checks happen. Update the logic
around `lstatat`, `read_file_alloc`, and the `open_beneath` path handling so the
source is opened/read only after verifying the full parent chain or by using a
verified `open_beneath(..., RDONLY, ..., false, ...)` file descriptor.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/patch/lib.rs`:
- Around line 467-477: The Windows final-symlink rejection in open_beneath() is
currently gated by O_CREAT, so FileModeChange/chmod paths can still follow a
final symlink before it is checked. In src/patch/lib.rs, move the lstatat(...
ISLNK ...) check out of the flags & sys::O::CREAT condition so it runs whenever
cfg!(windows) is true, and keep rejecting symlinks before sys::openat is called;
apply the same fix to the related open_beneath path referenced by the comment.

---

Outside diff comments:
In `@src/patch/lib.rs`:
- Around line 270-286: Reject intermediate symlinks before the file is read in
the patch loading flow. The current `lstatat()` check in `src/patch/lib.rs` only
protects the final path component, while `read_file_alloc()` can still traverse
a symlink in a parent directory and read outside `patch_dir` before later
containment checks happen. Update the logic around `lstatat`, `read_file_alloc`,
and the `open_beneath` path handling so the source is opened/read only after
verifying the full parent chain or by using a verified `open_beneath(...,
RDONLY, ..., false, ...)` file descriptor.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 31033103-68ff-4abd-9bbf-b31db40d67a2

📥 Commits

Reviewing files that changed from the base of the PR and between 942c222 and 08dff4b.

📒 Files selected for processing (2)

• src/patch/lib.rs
• test/js/bun/patch/patch.test.ts

[claude]

All the issues I raised across the earlier revisions look properly addressed now and the bug hunter found nothing new on 6a703f5 — but since this is new security-boundary code (symlink containment for the patch applier) with platform-specific branches that took several iterations to get right, I'd still like a human to give the final design a once-over before merge.

Extended reasoning...

Overview

This PR hardens PatchFile::apply in src/patch/lib.rs against symlink-based directory escapes. It adds three helpers — open_beneath, verify_parent_beneath_patch_dir, and verify_fd_beneath_patch_dir — and wires them into all five PatchFilePart arms (creation, patch, deletion, rename, mode change). apply_patch switches from fstatat/stat to lstatat and rejects symlink targets up front. ~150 lines of new Rust plus a 250-line non-Windows test suite, a snapshot normalizer tweak, and a dead-code-limits update.

Security risks

The change is the security boundary: it enforces that patch application cannot write/chmod/unlink/rename outside the package directory via symlinks. Getting it wrong reintroduces arbitrary file write through patchedDependencies. Across three prior review rounds I (and CodeRabbit) flagged seven distinct issues — a full rename/delete bypass, mkdir-before-verify leak, comparator looseness on macOS/POSIX, a ./-prefix regression, a Windows O_CREAT empty-file leak, and a Windows fchmod panic on HANDLE-backed fds — all of which the author fixed. The current revision looks correct to me on POSIX, and the Windows path is defended by an lstatat pre-check plus post-open canonical-path verification.

Level of scrutiny

High. This is newly-authored containment logic with platform-conditional behavior (cfg!(windows) drops O_NOFOLLOW and falls back to lstatat + post-open verify; FileModeChange uses path-based fchmodat on Windows vs fchmod(fd) on POSIX). The Windows branches have no direct test coverage (the new suite is gated !== "win32"), and the get_fd_path-based prefix comparison is subtle enough that a maintainer familiar with the sys layer should confirm the approach.

Other factors

The author was very responsive and each fix came with added test coverage where feasible. CI on the latest commit (6a703f5) hasn't reported yet; the prior commit's failures (Windows snapshot, dead-code-escapes) are exactly what 6a703f5 targets. No outstanding unresolved review threads. I'm deferring rather than approving solely because security-boundary code of this shape warrants human sign-off, not because of any specific remaining concern.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/patch/lib.rs`:
- Around line 253-258: The patch application flow in lib.rs still reuses the
path after containment is checked, which allows a symlink swap between
verification and the later read/chmod steps. Update the patching logic around
the verified handle/path flow used by lstatat, read_file_alloc, open_beneath,
and fchmodat so that fstat/read/chmod operate on the already-open
descriptor/handle instead of reopening by path. Keep the containment check as a
guard, but perform the actual file access and chmod through the verified
fd/handle to avoid check-then-act races.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 12f1f6bf-85f7-477b-8b88-ace3811c2ab0

📥 Commits

Reviewing files that changed from the base of the PR and between 08dff4b and 0265a5e.

📒 Files selected for processing (4)

• src/patch/lib.rs
• test/cli/install/bun-install-patch.test.ts
• test/internal/dead-code-escape-limits.json
• test/js/bun/patch/patch.test.ts

💤 Files with no reviewable changes (1)

• test/internal/dead-code-escape-limits.json

[robobun]

Status at 0a35b56 (current head)

Single commit on current main. @alii reviewed the approach, requested changes, and re-reviewed 0e8d71965b as ready to merge; this head is that plus a rebase, a rebase-forced change to how Windows chmod is performed, and one test assertion, all listed in the comment after his (his comment and the one following it).

What the head contains, briefly:

• every patch operation resolves its target through resolve_target (component walk with O_DIRECTORY | O_NOFOLLOW, then *at calls against the verified parent and final component); opened targets must be regular files with one link; the patched file is read and written through one fd;
• : rejected in patch paths on Windows (NUL rejection is already on main); O_NOFOLLOW on every platform plus the by-name reparse check on Windows;
• separately stated fix: directories a patch creates are 0755 instead of inheriting the file mode;
• errors name the failing file (per side for renames); snapshots updated accordingly.

All review threads are resolved. Locally: patch suites, install-patch and bounds-check snapshots, and the byte-search / dead-code-escapes source lints pass; cargo check clean on linux, windows, darwin; clippy clean. CI runs on 0a35b56d5d.

[claude]

All seven findings from the earlier review rounds look properly addressed and I have nothing new to flag, but since this is security-sensitive containment logic (with Windows paths that the new test suite skips) I'd like a human to give it a final look.

Extended reasoning...

Overview

This PR hardens PatchFile::apply in src/patch/lib.rs against symlink-traversal escapes. It adds three helpers — open_beneath, verify_parent_beneath_patch_dir, and verify_fd_beneath_patch_dir — and routes all five patch-operation arms (creation, patch, deletion, rename, mode-change) through them so that neither a final-component symlink nor an intermediate directory symlink can cause writes/unlinks/renames/chmods outside the patch directory. apply_patch switches to lstatat and rejects symlink targets before reading. A ~250-line POSIX-only test suite in test/js/bun/patch/patch.test.ts exercises the rejection and happy paths; bun-install-patch.test.ts and dead-code-escape-limits.json get small follow-on tweaks.

Security risks

This is squarely security-sensitive: the patch applier runs on untrusted input (patch files referenced by patchedDependencies, applied to package contents that may themselves be attacker-supplied via the registry). The change is a strict hardening over the prior O_CREAT|O_WRONLY|O_TRUNC open with no O_NOFOLLOW, and across three review rounds I raised seven issues — the rename/delete bypass, the pre-verify mkdir leak, comparator looseness (case-insensitive and is_sep_any), the ./ regression, a Windows empty-file leak via FILE_OPEN_IF, and a Windows fchmod panic on HANDLE-backed fds — all of which are now addressed and resolved in the thread. The remaining residual risk is the acknowledged TOCTOU on Windows between the lstatat pre-check / containment verify and the subsequent path-based fchmodat, which is acceptable under the threat model (no concurrent local attacker) and still backed by the post-open canonical-path check.

Level of scrutiny

High. This is new containment logic on a write path reachable from bun install, with platform-divergent behaviour (POSIX O_NOFOLLOW vs. Windows NtCreateFile semantics, HANDLE-vs-CRT fd kinds). The iteration history — seven distinct bugs surfaced across three bot review passes plus one CodeRabbit finding — confirms the code is subtle enough to merit a human maintainer's sign-off rather than bot auto-approval.

Other factors

The new refuses to follow symlinks suite is gated behind process.platform !== "win32", so the Windows-specific branches (the lstatat pre-check, the nofollow = 0 path, and the path-based fchmodat fallback) have no direct CI coverage; the existing mode change > simple test is likewise POSIX-only. The current bug-hunting pass found nothing new, all prior inline comments are resolved, and no human reviewer has weighed in yet.

[robobun]

Reworked the containment design in 20f36e8 to address the review. Point by point:

1. Read path follows symlinks out of the patch dir. Gone structurally: apply_patch now opens the target once with O_RDWR | O_NOFOLLOW relative to the verified parent fd and does the read, ftruncate, and write-back through that same fd. There is no path-resolved read left (read_file_alloc is deleted). Test: file patch through a symlink is rejected asserts the ELOOP and that the link target is untouched; the hunk is written so that a read-through-the-link implementation would have succeeded.

2. Hard links. Every opened target is fstated and must be a regular file with st_nlink == 1; a planted hard link is rejected with EMLINK before any truncation. No false positives for real installs: bun install applies patches to a freshly copied package (InstallMethod::Copyfile), so targets always have one link. Tests: refuses to write through hard links (file patch, creation, mode change; runs on every platform including Windows).

3. unlink/rename/mkdir check-then-act. All five operations now run relative to the verified parent directory fd (unlinkat(parent, base), renameat(from_parent, from_base, to_parent, to_base), mkdirat per component during the walk), so there is no re-resolution of a path between verification and effect.

4. procfs dependency. Gone: get_fd_path/canonical-path comparison is deleted, so patching works in chroot/distroless/pid-namespaced environments. I did not take the openat2 route: RESOLVE_BENEATH still needs exactly this walk as a fallback (pre-5.6 kernels, seccomp profiles that reject openat2) and adds no containment on top of the per-component O_NOFOLLOW walk, so there is one mechanism on all POSIX platforms instead of two.

5. Windows tests. Added refuses to traverse junctions on Windows (directory junctions need no privilege in CI): creation, patch, and deletion through an intermediate junction, a junction as the chmod target, plus positive tests for regular files, new directories, and mode changes. While writing them I found that the lstatat + ISLNK check the previous revision relied on never detected reparse points (libuv's handle fstat only reports links under lstat), so the Windows branch now uses NtQueryAttributesFile relative to the verified parent handle (new bun_sys::get_file_attributes_at), which reports the reparse point's own attributes like GetFileAttributesW. That check is path-based because O_NOFOLLOW cannot be used for the final file open on Windows, so the PR body states Windows containment is best-effort; the hard link check is handle-based and fully effective there.

Error codes are now uniform: any symlink/junction in the target path is ELOOP on every platform, a multiply-linked target is EMLINK.

[alii]

@robobun bump — no commits since 6-26. The symlink fix leaves same-severity holes: (1) the READ path still follows symlinks out of the patch dir — apply_patch reads via read_file_alloc → plain RDONLY openat, no NOFOLLOW/containment; route it through open_beneath(...). (2) hardlinks wholly undefended (lstat sees a regular file, NOFOLLOW is a no-op) — reject nlink>1 on the opened fd or document out-of-scope. (3) unlink/rename/mkdir keep the check-then-act race; operate via the verified dirfd or openat2(RESOLVE_BENEATH). (4) containment hard-depends on /proc → chroot/distroless hard-fail; prefer openat2 on Linux. (5) Windows branch (NOFOLLOW dropped) has zero tests — add junction-based cases.

[robobun]

@alii Thanks for the review. I think the five points describe the revision before the rework that landed on 2026-06-26 as 20f36e8 ("patch: resolve targets beneath the patch dir without procfs, reject hard links"). That is the "no commits since 6-26" commit, and it replaced the mechanism wholesale: open_beneath and read_file_alloc no longer exist at head, so neither do the code paths through them. Taking each point against the current head:

1. Read path. read_file_alloc is gone. apply_patch resolves the target once with the component walk and opens it O_RDWR | O_NOFOLLOW via open_target_file (lib.rs:239), then reads through that same fd (lib.rs:250). The read and the write-back share one verified descriptor, so there is no separate path-resolved read left to follow a symlink.

2. Hard links. open_target_file fstats the opened fd and rejects st_nlink > 1 with EMLINK before anything is truncated (lib.rs:537). This guards FileCreation, FilePatch, and FileModeChange. It is correct for legitimate targets because bun install always applies patches to a freshly copied package (InstallMethod::Copyfile in patch_install.rs), so real targets have exactly one link. Covered by the "refuses to write through hard links" suite (patch.test.ts:896).

3. unlink / rename / mkdir. open_parent_beneath (lib.rs:420) walks the target's parent one component at a time, each opened O_RDONLY | O_DIRECTORY | O_NOFOLLOW (lib.rs:474), and every mutation then runs relative to that verified dirfd: unlinkat(parent.fd, base) (lib.rs:61), renameat(from_parent.fd, from_base, to_parent.fd, to_base) (lib.rs:89), and mkdirat(dir, component) inside the walk itself (lib.rs:478). There is no path re-resolution between the check and the effect.

4. procfs / openat2. The procfs dependency is fully removed; get_fd_path and /proc no longer appear in the file, so chroot and distroless work. On openat2(RESOLVE_BENEATH): it is Linux 5.6+ only and is commonly denied by seccomp profiles (Docker's default filter among them), so it would still need exactly this walk as its fallback on every other path, and it adds no containment the walk does not already provide, since the walk is the same kernel-enforced per-component no-follow resolution. The portable walk is therefore the single mechanism rather than a second parallel one. The longer form of this is in the PR description.

5. Windows tests. There is a Windows-only junction suite at patch.test.ts:974 covering file creation, file patch, deletion, and mode change through an NTFS junction, plus the positive cases. It ran and passed on all three Windows lanes (2019 x64, 2019 x64-baseline, 11 aarch64) in build 64940.

The only thing keeping the check red is unrelated to this diff: test/napi/napi.test.ts fails on the Windows lanes because napi_is_arraybuffer splits CRLF output with "\n". That test came from #32629, the failure is deterministic (it reproduced on two consecutive builds plus an in-build retry), and main has not run a Windows test matrix since that commit landed, so this branch is just the first one to surface it. Full details with links are in the CI status comment above. Happy to make any further changes you want here.

[alii]

@robobun you're right — I verified all five against 20f36e8d and concede. My review (and the bump) critiqued the pre-rework revision; open_beneath/read_file_alloc/get_fd_path are gone at head and the component-walk rework closes each point:

1. Read shares the verified fd via open_target_file (read_to_end at lib.rs:308) — no separate path-resolved read.
2. st_nlink > 1 → EMLINK before truncate (lib.rs:660), covered by the hard-link suite (patch.test.ts:896).
3. unlinkat/renameat/mkdirat all run relative to the per-component-walked dirfd — no re-resolution.
4. procfs fully removed; openat2(RESOLVE_BENEATH) would need this same walk as its seccomp fallback anyway, agreed.
5. Junction suite at patch.test.ts:974, green on all three Windows lanes.
   Withdrawing the five points. The only red is the unrelated napi CRLF flake from napi: napi_is_arraybuffer returns false for SharedArrayBuffer #32629. LGTM from my side — sorry for the stale bump.

[robobun]

Thanks for taking the time to re-verify, and no problem at all. Nothing further is needed from this side, so I will leave the branch as is rather than push and reset CI on a signed-off diff.

On the one remaining red: if it would help to land this on a green check, I am happy to put up a separate small PR for the napi_is_arraybuffer failure in test/napi/napi.test.ts. The Windows failure output shows the test splitting CRLF output on "\n", leaving a trailing \r on each compared line, so it is a test-only fix. Otherwise it is unrelated to this diff and safe to merge over.

[alii]

@claude review

[robobun]

@alii heads up: the @claude review you triggered found a real containment bypass in the head you approved, so please treat your LGTM on 20f36e8d as withdrawn. Good call running one more pass.

The bug. A path component of .. followed by a NUL byte (e.g. "..\0x") is not byte-equal to b"..", so both is_safe_patch_path and the walk's .. guard accepted it, but each component is handed to openat as a NUL-terminated C string, so the kernel truncates at the NUL and resolves a real ... Chained components walk straight out of patch_dir, and every operation (create, truncate, write, unlink, rename, chmod) then runs at the attacker's chosen path. It needs nothing on disk, only bytes in the .patch file, and it was a regression introduced by the component-walk rework itself (the pre-rework code passed the whole path as one C string, so the kernel truncated the lot and returned EISDIR).

Reproduced before fixing. On the unfixed head, a FileCreation for ".." + NUL + "1/.." + NUL + "2/.." + NUL + "3/outside/escaped.txt" reaches the C-string boundary inside the walk; a debug build aborts right there:

panic: ZStr::as_cstr: interior NUL would truncate the C view
  bun_patch::open_dir_component    src/patch/lib.rs:475
  bun_patch::open_parent_beneath   src/patch/lib.rs:440
  bun_patch::PatchFile::apply      src/patch/lib.rs:99

That assert is debug-only, so on release (and macOS) the same input is a silent .. ascent instead of an abort. Full detail is in the resolved review thread.

The fix (7dd4527). is_safe_patch_path now rejects any path containing a NUL byte, which is never valid in a filename, so no legitimate patch changes behavior; open_parent_beneath refuses NUL-bearing components as a backstop next to its existing .. backstop. The review's second point (a FIFO target blocking the O_RDONLY open before the !ISREG check could reject it) is also fixed by opening targets with O_NONBLOCK on POSIX. Three new subprocess-isolated tests cover the escape, a NUL in the final component, and the FIFO case; all three fail on the unfixed build and pass on the fixed one, and the full patch suite is green (45 pass, 0 fail).

The incremental diff is small (two guard clauses and one open flag, plus tests), so it should be a quick re-look when you have a moment. CI is running on the new head. Sorry for the churn right after your sign-off; the extra review pass clearly paid for itself.

[claude]

7dd4527 addresses both findings from my last pass (NUL-byte .. escape and the FIFO O_NONBLOCK gap), and I found nothing new this round — deferring final sign-off to a human given this is security-critical containment code.

Extended reasoning...

Overview

This PR replaces the patch applier's path resolution with a per-component O_NOFOLLOW | O_DIRECTORY walk (open_parent_beneath) plus an fd-based target open (open_target_file) that enforces ISREG and st_nlink == 1, so symlink and hard-link escapes are refused before any write/truncate/chmod/unlink/rename. It touches src/patch/lib.rs (~250 lines net), adds a Windows get_file_attributes_at helper in src/sys/lib.rs, and adds ~600 lines of tests across POSIX symlinks, NTFS junctions, hard links, NUL-byte paths, and FIFOs.

What changed since my last review

My two outstanding inline comments (the 🔴 NUL-byte ..\0x component escape and the 🟡 FIFO blocking-open nit) are both addressed in 7dd4527:

• is_safe_patch_path now rejects path.contains(&0), and open_parent_beneath independently rejects any component containing a NUL — defence in depth, with two new tests covering the ..\0 escape and a NUL in the final component.
• open_target_file now adds O_NONBLOCK on POSIX so a FIFO target returns immediately for the !ISREG → EINVAL check, with a new test (mode change on a FIFO is rejected instead of hanging).

The bug-hunting system found nothing new at this revision.

Security risks

This is the security-relevant surface itself: containment of untrusted patch-file paths against the filesystem during bun install. The mechanism (per-component O_NOFOLLOW dirfd walk + fd-relative *at syscalls + fstat nlink/ISREG gate) is the standard correct shape on POSIX. Windows is documented best-effort via NtQueryAttributesFile reparse-point checks. The PR has been through eight rounds of review and a full rework; every issue I and others raised has been addressed in code with regression tests.

Level of scrutiny

High. This is newly-written filesystem containment for adversarial input, with platform-specific syscall semantics (Linux vs Darwin O_NOFOLLOW|O_DIRECTORY errno, Windows HANDLE-vs-CRT-fd, NtCreateFile reparse handling). It is not a mechanical change.

Other factors

• A human reviewer (alii) gave LGTM on the revision immediately prior to 7dd4527; the only delta since then is the two targeted fixes for my findings plus their tests.
• Test coverage is thorough across all five patch operations, all three platform families, and the specific bypass vectors raised in review.
• The one acknowledged-and-deferred item (Windows CRT-fd leak in sys::fstat) is a pre-existing limitation in shared code, scoped for follow-up.

Given the security-critical nature, I'm not auto-approving; a human should give the final nod, but from my side the diff is clean.

[robobun]

@alii closing the loop: the review you triggered has re-run against 7dd4527347 and reports both of its findings (the NUL-byte .. escape and the FIFO O_NONBLOCK gap) as addressed, with nothing new found this round; it defers the final sign-off to a human. All 12 review threads on the PR are resolved.

CI on 7dd4527347 is still running, but every patch suite that has reported so far is green on every lane, including the new NUL-traversal, FIFO, and Windows junction tests on the ASAN and Windows lanes. The only red lanes are unrelated to this diff and already documented in the CI status comment: the napi_is_arraybuffer CRLF failure from #32629 on Windows, a node-tls-connect > should have peer certificate failure on the Windows 2019 lanes that also does not touch this code, and a few retried flakes.

So this is ready for your re-look whenever you have a moment. The entire delta since the head you LGTM'd is the two NUL guard clauses, the O_NONBLOCK flag, and their tests.

[robobun]

@alii one more small update, no code change: I rebased onto current main, so the head is now f6923bc401.

Reason: the last CI run was red on every single test lane, and both causes turned out to be test bugs that main has already fixed since this branch's base.

• test/js/node/tls/node-tls-connect.test.ts failed on all 14 lanes because it asserts the live bun.sh certificate advertises an OCSP URI (infoAccess["OCSP - URI"]), and the real certificate no longer does (CAs have dropped OCSP URLs). Main removed that assertion three days ago in test: stop asserting an OCSP URI on the live bun.sh certificate #32789; this branch's base was one day older than the fix, so it still carried the time bomb.
• test/napi/napi.test.ts on the Windows lanes was the napi_is_arraybuffer CRLF split from napi: napi_is_arraybuffer returns false for SharedArrayBuffer #32629, fixed on main in test(napi): split napi_is_arraybuffer output on /\r?\n/ for Windows #32722.

Neither has any connection to this PR, and none of the patch suites (including the new NUL, FIFO, and junction tests) failed on any lane of that build.

On the rebase itself: git range-diff reports all 7 commits as =, meaning the patch content you and the triggered review looked at is unchanged to the byte; only the base moved. No conflicts. Locally on the rebased tree the full patch suite set is green (93 pass, 0 fail) and cargo check is clean for linux, windows, and darwin.

CI is running on f6923bc401; with both of those upstream test fixes now included, it should finally come back green rather than needing the "these reds are unrelated" explanation.

[robobun]

@alii one more small commit on top, 5695567b12, from the follow-up automated review: FileCreation was passing the new file's mode from the patch header as the create mode for any missing parent directories, so a standard new file mode 100644 patch produced 0o644 directories, which are not traversable, and the subsequent create inside them fails with EACCES for any non-root user. It is pre-existing behavior (the old mkdir_recursive_at_mode call did the same thing) and it never showed up because CI and the test suite run as root, which bypasses directory permission checks, while the one test covering the scenario happened to use 100755.

The fix is one token: created parents now use Some(0o755), exactly like the rename arm a few lines above already did. I reproduced it before fixing (the created directory really is 0o644 on the unfixed build) and the test now uses the common 100644 header and asserts the created directory is traversable and the created file is not executable, so it catches a regression with or without root. Full patch suite is green (93 pass, 0 fail) and review threads are all resolved.

Sorry for the drip of small commits while you have this on your plate; each one has been a concrete finding from the extra review passes, and I would rather land them before your final look than after.

[robobun]

@alii all five points are addressed in 165adf292b, which is now a single commit on current main (the earlier 11-commit history, the autofix docs hunks, and the old escape-limits path are gone with the rebase). Per-point replies are on the threads; the short version:

• Rebase fallout: main's NUL check kept, : check and the walk use strings::contains_char / strings::split_any, redundant NUL test dropped, byte-search and dead-code-escapes lints pass.
• The repeated block is one resolve_target returning (ParentDir, basename), used by five small per-operation functions; the dead re-check in the walk is deleted.
• The directory-mode change is stated in the body as a separate pre-existing bug, is a named CREATED_DIR_MODE, and its test lives in describe("creation").
• O_NOFOLLOW is passed on every platform; reject_reparse_point stays with its real rationale; the Windows paragraph in the body is rewritten.
• Normalizer lines deleted and both snapshots updated; apply now attaches the patch path to errors (the helpers no longer touch the path), so a failing multi-file patch names the file.

The body also now says up front that the only production caller applies to a fresh copyfile'd temp directory, so this hardens the applier's own contract rather than closing something reachable from bun install.

Locally at this head: patch.test.ts, bun-install-patch.test.ts, patch-bounds-check.test.ts, and both source lints are green (95 pass), and cargo check is clean for linux, windows, and darwin. I also resolved the batch of identical comment-cop threads after a pass that removed or shortened the comments it flagged; the ones left are doc comments stating a precondition or a platform difference.

[alii]

Close. The containment code at this head holds up: is_safe_patch_path and the walk split on the same separators so a walk component can never be .., every syscall now runs against the verified (parent fd, basename) pair, open_target_file's NOFOLLOW-everywhere plus NONBLOCK-on-POSIX split matches what openat_windows_impl does with each flag (NOFOLLOW keeps FILE_SYNCHRONOUS_IO_NONALERT, NONBLOCK drops it), and the fd ownership in open_parent_beneath is right. Four of the five earlier points are resolved; the Windows NOFOLLOW one still has a stale copy in the test file, and the new error attribution names the wrong file when a rename's source fails. Fix those two and this is ready from my side. No conflict with main. Windows behaviour is unverified here, read only.

[robobun]

@alii both remaining items are in 0e8d71965b: rename errors are attributed per side (source resolve -> from_path, destination resolve -> to_path, renameat itself -> from -> to), with the two rename tests now asserting the reported path in each direction, and the test-file comment is reworded to match reject_reparse_point.

One correction on my side: when I said the body had been rewritten, it had not actually taken effect. The gh pr edit failed on a token scope error and my verification grepped for a string the old body also contained. It is applied now (Problem / Fix / Background, including the copyfile'd-tempdir scope note, the directory-mode change as a separate fix, and the corrected Windows paragraph), so if you read the body earlier, it is worth one more glance.

Locally at this head the patch suites, the two snapshot files, and both source lints pass, and clippy is clean. Nothing else outstanding from my side.

[alii]

Looks ready to merge from my side. Checked the two remaining points at 0e8d719: rename_file now attaches from_path or to_path to whichever resolve failed (and from -> to only when renameat itself fails), the two rename tests assert the reported path and still fail against main, and the junction comment in patch.test.ts now matches reject_reparse_point. Nothing else changed in this push; the branch is on today's main and the touched files have not moved there. Windows behaviour is still unverified here, read only.

[robobun]

Thanks for the careful passes. One note so it is your call rather than mine: the automated review left a single cosmetic test nit on this head (the hard-link mode-change test asserts file content, which a chmod cannot change; its EMLINK assertion is the one doing the work). I have left the sha exactly as you reviewed it rather than push for that alone. If you would like it folded in before merging, say so and it is a two-line push; otherwise it can ride along with any later change to that file.

[robobun]

@alii main moved under this and it stopped merging cleanly, so the head is now 594b974fdb. Three things differ from the 0e8d719 you signed off on, so you can re-check just these:

1. Rebase onto current main. The only textual conflict was one line in dead-code-escape-limits.json; src/patch/lib.rs and patch.test.ts were untouched upstream.
2. One change the rebase forced: main's dead-code pass removed the Windows sys::fchmodat that change_mode was using, and Windows sys::fchmod still panics on a HANDLE-backed fd. change_mode now does fd.make_libuv_owned() and then sys::fchmod on every platform (the conversion is the identity off Windows), so the platform split is gone and the chmod runs on the verified handle rather than by name. It is exercised on the Windows lanes by the junction suite's "still applies a mode change to a regular file". Related: main has since made Windows fstat stat the HANDLE directly, so the CRT-slot leak noted earlier in review no longer applies to open_target_file.
3. The cosmetic test nit I mentioned, folded in since a push was needed anyway: the hard-link mode-change test now asserts the outside file's mode is unchanged (captured before the apply, so it is platform-independent) instead of its contents.

At this head: cargo check is clean for linux, windows, and darwin, clippy is clean, and the patch suites, both snapshot files, and both source lints pass locally (95 pass). CI is running on it.

[robobun]

@alii one refinement to item 2 from my previous comment, now at 0a35b56d5d: automated review pointed out that with the conversion done after arming the scopeguard, Windows would leak one CRT slot per mode change (the guard only NtClosed the HANDLE; the converted fd's close is what releases both). change_mode now converts first and arms the guard on the converted fd, closing the original HANDLE if the conversion fails. That is the whole delta from 594b974fdb; POSIX is unchanged (identity conversion) and the patch suite passes locally. Sorry for the extra sha; nothing else is pending from my side.

[claude]

The CRT-fd-slot leak I flagged on the previous head is fixed — change_mode now converts with make_libuv_owned() before arming the guard (and closes the original fd on the Err arm), so the guarded close goes through uv_fs_close and releases both the CRT slot and the HANDLE. This run found nothing further. Since the last human sign-off was on 0e8d719 and the change_mode rewrite landed after it, a quick human re-check of that delta is still worth having before merge.

What was reviewed

• change_mode fd ownership after make_libuv_owned(): guard is now on the Uv-kind fd; the Err(()) arm closes the System-kind fd, so no path leaks.
• resolve_target / open_parent_beneath walk: ParentDir::drop releases each intermediate fd; patch_dir itself is borrowed (owned: false).
• rename_file error attribution: per-side with_path on each resolve_target, with_path_dest only on renameat — matches the tests' err.path assertions.
• nt_query_attributes_at refactor in src/sys/lib.rs: pure extraction; exists_at_type_nt behavior unchanged.

Extended reasoning...

Overview

Rewrites PatchFile::apply in src/patch/lib.rs to enforce containment when applying a patch: every operation now goes through resolve_target, which validates the path lexically and walks parent components one at a time with O_DIRECTORY | O_NOFOLLOW (creating them with mkdirat when allowed), then operates on the verified (parent_fd, basename) pair. open_target_file opens the final component O_NOFOLLOW | O_NONBLOCK, requires S_ISREG and st_nlink == 1, and returns the fd used for both read and write-back. src/sys/lib.rs gains get_file_attributes_at (a small refactor of the existing NtQueryAttributesFile body) for the Windows reparse-point check. ~600 lines of new tests cover symlink/junction/hardlink/FIFO/NUL/drive-relative escapes plus the fixed directory-mode bug; two snapshot files are updated for the new path-bearing error messages.

Security risks

This is defense-in-depth for the patch applier itself. The only production caller today applies to a fresh copyfile'd temp directory, so the escapes are not reachable from bun install; the change hardens the applier's contract. The Windows containment is a by-name attribute check before each open (documented as non-atomic vs. concurrent swap); on POSIX the walk is atomic per component. I did not find any way for the new code to widen the pre-PR attack surface.

Level of scrutiny

High — this is security-relevant path-containment logic in native code with per-platform branches, and it substantially rewrites five operation paths. It has already had multiple human review rounds; the last human sign-off was on 0e8d719. Since then the branch was rebased and change_mode was reworked to use make_libuv_owned() + fchmod on every platform (main removed Windows fchmodat), and then reworked once more to fix the CRT-slot leak I flagged on the intermediate head. Those two post-sign-off deltas to change_mode are the part a maintainer has not yet re-confirmed.

Other factors

The bug-hunting pass on the current head found nothing. My earlier inline finding (Windows CRT fd slot leak in change_mode) is addressed exactly as suggested — the guard is now armed on the post-conversion fd. All prior review threads (rename error attribution, stale Windows comments, hard-link mode-change test assertion) are resolved in the current diff. The one open comment-cop note on line 150 targets a two-line comment and looks like a heuristic false positive. Given the security-sensitive scope and the pending human re-check of the post-rebase delta, deferring rather than approving.

[Jarred-Sumner]

This is Claude (an AI assistant) writing on Jarred's behalf.

Included in #44968.