Conversation
…ytes instead of aborting join_abs_string / join_abs_string_z wrote their normalized result into a fixed 4096-byte threadlocal buffer with no bound check. An HTML entrypoint with a rooted <script src="/..."> is re-based onto the project root through that primitive, so an attacker-authored src >= 4096 bytes aborted the process with 'range end index ... out of range for slice of length 4095' instead of producing a resolution error. The same primitive backs bundler 'external' path normalization. Spill the output buffer to a threadlocal Vec when cwd + parts exceed 4096, matching the existing join_spill pattern. Also guard load_as_file's extension-probing copy so the resolver returns not-found for paths that already exceed its PathBuffer instead of panicking on the slice bound.
WalkthroughChangesThe PR adds spill-buffer support for oversized absolute path joins, tightens resolver and bundler buffer-boundary checks, and adds a concurrent build regression test for long HTML script paths. Path Length Safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:27 PM PT - Jul 25th, 2026
❌ @robobun, your commit 399176e has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35860That installs a local version of the PR into your bun-35860 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Overlap with the two linked PRs, for whoever reviews:
This PR is the only one of the three that tests the HTML-entrypoint panic end to end (rooted Happy to rebase and drop my |
|
This overlaps with #35861 (same The branch |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…YTES A path in the [MAX_PATH_BYTES - ext.len(), MAX_PATH_BYTES) window passed the load_as_file guard but still panicked when load_extension sliced [0..path.len() + ext.len()], and a path of exactly MAX_PATH_BYTES passed the existing dir_info_cached check but panicked on the [..len + 1] slice in dir_info_cached_miss. Bound both, and drop the stale comment claiming PathBuffer carries a +1 sentinel slot. The HTML test now sweeps a 16-byte window around MAX_PATH_BYTES plus one well past it, and asserts stderr is empty rather than checking for a panic substring.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both earlier findings are addressed — load_extension now guards path.len() + ext.len(), dir_info_cached is tightened to >=, and the test sweeps the boundary with a positive stderr check. I didn't find anything new, but this reworks a path-join primitive used across the resolver/bundler and overlaps with #35857, so a human should coordinate which lands.
What was reviewed:
join_abs_neededupper bound vs. both_join_abs_string_bufvariants — normalization never grows the concatenation, and no in-tree caller usesplatform::Nt(the only case that would add bytes beyond the estimate).parser_join_out_bufspill mirrors the existingjoin_spill/join_z_spillpattern in the same file; the fixed-buffer fast path is unchanged.dec_ret!(None)matches sibling exit sites inload_as_file; the>=vs>split betweenload_as_fileandload_extensionis correct for their respective slice bounds.
Extended reasoning...
Overview
Three files: src/paths/resolve_path.rs adds a threadlocal Vec<u8> spill buffer so join_abs_string/join_abs_string_z heap-allocate when cwd + parts exceeds 4096 instead of panicking on the fixed buffer; src/resolver/resolver.rs adds three length guards (dir_info_cached tightened from > to >= to protect the [..len+1] slice in dir_info_cached_miss, load_as_file bails when the path won't fit the extension-probe buffer, load_extension bails when path + ext won't fit); test/bundler/bun-build-api.test.ts adds a subprocess test that sweeps a 16-byte window around MAX_PATH_BYTES plus one well past it and asserts a ResolveMessage instead of an abort.
Security risks
The bug is a user-input-reachable panic (DoS) in Bun.build via an attacker-authored HTML entrypoint. The fix converts the panic into a resolve error. No new trust boundaries are crossed; the spill buffer is a per-thread Vec that grows monotonically (same retention model as the existing join_spill helpers). No injection/auth/data-exposure surface.
Level of scrutiny
Medium-high. join_abs_string is a shared primitive used throughout the resolver, bundler external validation, and HTML scanner — the upper-bound estimate in join_abs_needed has to hold for POSIX, Loose, and Windows (_join_abs_string_buf_windows) code paths. I traced both branches and confirmed normalization never grows the concatenation and the +1-per-part / +2 slack covers inserted separators and the sentinel NUL; the only path that would exceed the estimate is Platform::Nt (adds a 4-byte \\?\ prefix), and grep confirms no caller invokes join_abs_string/_z with platform::Nt. The resolver guards are simple early-return-None checks.
Other factors
My previous review flagged the missing load_extension headroom and the negative panic-string assertion; both were fixed in a7840b9 (verified by re-reading the diff), and the comment-cop trims landed in b7a4a02. All threads are resolved. The remaining reason to defer is coordination: #35857 (opened first) carries a superset of the resolver.rs guards and #35861 was closed in favor of this PR — a maintainer should decide the merge order rather than me approving one side of an acknowledged overlap. The change to a widely-shared path primitive also merits a human sanity check even though it follows the in-file join_spill pattern closely.
…rror message On Windows, the 'Could not resolve' error message for an HTML import re-slashes the specifier via path_to_posix_buf into a pooled PathBuffer (MAX_PATH_BYTES). A rooted <script src> longer than that panicked on the slice copy. Fall back to the un-reslashed specifier when it does not fit; the error text keeps a backslash instead of aborting.
…inks abs_buf_checked can return a result of exactly buf.len(); appending the trailing separator then indexed one past the PathBuffer. Reachable via an absolute HTML <script src> containing an embedded '..' and a trailing '/' sized so the re-based path lands exactly on MAX_PATH_BYTES. The boundary test now sweeps this shape too.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/paths/resolve_path.rs`:
- Around line 1351-1354: Prevent usize overflow in both path-length checks: in
src/paths/resolve_path.rs:1351-1354, update join_abs_needed to use checked
accumulation and return the established oversized-path error on overflow; in
src/resolver/resolver.rs:5875-5878, replace unchecked length addition before
load_as_file slicing with checked_add or an equivalent subtraction-based bound
so oversized inputs cannot bypass validation or panic.
- Around line 1351-1365: Bound the size accepted by parser_join_out_buf before
the spill Vec resize, using the existing path-resolution limit or a dedicated
maximum for user-controlled inputs. Ensure oversized needed values are rejected
through the normal catchable resolution-error path rather than attempting
allocation, while preserving the existing fixed-buffer and valid spill-buffer
behavior.
- Line 16: Update join_abs_string and join_abs_string_z so their returned slices
never reference the thread-local PARSER_JOIN_INPUT_BUFFER or
PARSER_JOIN_SPILL_BUFFER. Return owned or otherwise stable storage, or require
an explicit caller copy before the result can escape, while preserving the
existing path-joining behavior.
In `@test/bundler/bun-build-api.test.ts`:
- Around line 802-809: Update the subprocess assertions around the Promise.all
result to validate the combined { stderr, exitCode } failure signal before
calling JSON.parse(stdout). Keep the existing expected stderr and exitCode
values, then parse and assert the JSON payload only after those diagnostics have
been verified.
🪄 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: fe6d1d5f-1426-4343-859f-a26b7bdca3c6
📒 Files selected for processing (4)
src/bundler/bundle_v2.rssrc/paths/resolve_path.rssrc/resolver/resolver.rstest/bundler/bun-build-api.test.ts
|
CI at 399176e (build 82121, finished): 195/196 jobs passed; The only hard failure is the The five yellow annotations ( Ready for a maintainer; see the earlier comment for the overlap with #35857 / #35861. |
There was a problem hiding this comment.
The trailing-separator write at resolver.rs:1821 and the Windows path_to_posix_buf site are now guarded — all three of my earlier findings are addressed and this pass found nothing new. Deferring to a human because join_abs_string is a shared primitive under every resolver caller, and this overlaps #35857's resolver.rs hunk.
Checked this pass: join_abs_needed's upper bound holds for both the POSIX and Windows join branches (normalization never grows the concat, absolute-part promotion is covered by summing all inputs); the spill Vec keeps the same single-live-borrow TLS contract as the fixed buffer; the bundle_v2.rs fallback is error-message-only so raw backslashes on Windows are cosmetically fine; the boundary-sweep test now covers the foo../ shape at exactly MAX_PATH_BYTES.
Extended reasoning...
Overview
Four files: src/paths/resolve_path.rs adds a threadlocal Vec<u8> spill for join_abs_string/join_abs_string_z when cwd + parts exceeds 4096, mirroring the existing join_spill pattern. src/resolver/resolver.rs adds four bounds guards (trailing-separator append in resolve_without_remapping, >= on dir_info_cached, load_as_file copy, load_extension slice) plus tightens a stale comment. src/bundler/bundle_v2.rs skips the Windows-only path_to_posix_buf when the specifier already exceeds the pool buffer (error-message rendering only). test/bundler/bun-build-api.test.ts adds a subprocess test that sweeps a 16-byte window around MAX_PATH_BYTES with both plain-rooted and foo../-shaped script src values.
Security risks
This is DoS hardening — user-controlled HTML <script src> and external: values previously reached unchecked slice writes into fixed PathBuffers, panicking and aborting the process. The fix converts those to NotFound/ResolveMessage. No new attack surface is introduced; the spill allocation is bounded by input the caller already holds in memory (no amplification), and the TLS UnsafeCell contract is unchanged from the fixed buffer it supplements.
Level of scrutiny
High. join_abs_string backs every path-join in the HTML scanner, external validator, and several resolver call sites; resolver.rs is the module-resolution hot path. Each of my two prior review passes found an additional unguarded same-class site (the load_extension window, then the resolve_without_remapping trailing-separator write), and CI independently caught the Windows bundle_v2.rs site — the author reproduced and fixed all of them, but the "fix the whole class" history warrants a human confirming the class is closed rather than a bot approving after three iterations.
Other factors
- The author flagged overlap with #35857 (a superset of the
resolver.rsguards) and #35861 (closed in favor of this PR) — a human should decide the merge order. - CodeRabbit raised four concerns and withdrew all of them after the author explained the pre-existing TLS-scratch contract, the impracticality of
usizeoverflow on 64-bit, and thestderr-first assertion order. - The PR description's evidence block notes the test is platform-gated and deferred to CI on this machine; the Windows hunk was fixed after a CI failure (b1c8c40), not local repro.
|
Two notes for whoever rebases or reviews this, from a second report of the same abort:
|
|
The dev server is another way to reach this abort. Measured on Linux x64 with stock // index.html: <script type="module" src="/aaa...(5,000 bytes).js"></script>
import page from "./index.html";
const s = Bun.serve({ port: 0, hostname: "127.0.0.1", development: true, routes: { "/": page } });
await fetch(`http://127.0.0.1:${s.port}/`);The first Update: #42806 now bounds both joins in |
|
Closing in favor of #43067. It fixes this trigger with the shared checked path helpers and carries the tests from this pull request. |
2 participants
Reproduction
Cause
HTMLScanner::create_import_recordre-bases a rooted<script src="/...">onto the project root viaresolve_path::join_abs_string. That primitive writes its normalized result into a fixed 4096-byte threadlocal (PARSER_JOIN_INPUT_BUFFER) with no bound check, so a src attribute of 4096 bytes or more panics on the slice copy and aborts the process. The same primitive backsvalidate_pathfor the bundlerexternaloption, which aborted on the same input.Once the path survives the join, the resolver's
load_as_filecopies it into anotherPathBuffer-sized threadlocal for extension probing with the same unchecked slice bound.Fix
join_abs_string/join_abs_string_znow size-checkcwd + partsup front and spill the output buffer to a threadlocalVec<u8>when it exceeds 4096, mirroring the existingjoin_spill/join_z_spillpattern. The fast path (fits in 4096) is unchanged.Resolver::load_as_filereturns not-found for paths that already exceed the extension-probe buffer instead of panicking on the copy; a path that long cannot name a file the OS would open.Verification
panic: range end index 5017 out of range for slice of length 4095Bun.buildresolves withsuccess: falseand aResolveMessage("Could not resolve: ...")external: ["./" + "a".repeat(5000) + ".js"](same primitive) also survives nowbun-build-api.test.ts,bundler_html.test.ts,bundler_edgecase.test.ts,test/js/bun/resolve/resolve.test.ts,test/js/node/pathall greenno test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts