fetch: reject a FormData body whose Bun.file() cannot be read with a TypeError - #37514
fetch: reject a FormData body whose Bun.file() cannot be read with a TypeError#37514robobun wants to merge 1 commit into
Conversation
…TypeError
Blob::from_dom_form_data threw the read failure as a plain Error while
serializing the multipart body, so fetch(url, { body: formData }) rejected
with that Error even though every other fetch() network error has been a
TypeError since #35855. It now returns the bun_sys::Error to its caller:
Request/Response construction keeps throwing it as the plain system error,
and HTTPRequestBody::from_js (fetch) materializes it as a TypeError that
still carries code, errno, syscall and path.
|
Warning Review limit reached
Next review available in: 4 seconds 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 (4)
Comment |
|
Status Reproduced on the 1.4.0 release and on main ( Fix is in this PR. The three new Related: #37501 covers the bare |
There was a problem hiding this comment.
LGTM — narrow error-class fix that brings the FormData-body fetch() rejection in line with #35855's TypeError convention.
What was reviewed:
from_dom_form_datanow returnsResultinstead of throwing in-place; both callers (Body.rs, FetchTasklet.rs) are updated, no others exist.Value::from_jscallers (Request ctor, Response ctor viabody::extract, HTMLRewriter) already handleErr— the switch fromOk(empty) + pending exceptiontoErris behavior-preserving there.HTTPRequestBody::from_jssuccess path returnsAnyBlob::Blob, identical to what the oldBodyValue::from_js→use_as_any_blob()fallthrough produced.SysErrorJsc::throw=to_system_error().to_error_instance()+throw_value, matching the oldon_entrybehavior byte-for-byte for the constructor path.
Extended reasoning...
Overview
Changes Blob::from_dom_form_data to return Result<Blob, bun_sys::Error> rather than throwing on the global and returning an empty blob. FormDataContext swaps its failed: bool + global_this fields for a single read_error: Option<bun_sys::Error>; the joiner cleanup on failure is unchanged. The two callers then choose the error class: Body::Value::from_js (Request/Response constructors, HTMLRewriter) throws it via SysErrorJsc::throw — the same plain Error as before — while HTTPRequestBody::from_js (both fetch() call forms) intercepts FormData ahead of BodyValue::from_js and throws it via SystemError::to_type_error_instance.
Security risks
None. This changes only which JS error prototype a rejection is constructed with; no new input parsing, no allocation-size arithmetic, no auth/crypto surface.
Level of scrutiny
Low-to-medium. The refactor from throw-in-helper to return-error-let-caller-decide is mechanical; the one design choice (TypeError for fetch, plain Error for constructors) is well-argued against the fetch spec, Node behavior, and Bun's own #35855 precedent, and the sibling PR #37501 makes the same split for the bare Bun.file() body. I traced every caller of from_dom_form_data (exactly two) and every caller of BodyValue::from_js / body::extract to confirm the Ok(empty)+pending-exception → Err change is behavior-preserving — Request.rs handles both via bail!, Response/HTMLRewriter via ?.
Other factors
Four new tests cover the variant matrix (both fetch overloads, ENOENT from open vs EISDIR from read, and a negative-contract test pinning constructor behavior). The success path in HTTPRequestBody::from_js short-circuits to AnyBlob::Blob(blob), which is exactly what the old Value::Blob → use_as_any_blob() fallthrough produced. The removed global_this field on FormDataContext had no other reader. No prior human review comments to address.
|
Updated 6:59 AM PT - Aug 11th, 2026
✅ @robobun, your commit 0e4fcc24cd76883bc2c7a2de3b32cae2f5fb1309 passed in 🧪 To try this PR locally: bunx bun-pr 37514That installs a local version of the PR into your bun-37514 --bun |
What does this PR do?
fetch()with aFormDatabody that holds aBun.file()which cannot be read rejects with a plainError. Since #35855 the network errorsfetch()produces areTypeErrors carrying the system error fields; this rejection was left out:The same applies to a directory entry (
EISDIRfrom the read) and to the object formfetch({ url, body }).Cause:
Blob::from_dom_form_data(src/runtime/webcore/Blob.rs) serializes the multipart body eagerly. When the read of a file entry failed,FormDataContext::on_entrythrew thebun_sys::Erroron the global right there withto_js(), i.e.SystemError::to_error_instance, a plainError.fetch_implthen saw the pending exception andreject_on_exceptionrejected with whatever had been thrown. The serializer does not know whether it is servingfetch()or aRequest/Responseconstructor, and those two callers want different things.Fix:
from_dom_form_datareturnsResult<Blob, bun_sys::Error>instead of throwing (FormDataContextrecords the first read failure in place of thefailedflag; the joiner cleanup on failure is unchanged). Its two callers pick the error class:Body::Value::from_js(new Request(),new Response(),HTMLRewriter.transform) throws it as the same plain system error as before. It now does so by returningErrrather thanOk(empty blob)with the exception left pending, which is what its callers already handle.HTTPRequestBody::from_js(src/runtime/webcore/fetch/FetchTasklet.rs, the body extraction bothfetch(url, { body })andfetch({ url, body })go through) handlesFormDatabefore delegating toValue::from_jsand materializes the failure withSystemError::to_type_error_instance, the binding fetch: mark body disturbed when a reader fails; reject network errors as TypeError #35855 added. The two builders share one body inbindings.cpp, socode,errno,syscall,pathand the message are identical; only the prototype differs.Why this split
The fetch spec turns a request body that fails to be read into a network error, and
fetch()rejects every network error with aTypeError; Node rejects the equivalent (a body over a missing file) withTypeError: fetch failed(causeENOENT). Bun's shape for this since #35855 is the system error fields directly on theTypeError, which is what this produces, soerr instanceof TypeErrornow identifies afetch()failure whether the body or the connection failed.The constructors are deliberately unchanged. The spec gives them no reason to throw at all (the eager read is a Bun choice), and everywhere else in Bun a file-backed body that cannot be read surfaces as the plain system error (
Bun.file(p).text()andnew Response(Bun.file(p)).text()both reject withError { code: "ENOENT" }); Node likewise hands back the underlying error when a body is consumed outsidefetch(). Changing their class would widen the change without a spec or Node argument behind it.test/js/web/html/FormData.test.ts("doesnt crash when file is missing") andFormData-file-error-leakkeep exercising that path and pass unchanged.#37501 does the same for a bare
body: Bun.file()infetch.rsand explicitly leaves thisFormDataroute out; the two are independent (both add tests at the end offetch-file-upload.test.ts, so whichever lands second needs a trivial rebase). #35792 (streamingFormDatauploads) keeps unreadable files on this buffered path, so this still applies there.How did you verify your code works?
New
describeblock intest/js/bun/http/fetch-file-upload.test.ts:fetch(url, { body })with a string entry, a readable file and a missing file: already rejected whenfetch()returns (Bun.peek.status),instanceof TypeError,{ code: "ENOENT", syscall: "open", path, errno }, message still names the filefetch({ url, body }): sameTypeErrorwith{ code: "EISDIR", syscall: "read" }, the failure coming out of the read rather than the open (skipped on Windows)new Response(body)andnew Request(url, { body })still throw synchronously, still a plainError(not aTypeError) with the same fieldsThe first three fail on the current release (
Expected constructor: TypeError, received the plainErrorwith the same fields) and pass with this change; the fourth pins the unchanged side. Also run with the debug build: the rest of this file,test/js/web/html/FormData*.test.ts(including the file-error leak test),test/js/web/fetch/{body,body-mixin-errors,body-clone,content-length}tests,test/js/bun/http/form-data-set-append.test.js, the HTMLRewriter suite, andcargo clippy -p bun_runtime.