Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughMultipart form-data parsing now retains header values as borrowed byte slices, selectively decodes percent-encoded quotes and CRLF sequences in names and filenames, skips empty names, and updates the multipart callback contract. Tests cover parsing and round-trip serialization behavior. ChangesMultipart decoding
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:20 AM PT - Jul 11th, 2026
✅ @robobun, your commit 23cd7ad50279fd3bf7453025386014a5cb716c8f passed in 🧪 To try this PR locally: bunx bun-pr 32958That installs a local version of the PR into your bun-32958 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused spec-compliance fix with thorough test coverage.
Extended reasoning...
Overview
This PR makes Bun's multipart/form-data parser apply the WHATWG "parse a multipart/form-data name" algorithm: %22/%0D/%0A (case-insensitive hex) in part names and filenames are decoded to ", CR and LF, while all other % sequences stay literal. The serializer already escapes these characters, so this restores round-trip symmetry and matches Node/undici. To support decoded names that are no longer subslices of the input buffer, Field is refactored from offset-based bun_semver::String to plain &[u8] slices, and the unused zero_count field is dropped. Three files touched: src/runtime/webcore/FormData.rs plus two test files adding fixtures and a round-trip test.
Security risks
The parser handles untrusted multipart bodies, so decoding control characters into names is worth a second look. The decoded values land in JS FormData entry names and File.name strings — they are not written into HTTP headers, and when re-serialized the existing serializer re-escapes them. The decode is strictly limited to the three spec-mandated sequences (no general percent-decoding), so there's no new path for arbitrary byte injection. This is the spec-defined behavior and identical to undici/Node. I don't see a new security risk here.
Level of scrutiny
Medium. It's a behavior change in request-body parsing, but the implementation is small and mechanical: decode_multipart_name is a ~15-line pattern match that's easy to verify against the spec by inspection, with a zero-copy fast path when no % is present. The Field refactor is a type swap with no semantic change (the removed zero_count was never written after default init, so name.len() + zero_count == 0 was always equivalent to name.is_empty()). for_each_multipart_entry and Field have no other callers in the tree, so the signature change is fully self-contained.
Other factors
Test coverage is strong: raw-body fixtures exercise uppercase and lowercase hex, mixed sequences, and a name whose unrelated % sequences must stay literal (across Response, Request, and Blob paths via the existing fixture harness), plus a dedicated filename-decode test and a serialize→parse round-trip. No CODEOWNERS apply to these paths. The bug-hunting system found nothing. The PR description flags an upcoming trivial rebase against #30609, which is informational only.
|
The duplicate flag is correct: #31526 (opened May 28) implements the same parse-side decode and predates this PR. I missed it when I checked for existing PRs before opening this one, sorry for the duplicate. Behavior is the same in both:
Happy to close this one in favor of #31526 once that lands or gets rebased; until then this one is ready as is. |
|
Rebased onto fc44406. Two mechanical conflict resolutions across the rebases so far:
145/145 FormData tests pass on the rebased head; CI on 23cd7ad (build 71856): 284 jobs passed, 0 failed. Two CI history on the first headAcross builds 65900, 66061 and 66673, every failing job was unrelated to this diff: Buildkite artifact-download timeouts on the darwin 26 aarch64 agents (three consecutive builds), Docker Hub |
…rt names and string values (#32975) ### What The WHATWG [multipart/form-data encoding algorithm](https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart-form-data) starts by replacing every lone CR, lone LF, and CRLF in an entry's name (and in its value, when the value is not a File) with CRLF, and only then percent-encodes `"`/CR/LF in names and filenames. Bun's serializer skipped the normalization step, so lone CR and lone LF survived into the escaped output and the same logical field name left Bun looking different than it does from Node, undici, or any browser. ```js const fd = new FormData(); fd.append("a\rb", "1"); fd.append("a\nb", "2"); fd.append("a\r\nb", "3"); await fetch(url, { method: "POST", body: fd }); ``` Raw bytes on the wire (recorded by a plain `net` server): ``` node: name="a%0D%0Ab" name="a%0D%0Ab" name="a%0D%0Ab" bun: name="a%0Db" name="a%0Ab" name="a%0D%0Ab" ``` Non-File string values have the same missing step: `fd.append("v", "x\ry\nz")` serialized the value as the literal bytes `x\ry\nz` where Node emits `x\r\ny\r\nz`. Servers that key on the decoded field name see a different name from Bun than from every other client, and the lone CR / lone LF vs CRLF distinction is exactly the ambiguity that parsers which re-normalize disagree on. ### Fix `src/runtime/webcore/Blob.rs`: replace `escape_form_data_name` with `encode_form_data_component`, which takes which piece of the entry it is encoding and applies the spec's transforms for it: | component | normalize newlines to CRLF | percent-encode `"`/CR/LF | |---|---|---| | name | yes | yes | | string value | yes | no | | filename | no | yes | Filenames are deliberately left unnormalized: the spec's normalization step applies only to names and non-File values, and Node and browsers agree (a filename of `q\rw\ne` serializes as `q%0Dw%0Ae` in both Node and Bun, before and after this change). File contents are untouched. After the fix Bun's output for the repro above is byte-identical to Node's. This is the serializer half of the pair; #32958 fixes the inverse bug in the parser (the `%0D`/`%0A`/`%22` escapes were never decoded back). The two are independent. ### Test Added `normalizes lone CR and lone LF to CRLF in names and string values` to `test/js/web/html/FormData-multipart-serialization.test.ts`, alongside the existing test that already covered the (previously correct) CRLF-in-name case. It locks in the exact serialized bytes for all three newline spellings of a name, a string value containing all three, and a filename containing lone CR / lone LF. ``` $ USE_SYSTEM_BUN=1 bun test test/js/web/html/FormData-multipart-serialization.test.ts # bun 1.4.0 4 pass, 1 fail (name="a%0Db", name="a%0Ab") $ bun bd test test/js/web/html/FormData-multipart-serialization.test.ts 5 pass, 0 fail ``` Also re-ran the adjacent FormData/body suites (`FormData.test.ts`, `FormData-file-error-leak.test.ts`, `form-data-boundary-crash.test.ts`, `form-data-set-append.test.js`, `body.test.ts`, `content-length.test.js`): 486 pass, 0 fail.
300bf4e to
e3cfcb7
Compare
There was a problem hiding this comment.
Code looks correct and my earlier nit is addressed — deferring to a maintainer since this is a user-visible change to multipart parsing and #31526 implements the same fix, so someone should pick which one lands.
Extended reasoning...
Overview
This PR changes the multipart/form-data parser in src/runtime/webcore/FormData.rs to apply the WHATWG "parse a multipart/form-data name" algorithm: %22, %0D, and %0A (case-insensitive hex) in entry names and filenames are decoded to ", CR, and LF, while all other % sequences stay literal. To support names that are no longer subslices of the input, Field switches filename/content_type from offset-based bun_semver::String to plain &[u8] slices, the unused zero_count field and hand-rolled Default impl are removed, and the iterator callback signature drops the buf parameter. A new decode_multipart_name helper returns Cow<[u8]> with a fast borrowed path when no % is present. Four new tests cover raw-body decoding (both cases of hex), the literal-% negative case, filename decoding, and a serialize→parse round-trip.
Security risks
None identified. Decoded names/filenames flow into DOMFormData entries (JS strings) and Blob filenames, not into any outgoing wire bytes; the serializer already re-escapes these characters on the way out, so this closes a round-trip asymmetry rather than opening one. The decode is narrowly scoped to exactly three sequences and cannot turn an arbitrary %XX into a control byte. The Cow lifetime is sound — it lives across the single iterator call that borrows it.
Level of scrutiny
Medium-high. This is production runtime code on a hot web-standard path (Request/Response/Blob .formData()) and it changes user-visible parsing behavior: names that previously surfaced as a%22b will now surface as a"b. The change is spec-aligned and matches Node/undici, but it's still a behavior change that a maintainer should sign off on. The struct/signature refactor is fully contained (for_each_multipart_entry and Field have no callers outside this file), and the zero_count removal is safe (it was never written, only read as 0).
Other factors
- Duplicate PR: #31526 implements the same decode at a different layer and predates this PR. The author has acknowledged it and laid out the differences; a maintainer needs to decide which to land. That coordination decision alone is reason enough not to bot-approve.
- Prior feedback: My earlier 4-line-comment nit was addressed in e3cfcb7. The pre-existing header-loop early-exit I flagged was confirmed and intentionally left for a separate PR; both threads are resolved.
- CI: Author reports all completed lanes green (including ASAN); failures are infra-only.
- CODEOWNERS: None cover these paths.
- No bugs were found by the bug-hunting system this run.
e3cfcb7 to
c5c86d8
Compare
…lenames The multipart/form-data serializer escapes `"`, CR and LF in entry names and filenames as %22, %0D and %0A, but the parser handed the escaped text through verbatim. Bun could not read back the names it wrote into its own multipart output (a field appended as `a"b` came back as `a%22b`), and names/filenames produced by other clients kept the literal %XX sequences. Apply the WHATWG "parse a multipart/form-data name" algorithm when parsing: replace %0A, %0D (hex digits in either case) and %22 with LF, CR and `"` in both the entry name and the filename. Other %-sequences are left untouched. Output now matches Node. Because a decoded name is no longer a subslice of the input, Field stores plain byte slices instead of offset-based bun_semver::String values (which also could not represent short names containing null bytes); the unused zero_count field and the Default impl that existed for it are gone.
c5c86d8 to
b8b885c
Compare
There was a problem hiding this comment.
No issues found in this pass. The decode logic and Field refactor look correct and well-tested, but this is a user-visible behavior change to Request/Response/Blob.formData() parsing plus a signature change to a pub struct/fn, so leaving the merge call to a maintainer (who can also settle the #31526 overlap already discussed above).
What was reviewed:
decode_multipart_name: only%0A/%0D(either hex case) and%22are decoded, everything else falls through byte-by-byte; output length ≤ input length, so no allocation blowup on adversarial input.Cowlifetimes: bothnameandfilenameCows outlive theiterator(ctx, &name, &field)call, sofield.filename's borrow into the localfilenameCow is sound.for_each_multipart_entry/Field/zero_count: no other callers or users in-tree; the removedzero_countfield was dead (the only read wasname.len() + field.zero_count as usize == 0with nothing ever writing it).
Extended reasoning...
Overview
Touches src/runtime/webcore/FormData.rs (the multipart/form-data parser) and adds tests in test/js/web/html/FormData.test.ts and FormData-multipart-serialization.test.ts. The functional change is a new decode_multipart_name helper that reverses the serializer's %22/%0D/%0A escaping in parsed entry names and filenames, per the WHATWG multipart-form-data parse algorithm. To carry decoded (non-subslice) names, Field is refactored from offset-based bun_semver::String to plain &[u8], the unused zero_count field and hand-written Default impl are removed, and the for_each_multipart_entry iterator signature drops the buf parameter.
Security risks
The decoded names/filenames come from untrusted request bodies. The decode is strictly length-reducing (three input bytes → one output byte, or pass-through), so there's no amplification. Decoded values flow into DOMFormData entries and File.name, not into outgoing headers, so the CR/LF that reappear can't cause header injection here. The fast path (contains_char(raw, b'%')) keeps the common case zero-alloc. I don't see a new security surface.
Level of scrutiny
Medium. This is production runtime code that parses network input and changes user-visible .formData() output for any body containing %22/%0D/%0A in a name or filename. The change is spec-cited and Node-matching, but it's not a mechanical/config change — it's a behavior change to a Web API plus a small refactor of a pub struct and pub fn (both of which turn out to have only in-file consumers, verified via grep).
Other factors
- Test coverage is good: raw-body fixtures exercised through
Response,RequestandBlob; both hex cases; a fixture asserting other%sequences (%41,%25, trailing%) stay literal; and a serialize→parse round-trip. - My earlier inline comments (3-line doc-comment nit and the pre-existing Content-Type-after-CD early-exit) are both resolved; the author's decision to defer the Content-Type fix to a separate PR is reasonable since it changes an existing test's pinned behavior.
- #31526 implements the same decode; the author has acknowledged this and explained the differences in-thread. Which one lands is a maintainer call, not a code-review question.
- Prior CI head was fully green (284/284) per the author's summary; the current head only adds a doc-comment tightening and a CI retrigger on top.
Given the user-visible Web API behavior change and the open coordination question with #31526, deferring rather than shadow-approving.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/runtime/webcore/FormData.rs`:
- Around line 82-90: Add a regression test for multipart FormData parsing that
supplies a filename and content type containing embedded null bytes, then assert
the resulting Field values preserve the complete byte sequences without
truncation. Locate the existing FormData parsing/test helpers near the Field
handling and extend them with this case, covering both filename and content_type
round-tripping.
- Around line 82-84: Update the doc comment above the raw slice field to
reference the Rust type as `bun_semver::String` instead of the Zig-style
`bun.Semver.String`, preserving the existing explanation about binary bytes and
null terminators.
- Around line 414-421: Update the multipart parsing logic around the name check
to distinguish an absent name from an explicitly present empty name. Track
whether the Content-Disposition parameter exists separately, and skip only parts
with no name parameter; preserve `name=""` so it is added to `FormData` under an
empty-string key.
🪄 Autofix (Beta)
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: 99262810-9773-4528-88d3-b8c524e831d1
📒 Files selected for processing (3)
src/runtime/webcore/FormData.rstest/js/web/html/FormData-multipart-serialization.test.tstest/js/web/html/FormData.test.ts
b8b885c to
23cd7ad
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this is a user-visible Web API behavior change plus a Field struct refactor, and there's an open duplicate (#31526) that a maintainer needs to resolve — worth a human look before merging.
What was reviewed:
decode_multipart_namematches the cited WHATWG algorithm (only %0A/%0D/%22, case-insensitive hex, other % literal); zero-copy fast path andCowlifetimes intoFieldare sound.for_each_multipart_entryhas no other callers, so the signature change is self-contained; removedzero_countwas never written, soname.is_empty()is behavior-identical to the old check (CodeRabbit's empty-name note is pre-existing).- New tests cover names, filenames, lowercase hex, unrelated-% preservation, and the serialize→parse round-trip.
Extended reasoning...
Overview
This PR changes src/runtime/webcore/FormData.rs to percent-decode %22/%0D/%0A in multipart name/filename fields per the WHATWG parse algorithm, adds a decode_multipart_name helper returning Cow<[u8]>, and refactors Field from offset-based bun_semver::String to plain &[u8] slices (dropping the never-written zero_count field and the Default impl). The for_each_multipart_entry callback signature changes accordingly; its only caller is Wrapper::on_entry in the same file. Two test files gain fixtures and a round-trip test.
Security risks
None identified. The decode is narrowly scoped to three fixed byte sequences and cannot expand output beyond input length. The existing control-character filter on content_type is unchanged. No new untrusted-input surface is introduced beyond what the parser already accepted.
Level of scrutiny
Moderate. This is a spec-driven compat fix in a Web API parser that handles untrusted request bodies, and it changes user-visible .formData() output. The Rust change is small and borrowck-verified, but it also refactors a public struct's field types. The PR description explicitly notes overlap with #30609 (same slice refactor for a different reason) and duplication of #31526, both of which need a maintainer to sequence.
Other factors
- I previously left two inline comments on 2026-06-28 (a 3-line-comment nit and a pre-existing Content-Type early-exit bug); both are resolved and the author scoped the pre-existing bug out with a repro.
- CodeRabbit's unresolved "empty name" comment describes pre-existing behavior: the old
name.len() + field.zero_count as usize == 0check was equivalent toname.is_empty()becausezero_countwas never assigned. Not a regression from this PR. - Test coverage is thorough (fixture table entry exercised via Response/Request/Blob, dedicated filename test with mixed-case hex and literal-% preservation, serialize→parse round-trip).
- CI on the previous head was fully green (284/284); the latest build on 23cd7ad was still in progress at review time.
- The open duplicate #31526 predates this PR and implements the same decode differently; choosing between them is a maintainer call, not something I should auto-approve past.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-11 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
Bun's multipart/form-data serializer escapes
", CR and LF in entry names and filenames as%22,%0Dand%0A(as the spec requires), but the parser handed the escaped text through verbatim. So Bun could not read back the names it wrote into its own multipart output, and quotes/newlines in names coming from browsers or undici arrived as literal%22/%0D/%0Asequences.Parsing a raw body containing
name="a%22b%0D%0Ac"gavea%22b%0D%0Ac; Node givesa"b\r\nc.Fix
Apply the WHATWG "parse a multipart/form-data name" algorithm to both the entry name and the filename: replace
%0A,%0D(hex digits in either case) and%22with LF, CR and". Other%sequences stay literal. Output now matches Node/undici for every case in the new tests.Because a decoded name is no longer a subslice of the input buffer,
Fieldnow stores plain byte slices instead of offset-basedbun_semver::Stringvalues, and the unusedzero_countfield goes away. The common case (no%in the name) stays zero-copy.Note: #30609 replaces the same
bun_semver::Stringfields with slices for an unrelated reason (4 GiB offset truncation), so one of the two will need a trivial rebase once the other lands.Note: #31526 (opened May 28) implements the same decode and predates this PR; see the comment below for how the two differ.
Tests
test/js/web/html/FormData.test.ts: a raw-body fixture with%22/%0D%0A/lowercase%0d%0ain names plus a name whose other%sequences must stay literal (exercised throughResponse,RequestandBlob), and a test asserting decoded filenames.test/js/web/html/FormData-multipart-serialization.test.ts: serialize -> parse round-trip of names and filenames containing"and CRLF.Before the fix, 3 of the new tests fail in
FormData.test.tsand 1 inFormData-multipart-serialization.test.ts; all pass with it.no test proof · iteration 10 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/html/FormData-multipart-serialization.test.ts