Skip to content

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

Draft
robobun wants to merge 7 commits into
mainfrom
robobun/818c1ddd/formdata-mime-check-consumes-body
Draft

robobun wants to merge 7 commits into
mainfrom
robobun/818c1ddd/formdata-mime-check-consumes-body

Conversation

@robobun

@robobun robobun commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

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

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 webcore: throw when new Request() copies a Request whose body is used or locked #44332 or webcore: new Request(request) transfers the body; a used or locked body throws in every copy #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 Keep the body's Content-Type on Response and Request after the body is consumed #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 webcore: consume body when formData() rejects on MIME type #35847, which closed without a maintainer comment. webcore: consume body when formData() rejects on MIME type #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.
  • formData(): check the Content-Type before the lift that spends the .body stream #44999 changes the same lines of get_form_data. Whichever lands second is merged onto the other. Bun.serve: share a route's payload with the Response's unread .body stream #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 formData(): check the Content-Type before the lift that spends the .body stream #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.

@robobun

robobun commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:35 PM PT - Oct 11th, 2026

✅ @robobun, your commit a02fcc19ca885a7c9e3ae701fb9e2b33c1b306b5 passed in Build #124682! 🎉


🧪   To try this PR locally:

bunx bun-pr 44969

That installs a local version of the PR into your bun-44969 executable, so you can run:

bun-44969 --bun

@robobun

robobun commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Reproduced on the released 1.4.3 line with two reduced programs, each run on that build and on this branch:

  1. const r = new Response(Bun.file(p)), s = r.body, then a formData() that fails on the type, then five uses (read s, s.tee(), new Response(s).text(), Bun.write(path, r), r.text()):
    1.4.3   file body, failed formData(): bodyUsed false, s.locked true | read s: throws TypeError | s.tee(): throws TypeError | new Response(s).text(): throws TypeError | Bun.write(path, r): throws TypeError | r.text(): whole
    branch  file body, failed formData(): bodyUsed true, s.locked true | read s: throws TypeError | s.tee(): throws TypeError | new Response(s).text(): throws TypeError | Bun.write(path, r): throws TypeError | r.text(): throws TypeError
    
    The string-body line reads the same way. On the branch every reader gives the answer that bodyUsed gives.
  2. The cost, try { data = await r.formData() } catch { data = await r.json() } on a JSON request:
    1.4.3   formData() rejected with TypeError, bodyUsed false, then json(): {"a":1}
    branch  formData() rejected with TypeError, bodyUsed true, then json(): "json() threw TypeError"
    

Test: bun bd test test/js/web/fetch/body.test.ts (47 new cases, 46 fail with origin/main's src/, all pass on this branch).

This is a draft. It is a breaking change and needs a maintainer's yes. #44999 fixes the locked stream of 1.4.3 with no behaviour change (a failed formData() leaves the body as it was).

@robobun

robobun commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator Author

The commit a02fcc1 in this PR (test: the string-body leak cases do fewer reads on a debug build) changes the same four cases of body.test.ts as #48196. That PR is only about those cases. If #48196 merges first, this commit conflicts with it and can be dropped here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants