Repository navigation
Conversation
The Request constructor parsed its init object with the ResponseInit parser. That parser always yields a method (GET when the member is absent), and also reads status and statusText. A plain-object init was special-cased to check for an own method first. Every other init object (a Response, a Proxy, an array, a function, a Headers, a URL) reset the input Request's method to GET. Read the headers and method members of the init object directly, and apply a method only when the member is present. A Response matched by the internal fast path contributes its method only as input, never as init, since a Response has no method property.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. WalkthroughChangesThe change updates Request initialization
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The constructor change preserves request methods and applies explicit initialization values without an identified merge-blocking regression. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. Also fixes #42759. Reproduced on bun 1.4.3 with the cases in Latest push (cbd1424) only shortens code comments that the comment check flagged. The four review threads are resolved. Needs a maintainer review. |
There was a problem hiding this comment.
I reviewed this and found no bugs. It restructures the Request constructor's init handling with several intentional observable changes and documented sequencing against four other open PRs on the same block, so a human look at the coordination and spec-divergence choices would still be worthwhile.
What was reviewed:
is_inputderivation inconstruct_into— traced all fourvalues_to_tryshapes (URL/object input × with/without init); the flag is correct in each.copied_internal_fieldsgate — for a pristineRequestas init, method/headers are already copied from internal state so skippingfast_getpreserves prior behaviour; for a subclassedResponse,as_directfails so the getter path still runs (matches theWithMethodtest).- Exception propagation — every
fast_get/from_js/from_init_valueresult is matched withErr(e) => bail!before use. HeadersRef::from_init_value— verbatim extraction of theInit::initblock including the empty-→-Noneandadopt(+1)semantics; both call sites now share it.
Extended reasoning...
Overview
The PR changes how new Request(input, init) decides which candidate contributes method and headers. In src/runtime/webcore/Request.rs::construct_into, the explicit_check heuristic (which compared values_to_try JSTypes to guess whether init was a plain object) and the call into response::Init::init() are removed. Each candidate is now probed directly with fast_get(Headers) / fast_get(Method), gated by a copied_internal_fields flag (set when a pristine Request/Response DOM wrapper already had its internal state copied) and an is_input flag distinguishing the input argument from the init dictionary. A pristine Response in the init position no longer contributes its internal method. The Headers-object-vs-HeadersInit branch is factored into HeadersRef::from_init_value() in src/runtime/webcore/Response.rs and reused from Init::init. Twelve new test cases in test/js/web/request/request.test.ts cover the input-shape matrix.
Security risks
None identified. The change reads fewer properties from the init object than before (status/statusText are no longer touched), which reduces the observable surface. All user-controlled property reads go through fast_get with exception propagation on every arm; no new unsafe is introduced beyond the moved Self::adopt(p) call, which carries the same +1 ownership invariant as the original site.
Level of scrutiny
Moderate-to-high. Request::construct_into is a spec-defined Web API constructor on a hot path, and the change is a control-flow restructuring rather than a local guard. I traced the values_to_try_ construction against all four (input-is-URL × init-is-object) combinations to confirm is_input is correct in each; checked that copied_internal_fields = true on the pristine-Request branch does not lose any read that the removed Init::init call previously performed (that call's Request fast-path would have returned the same values, and Fields::Method/Fields::Headers gating already short-circuited it); and confirmed the extracted from_init_value is byte-equivalent to what was inline in Init::init, including returning None for an empty Headers object.
Other factors
The PR description enumerates several intentional behaviour changes beyond the headline fix (method getter now read once, status/statusText no longer read, a Response's Bun-internal method no longer leaks into init), plus three deliberately-unchanged divergences from undici (empty headers still treated as absent, body: null, unrecognized method → GET) and textual/semantic overlap with four other open PRs on this same loop (#41841, #33469, #40411, #41902). None of those are defects in this diff, but the sequencing plan and the "empty headers counts as absent" spec-divergence decision are the kind of thing a maintainer should ratify rather than an automated reviewer. The new tests use test.each matrices, exact-value assertions, and no timing dependencies; they are placed in the existing request.test.ts per convention. No CODEOWNERS entry covers the changed paths.
|
The automated review found no defects. It defers two decisions to a maintainer, and both are spelled out under Notes in the PR body:
CI so far: the red entries are |
Add the two cases from #42759: a Request look-alike (an object whose prototype chain reaches Request.prototype) and a plain { url, method, body, headers } object, each passed as input with an init that sets no method. The copy keeps the method, the headers and the body of the input, and init.headers or init.method still override them. The plain object case replaces "an object input keeps its method when init has none", which it covers. Three @ts-expect-error directives that tsc reports as unused are removed.
|
This PR now also fixes #42759, and the body links it. #42761 fixed that issue alone, on the same Pushed:
Checked on a debug build of this branch: the snippet from #42759 prints POST, POST, POST, and the tests of #42761 pass as written. On the code of #42761, 4 cases of |
|
Updated 12:55 AM PT - Sep 15th, 2026
✅ @robobun, your commit cbd1424372135c9065d9a87b93315d3939c68762 passed in 🧪 To try this PR locally: bunx bun-pr 41994That installs a local version of the PR into your bun-41994 --bun |
|
The comment check flagged four multi-line code comments in |
CI runs turbo check:svelte build (build depends on test), so an import
only Vite fails to resolve breaks the gate. A Biome GritQL plugin
rejects new Request(request, { headers }): under Bun an empty headers
init keeps the original headers (oven-sh/bun#41994), which once let a
spoofed client IP through.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #42759
Problem
new Request(input, init)returns a GET wheninithas nomethod, unlessinitis a plain object andinputis a native Request.new Request({ url, method: "POST", body }, {})is a GET that keeps the body (new Request(input, init) drops the input's method when input is not a native Request #42759). So isnew Request(put, new Response("b"))(undici keeps PUT), or a Proxy, an array, aHeadersor aURLasinit.Request::construct_into(src/runtime/webcore/Request.rs:1301). It parsedinitwithresponse::Init::init, theResponseInitparser, which yields GET whenmethodis absent. Theexplicit_checkguard from fix: don't set default request method when creating a Request from another #6154 covers only that one pair. TheResponsedirect-copy branch (Request.rs:1151) also copied the Response's internal method in theinitposition.Fix
headersandmethodmembers of each candidate withfast_get. Set a method only when the member is present.explicit_checkand theInit::initcall go away, soinitis not probed forstatus/statusTexteither.Responsedirect-copy branch sets a method only when the Response is theinput.Headers-or-HeadersInitconversion moves intoHeadersRef::from_init_value, shared withInit::init. Header semantics do not change.test/js/web/request/request.test.ts(13 cases fail on main, two came from Request: read method and headers from a non-native input when init does not set them #42761 for new Request(input, init) drops the input's method when input is not a native Request #42759). Self-reviewed: 1 concern (order against open PRs on this loop), addressed in Notes.Background
RequestInitis a WebIDL dictionary: oneGetper member, andundefinedmeans absent. AResponseasinitcontributesheadersandbody(its getters), nothing else.construct_intovisits[init?, input?]in that order and takes each field from the first candidate that has it. A default out ofinitshadows the real value oninput.Request/Responsewrapper (as_direct) is copied from internal state without getters. Any other object has its members read as properties, also asinput(a Bun extension, test "Creating Request from existing Request #874").Notes
Repro (bun 1.4.3 vs node v26.3.0/undici):
Repro from #42759. A Request look-alike is an object that is not a native Request, but whose prototype chain reaches
Request.prototype. srvx hands Node.js requests to fetch handlers in this form. Node 26 throws a TypeError for all three lines, because the object fails the Request brand check.Relation to #42761: that PR fixed the #42759 face alone. It made
explicit_checkapply toinitfor every kind ofinput. It is closed in favor of this PR. Its two tests are nowRequestInit method presence > a non-native inputinrequest.test.ts, and the original two tests of #42761 also pass on this branch as written. On the code of #42761, 4 of the cases in this file still fail: the twoResponse-as-initcases, the double read of aninit.methodgetter, andstatusread from aRequestInit.Other faces of the same cause, all fixed here (method was GET before, stays PUT now, as in undici):
new Request(put, new Response(null)),new Request(put, new Proxy({}, {})),new Request(put, []),new Request(put, function(){}),new Request(put, new Headers({a: "1"})),new Request(put, new Blob([])),new Request(put, new URL("http://z/")),new Request({ url, method: "PUT" }, { headers }). On 1.4.3fetch(put, init)already sends PUT for all of these (fetch.rsreadsmethodby presence), whilefetch(new Request(put, init))failed withfetch() request with GET/HEAD method cannot have body.Also changed as a consequence:
methodgetter oninitis read once (it was read twice whenexplicit_checkapplied).new Request(url, { status: 0 })no longer throwsRangeError: The status provided (0) must be 101 or in the range of [200, 599].statusandstatusTextare notRequestInitmembers and are not read anymore.new Request(url, new Response(null, { method: "DELETE" })).methodis GET, not DELETE. ThemethodaResponseInitcan carry in Bun is internal Response state (HTMLRewriter uses it), not a property.Deliberately not changed here:
init.urland a Response's url in theinitposition: Request: never read a url from the RequestInit dictionary #41902. That PR computes the sameis_inputflag; whichever lands second drops the duplicate line.init.body: nulldropping the input body: Keep the input Request's body when init.body is null #41901.headers({},[],new Headers(), a Response without headers) still counts as absent and keeps the input's headers. undici empties the header list in that case. Separate behaviour change.{ method: "FOO" },{ method: null }), as before: fetch: stop replacing unrecognized HTTP methods with GET #33469.Init::initkeeps its Request/Response fast paths fornew Response(body, requestOrResponse).inputthat is not a native Request is still accepted (test "Creating Request from existing Request #874"). A Node-style brand check that throws is a separate decision.Sequencing: #41841 (
Init::init_without_status) and #33469 (Init::init_for_request) each add a Request-specific variant ofInit::initfor this call site. With this change the call site is gone, so those hunks become unnecessary. #40411 rewrites the same block mechanically (bail!to?); the conflict is textual.Suites run on the debug build:
test/js/web/request/(all four files),test/js/web/fetch/{fetch -t "Request|Response|Headers",body,body-clone,body-mixin-errors,body-stream,fetch-args,response,headers,headers-case,client-fetch,request-cyclic-reference},test/js/web/html/FormData,test/js/workerd/html-rewriter,test/js/deno/fetch/{request,body},test/js/bun/globals,test/js/bun/util/filesystem_router,test/regression/issue/{02368,2993}. Pre-existing under the debug ASAN build with or without this change: theutf16 ... (with gc)cases infetch.test.tsand the loops inrequest-clone-leak.test.ts/request-method-getter.test.tsexceed the 5 s default timeout; their assertions pass with a longer timeout.Run again on the debug build after the merge of main (7e56b40):
test/js/web/request/{request,request-subclass},test/js/web/fetch/{fetch-args,body,body-clone,body-mixin-errors,request-cyclic-reference,headers,headers-case,response},test/js/workerd/html-rewriter,test/js/deno/fetch/request. All pass, except thatresponse.test.ts"handle stack overflow" (no init involved) takes 7.5 s under the debug ASAN build and exceeds the 5 s default timeout.[human-review] gate passed · iteration 3 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file