node:fs: ignore trailing slashes in realpath and realpathSync - #32920
node:fs: ignore trailing slashes in realpath and realpathSync#32920robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 10:07 PM PT - Jul 17th, 2026
❌ @robobun, your commit 515a9af has some failures in 🧪 To try this PR locally: bunx bun-pr 32920That installs a local version of the PR into your bun-32920 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Not adding |
|
Updated in 515a9af: switched the trailing-separator strip to Build 75108 never compiled or tested this commit. All 32 A Rebuild of build 75108 from the Buildkite UI runs the same commit with no extra git noise. I already used my one empty Earlier CI notes (builds 65782 and 65902, pre-rebase)
|
Node's JS realpath implementation strips trailing separators before
walking path components, so realpathSync("<file>/") resolves to the
file's real path. Only realpath.native, which calls realpath(3),
reports ENOTDIR. Bun's non-native realpath passed the path straight to
open(2), which treats a trailing slash as "must be a directory" and
fails with ENOTDIR.
Strip trailing separators from the joined path in realpath_inner when
the variant is Emulated. realpath.native is unchanged.
91fe94a to
515a9af
Compare
There was a problem hiding this comment.
LGTM — my earlier comments (Windows scope, promises.realpath trade-off) are addressed in the description and tracked in #32963, and the switch to strings::without_trailing_slash per review is in.
What was reviewed:
without_trailing_slashpreserves/(loop floor ise > 1), so root and//don't collapse to empty;path_lenonly shrinks, so the NUL write stays in-bounds.- Confirmed
fs.ts:669/777routes Windowsrealpath/realpathSyncto the JS component-walker, so the#[cfg(not(windows))]scoping is complete for the callback/sync forms. - The helper also strips trailing
\\on POSIX (valid filename char) — flagged by finders, ruled out by verifiers as not a practical concern; the maintainer explicitly requested the shared helper. - Test covers
file/,file//, symlink, Buffer, dir,/, callback form, and asserts.nativestill throwsENOTDIR.
Extended reasoning...
Overview
Seven-line change in the #[cfg(not(windows))] arm of realpath_inner (src/runtime/node/node_fs.rs): when variant == RealpathVariant::Emulated, compute path_len from strings::without_trailing_slash(joined) instead of joined directly, so open() sees the path without trailing separators. Also removes the now-dead let _ = variant;. One new POSIX-gated test in test/js/node/fs/fs.test.ts.
Security risks
None. The change only shortens the path passed to open(2) by trimming trailing separators; without_trailing_slash has a floor of 1 so / is preserved, and path_len can only be ≤ the original joined.len(), so inbuf[path_len] = 0 writes within the already-populated region. No new untrusted-input parsing.
Level of scrutiny
Moderate — Node compat behavior in node:fs. The mechanism is straightforward (input trimming before a syscall), the variant gate keeps .native semantics unchanged, and the PR description exhaustively documents scope: Windows realpath/realpathSync never reach this Rust arm (verified in src/js/node/fs.ts:669,777), and the fs.promises.realpath mis-routing that this incidentally makes more permissive on POSIX is a pre-existing, independently-observable gap now tracked as #32963.
Other factors
I previously left two review comments here (Windows sibling arm; POSIX promises.realpath trade-off). Both were addressed via the Scope section and #32963, and the threads are resolved. dylan-conway then asked for the shared without_trailing_slash helper, which was applied in 515a9af. The bug-hunting system's finders raised the "backslash is a filename char on POSIX" concern about that helper three times; verifiers refuted it each time, and the switch was maintainer-requested. Test coverage is thorough (single/multiple trailing slashes, symlink, Buffer, directory, root, callback form, and the negative assertion that .native still throws ENOTDIR with syscall: 'realpath'), and the description records that the test fails on stock Bun and that Node's own test-fs-realpath* suite still passes.
What
fs.realpathSync("<regular file>/")(trailing slash) throws on Bun where Node resolves it:Node v26.3.0 prints the file's real path. Bun throws:
Node's JS
realpath/realpathSyncimplementation walks path components, so trailing separators on a regular file are a no-op. Onlyrealpath.native/realpathSync.native, which callrealpath(3), reject it withENOTDIR. Bun made the non-native variant behave like the native one.Cause
On POSIX,
realpath_inner(src/runtime/node/node_fs.rs) implements both variants asopen()+getFdPath().open(2)onfile/fails withENOTDIRbecause a trailing slash requires the last component to be a directory, so the native syscall's semantics leaked into the emulated variant. Before this change the two POSIX variants were byte-for-byte identical and only differed in thesyscalltag attached to the error.Fix
Strip trailing separators from the joined path before
open()when the variant isEmulated.realpath.nativeandrealpathSync.nativeare unchanged and still reportENOTDIR.Scope
The change is POSIX only (
#[cfg(not(windows))]). On Windows,fs.realpathSyncandfs.realpathnever reachrealpath_inner:src/js/node/fs.tsdispatches both to a JS port of Node's component-walking implementation whenprocess.platform === "win32"(lines 658 and 766), so trailing separators on that platform are handled there, not in this Rust function.One caller of the
Emulatedvariant isfs.promises.realpath, on every platform. That is a pre-existing mis-routing: Node'sfsPromises.realpathuses the same semantics asrealpath.native()and rejects this input withENOTDIRandsyscall: "realpath", while Bun wirespromises.realpathto the non-native variant (today its errors carrysyscall: "lstat"for every failure, not just this one). Because of that routing, this PR also makesfs.promises.realpath("<file>/")resolve on POSIX where Node rejects. Re-routing it is a one-line but cross-platform change with its own blast radius (it changes the errorsyscalltag everywhere, which Windows arm ofrealpath_innerpromises.realpathruns through, and the variantinternal/fs/glob.tsuses internally), so it is tracked separately in #32963 rather than bundled here. Once that lands,promises.realpathis on the native variant and its trailing-slash behavior matches Node with no further change to this code.The new test is gated
it.if(isPosix), matching the neighboring realpath tests infs.test.ts.Verification
New test in
test/js/node/fs/fs.test.tscoversfile/,file//, a symlink with a trailing slash, aBufferpath, a directory with a trailing slash,/, thefs.realpathcallback form, and assertsrealpathSync.nativestill throwsENOTDIR.USE_SYSTEM_BUN=1 bun test test/js/node/fs/fs.test.ts -t "trailing slash"fails with theENOTDIRabove.bun bd test test/js/node/fs/fs.test.ts -t realpath: 8 pass, 0 fail.test-fs-realpath.js,test-fs-realpath-native.js,test-fs-realpath-buffer-encoding.js, andtest-fs-realpath-pipe.jsall pass.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts