Repository navigation
Conversation
…es not set them new Request(input, init) walked init before input. For a non-native input (a plain object or a Request look-alike) the init object claimed the method field with the default GET even when it had no method property, so the input's method was never read. The same happened for headers. Only a native Request input was probed for an explicit property. Apply that probe to every init object so the missing fields fall through to the input. Fixes #42759
|
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; 1 remains after this review. Walkthrough
ChangesRequest initialization
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The Request initialization change preserves the intended fallback and override behavior, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The review raised no concerns. No open review threads remain. Waiting on CI. |
There was a problem hiding this comment.
LGTM — small, well-scoped fix that makes the init/input merge rule consistent regardless of input's JSType.
What was reviewed:
- Traced
values_to_tryconstruction: whenlen == 2,[0]is alwaysinitand[1]is alwaysinput, sovalue == values_to_try[0]correctly identifies the init iteration; the same identity comparison is already used at line 1252 for the url fallback. - Checked the second iteration and the
len == 1cases (URL-only input, object-only input) — behavior is unchanged; the DOMWrapper fast path still sets Method/Headers before theInit::initblock is reached, so a nativeinitis unaffected. - Verified the other per-field reads (body, url, signal, redirect, cache, mode) already use
fast_get/get_optional_enumwhich returnNonefor absent, so method/headers viaInit::initwere the only fields with this defaulting bug. - Tests cover the matrix: RequestLike vs plain object, init absent/empty/overriding, method and headers, and header replacement semantics.
Extended reasoning...
Overview
The PR changes one line in Request::from_init (src/runtime/webcore/Request.rs) that computes explicit_check, the guard that makes the constructor's merge loop probe init for an actual method/headers property before letting Init::init's default (GET) claim the field. Previously the guard only fired when init was a FinalObject and input was a native DOMWrapper; now it fires whenever two values are being merged and we are on the first (init) iteration. A new describe block in test/js/web/fetch/fetch.test.ts exercises both a Request-prototype-forwarding look-alike and a plain { url, method, headers, body } input against several init shapes.
Security risks
None. This is option-merging logic in the Request constructor; no parsing of untrusted bytes, no auth/crypto/permissions, no allocation-size or path handling changes. The only new observable calls are the existing fast_get property probes on init, now reached in more cases — same exception-propagation pattern as before.
Level of scrutiny
Low-to-moderate. The diff is a three-line predicate rewrite plus a two-line comment. I walked the values_to_try_ array construction to confirm index 0 is always init when length is 2, and confirmed the JSValue == JSValue identity comparison is already the idiom this function uses (line 1252) to tell which iteration it is on. I checked each other branch that reads explicit_check (header_check, method_check) and each path where explicit_check flips from false→true under the new rule (Proxy init, DOMWrapper init, non-native input) — in every case the new behavior is either identical (fields already set via the DOMWrapper fast path) or strictly more correct (falls through to input instead of pinning GET). The degenerate new Request(obj, obj) case where init === input still resolves sensibly since both iterations read the same object.
Other factors
Tests are added to the existing fetch.test.ts alongside the related #874 object-input test, use the one-line issue-link comment style, and cover the variant matrix REVIEW.md asks for (init present/absent, override vs inherit, both field kinds, both non-native input shapes). The PR notes the tests fail on the released binary. Bun intentionally diverges from the WHATWG brand check here (per the pre-existing #874 test), so matching Node's TypeError is not the goal — making the accepted-object path internally consistent is, and this does that.
|
Thanks for the review. No changes needed. Buildkite build #115699 is still in progress. |
|
Updated 5:21 PM PT - Sep 14th, 2026
✅ @robobun, your commit f94f7201a19d2235f93fb3533964c16bb04ae453 passed in 🧪 To try this PR locally: bunx bun-pr 42761That installs a local version of the PR into your bun-42761 --bun |
Problem
new Request(input, init)returns a GET wheninputis not a native Request andinithas nomethod.new Request({ url, method: "POST", body }, {})givesmethod === "GET"with the body still attached. A Request look-alike (a prototype chain that reachesRequest.prototype, as srvx builds) fails the same way. Without an init the method is kept.src/runtime/webcore/Request.rs:1080walksinitbeforeinput.Init::init(src/runtime/webcore/Response.rs:1230) returnsmethod: GETfor an object with nomethodproperty. Theexplicit_checkguard at line 1082 only madeinitprobe for an ownmethodorheadersproperty wheninputwas a native Request (DOMWrapper). For any other input the default GET was stored andinput.methodwas never read.Fix
explicit_checkis now true for the init object whenever two values are walked. It no longer depends on the type ofinput. Soinitonly claimsmethodandheaderswhen it has that property. Otherwise the loop falls through toinput, native or not.test/js/web/fetch/fetch.test.ts(newdescribe("new Request(input, init) with a non-native input"), two tests, both fail on the released binary). Alsofetch-args.test.ts,body-clone.test.ts,body.test.ts,headers-case.test.ts,body-mixin-errors.test.ts,request-cyclic-reference.test.ts,bun-server.test.ts.Background
Request::constructormerges up to two JS values into oneRequest: theinitdictionary and theinputobject. AFieldsbitset records which fields are set. Each value only fills fields that are still unset.Init::initis shared withResponse. It parsesheaders,status,statusTextandmethodfrom an object. It has no "absent" state formethod: the default is GET.DOMWrapperis a JS cell backed by a native class.as_direct::<Request>()on it returns the native payload, which is the fast path fornew Request(nativeRequest, init).Notes
Repro (released 1.4.3 and main):
Headers followed the same pattern.
Init::initreturnsheaders: Nonefor{}so they happened to survive, but the old guard still readinput.headersonly for a native input. With the fixnew Request(input, { headers })replaces the input headers (as with a native input) andnew Request(input, {})keeps them.Node 26 throws a TypeError for a look-alike input (brand check). Bun keeps its object-input path, so the fix is the consistent merge, not a throw.
Failures seen in the full
fetch.test.tsrun are environmental: public network (simultaneous HTTPS fetch,tlsextname), root permissions (bad permissions throws), IPv6 on the container, and GC timeouts under the ASAN debug build. None touch the constructor.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch.test.ts