Make fetch API more standards compliant (#3667)

5 files changed, 194 insertions, 9 deletions

diff --git a/cli/js/dom_types.ts b/cli/js/dom_types.ts
index e32b5f0f1..1e6c622c8 100644
--- a/cli/js/dom_types.ts
+++ b/cli/js/dom_types.ts
@@ -530,8 +530,8 @@ type RequestDestination =
   | "worker"
   | "xslt";
 type RequestMode = "navigate" | "same-origin" | "no-cors" | "cors";
-type RequestRedirect = "follow" | "error" | "manual";
-type ResponseType =
+type RequestRedirect = "follow" | "nofollow" | "error" | "manual";
+export type ResponseType =
   | "basic"
   | "cors"
   | "default"
diff --git a/cli/js/fetch.ts b/cli/js/fetch.ts
index 929b075fb..43a23cb59 100644
--- a/cli/js/fetch.ts
+++ b/cli/js/fetch.ts
@@ -252,11 +252,11 @@ class Body implements domTypes.Body, domTypes.ReadableStream, io.ReadCloser {
 }
 
 export class Response implements domTypes.Response {
-  readonly type = "basic"; // TODO
+  readonly type: domTypes.ResponseType;
   readonly redirected: boolean;
   headers: domTypes.Headers;
   readonly trailer: Promise<domTypes.Headers>;
-  readonly body: Body;
+  readonly body: null | Body;
 
   constructor(
     readonly url: string,
@@ -265,6 +265,7 @@ export class Response implements domTypes.Response {
     headersList: Array<[string, string]>,
     rid: number,
     redirected_: boolean,
+    readonly type_: null | domTypes.ResponseType = "default",
     body_: null | Body = null
   ) {
     this.trailer = createResolvable();
@@ -277,27 +278,112 @@ export class Response implements domTypes.Response {
       this.body = body_;
     }
 
+    if (type_ == null) {
+      this.type = "default";
+    } else {
+      this.type = type_;
+      if (type_ == "error") {
+        // spec: https://fetch.spec.whatwg.org/#concept-network-error
+        this.status = 0;
+        this.statusText = "";
+        this.headers = new Headers();
+        this.body = null;
+        /* spec for other Response types:
+           https://fetch.spec.whatwg.org/#concept-filtered-response-basic
+           Please note that type "basic" is not the same thing as "default".*/
+      } else if (type_ == "basic") {
+        for (const h of this.headers) {
+          /* Forbidden Response-Header Names:
+             https://fetch.spec.whatwg.org/#forbidden-response-header-name */
+          if (["set-cookie", "set-cookie2"].includes(h[0].toLowerCase())) {
+            this.headers.delete(h[0]);
+          }
+        }
+      } else if (type_ == "cors") {
+        /* CORS-safelisted Response-Header Names:
+             https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name */
+        const allowedHeaders = [
+          "Cache-Control",
+          "Content-Language",
+          "Content-Length",
+          "Content-Type",
+          "Expires",
+          "Last-Modified",
+          "Pragma"
+        ].map((c: string) => c.toLowerCase());
+        for (const h of this.headers) {
+          /* Technically this is still not standards compliant because we are
+             supposed to allow headers allowed in the
+             'Access-Control-Expose-Headers' header in the 'internal response'
+             However, this implementation of response doesn't seem to have an
+             easy way to access the internal response, so we ignore that
+             header.
+             TODO(serverhiccups): change how internal responses are handled
+             so we can do this properly. */
+          if (!allowedHeaders.includes(h[0].toLowerCase())) {
+            this.headers.delete(h[0]);
+          }
+        }
+        /* TODO(serverhiccups): Once I fix the 'internal response' thing,
+           these actually need to treat the internal response differently */
+      } else if (type_ == "opaque" || type_ == "opaqueredirect") {
+        this.url = "";
+        this.status = 0;
+        this.statusText = "";
+        this.headers = new Headers();
+        this.body = null;
+      }
+    }
+
     this.redirected = redirected_;
   }
 
+  private bodyViewable(): boolean {
+    if (
+      this.type == "error" ||
+      this.type == "opaque" ||
+      this.type == "opaqueredirect" ||
+      this.body == undefined
+    )
+      return true;
+    return false;
+  }
+
   async arrayBuffer(): Promise<ArrayBuffer> {
+    /* You have to do the null check here and not in the function because
+     * otherwise TS complains about this.body potentially being null */
+    if (this.bodyViewable() || this.body == null) {
+      return Promise.reject(new Error("Response body is null"));
+    }
     return this.body.arrayBuffer();
   }
 
   async blob(): Promise<domTypes.Blob> {
+    if (this.bodyViewable() || this.body == null) {
+      return Promise.reject(new Error("Response body is null"));
+    }
     return this.body.blob();
   }
 
   async formData(): Promise<domTypes.FormData> {
+    if (this.bodyViewable() || this.body == null) {
+      return Promise.reject(new Error("Response body is null"));
+    }
     return this.body.formData();
   }
 
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   async json(): Promise<any> {
+    if (this.bodyViewable() || this.body == null) {
+      return Promise.reject(new Error("Response body is null"));
+    }
     return this.body.json();
   }
 
   async text(): Promise<string> {
+    if (this.bodyViewable() || this.body == null) {
+      return Promise.reject(new Error("Response body is null"));
+    }
     return this.body.text();
   }
 
@@ -306,6 +392,7 @@ export class Response implements domTypes.Response {
   }
 
   get bodyUsed(): boolean {
+    if (this.body === null) return false;
     return this.body.bodyUsed;
   }
 
@@ -329,9 +416,28 @@ export class Response implements domTypes.Response {
       headersList,
       -1,
       this.redirected,
+      this.type,
       this.body
     );
   }
+
+  redirect(url: URL | string, status: number): domTypes.Response {
+    if (![301, 302, 303, 307, 308].includes(status)) {
+      throw new RangeError(
+        "The redirection status must be one of 301, 302, 303, 307 and 308."
+      );
+    }
+    return new Response(
+      "",
+      status,
+      "",
+      [["Location", typeof url === "string" ? url : url.toString()]],
+      -1,
+      false,
+      "default",
+      null
+    );
+  }
 }
 
 interface FetchResponse {
@@ -445,9 +551,11 @@ export async function fetch(
       // We're in a redirect status
       switch ((init && init.redirect) || "follow") {
         case "error":
-          throw notImplemented();
+          /* I suspect that deno will probably crash if you try to use that
+             rid, which suggests to me that Response needs to be refactored */
+          return new Response("", 0, "", [], -1, false, "error", null);
         case "manual":
-          throw notImplemented();
+          return new Response("", 0, "", [], -1, false, "opaqueredirect", null);
         case "follow":
         default:
           let redirectUrl = response.headers.get("Location");
diff --git a/cli/js/fetch_test.ts b/cli/js/fetch_test.ts
index b8a7c8641..2b3d03a38 100644
--- a/cli/js/fetch_test.ts
+++ b/cli/js/fetch_test.ts
@@ -5,7 +5,8 @@ import {
   assert,
   assertEquals,
   assertStrContains,
-  assertThrows
+  assertThrows,
+  fail
 } from "./test_util.ts";
 
 testPerm({ net: true }, async function fetchConnectionError(): Promise<void> {
@@ -360,3 +361,75 @@ testPerm({ net: true }, async function fetchPostBodyTypedArray():Promise<void> {
   assertEquals(actual, expected);
 });
 */
+
+testPerm({ net: true }, async function fetchWithManualRedirection(): Promise<
+  void
+> {
+  const response = await fetch("http://localhost:4546/", {
+    redirect: "manual"
+  }); // will redirect to http://localhost:4545/
+  assertEquals(response.status, 0);
+  assertEquals(response.statusText, "");
+  assertEquals(response.url, "");
+  assertEquals(response.type, "opaqueredirect");
+  try {
+    await response.text();
+    fail(
+      "Reponse.text() didn't throw on a filtered response without a body (type opaqueredirect)"
+    );
+  } catch (e) {
+    return;
+  }
+});
+
+testPerm({ net: true }, async function fetchWithErrorRedirection(): Promise<
+  void
+> {
+  const response = await fetch("http://localhost:4546/", {
+    redirect: "error"
+  }); // will redirect to http://localhost:4545/
+  assertEquals(response.status, 0);
+  assertEquals(response.statusText, "");
+  assertEquals(response.url, "");
+  assertEquals(response.type, "error");
+  try {
+    await response.text();
+    fail(
+      "Reponse.text() didn't throw on a filtered response without a body (type error)"
+    );
+  } catch (e) {
+    return;
+  }
+});
+
+test(function responseRedirect(): void {
+  const response = new Response(
+    "example.com/beforeredirect",
+    200,
+    "OK",
+    [["This-Should", "Disappear"]],
+    -1,
+    false,
+    null
+  );
+  const redir = response.redirect("example.com/newLocation", 301);
+  assertEquals(redir.status, 301);
+  assertEquals(redir.statusText, "");
+  assertEquals(redir.url, "");
+  assertEquals(redir.headers.get("Location"), "example.com/newLocation");
+  assertEquals(redir.type, "default");
+});
+
+test(function responseConstructionHeaderRemoval(): void {
+  const res = new Response(
+    "example.com",
+    200,
+    "OK",
+    [["Set-Cookie", "mysessionid"]],
+    -1,
+    false,
+    "basic",
+    null
+  );
+  assert(res.headers.get("Set-Cookie") != "mysessionid");
+});
diff --git a/cli/js/lib.deno.shared_globals.d.ts b/cli/js/lib.deno.shared_globals.d.ts
index c8dfc833d..722bc6a49 100644
--- a/cli/js/lib.deno.shared_globals.d.ts
+++ b/cli/js/lib.deno.shared_globals.d.ts
@@ -1083,7 +1083,7 @@ declare namespace __fetch {
     readonly url: string;
     readonly status: number;
     statusText: string;
-    readonly type = "basic";
+    readonly type: __domTypes.ResponseType;
     readonly redirected: boolean;
     headers: __domTypes.Headers;
     readonly trailer: Promise<__domTypes.Headers>;
@@ -1092,9 +1092,11 @@ declare namespace __fetch {
     constructor(
       url: string,
       status: number,
+      statusText: string,
       headersList: Array<[string, string]>,
       rid: number,
       redirected_: boolean,
+      type_?: null | __domTypes.ResponseType,
       body_?: null | Body
     );
     arrayBuffer(): Promise<ArrayBuffer>;
@@ -1104,6 +1106,7 @@ declare namespace __fetch {
     text(): Promise<string>;
     readonly ok: boolean;
     clone(): __domTypes.Response;
+    redirect(url: URL | string, status: number): __domTypes.Response;
   }
   /** Fetch a resource from the network. */
   export function fetch(
diff --git a/cli/js/test_util.ts b/cli/js/test_util.ts
index 9bbc74783..4115bb5de 100644
--- a/cli/js/test_util.ts
+++ b/cli/js/test_util.ts
@@ -17,7 +17,8 @@ export {
   assertNotEquals,
   assertStrictEq,
   assertStrContains,
-  unreachable
+  unreachable,
+  fail
 } from "../../std/testing/asserts.ts";
 
 interface TestPermissions {