Response.redirect/json: check status the same way on the number and ResponseInit forms

src/runtime/webcore/Response.rs

@@ -916,14 +916,10 @@ impl Response {
             if arg_init.is_undefined_or_null() {
                 // no-op
             } else if arg_init.is_number() {
-                response.init.with_mut(|i| {
-                    i.status_code =
-                        u16::try_from(0.max(arg_init.to_int32()).min(i32::from(u16::MAX))).unwrap();
-                });
-            } else {
-                if let Some(init) = Init::init(global_this, arg_init)? {
-                    response.init.set(init);
-                }
+                let status = StatusRule::Response.check_value(global_this, arg_init)?;
+                response.init.with_mut(|i| i.status_code = status);
+            } else if let Some(init) = Init::init(global_this, arg_init)? {
+                response.init.set(init);
             }
         }
 
@@ -942,21 +938,6 @@ impl Response {
         Ok(unsafe { (*ptr).to_js(global_this) })
     }
 
-    fn validate_redirect_status_code(
-        global_this: &JSGlobalObject,
-        status_code: i32,
-    ) -> JsResult<u16> {
-        match status_code {
-            301 | 302 | 303 | 307 | 308 => Ok(u16::try_from(status_code).expect("int cast")),
-            _ => {
-                let err = global_this.create_range_error_instance(format_args!(
-                    "Failed to execute 'redirect' on 'Response': Invalid status code"
-                ));
-                Err(global_this.throw_value(err))
-            }
-        }
-    }
-
     pub(crate) fn construct_redirect(
         global_this: &JSGlobalObject,
         callframe: &CallFrame,
@@ -998,19 +979,15 @@ impl Response {
                 if arg_init.is_undefined_or_null() {
                     // no-op
                 } else if arg_init.is_number() {
-                    let status =
-                        Self::validate_redirect_status_code(global_this, arg_init.to_int32())?;
+                    let status = StatusRule::Redirect.check_value(global_this, arg_init)?;
                     response.init.with_mut(|i| i.status_code = status);
-                } else if let Some(init) = Init::init(global_this, arg_init)? {
-                    // cleanup is handled by Init's drop glue on `?` below
+                } else if let Some(init) =
+                    Init::init_with_rule(global_this, arg_init, StatusRule::Redirect)?
+                {
+                    // `init_with_rule` copies the status of a Response passed as `init`
+                    // without a check, so check the result, not only the `status` member.
+                    StatusRule::Redirect.check(global_this, i64::from(init.status_code))?;
                     response.init.set(init);
-
-                    let status = response.init.get().status_code;
-                    if status != 200 {
-                        let status =
-                            Self::validate_redirect_status_code(global_this, i32::from(status))?;
-                        response.init.with_mut(|i| i.status_code = status);
-                    }
                 }
             }
 
@@ -1230,9 +1207,19 @@ impl Init {
     pub(crate) fn init(
         global_this: &JSGlobalObject,
         response_init: JSValue,
+    ) -> JsResult<Option<Init>> {
+        Self::init_with_rule(global_this, response_init, StatusRule::Response)
+    }
+
+    /// `rule` supplies the default status and checks a `status` member when one is
+    /// present. A `Response` passed as `response_init` lends its status without a check.
+    pub(crate) fn init_with_rule(
+        global_this: &JSGlobalObject,
+        response_init: JSValue,
+        rule: StatusRule,
     ) -> JsResult<Option<Init>> {
         let mut result = Init {
-            status_code: 200,
+            status_code: rule.default_status(),
             ..Default::default()
         };
 
@@ -1292,16 +1279,7 @@ impl Init {
         }
 
         if let Some(status_value) = response_init.fast_get(global_this, BuiltinName::status)? {
-            let number = status_value.coerce_to_int64(global_this)?;
-            if (200 <= number && number < 600) || number == 101 {
-                result.status_code = (u32::try_from(number).expect("int cast")) as u16;
-            } else {
-                let err = global_this.create_range_error_instance(format_args!(
-                    "The status provided ({}) must be 101 or in the range of [200, 599]",
-                    number
-                ));
-                return Err(global_this.throw_value(err));
-            }
+            result.status_code = rule.check_value(global_this, status_value)?;
         }
 
         if let Some(status_text) =
@@ -1322,5 +1300,54 @@ impl Init {
     }
 }
 
+/// The statuses one constructor accepts, whether the status arrives as the number
+/// shorthand (`Response.json(data, 404)`) or as `ResponseInit.status`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub(crate) enum StatusRule {
+    /// https://fetch.spec.whatwg.org/#initialize-a-response step 1, plus 101 so a
+    /// handler can answer an upgrade.
+    Response,
+    /// https://fetch.spec.whatwg.org/#redirect-status
+    Redirect,
+}
+
+impl StatusRule {
+    pub(crate) const fn default_status(self) -> u16 {
+        match self {
+            StatusRule::Response => 200,
+            StatusRule::Redirect => 302,
+        }
+    }
+
+    pub(crate) fn check(self, global_this: &JSGlobalObject, status: i64) -> JsResult<u16> {
+        let accepted = match self {
+            StatusRule::Response => (200..=599).contains(&status) || status == 101,
+            StatusRule::Redirect => matches!(status, 301 | 302 | 303 | 307 | 308),
+        };
+        if accepted {
+            return Ok(u16::try_from(status).expect("checked range"));
+        }
+        let err = match self {
+            StatusRule::Response => global_this.create_range_error_instance(format_args!(
+                "The status provided ({status}) must be 101 or in the range of [200, 599]"
+            )),
+            StatusRule::Redirect => global_this.create_range_error_instance(format_args!(
+                "Failed to execute 'redirect' on 'Response': Invalid status code {status}"
+            )),
+        };
+        Err(global_this.throw_value(err))
+    }
+
+    /// ToNumber, truncate toward zero (non-finite is 0, as in WebIDL), then `check`.
+    pub(crate) fn check_value(self, global_this: &JSGlobalObject, value: JSValue) -> JsResult<u16> {
+        let status = match value.get_number() {
+            Some(number) if number.is_finite() => number as i64,
+            Some(_) => 0,
+            None => value.coerce_to_int64(global_this)?,
+        };
+        self.check(global_this, status)
+    }
+}
+
 // https://developer.mozilla.org/en-US/docs/Web/API/Headers
 // TODO: move to the http module. this has nothing to do with jsc or WebCore

test/js/web/fetch/response.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test } from "bun:test";
-import { normalizeBunSnapshot } from "harness";
+import { isASAN, isDebug, normalizeBunSnapshot } from "harness";
 
 test("zero args returns an otherwise empty 200 response", () => {
   const response = new Response();
@@ -47,9 +47,13 @@ describe("2-arg form", () => {
 });
 
 test("print size", () => {
-  expect(normalizeBunSnapshot(Bun.inspect(new Response(Bun.file(import.meta.filename)))), import.meta.dir)
-    .toMatchInlineSnapshot(`
-    "Response (8.0 KB) {
+  // this file's own size changes with every edit, so pin only the shape
+  const inspected = normalizeBunSnapshot(Bun.inspect(new Response(Bun.file(import.meta.filename)))).replace(
+    /^Response \(\d+(?:\.\d+)? KB\)/,
+    "Response (<size> KB)",
+  );
+  expect(inspected).toMatchInlineSnapshot(`
+    "Response (<size> KB) {
       ok: true,
       url: "",
       status: 200,
@@ -100,6 +104,54 @@ test("Response.redirect status code validation", () => {
   expect(Response.redirect("url", { status: 308 }).status).toBe(308);
 });
 
+test("Response.redirect with a ResponseInit checks status the same way as the number form", () => {
+  // no `status` member: the redirect default, not the ResponseInit default of 200
+  expect(Response.redirect("url", {}).status).toBe(302);
+  expect(Response.redirect("url", { statusText: "Found It" }).status).toBe(302);
+  const withHeaders = Response.redirect("http://x/", { headers: { "X-A": "1" } });
+  expect(withHeaders.status).toBe(302);
+  expect(withHeaders.headers.get("X-A")).toBe("1");
+  expect(withHeaders.headers.get("Location")).toBe("http://x/");
+
+  // a `status` member goes through the same check as `Response.redirect(url, status)`
+  for (const status of [301, 302, 303, 307, 308]) {
+    expect(Response.redirect("url", { status }).status).toBe(status);
+    expect(Response.redirect("url", { status: String(status) }).status).toBe(status);
+  }
+  for (const status of [0, 101, 200, 204, 304, 399, 404, 600, NaN, "200"]) {
+    expect(() => Response.redirect("url", { status })).toThrow(RangeError);
+  }
+  // a Response used as the init lends its status, which must still be a redirect status
+  expect(() => Response.redirect("url", new Response())).toThrow(RangeError);
+  expect(Response.redirect("url", Response.redirect("other", 307)).status).toBe(307);
+
+  // `status` is read once
+  let reads = 0;
+  const init = {
+    get status() {
+      reads++;
+      return 307;
+    },
+  };
+  expect(Response.redirect("url", init).status).toBe(307);
+  expect(reads).toBe(1);
+});
+
+test("Response.json(data, status) checks status the same way as Response.json(data, { status })", () => {
+  for (const status of [101, 200, 201, 204, 301, 404, 418, 500, 599]) {
+    expect(Response.json({}, status).status).toBe(status);
+    expect(Response.json({}, { status }).status).toBe(status);
+  }
+  expect(Response.json({}, 404.9).status).toBe(404);
+  for (const status of [0, -1, 99, 100, 103, 199, 600, 999, 65536 + 200, NaN, Infinity, -Infinity]) {
+    expect(() => Response.json({}, status)).toThrow(RangeError);
+    expect(() => Response.json({}, { status })).toThrow(RangeError);
+  }
+  // null and undefined still mean "no init"
+  expect(Response.json({}, null).status).toBe(200);
+  expect(Response.json({}, undefined).status).toBe(200);
+});
+
 // https://fetch.spec.whatwg.org/#dom-response-redirect
 // `Location` gets the serialization of the parsed url, not the raw input string.
 test.each([
@@ -149,19 +201,24 @@ test("new Response(123, { method: 456 }) does not throw", () => {
   expect(() => new Response("123", { method: 456 })).not.toThrow();
 });
 
-test("handle stack overflow", () => {
-  function f0(a1, a2) {
-    const v4 = new Response();
-    // @ts-ignore
-    const v5 = v4.text(a2, a2, v4, f0, f0);
-    a1(a1); // Recursive call causes stack overflow
-    return v5;
-  }
-  expect(() => {
-    // @ts-ignore
-    f0(f0);
-  }).toThrow("Maximum call stack size exceeded.");
-});
+test(
+  "handle stack overflow",
+  () => {
+    function f0(a1, a2) {
+      const v4 = new Response();
+      // @ts-ignore
+      const v5 = v4.text(a2, a2, v4, f0, f0);
+      a1(a1); // Recursive call causes stack overflow
+      return v5;
+    }
+    expect(() => {
+      // @ts-ignore
+      f0(f0);
+    }).toThrow("Maximum call stack size exceeded.");
+  },
+  // one Response and one pending text() per frame until the stack runs out: seconds under ASAN
+  isDebug || isASAN ? 30_000 : 5000,
+);
 
 describe("clone()", () => {
   test("does not lock original body when body was accessed before clone", async () => {