Skip to content

formData(): check the Content-Type before the lift that spends the .body stream - #44999

Open
robobun wants to merge 4 commits into
mainfrom
robobun/818c1ddd/formdata-type-before-lift
Open

robobun wants to merge 4 commits into
mainfrom
robobun/818c1ddd/formdata-type-before-lift

Conversation

@robobun

@robobun robobun commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • const s = r.body; await r.formData() on a body that is not form data rejects and leaves s locked while r.bodyUsed is false. s.getReader() throws ReadableStream is locked, and Bun.write(path, r) fails with ERR_BODY_ALREADY_USED.
  • Cause: get_form_data (src/runtime/webcore/Body.rs) calls to_blob_if_possible() before it looks at the Content-Type. Since Close a body's stream when a consumer takes its payload without reading it #42516 that lift closes and locks the stream.

Fix

Background

  • Reading .body makes a body Locked: its payload sits behind a native stream. A lift takes the payload out as a Blob and spends the stream.
  • A FormData or URLSearchParams body carries its type on its Blob, not in a header.
  • Considered reading the body first, as Fetch does: that is formData(): read the body before the MIME type check rejects #44969, and it is breaking. Here a failed formData() leaves the body as it was, as in every Bun release.

Downsides

  • None found for a body whose .body was not read: it takes the same path as before. Checked the body, FormData, body-clone and body-stream suites.
  • Size: +512 bytes of text (release, linux x64, size).
Notes
  • The reduced program from the report, on the released 1.4.3 line and on this branch. const r = new Response(Bun.file(p)), s = r.body, 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: bodyUsed false, s.locked true, four uses throw and r.text() gives the file.
    • This branch: bodyUsed false, s.locked false, all five give the whole file. The same holds for a string body.
  • 1.4.2 also left the stream unlocked, but for an in-memory body that stream was empty. Here the stream keeps every byte.
  • The order of the checks for a stream-backed body does not change: an unusable stream rejects first, then a body that already has a reader, then the type. Only the lift moved behind the type check.
  • A Content-Type header still wins over the body's own type. new Response(new URLSearchParams("a=1"), { headers: { "content-type": "application/json" } }) rejects with or without a .body read.
  • An empty Content-Type header counts as no header, as it does in get_content_type: the check uses the same fast_get call.
  • Each new clause has a test that fails without it, checked on a local build: without the header check the four URLSearchParams cases fail, without the read of the stream's Blob type the two "still parses after .body" cases fail, and with fast_has in place of fast_get the empty-header case fails.
  • What the two reviews said about this shape: keep the type fallback inside formData() (Response::get_content_type is also the MIME check of WebAssembly.compileStreaming and does not change here), say in the code that Fetch reads first, and keep the tests out of the describe that pins what undici does.
  • This PR does not depend on Bun.serve: share a route's payload with the Response's unread .body stream #44930 (the same lift in Bun.serve routes). The two touch different functions.
  • If formData(): read the body before the MIME type check rejects #44969 is wanted later, it replaces the end state of this PR: the type is still decided first, then the body is read to its end.
  • Suites run on the debug build: FormData (183 pass with the new cases), body (808 pass. The two "string body consumption does not leak" json cases time out at 5 s on the debug build, as they do without this change. The release build passes all), body-clone (100), body-stream (9086), body-mixin-errors (23), request (19), response (22 pass. "handle stack overflow" fails on the debug build, as it does without this change).
  • Keep the body's Content-Type on Response and Request after the body is consumed #45601 (open) captures the body's type when a Request or Response is made. If it lands first, get_content_type sees that type also for a body behind a stream. Then the read of the stream's Blob type in this PR is not needed, and this PR reduces to the move of the lift behind the type check.

@robobun

robobun commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:08 PM PT - Oct 11th, 2026

✅ @robobun, your commit be1f3a208af21f110fa4ee8e9b7dae5011bf52ae passed in Build #124712! 🎉


🧪   To try this PR locally:

bunx bun-pr 44999

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

bun-44999 --bun

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Essentials
  • Run ID: f35042d9-40b1-40bc-92d9-203835367cc3
📥 Commits

Reviewing files that changed from the base of the PR and between 28caa6a and be1f3a2.

📒 Files selected for processing (1)
  • test/js/web/html/FormData.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Walkthrough

FormData parsing now checks for an encoder before lifting a locked body. If no encoder is available, parsing rejects without lifting the body. Tests cover body reuse after failed parsing and parsing when the Content-Type header is absent or empty.

Changes

FormData Body Parsing

Layer / File(s) Summary
Encoding detection and parse flow
src/runtime/webcore/Body.rs
Locked Blob-backed streams can provide an encoding. get_form_data rejects when no encoder is available, before lifting the body.
Body parsing and reuse tests
test/js/web/html/FormData.test.ts
Tests cover failed parsing without body consumption, successful parsing without a Content-Type header, and file-backed body reuse.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to be1f3

The changed parsing paths preserve the body after an invalid-type rejection, and the empty-header parsing case is consistent with the implementation. No merge-blocking issue remains from this review.

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: validating Content-Type before lifting the body stream in formData().
Description check Passed The description explains the problem, root cause, fix, behavior, tradeoffs, and verification results. It does not use the exact template headings, but it provides the required implementation and verif…
Full details: Description check

Explanation

The description explains the problem, root cause, fix, behavior, tradeoffs, and verification results. It does not use the exact template headings, but it provides the required implementation and verification information in equivalent sections.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked whether moving the lift behind the type check changes the unusable-stream / already-has-reader ordering for a Locked body — it does not, those two checks still run first in the same order; and whether the single remaining to_blob_if_possible() after an encoder is obtained can miss a Locked body — it is gated on matches!(value, Value::Locked(_)) and runs before the existing Locked branch, so the parse path is unchanged for form-data bodies.

Extended reasoning...

The change is confined to BodyMixin::get_form_data and two new helpers in src/runtime/webcore/Body.rs plus a new describe block in test/js/web/fetch/body.test.ts; it touches no auth, crypto or injection surface. Two confirmed inline findings (empty Content-Type header handled differently by fast_has vs fast_get, and the Source::Blob-only fallback missing file-backed bodies) mean a human should weigh them, so this is a findings-present ruled-out note only.

Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs
@robobun

robobun commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Reproduced on the released 1.4.3 line with the reduced program from the report, run on that build and on this branch. 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 false, s.locked false | read s: whole | s.tee(): whole | new Response(s).text(): whole | Bun.write(path, r): whole | r.text(): whole

The string-body line reads the same way.

Test: bun bd test test/js/web/html/FormData.test.ts -t "leaves the body as it was" (34 cases, 13 fail with origin/main's src/, all pass on this branch).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/runtime/webcore/Body.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/runtime/webcore/Body.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants