Repository navigation
Conversation
A `headers` value that is not an object threw `TypeError: Type error`.
An entry that is not a pair threw `Value is not a sequence`, and `[{}]`
threw `Type error` again. No message named the option. `new Headers()`,
`fetch`, `Request`, `Response`, `Bun.FetchSession`, `Bun.serve` and
`WebSocket` all converted the value with the generic IDLUnion converter.
Add `convertHeadersInit`, one converter for HeadersInit. It runs the
steps of the union converter. The TypeErrors that it raises name the
argument or the option, and the index of the entry. A primitive is
echoed. An object entry is not described, because that reads
`constructor.name`. An exception from user code propagates unchanged. A
fast array entry takes the same path as before, with no Symbol.iterator
lookup.
`WebCore__FetchHeaders__createFromJS` takes a `HeadersInitName`.
`create_from_js` keeps its signature and names `headers`.
`create_from_js_named` is for `proxy.headers`.
|
Warning Review limit reached
On-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (14)
Comment |
|
Status: ready for review. The diff is green, and the red CI job is not from this PR. Reproduced on bun 1.4.3-canary (c6b7fcb): new Headers(1); // TypeError: Type error
await fetch(url, { proxy: { url, headers: 1 } }); // rejects with TypeError: Type error
new Response("", { headers: [["a", "1"], "bc"] }); // TypeError: Value is not a sequenceWithout the fix, this command fails 5 tests. With this branch, USE_SYSTEM_BUN=1 bun test test/js/web/fetch/headers.test.ts test/js/web/fetch/fetch-session.test.ts test/js/web/websocket/websocket-custom-headers.test.tsCI, build #116693: 180 of 181 jobs pass.
PR: #42961 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also diffed the new convertHeadersInit against the old IDLUnion dispatch in JSDOMConvertUnion.h — the acceptance set is unchanged (object with Symbol.iterator → sequence, other object → record, non-object/null → TypeError), and the entry-level Symbol.iterator lookup is only done off the FastArray path, so the record path and fast-array entries take the same converter calls as before.
Extended reasoning...
Checked the pre-change union converter (src/jsc/bindings/webcore/JSDOMConvertUnion.h steps 11–17) against the new helper in src/jsc/bindings/webcore/JSFetchHeaders.cpp: both call iteratorMethod on the top-level object, route to the sequence converter when defined and to the record converter otherwise, and throw a TypeError for primitives and null. The new per-entry branch only differs for non-FastArray entries, where it does the Symbol.iterator lookup that forEachInIterable would otherwise do internally, so accepted inputs and iteration order are preserved. The remaining inline finding (new helper placed in monolithic bindings.cpp) and the ruled-out candidates from the hunt cover the rest; nothing further to add.
2 verified lower-impact observations (convention, logging or cleanup points) were not posted.
`convertHeadersInit` takes a `HeadersInitName`, so `bindings.cpp` only forwards the enum and the WebSocket constructor uses the same values. The names `headers` and `proxy.headers` now exist in one place.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed after 339040e and 29b3e93: the enum-to-name mapping now lives in convertHeadersInit in src/jsc/bindings/webcore/JSFetchHeaders.cpp and the bindings.cpp helper is gone, which addresses my earlier comment, and I found no bugs in this version. Because this replaces the generic Web IDL union converter with a hand-written iterator walk for every HeadersInit entry point (Headers, Request, Response, fetch, FetchSession, Bun.serve, WebSocket) and changes a C ABI signature, a human look would still be worthwhile.
What was reviewed:
- Exception-scope discipline in
convertHeadersInitNamed/throwEntryIsNotPair: every throwing call (iteratorMethod,forEachInIterable, bothConverter::convertpaths) is followed byRETURN_IF_EXCEPTION/RELEASE_AND_RETURN, and the lambda mirrors the pattern inJSDOMConvertSequences.h:218-225. - Behavior parity with the old union converter: object check, then
Symbol.iteratorlookup, sequence-vs-record dispatch, and the fast-array entry path call the sameConverter<IDLSequence>::convert(global, value)the generic converter used, so only the error messages change. - The C ABI:
HeadersInitNameisuint8_t/#[repr(u8)]with matching discriminants,WebCore__FetchHeaders__createFromJSis the only extern with the new parameter and all callers (Rust extern,headers.h,bindings.cpp) were updated together. - Tests pin exact
name/code/message, cover the user-code-propagation and no-constructor-read contracts, and the WebSocket cases throw in the constructor before any dial.
Extended reasoning...
Overview
The PR replaces the generic IDLUnion<IDLSequence<IDLSequence<S>>, IDLRecord<S,S>> conversion for HeadersInit values with a hand-written convertHeadersInit<IDLStringType> in src/jsc/bindings/webcore/JSFetchHeaders.cpp so that conversion TypeErrors are ERR_INVALID_ARG_TYPE errors naming the argument/option (init, headers, proxy.headers) and the bad entry index. It adds a HeadersInitName enum mirrored between headers-handwritten.h and src/jsc/FetchHeaders.rs, threads it through WebCore__FetchHeaders__createFromJS (new trailing parameter), adds create_from_js_named wrappers in Rust with create_from_js delegating to Headers, and updates the C++ call sites in the Headers constructor and JSWebSocket.cpp. Tests were added to the three existing suites for the affected modules.
Since my prior review, commit 339040e removed the headersInitNameLiteral helper from bindings.cpp and moved the enum-to-literal switch into convertHeadersInit in JSFetchHeaders.cpp, which is what my inline comment asked for. Commit 29b3e93 only shortened comments.
Security risks
The converter parses user-supplied JS values in native code. I checked that no user code runs on the error path (the constructor.name read of INVALID_ARG_INSTANCE is only reached for primitives; objects get a message with no Received part), that getIterationMode on a non-cell value is safe and routes to the generic branch, and that the only user-observable reads are the Symbol.iterator lookups Web IDL already requires. Header name/value validation is unchanged and still happens in FetchHeaders::fill. No injection, auth, or data-exposure surface is introduced.
Level of scrutiny
Moderate-to-high. This is a new IDL converter in JSC bindings on a hot path used by every fetch/Headers/Response construction, plus an FFI signature change. The exception-check pattern matches the existing SequenceConverter::convert lambdas, the fast-array path makes the same Converter<IDLSequence<S>>::convert(global, value) call the generic converter made, and the enum mirroring follows the BunErrorKind/UWSResponseKind precedent in the same header. I did not find a defect, but a hand-written replacement for a spec-derived converter is the kind of change a maintainer should eyeball rather than have a bot approve outright.
Other factors
The bug hunt ran dry with no findings and no ruled-out candidates. Tests assert exact name/code/message triples, cover the negative contract (user exceptions propagate unchanged, constructor is never read), and the WebSocket cases throw synchronously before connecting so no server is needed. The fetch-session.test.ts suite did not run on the author's machine per the evidence marker and is deferred to CI. No CODEOWNERS entry covers the changed files. No outstanding third-party objections are visible in the timeline; the only reviews are bot comments and my own prior inline note, which the code now addresses.
|
One correction to the note on
The file has three tests with a Windows |
|
Updated 1:16 PM PT - Sep 16th, 2026
❌ @robobun, your commit 18782ac has some failures in 🧪 To try this PR locally: bunx bun-pr 42961That installs a local version of the PR into your bun-42961 --bun |
Problem
headersvalue of the wrong type throwsTypeError: Type errororValue is not a sequence. No message names the option, sofetch(url, { headers, proxy: { url, headers } })does not say which one is wrong.new Headers(),fetch,Request,Response,Bun.FetchSession,Bun.serveandWebSocketall use the generic union converter. Its last step isthrowTypeError()with no message (src/jsc/bindings/webcore/JSDOMConvertUnion.h:383).Fix
convertHeadersInit(src/jsc/bindings/webcore/JSFetchHeaders.cpp) for all of these call sites. It runs the steps of the union converter, and its TypeErrors name the option and the entry:The "proxy.headers" property must be an instance of Headers, Array, or Object. Received type number (1)The "headers[1]" argument must be an instance of Array. Received type string ('bc')test/js/web/fetch/headers.test.ts,fetch-session.test.ts,websocket-custom-headers.test.ts(five tests fail without the fix). The Notes have the other suites and a benchmark.Background
HeadersInitis the Web IDL type of everyheadersvalue: a sequence of[name, value]pairs, or a record. When no member of a union matches, Web IDL requires a TypeError and no message.FetchHeaders::fillvalidates names and values after the conversion. Its errors name the header already, and they do not change.Notes
Origin. No user reported this. A sweep of the
Bun.FetchSessionoption bag found it during the work on #42937:proxy: { url, headers: 1 }was the only key whose error did not name itself. #42937 left it out because the message comes from the shared conversion.Messages, before and after (release build of main, then this branch):
new Headers(1)Type errorThe "init" argument must be an instance of Headers, Array, or Object. Received type number (1)fetch(url, { headers: "x" })Type errorThe "headers" argument must be ... Received type string ('x')new Bun.FetchSession({ proxy: { url, headers: true } })Type errorThe "proxy.headers" property must be ... Received type boolean (true)new Response("", { headers: [["a","1"], "bc"] })Value is not a sequenceThe "headers[1]" argument must be an instance of Array. Received type string ('bc')new WebSocket(url, { proxy: { url, headers: [{}] } })Type errorThe "proxy.headers[0]" property must be an instance of ArrayAll are
TypeErrorwithcode: "ERR_INVALID_ARG_TYPE".Value is not a sequencehad that code already, and so doessignal is not of type AbortSignal.infetch. "argument" or "property" follows the rule ofBun::ERR::INVALID_ARG_INSTANCE: a name with a dot is a property.FFI.
WebCore__FetchHeaders__createFromJStakes aHeadersInitNameenum (headersorproxy.headers), mirrored inheaders-handwritten.h. The Rustcreate_from_jskeeps its two-argument signature and namesheaders, so its seven callers do not change.create_from_js_namedis for the oneproxy.headerscaller, inFetchSession.rs. On the C++ sideconvertHeadersInittakes the same enum, and onlyJSFetchHeaders.cppmaps it to the option names.bindings.cppforwards the enum, and theWebSocketconstructor passes the enum values. TheHeadersconstructor, in the same file, names its argumentinit.No user code on the error path.
Bun::ERR::INVALID_ARG_INSTANCEdescribes an object withconstructor.name, which can run a getter or a Proxy trap. So the converter echoes a primitive only. An object entry gets the message with noReceivedpart. A test wraps an entry in a Proxy and checks that the only read isSymbol.iterator, and that a throwingconstructorgetter is never called.Benchmark. Release builds,
new Headers(init)andnew Response("", { headers }), best of 5 interleaved runs, ns per call:new Headers, 1 pairnew Headers, 8 pairsnew Headers, 32 pairsnew Headers, record of 8new Response, 8 pairsnew Response, record of 8A first version did a
Symbol.iteratorlookup on every entry, and that cost 12 to 13% for 8 and 32 pairs. The converter now sends a fast array entry to the sequence converter directly, which is the call that ran before this PR. The record path did not change, so its spread shows the noise between two builds.Why a HeadersInit converter and not a change to the union converter. The union converter is a port of WebKit's and serves about 60 other call sites, most of them in
src/jsc/bindings/webcrypto/. TheType errorfor[{}]does not come from it. It comes fromJSC::forEachInIterable, which finds no iterator. To name that entry, the caller must do theSymbol.iteratorlookup, and that is what this converter does. The other unions that can reach the message-less TypeError (mostly the WebCryptoBufferSourceunions) are out of scope here.WebSocket keeps
IDLByteString. A switch toIDLDOMStringwould turn an invalidproxy.headersvalue from a throw into a silent drop until #37638 lands, becausesetupProxydiscards afillfailure today. A value outside Latin-1 still throwsType errorthere.Not changed. The
fillerrors, includingHeader sub-sequence must contain exactly two items.server.upgrade()keeps its ownupgrade options.headers must be a Headers or an object.Self-review. A review of the first version raised four concerns, and all four are addressed:
constructor.nameread (fixed, with a test)create_from_jsthat touched seven call sites and conflicted with open PRs (now one call site)Suites run on the debug ASAN build. The three files above (147 pass), also under
BUN_JSC_validateExceptionChecks=1.fetch_headers.test.js,fetch-args.test.ts,headers.undici.test.ts,request-subclass.test.ts,request.test.ts,test/js/deno/fetch/{headers,request,response}.test.ts,bun-serve-static.test.ts,proxy.test.ts,websocket-proxy.test.ts: all pass.response.test.ts"handle stack overflow" times out at 5 s on this machine with and without the change. Two tests inwebsocket.test.jsneed the public internet and fail the same way on main.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-session.test.ts