formData(): read the body before the MIME type check rejects

Draft
RO
2d ago
+309-41

Problem

  • Fetch reads the whole body before formData() looks at the type, so a formData() that rejects on the type has used the body up. WPT asserts it (fetch/api/response/response-consume.html, checkBodyFormDataError). Bun leaves bodyUsed false.
  • Cause: get_form_data (src/runtime/webcore/Body.rs) checks the type before it reads anything.

Fix

  • formData() reads the body first, then rejects with the same ERR_FORMDATA_PARSE_ERROR. bodyUsed is then true for every body that is not null.
  • The new readableStreamConsumeThenReject drains a stream body. A body still arriving waits for its end.
  • Verified: test/js/web/fetch/body.test.ts (47 new cases, 46 fail on main). Mirrored WPT assertions: 15 of 15 (1.4.3: 6).
  • Self-reviewed: 9 concerns raised, 7 addressed (notes).

Background

  • This is a breaking conformance change and needs a maintainer's yes. Merged now, it ships in 1.4.4. #35855 and #33129, the same kind of change, shipped in 1.4 (#28792).
  • The 1.4.3 locked stream has its own fix, #44999, with no behaviour change.
  • Migration: call x.clone().formData(), or test the Content-Type before the call.

Downsides

  • Breaking: try { await r.formData() } catch { await r.json() } works on every Bun release. Here r.json() rejects with ERR_BODY_ALREADY_USED, also in Bun's node-fetch.
  • A wrong-type upload is read whole before the handler can answer: 64 MB took 426 ms and +67 MB RSS (main: 0). Size: +768 bytes of text.
  • Needs other PRs first (notes): a forward after a swallowed failure sends 0 bytes, and a typed Blob body loses its Content-Type.
Notes
  • Label: breaking.
  • Mirror of the WPT assertions (response-consume.html, response-consume-empty.any.js, request-consume-empty.any.js), 15 cases: node v26.3.0 passes 15, the 1.4.3 line passes 6, this branch passes 15. The 9 that 1.4.3 fails are the checkBodyFormDataError cases. For the stream error shape (response-error-from-stream.any.js): new Response(erroringStream).formData() rejects with ERR_FORMDATA_PARSE_ERROR on 1.4.3, and with the stream's own error in node and here.
  • Open, the WPT files: they are not vendored in this PR. The repo's WPT runners take .any.js files and response-consume.html is a page, so it needs a driver of its own. The cases are in body.test.ts.
  • Open, node-fetch: Bun's built-in node-fetch is the native Response, so a failed formData() uses the body up there too. node-fetch 3.3.2 checks the type before it reads (toFormData throws first), so its body stays unused. A maintainer must choose which one Bun's node-fetch follows.
  • Needs #44332 or #35858 first: try { await req.formData() } catch {} followed by fetch(new Request(upstream, req)) forwards 0 bytes with status 200 on this branch. 1.4.3 forwards the body. Node throws, because new Request() refuses a used body, and that is what those two PRs add.
  • Needs #45601 to keep the type: after a failed formData() on new Response(new Blob(["x"], { type: "text/plain" })), headers.get("content-type") is null here. 1.4.3 and node keep it. The type lives only on the Blob until .headers is read, and the Blob is gone once the body is used. The other readers (text(), json()) lose it the same way on main.
  • The read cost, measured: a handler that calls req.formData() with no type check, and a 64 MB application/octet-stream upload. 1.4.3 answers 415 after 2 MB were sent, in 0 ms. This branch answers after all 64 MB, in 426 ms, with +67 MB RSS. The drain buffers the body as the other readers do.
  • The break, measured: try { data = await r.formData() } catch { data = await r.json() } on a JSON request prints {"a":1} on 1.4.3 and json() threw TypeError here. A search of public code for that exact fallback found one use. Most code reads the Content-Type first. Node and browsers already fail that fallback.
  • This revives #35847, which closed without a maintainer comment. #35847 passed false as the content type to mean "drain, then reject". That sentinel is reachable from user code through Bun.readableStreamToFormData(stream, false). Here the drain is its own entry point and its own reaction handler (onReadableStreamConsumedThenReject), and Action::GetFormData(None) carries "not form data" on the Rust side.
  • The three paths of get_form_data after the change:
    • A body behind a stream (Locked): set_promise with Action::GetFormData(None). A stream is drained by readableStreamConsumeThenReject. A body that is still arriving rejects in Value::resolve when its last chunk is in.
    • An in-memory body: it becomes Used, and the call rejects.
    • A null body: there is nothing to read, so it stays as it was. bodyUsed is false for a null body after every reader.
  • This does not bring back 1.4.2. There the stream stayed unlocked, and for an in-memory body it was empty.
  • #44999 changes the same lines of get_form_data. Whichever lands second is merged onto the other. #44930 fixes the same lift in Bun.serve routes and does not depend on this PR.
  • Self-review concerns: the text sold this as the 1.4.3 regression fix (rewritten, and #44999 opened for the regression), no breaking label or target release (added), no migration (added), the read cost was not stated (measured above), the forward of 0 bytes (stated, with the PRs it needs), the lost Content-Type (stated, with the PRs it needs), the stream error case was not claimed (added). Open: node-fetch, and the WPT files.
  • Suites run on the debug build: body (823 pass on the release build. On the debug build the four "string body consumption does not leak" cases time out at 5 s, as they do without this change), body-clone, body-mixin-errors, FormData, streams.
  • One commit is not about this change. In the same test file, the two json cases of "string body consumption does not leak" need about 5.5 s on a debug build against the 5 s default timeout, with or without this PR. They do fewer reads on isDebug builds now. Release and ASAN builds run them as before.