webcore: copy internal state in new Request(request) instead of calling getters

src/runtime/webcore/Request.rs

@@ -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::<Request>() {
-                    // 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_::<Request>()
+                } else {
+                    value.as_direct::<Request>()
+                };
+                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,30 @@ 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
+                            // from the request context.
+                            let _ = request.ensure_url();
+                            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::<Response>() {

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);

test/js/web/request/request.test.ts

@@ -175,3 +175,239 @@ 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", () => {
+    class MyRequest extends Request {
+      get url() {
+        return "http://localhost/from-getter";
+      }
+      get headers() {
+        return new Headers({ "x-from-getter": "1" });
+      }
+      get redirect() {
+        return "error";
+      }
+    }
+
+    const input = new MyRequest("http://localhost/original", { headers: { "x-real": "1" } });
+    const copy = new Request(input, { method: "POST" });
+    expect({
+      url: copy.url,
+      method: copy.method,
+      headers: [...copy.headers],
+      redirect: copy.redirect,
+    }).toEqual({
+      url: "http://localhost/original",
+      method: "POST",
+      headers: [["x-real", "1"]],
+      redirect: "follow",
+    });
+  });
+
+  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 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,
+    });
+  });
+});