fetch: reject an unreadable Bun.file() request body with a TypeError - #37501
fetch: reject an unreadable Bun.file() request body with a TypeError#37501robobun wants to merge 4 commits into
Conversation
The open and read failures for a Bun.file() request body were built with SystemError::to_error_instance, so they rejected as a plain Error while every network error fetch() produces is a TypeError carrying the same system error fields. Build them with to_type_error_instance instead; code, errno, syscall and path are unchanged.
|
Updated 6:11 AM PT - Aug 11th, 2026
✅ @robobun, your commit 20abf1187942bfa3b5491844484841de96b26f27 passed in 🧪 To try this PR locally: bunx bun-pr 37501That installs a local version of the PR into your bun-37501 --bun |
|
Status Reproduced on the 1.4.0 release with Fix is in this PR (#37501): the two rejection sites in the |
|
Warning Review limit reached
Next review available in: 21 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)
WalkthroughThe fetch implementation now converts file-body open and read failures into ChangesFetch file-body error handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
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 `@test/js/bun/http/fetch-file-upload.test.ts`:
- Around line 260-297: Strengthen the assertions in the four fetch error tests
so each verifies all retained system error fields: include errno alongside code,
syscall, and path where applicable, and assert syscall plus fd for the invalid
descriptor case. Preserve platform-specific errno differences by checking the
expected Windows value separately from POSIX EBADF, while still requiring the
error to be a TypeError on every platform.
🪄 Autofix
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: 74413441-a90b-4b6d-a9fb-8c966f4c42ed
📒 Files selected for processing (2)
src/runtime/webcore/fetch.rstest/js/bun/http/fetch-file-upload.test.ts
There was a problem hiding this comment.
LGTM — the two file-body rejection sites now build the same TypeError-with-system-fields shape as FetchTasklet::on_reject, and only the error prototype changes.
What was reviewed
- Confirmed
SystemError::to_type_error_instanceis the same bindingValueError::SystemTypeErroruses for network errors, and that the removedSysErrorJscimport has no remaining uses infetch.rs. - Checked the open-failure early return: the missing
body.detach()there is pre-existing and unchanged by this diff. - Tests cover open/read/dup failures and the
Request-body variant, pin synchronous rejection viaBun.peek.status, and assert the retainedcode/errno/syscall/path/fdfields (Windowsduperrno intentionally left unpinned).
Extended reasoning...
Overview
Two call sites in the needs_to_read_file() block of src/runtime/webcore/fetch.rs — the open/dup failure and the read_file failure — swap err.to_js(global_this) (plain Error) for a small helper request_body_file_error that produces jsc::SystemError::from(err.to_system_error()).to_type_error_instance(global_this). The now-unused SysErrorJsc trait import is dropped. A new describe.concurrent block in test/js/bun/http/fetch-file-upload.test.ts covers ENOENT (direct and via Request), EISDIR, and EBADF, each asserting instanceof TypeError, synchronous rejection (Bun.peek.status), and the retained system-error fields.
Security risks
None. This changes only which JS error prototype the rejection uses; the error data (code, errno, syscall, path/fd, message) is identical to before. No new inputs are parsed and no control flow around fd ownership or cleanup is altered.
Level of scrutiny
Low-to-medium. The Rust diff is a two-line substitution plus a three-line helper and an import cleanup; the surrounding rejection/cleanup logic is untouched. I verified to_type_error_instance exists on jsc::SystemError (src/jsc/SystemError.rs:91) and is exactly what ValueError::SystemTypeError routes through in Body.rs, so the helper matches the sibling network-error shape the doc comment references. A grep confirms no remaining SysErrorJsc / .to_js(global uses in fetch.rs, so the import removal is safe.
Other factors
All prior review threads are resolved: the comment-cop feedback was addressed by trimming the helper's doc comment to one line, and the CodeRabbit request to strengthen assertions was applied in 6bb2428 (errno on all four cases, fd on POSIX for the descriptor case; Windows errno intentionally not pinned per the noted dup() quirk). The bug-hunting system found nothing; the one candidate it examined (missing body.detach() on the open-failure path) is pre-existing and out of scope — this diff only changes the value passed to the rejected promise, not the cleanup around it. Tests use tempDir, 127.0.0.1:1, and describe.concurrent, so they are hermetic and fast.
What does this PR do?
fetch()with aBun.file()request body that cannot be read rejects with a plainError. Since #35855 the network errorsfetch()produces areTypeErrors that carry the system error fields, and this rejection is the one remaining system error on the request side that was left out:The same applies to a directory as the body (
EISDIRfrom the read), a descriptor that is not open (EBADFfrom the dup), and aRequestwhose body is theBun.file().Cause: in the
needs_to_read_file()block ofsrc/runtime/webcore/fetch.rs, both the open/dup failure and theread_filefailure converted thebun_sys::Errorwithto_js(), which isSystemError::to_error_instance, i.e. a plainError.Fix: both sites now go through a small helper that builds the same
SystemErrorwithto_type_error_instance(the binding #35855 added, and whatValueError::SystemTypeErroruses for the network errors fromFetchTasklet::on_reject).code,errno,syscall,path/fdand the message are unchanged; only the prototype differs. TheSysErrorJscimport goes away because these were its only uses in the file.Why a TypeError
The fetch spec turns a request body that fails to be read into a network error (HTTP-network fetch,
processBodyError), andfetch()rejects every network error with aTypeError. Node does the same for the equivalent input: a body stream over a missing file rejects withTypeError: fetch failed(causeENOENT). Bun's convention since #35855 is to put the system error fields directly on theTypeErrorrather than undercause, and that is the shape this change produces, soerr instanceof TypeErrornow identifies all offetch()'s own failures regardless of whether the body or the connection failed.Out of scope and unchanged here: a
FormDatabody holding an unreadableBun.file()(a body extraction error shared with theRequest/Responseconstructors) anddata:URL failures still reject with a plainError; those are separate code paths. The rejected promise is still created the same way as before, so this composes with #37434, which changes how these early rejections are reported but not their class.How did you verify your code works?
New
describeblock intest/js/bun/http/fetch-file-upload.test.tscovering the open failure (missing file, also via aRequest), the read failure (directory) and the dup failure (descriptor that is not open). Each test checks that the promise comes back already rejected (Bun.peek.status), that the error isinstanceof TypeError, and that the system error fields are still there. All four fail on the current release (Expected constructor: TypeError, received the plainErrorwith the same fields) and pass with this change; the rest of the file passes as well.cargo clippy -p bun_runtimeis clean.