diff --git a/src/runtime/webcore/Request.rs b/src/runtime/webcore/Request.rs index 075d91942968..6135171cb366 100644 --- a/src/runtime/webcore/Request.rs +++ b/src/runtime/webcore/Request.rs @@ -18,7 +18,7 @@ use crate::webcore::jsc::{ }; use crate::webcore::{AbortSignal, Blob, CookieMap, FetchHeaders, ReadableStream, Response}; use bun_alloc::AllocError; -use bun_core::{Output, fmt as bun_fmt}; +use bun_core::{Output, UnwrapOrOom, fmt as bun_fmt}; use bun_core::{OwnedStringCell, String as BunString, ZigString, strings}; use bun_http_jsc::fetch_enums_jsc::{ fetch_cache_mode_to_js, fetch_redirect_to_js, fetch_request_mode_to_js, @@ -1121,15 +1121,42 @@ impl Request { let values_to_try = &values_to_try_[0..((!is_first_argument_a_url) as usize + (arguments.len() > 1 && arguments[1].is_object()) as usize)]; - for &value in values_to_try { + for (value_index, &value) in values_to_try.iter().enumerate() { let value_type = value.js_type(); + // The last entry is the `input` argument when it was an object + // rather than a URL; other entries are `init` dictionaries. + let is_input_argument = + !is_first_argument_a_url && value_index == values_to_try.len() - 1; let explicit_check = values_to_try.len() == 2 && value_type == bun_jsc::JSType::FinalObject && values_to_try[1].js_type() == bun_jsc::JSType::DOMWrapper; if value_type == bun_jsc::JSType::DOMWrapper { - if let Some(request) = value.as_direct::() { - // SAFETY: as_direct returns a live *mut Request payload (m_ctx) + // Fetch spec: a Request `input` is copied from internal state, + // never via JS getters, so subclasses and shadowed instances + // (rejected by `as_direct`'s pristine-structure check) must + // match too. A Request `init` keeps dictionary semantics. + let request_ptr = if is_input_argument { + value.as_::() + } else { + value.as_direct::() + }; + if let Some(request) = request_ptr { + // SAFETY: the cast returns a live *mut Request payload (m_ctx) let request = unsafe { &*request }; + // Fetch spec: `init.body: null`/absent contributes no + // body, so the input's body applies and must be usable. + let input_body_applies = is_input_argument + && (!fields.contains(Fields::Body) + || matches!(req.body_value(), BodyValue::Null)); + if input_body_applies + && request.body_stream_check(global_this, |s, g| { + s.is_disturbed(g) || s.is_locked(g) + }) + { + bail!(Err(global_this.throw_type_error(format_args!( + "Cannot construct a Request with a Request object that has already been used." + )))); + } if values_to_try.len() == 1 { match Request::clone_into( request, @@ -1180,9 +1207,13 @@ impl Request { } } - if !fields.contains(Fields::Body) { + if !fields.contains(Fields::Body) || input_body_applies { match request.body_value() { - BodyValue::Null | BodyValue::Empty | BodyValue::Used => {} + BodyValue::Null | BodyValue::Used => {} + // `Empty` is a non-null body; an input copy keeps + // it, while an `init` entry leaves it to the + // dictionary fallback. + BodyValue::Empty if !is_input_argument => {} _ => { match request.clone_body_value_via_cached_stream(global_this) { Ok(v) => { @@ -1194,6 +1225,31 @@ impl Request { } } } + + if is_input_argument { + // Mark every remaining field consumed so the dictionary + // fallbacks below never run JS getters against the input. + if !fields.contains(Fields::Url) { + // A Bun.serve request materializes its URL lazily; + // a detached one stays empty and reaches the "url + // is required" throw, with the field consumed. + request.ensure_url().unwrap_or_oom(); + let url = request.url.get(); + if !url.is_empty() { + req.url.set(url.dupe_ref()); + } + fields.insert(Fields::Url); + } + if !fields.contains(Fields::Signal) { + if let Some(signal) = request.signal.get() { + // `AbortSignalRef::clone` is a C++ `ref()`. + req.signal.set(Some(signal.clone())); + } + fields.insert(Fields::Signal); + } + fields.insert(Fields::Headers); + fields.insert(Fields::Body); + } } if let Some(response) = value.as_direct::() { @@ -1552,7 +1608,7 @@ impl Request { preserve_url: bool, ) -> JsResult<()> { // allocator param dropped (global mimalloc) - let _ = self.ensure_url(); + self.ensure_url().unwrap_or_oom(); let body_ = self.clone_body_value_via_cached_stream(global_this)?; // BodyValue's Drop frees `body_` on the `?` error path let body = body::hive_alloc(body_); diff --git a/test/js/web/fetch/body-clone.test.ts b/test/js/web/fetch/body-clone.test.ts index 52532679f875..953bcfae6dbd 100644 --- a/test/js/web/fetch/body-clone.test.ts +++ b/test/js/web/fetch/body-clone.test.ts @@ -630,18 +630,18 @@ test.each(["Request", "Response"])( }, ); -// clone()'s usability check now fires before the stream is teed, so the -// readableStreamTee C++ bridge's exception propagation (which used to be -// covered by the test above) is exercised via `new Request(lockedRequest)`, -// which still tees. It must throw a single catchable TypeError, not also -// report it as uncaught (exit code 1) or surface a bogus follow-up error. -test("new Request(request) with a locked stream body throws a catchable TypeError from the tee and does not fail the process", async () => { +// The usability checks in clone() and in Request(input) fire before the +// stream is teed, so the readableStreamTee C++ bridge's exception propagation +// is exercised via a locked Request passed as init (second argument), which +// still tees. It must throw a single catchable TypeError, not also report it +// as uncaught (exit code 1) or surface a bogus follow-up error. +test("new Request(url, lockedRequest) throws a catchable TypeError from the tee and does not fail the process", async () => { const script = ` const stream = new ReadableStream({ start() {} }); const source = new Request("http://example.com/", { method: "POST", body: stream, duplex: "half" }); source.body.getReader(); // lock the body stream try { - new Request(source); + new Request("http://other.example/", source); console.log("no throw"); } catch (e) { console.log("caught " + e.constructor.name + ": " + e.message); diff --git a/test/js/web/request/request.test.ts b/test/js/web/request/request.test.ts index 85e42bf8e420..9ec18a9c54aa 100644 --- a/test/js/web/request/request.test.ts +++ b/test/js/web/request/request.test.ts @@ -175,3 +175,294 @@ describe("RequestInit signal presence", () => { }); }); }); + +// The fetch spec copies a Request `input`'s internal state ("Set request to +// input's request") without consulting JS-visible getters, even when the input +// is a subclass instance or carries shadowing own properties. Node behaves the +// same. Only the `init` argument has dictionary (getter) semantics. +describe("new Request(request) copies internal state without calling getters", () => { + test("own-property getters on the input are never called", () => { + let getterHits = 0; + const input = new Request("http://localhost/original", { + method: "PUT", + headers: { "x-real": "1" }, + }); + for (const name of ["url", "method", "headers", "body", "signal", "redirect", "cache", "mode"]) { + Object.defineProperty(input, name, { + get() { + getterHits++; + return undefined; + }, + }); + } + + const copy = new Request(input); + expect({ + url: copy.url, + method: copy.method, + headers: [...copy.headers], + redirect: copy.redirect, + getterHits, + }).toEqual({ + url: "http://localhost/original", + method: "PUT", + headers: [["x-real", "1"]], + redirect: "follow", + getterHits: 0, + }); + }); + + test("subclass getter overrides on the input are ignored", () => { + let getterHits = 0; + class MyRequest extends Request { + get url() { + getterHits++; + return "http://localhost/from-getter"; + } + get method() { + getterHits++; + return "DELETE"; + } + get headers() { + getterHits++; + return new Headers({ "x-from-getter": "1" }); + } + } + + const copy = new Request(new MyRequest("http://localhost/original", { headers: { "x-real": "1" } })); + expect({ + url: copy.url, + method: copy.method, + headers: [...copy.headers], + getterHits, + }).toEqual({ + url: "http://localhost/original", + method: "GET", + headers: [["x-real", "1"]], + getterHits: 0, + }); + }); + + test("init members win; everything else comes from the input's internal state", () => { + // Internal values differ from both the defaults and the getter values, so + // this discriminates "getter ignored" and "internal value actually copied". + class MyRequest extends Request { + get url() { + return "http://localhost/from-getter"; + } + get headers() { + return new Headers({ "x-from-getter": "1" }); + } + get redirect() { + return "manual"; + } + get cache() { + return "force-cache"; + } + get mode() { + return "no-cors"; + } + } + + const input = new MyRequest("http://localhost/original", { + headers: { "x-real": "1" }, + redirect: "error", + cache: "no-store", + mode: "same-origin", + }); + const copy = new Request(input, { method: "POST" }); + expect({ + url: copy.url, + method: copy.method, + headers: [...copy.headers], + redirect: copy.redirect, + cache: copy.cache, + mode: copy.mode, + }).toEqual({ + url: "http://localhost/original", + method: "POST", + headers: [["x-real", "1"]], + redirect: "error", + cache: "no-store", + mode: "same-origin", + }); + }); + + test("body comes from the input's internal state, not the body getter", async () => { + const make = () => { + const input = new Request("http://localhost/original", { method: "POST", body: "real-body" }); + Object.defineProperty(input, "body", { + get() { + return "getter-body"; + }, + }); + return input; + }; + expect(await new Request(make()).text()).toBe("real-body"); + expect(await new Request(make(), {}).text()).toBe("real-body"); + }); + + test("init body: null contributes no body, so the input's body is copied", async () => { + const input = new Request("http://localhost/", { method: "POST", body: "hello" }); + const copy = new Request(input, { body: null }); + expect(copy.body).not.toBeNull(); + expect(await copy.text()).toBe("hello"); + }); + + test("an empty-string body input copies as a non-null empty body", async () => { + const make = () => new Request("http://localhost/", { method: "POST", body: "" }); + const single = new Request(make()); + const withInit = new Request(make(), {}); + expect(single.body).not.toBeNull(); + expect(withInit.body).not.toBeNull(); + expect(await single.text()).toBe(""); + expect(await withInit.text()).toBe(""); + }); + + test("the input's signal carries over even when its signal getter is shadowed", () => { + const ctl = new AbortController(); + const input = new Request("http://localhost/", { signal: ctl.signal }); + Object.defineProperty(input, "signal", { + get() { + return undefined; + }, + }); + const copy = new Request(input, {}); + ctl.abort(); + expect(copy.signal.aborted).toBe(true); + }); + + test("throws TypeError when the input's body is already used", async () => { + const input = new Request("http://localhost/", { method: "POST", body: "x" }); + await input.text(); + expect(() => new Request(input)).toThrow(TypeError); + for (const init of [undefined, {}, { body: null }] as const) { + expect(() => new Request(input, init)).toThrow( + "Cannot construct a Request with a Request object that has already been used.", + ); + } + + // Node throws "Request with GET/HEAD method cannot have body." here + // because its GET/HEAD-body check precedes the unusable check. Bun has no + // constructor-level GET/HEAD-body check (it enforces at fetch() time), so + // the unusable error fires; adding that check later must consciously flip + // this precedence. + expect(() => new Request(input, { method: "GET" })).toThrow( + "Cannot construct a Request with a Request object that has already been used.", + ); + + // an init-provided body replaces the input's, so the used input body is + // never read and nothing throws + const replaced = new Request(input, { body: "fresh" }); + expect(await replaced.text()).toBe("fresh"); + }); + + test("throws TypeError when the input's body stream is locked", () => { + const input = new Request("http://localhost/", { + method: "POST", + body: new ReadableStream({ + start(c) { + c.enqueue(new Uint8Array([97])); + c.close(); + }, + }), + }); + input.body!.getReader(); + expect(() => new Request(input)).toThrow( + "Cannot construct a Request with a Request object that has already been used.", + ); + }); + + test("an unlocked stream-body input still copies fine", async () => { + const input = new Request("http://localhost/", { + method: "POST", + body: new ReadableStream({ + start(c) { + c.enqueue(new TextEncoder().encode("st")); + c.close(); + }, + }), + }); + expect(await new Request(input).text()).toBe("st"); + }); + + test("a Bun.serve request's lazily materialized url is copied, not read via getter", async () => { + using server = Bun.serve({ + port: 0, + fetch(req) { + let hits = 0; + Object.defineProperty(req, "url", { + get() { + hits++; + return "http://127.0.0.1:9/from-getter"; + }, + }); + const copy = new Request(req, { method: "POST" }); + const single = new Request(req); + return Response.json({ hits, url: copy.url, method: copy.method, single: single.url }); + }, + }); + const res = await fetch(`http://localhost:${server.port}/real-path`); + expect(await res.json()).toEqual({ + hits: 0, + url: `http://localhost:${server.port}/real-path`, + method: "POST", + single: `http://localhost:${server.port}/real-path`, + }); + }); + + test("a detached Bun.serve request never falls back to its url getter", async () => { + // A handler that responds synchronously without touching req.url leaves + // the internal url empty forever once the request context is torn down. + let saved: Request | undefined; + using server = Bun.serve({ + port: 0, + fetch(req) { + saved = req; + return new Response("ok"); + }, + }); + const res = await fetch(`http://localhost:${server.port}/x`); + expect(await res.text()).toBe("ok"); + + let hits = 0; + Object.defineProperty(saved!, "url", { + get() { + hits++; + return "http://localhost/from-getter"; + }, + }); + expect(() => new Request(saved!, {})).toThrow("url is required"); + expect(hits).toBe(0); + }); + + test('init body: "" is a real empty body and replaces the input body', async () => { + const fresh = new Request("http://localhost/", { method: "POST", body: "hello" }); + const replaced = new Request(fresh, { body: "" }); + expect(replaced.body).not.toBeNull(); + expect(await replaced.text()).toBe(""); + + // a non-null init body means the used input body is never read: no throw + const used = new Request("http://localhost/", { method: "POST", body: "x" }); + await used.text(); + const afterUsed = new Request(used, { body: "" }); + expect(await afterUsed.text()).toBe(""); + }); + + test("a Request passed as init (second argument) keeps dictionary getter semantics", () => { + let getterHits = 0; + const asInit = new Request("http://localhost/original", { method: "PUT" }); + Object.defineProperty(asInit, "method", { + get() { + getterHits++; + return "PATCH"; + }, + }); + const built = new Request("http://localhost/base", asInit); + expect({ url: built.url, method: built.method, getterHits }).toEqual({ + url: "http://localhost/base", + method: "PATCH", + getterHits: 1, + }); + }); +});