webcore: copy internal state in new Request(request) instead of calling getters - #37033
webcore: copy internal state in new Request(request) instead of calling getters#37033robobun wants to merge 7 commits into
Conversation
When the input Request's structure is not pristine (a subclass instance, or an instance carrying shadowing own properties), the constructor fell through to the dictionary path and read url, method, headers, body and signal through JS-visible getters. The fetch spec says to copy the input's request state directly, and Node does the same; only the init argument has dictionary (getter) semantics. Also throw the spec's TypeError when the input's body is already disturbed or locked and init supplies no replacement body.
WalkthroughChangesRequest construction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…te copy A Bun.serve request's URL is synthesized from the request context on first access, so the input-position copy has to run ensure_url() before reading the internal field, as clone_into() already does. Otherwise the two-argument path fell back to the JS url getter.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
An empty-string body extracts to BodyValue::Empty, which is a non-null body. The input-position field marking suppressed the dictionary fallback that used to materialize it, so the copy's body became null. Clone Empty for input copies; init entries keep the fallback. Also pin the used-input + GET-method init precedence with a test.
|
Updated 4:52 AM PT - Aug 6th, 2026
✅ @robobun, your commit 0855f3fa229ff6cd417d7cb09fa09539fa93f192 passed in 🧪 To try this PR locally: bunx bun-pr 37033That installs a local version of the PR into your bun-37033 --bun |
The unusable-input check already treats a null init body as absent, per
the spec's initBody-is-null reading. Apply the same predicate to the
body copy so new Request(input, { body: null }) carries the input's
body like Node does, instead of throwing about a body it then drops.
There was a problem hiding this comment.
Both earlier findings are addressed — ensure_url() before the internal read (7f843ef) and the shared input_body_applies predicate so the throw check and body-copy gate agree on {body: null} (d7b44a9) — and I found nothing new on this revision. Deferring to a maintainer for the user-visible compat changes: new Request(usedInput) now throws, and {body: null} now copies the input body.
What was reviewed:
- Traced
is_input_argument/input_body_appliesthrough single-arg, two-arg, subclass, own-property, and Request-as-init paths; the disturbed-body check runs before theclone_intofast path so the one-arg form is covered too. - Verified
body_stream_checkreturns true forValue::Usedand locked streams, and false for fresh string/blob bodies, so the throw matches the tests. - The repurposed
body-clone.test.tscase still reaches the readableStreamTee bridge via the init-position Request, which keepsas_directand still tees a locked stream.
Extended reasoning...
Overview
The PR changes Request::construct_into in src/runtime/webcore/Request.rs so a Request passed as the input (first) argument is copied from internal state via an inherits-based cast (as_) instead of the pristine-structure as_direct, adds the fetch spec's unusable-input throw, and fixes two body-copy edge cases (Empty bodies and init.body: null). It adds ~230 lines of tests to request.test.ts and repurposes one body-clone.test.ts test whose old form is now covered by the new throw.
Prior review follow-up
I raised two issues on earlier revisions; both are fixed with dedicated tests:
- The two-arg path now calls
request.ensure_url()before reading the internal URL, mirroringclone_into, so a Bun.serve request's lazily-materialized URL is captured (test: "a Bun.serve request's lazily materialized url is copied"). - The disturbed-input check and the body-copy gate now share one
input_body_appliespredicate, sonew Request(freshInput, {body: null})copies the input body andnew Request(usedInput, {body: null})throws about a body it would actually use (test: "init body: null contributes no body").
I re-traced the loop on d7b44a9 for the single-arg (values_to_try.len()==1 → disturbed check → clone_into), two-arg init-first, and new Request(url, requestAsInit) (is_first_argument_a_url keeps is_input_argument false, so init-position Requests retain as_direct dictionary semantics) shapes and did not find further gaps.
Security risks
None identified. The change narrows what user JS can observe/influence during construction (getters on the input are no longer consulted), which is strictly a reduction in attacker-controlled surface.
Level of scrutiny
Moderate-to-high. Request construction is a hot, pervasively-used Web API path; the change is compat-sensitive (new throw for used/locked input bodies; {body: null} now copies input body). Both are spec- and Node-aligned, but they are user-visible behavior changes that a maintainer should consciously sign off on. The pinned test documenting the GET/HEAD-body precedence divergence from Node is another deliberate compat call.
Other factors
Test coverage is thorough (11 new tests spanning own-property/subclass/one-arg/two-arg/Bun.serve/empty-body/used/locked/init-position). The one open inline thread is a comment-cop lint the author declined; the comments in question are short spec notes, not workaround justifications. Not approving because the behavior changes and the intricacy of the values_to_try bookkeeping warrant a human maintainer's sign-off, but I have no outstanding correctness concerns.
A Bun.serve request kept past a synchronous response has an empty internal url and a torn-down request context, so ensure_url() cannot materialize it. Leaving Fields::Url unset let the dictionary fallback run the input's url getter. Mark the field consumed unconditionally so the empty url reaches the existing "url is required" throw instead, the same outcome the unshadowed case already had. Also cover two review gaps with tests: redirect/cache/mode copies asserted with non-default values, and init body "" (a real empty body) winning over the input's body where null does not.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Request.rs`:
- Line 1238: Update the request construction flow around ensure_url() to
propagate its AllocError instead of discarding it, routing the allocation
failure through the designated OOM handler. Preserve normal URL materialization
and valid server-request construction when ensure_url() succeeds.
🪄 Autofix
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: e093d765-38cf-4b33-8536-0952546ef277
📒 Files selected for processing (3)
src/runtime/webcore/Request.rstest/js/web/fetch/body-clone.test.tstest/js/web/request/request.test.ts
Discarding the AllocError turned an allocation failure into a misleading "url is required" TypeError. Crash controlledly instead, in both the constructor's input copy and clone_into.
There was a problem hiding this comment.
No issues found in this pass; both earlier findings (lazy Bun.serve URL and the body: null copy/throw asymmetry) are addressed, as is the ensure_url OOM routing. Deferring to a human because this changes user-visible Request constructor semantics — new TypeError on disturbed/locked input, and {body: null} now copying the input body — which is compat-sensitive enough to want eyes on.
What was reviewed:
is_input_argumentderivation against everyvalues_to_trylayout (URL/non-URL first arg, one/two-arg) — the last-index predicate is correct in each.input_body_appliesgating forbodyabsent /undefined/null/""/ real value — the throw and copy sites now share one predicate; theEmpty-body match arm keeps init-position behavior unchanged.- The rewritten
body-clone.test.tscase still reaches the tee (init-position pristine Request →clone_into→clone_body_value_via_cached_stream), so the C++ tee exception path stays covered.
Extended reasoning...
Overview
Changes Request::construct_into in src/runtime/webcore/Request.rs so that when the first constructor argument is a Request (including subclasses / instances with own properties), all fields are copied from internal state rather than via JS getters, matching the fetch spec and Node/undici. Adds the spec's unusable-input check (throws TypeError when the input body is disturbed or locked and init contributes no body). Fixes {body: null} to fall through to the input body instead of dropping to null. Also routes ensure_url() allocation failure through unwrap_or_oom() at both call sites. ~70 net Rust lines plus ~290 lines of new tests in test/js/web/request/request.test.ts, and a small rewrite of one body-clone.test.ts case whose original path now throws before reaching the tee.
Security risks
None identified. The change narrows attack surface slightly by no longer invoking user-controlled getters on the input Request during construction. No new untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
High. This is core Web API surface (new Request(...)) with observable behavioral changes: (1) code that previously silently constructed from a used/locked input now throws; (2) new Request(input, {body: null}) now carries the input body where it previously produced a null body; (3) subclass/shadowed getters on the input are no longer consulted. All three align with Node v26 and the fetch spec, and the PR's evidence shows the new tests fail on main and pass on the branch (ASAN + release), but any change in throw behavior on a hot constructor is compat-sensitive.
Other factors
I traced the values_to_try layout against is_input_argument = !is_first_argument_a_url && index == len-1: for a non-URL first arg the input is always the last slot (len 1 → index 0; len 2 → index 1 with init at 0), and for a URL first arg the flag is always false — so a Request passed as init correctly keeps as_direct / dictionary semantics. The new disturbed check reuses the existing body_stream_check(|s,g| s.is_disturbed(g) || s.is_locked(g)) shape from throw_if_body_unusable, so no new JS-reentrancy pattern is introduced. The Empty-body match arm's if !is_input_argument guard preserves pre-existing init-position behavior while letting the input path copy an empty-string body as non-null. Both of my earlier inline findings (missing ensure_url() on the two-arg input path; the throw-vs-copy predicate mismatch for {body: null}) were fixed with follow-up commits and regression tests, and the CodeRabbit OOM-routing note was applied at both ensure_url sites. Given the scope — a spec-driven behavioral change to Request construction — a maintainer should sign off.
1 participant
Problem
new Request(input)only took the internal-state copy path when the input Request's Structure was pristine. Aclass extends Requestinstance, or a genuine Request that gained any own property, fell through to the dictionary path and was read through JS-visible getters:The fetch spec's Request constructor sets the new request from the input's request directly ("Set request to input's request"), and Node v26/undici never consults getters for a Request passed as
input. Getters are only correct for theinitdictionary argument.Relatedly, the spec's unusable-input check was missing:
new Request(usedRequest)silently copied a disturbed body where Node throwsTypeError: Cannot construct a Request with a Request object that has already been used.(it only threw incidentally, with a different message, when an init object happened to route the disturbed stream through the getter path).Cause
In
Request::construct_into, both constructor arguments flow through the samevalues_to_tryloop, and the internal-state branch was gated onas_direct::<Request>(), whose generated check requires the cell's structure ID to equal the global pristineJSRequeststructure. Any structure transition (subclass prototype, added own property) returned null, so the input was parsed as a dictionary.Fix
The loop now knows which entry is the
inputargument (always the last entry when the first argument was not a URL):as_::<Request>()cast, so subclass instances and transitioned structures still take the internal-state copyensure_url()first, since a Bun.serve request builds it lazily from the request context (its signal is set eagerly at creation, so no equivalent step is needed there). A detached server request whose context is already gone keeps its empty url and hits the existing "url is required" throw instead of the getter fallback; the one-argument form's silent empty-url copy there is a pre-existing wart shared withRequest.clone()and is not changed hereEmptyinput body (extracted from an empty string) is copied as the non-null empty body it is, instead of being dropped to nullundefined, ornull), a disturbed-or-locked input body now throws the same TypeError as Node, before the teeinit(second argument) keeps dictionary semantics, where only a pristine instance may skip the getter pathSame bug class as #37027, which covers the
fetch(request)call site; this PR covers theRequestconstructor.Verification
These now match Node v26 (url/method/headers/body/signal from internal state, 0 getter hits, same throw behavior for used/locked/replaced bodies, init getter semantics unchanged), with one pinned exception: when init changes the method to GET/HEAD on a used-body input, Node throws its
Request with GET/HEAD method cannot have body.error first. That constructor-level GET/HEAD-body check does not exist in bun (bun enforces it at fetch() time), predates this PR, and is a compat-sensitive change of its own, so the unusable-input error fires there instead; a test pins the current precedence.test/js/web/request/request.test.ts: own-property and subclass getter shadowing (one-arg and two-arg forms), init-wins semantics, internal body/signal copy, a Bun.serve handler request with a shadowed url getter, empty-string-body copy, used/locked-body throws, and the preserved init-position getter semantics. 7 of them fail on bun 1.3.6 and pass with this changetest/js/web/fetch/body-clone.test.ts's tee-propagation test now uses a locked Request asinit, the remaining path that still tees a locked stream; the input-position form it used before is covered by the new constructor testsrequest-subclass.test.ts(fetch()-getter behavior from SvelteKit fetch issue #4718),body-clone.test.ts,response.test.ts,fetch.test.ts, undici tests: no new failures[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file