Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPath resolution now has a POSIX-strict mode that preserves backslashes as literal bytes. That mode is exposed through a new ChangesPOSIX-strict realpath resolution
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:09 AM PT - Jul 6th, 2026
❌ @robobun, your commit bf3de47 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33410That installs a local version of the PR into your bun-33410 --bun |
There was a problem hiding this comment.
No bugs found, but the change to normalize_string_buf_t's Platform::Posix arm affects every platform::Auto/platform::Posix normalization on POSIX hosts (bundler, resolver, install, shell — 100+ call sites), so a maintainer should confirm the blast radius is acceptable.
Extended reasoning...
Overview
This PR fixes #33403: fs.realpath on POSIX was rewriting literal \ bytes to / before hitting the filesystem, causing spurious ENOENT on paths containing backslashes. The fix has two layers: (1) a new normalize_string_posix_buf_t that splits on / only, wired into normalize_string_buf_t's Platform::Posix arm on non-Windows hosts; (2) a new FileSystem::abs_buf_checked_posix used from realpath_inner in node_fs.rs. A test in fs.test.ts covers sync/native/callback/promise realpath variants.
Security risks
None identified. The change tightens path handling to match POSIX semantics rather than loosening it, and doesn't touch auth, crypto, or trust boundaries.
Level of scrutiny
High. The node_fs.rs and resolver/lib.rs changes are narrowly scoped, but the resolve_path.rs change alters the behavior of normalize_string_buf_t for Platform::Posix — and on POSIX hosts platform::Auto resolves to Posix. That routine backs normalize_buf, relative_platform_buf, _join_abs_string_buf, and transitively ~100+ call sites across the bundler, package manager, resolver, shell builtins, bake dev server, glob walker, etc. The delta is only observable for inputs containing \, which is rare on POSIX and semantically correct — but any caller that was (perhaps accidentally) relying on \→/ normalization for cross-platform module specifiers or config paths will now see different output. The author explicitly kept abs_buf_checked on Loose for module specifiers and preserved the loose behavior on Windows hosts via cfg!(windows), which shows awareness of this, but the broader Platform::Posix change still reaches many subsystems the targeted test runs (fs, path, resolve, bundler_edgecase) don't fully cover.
Other factors
The reasoning in the PR description is thorough and the fix is architecturally sound (aligning Platform::Posix with its other separator predicates). The test is well-constructed. CI (#68705) is still building at time of review. Given this is a foundational path helper touching many critical subsystems, a maintainer should sign off on whether the normalize_string_buf_t change is the right layer for the fix vs. keeping it scoped to realpath_inner only.
|
Thanks, the scope of the Why not scope it to The delta is bounded to backslash inputs. Module specifiers are untouched. The resolver's specifier joins go through Windows is untouched. A few Windows callers reach the Suites exercised on Linux: If you would rather keep the blast radius at zero and absorb the duplication, I can move the posix resolve into a realpath-local helper instead. Happy to go either way. |
On POSIX a backslash is an ordinary filename byte, but realpath resolved its input through a join that treated `\` as a separator and rewrote it to `/`. realpathSync and realpathSync.native then ENOENT on a directory that existsSync and lstatSync resolve in the same process. realpath now resolves with strict POSIX separator semantics via a new abs_buf_checked_posix / join_abs_string_buf_checked_posix that splits on `/` only, so backslashes survive. This is threaded through the existing join via a STRICT_POSIX flag; the shared normalizer is unchanged, so callers that intentionally fold `\` to `/` (module specifiers on Loose, npm lockfile workspace paths) keep their behavior.
6ca6d08 to
f0e858f
Compare
|
Update: CI confirmed the blast-radius concern was real. The broad I reworked it to the scoped approach. The shared normalizer is reverted (unchanged from main), and realpath now gets strict POSIX separator semantics through a Verified on Linux: |
|
CI status for 3ab9cc4: green on every lane that exercises this change (282 jobs passed). The red lanes are unrelated to the diff, which only touches realpath / POSIX path joining:
The realpath fix and its regression test pass on the Linux lanes, and the earlier npm-lockfile-migration regression is resolved ( Update: re-triggered on a clean commit (build 68783) and the Build 68783 also showed a segfault on |
|
You can do it @robobun !!! Just find the relationship between words and find the Next TOKEN!!!! That's all you doing! YOU GO ROBOBUN!! |
|
Closing in favor of #42374. That PR makes POSIX I checked a debug build of main plus #42374 on Linux. The repro from #33403 returns #42965 covers the module resolver side of the same problem. |
What
On POSIX a backslash is an ordinary filename character, but
fs.realpathSyncandfs.realpathSync.nativerewrote\to/before touching the filesystem, so they threwENOENTon a directory thatexistsSync/lstatSyncresolve fine in the same process. Fixes #33403.Node v22+ returns
/tmp/back\slash.path.normalize/resolve/joinalready preserved the backslash; only realpath was affected.Cause
realpath_inner(non-Windows) resolves the input throughFileSystem::abs_buf_checked, which joins withplatform::Loose. The join's normalizer (normalize_string_buf_t) routedPlatform::Posixthrough the loose normalizer, whose separator predicate isis_sep_any(treats both/and\as separators). Soback\slashwas split and re-joined asback/slash, and the lateropenattargeted the wrong path:Every other
Platform::Posixseparator predicate (is_separator,get_separator_func,is_absolute,leading_separator_index) already treats\as a literal byte. The normalizer was the single inconsistent spot.Fix
Scoped to realpath so no other path consumer shifts behavior:
normalize_string_posix_buf_t, a POSIX normalizer that splits on/only (vs the loose normalizer'sis_sep_any, which also splits on\).STRICT_POSIXconst flag through_join_abs_string_buf; when set, the final normalization uses the POSIX normalizer. Every existing caller passesfalse, so their output is byte-identical.join_abs_string_buf_checked_posix/FileSystem::abs_buf_checked_posix, and call that fromrealpath_inner. The sharednormalize_string_buf_tandabs_buf_checked(Loose) are untouched, so module-specifier resolution and npm lockfile workspace-path folding (both of which intentionally treat\as a separator) keep their behavior.An earlier revision changed
normalize_string_buf_t'sPlatform::Posixarm directly; CI showed that broke npm lockfile migration (packages\pkg1workspace entries), so the fix was narrowed to realpath only.Error shape
The issue also noted the thrown error reported
syscall: "lstat". That was a side effect of the wrong path. For a genuinely missing path Bun already matches Node (lstatforfs.realpath,realpathfor.native, originalpath), so no error-shape change was needed.Test
test/js/node/fs/fs.test.tscreates a directory with a literal backslash in its name and assertsrealpathSync,realpathSync.native, the promisified callback forms, andpromises.realpathall return the path with the backslash intact (POSIX only;\is a separator on Windows).Verification
USE_SYSTEM_BUN=1):realpathSyncthrowsENOENT.bun bd).test/js/node/fs/fs.test.ts(353 pass),test/cli/install/migration/migrate.test.ts(20 pass),test/js/node/path/(122 pass),test/js/bun/resolve/{resolve,require}.test.ts(53 pass),test/bundler/bundler_edgecase.test.ts(107 pass).