From e4593873a9c791238685dfbb45e64b4485884174 Mon Sep 17 00:00:00 2001
From: Aravind <63452117+codesculpture@users.noreply.github.com>
Date: Wed, 8 Nov 2023 04:22:44 +0530
Subject: [PATCH] fix(ext/http): Throwing Error if the return value of
 Deno.serve handler is not a Response class (#21099)

---------

Co-authored-by: Matt Mastracci <matthew@mastracci.com>
---
 cli/tests/unit/serve_test.ts | 67 ++++++++++++++++++++++++++++++++++++
 ext/http/00_serve.js         | 14 +++++++-
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/cli/tests/unit/serve_test.ts b/cli/tests/unit/serve_test.ts
index 9f6bd4aa16..59334039ad 100644
--- a/cli/tests/unit/serve_test.ts
+++ b/cli/tests/unit/serve_test.ts
@@ -3805,3 +3805,70 @@ Deno.test(
     await server.finished;
   },
 );
+
+// serve Handler must return Response class or promise that resolves Response class
+Deno.test(
+  { permissions: { net: true, run: true } },
+  async function handleServeCallbackReturn() {
+    const d = deferred();
+    const listeningPromise = deferred();
+    const ac = new AbortController();
+
+    const server = Deno.serve(
+      {
+        port: servePort,
+        onListen: onListen(listeningPromise),
+        signal: ac.signal,
+        onError: (error) => {
+          assert(error instanceof TypeError);
+          assert(
+            error.message ===
+              "Return value from serve handler must be a response or a promise resolving to a response",
+          );
+          d.resolve();
+          return new Response("Customized Internal Error from onError");
+        },
+      },
+      () => {
+        // Trick the typechecker
+        return <Response> <unknown> undefined;
+      },
+    );
+    await listeningPromise;
+    const respText = await curlRequest([`http://localhost:${servePort}`]);
+    await d;
+    ac.abort();
+    await server.finished;
+    assert(respText === "Customized Internal Error from onError");
+  },
+);
+
+// onError Handler must return Response class or promise that resolves Response class
+Deno.test(
+  { permissions: { net: true, run: true } },
+  async function handleServeErrorCallbackReturn() {
+    const listeningPromise = deferred();
+    const ac = new AbortController();
+
+    const server = Deno.serve(
+      {
+        port: servePort,
+        onListen: onListen(listeningPromise),
+        signal: ac.signal,
+        onError: () => {
+          // Trick the typechecker
+          return <Response> <unknown> undefined;
+        },
+      },
+      () => {
+        // Trick the typechecker
+        return <Response> <unknown> undefined;
+      },
+    );
+    await listeningPromise;
+    const respText = await curlRequest([`http://localhost:${servePort}`]);
+    ac.abort();
+    await server.finished;
+    assert(respText === "Internal Server Error");
+  },
+);
diff --git a/ext/http/00_serve.js b/ext/http/00_serve.js
index b99f28c0fb..5b931ccd1e 100644
--- a/ext/http/00_serve.js
+++ b/ext/http/00_serve.js
@@ -10,6 +10,7 @@ import { Event } from "ext:deno_web/02_event.js";
 import {
   fromInnerResponse,
   newInnerResponse,
+  ResponsePrototype,
   toInnerResponse,
 } from "ext:deno_fetch/23_response.js";
 import { fromInnerRequest, toInnerRequest } from "ext:deno_fetch/23_request.js";
@@ -449,15 +450,26 @@ function mapToCallback(context, callback, onError) {
         fromInnerRequest(innerRequest, signal, "immutable"),
         new ServeHandlerInfo(innerRequest),
       );
+
+      // Throwing Error if the handler return value is not a Response class
+      if (!ObjectPrototypeIsPrototypeOf(ResponsePrototype, response)) {
+        throw TypeError(
+          "Return value from serve handler must be a response or a promise resolving to a response",
+        );
+      }
     } catch (error) {
       try {
         response = await onError(error);
+        if (!ObjectPrototypeIsPrototypeOf(ResponsePrototype, response)) {
+          throw TypeError(
+            "Return value from onError handler must be a response or a promise resolving to a response",
+          );
+        }
       } catch (error) {
         console.error("Exception in onError while handling exception", error);
         response = internalServerError();
       }
     }
-
     const inner = toInnerResponse(response);
     if (innerRequest?.[_upgraded]) {
       // We're done here as the connection has been upgraded during the callback and no longer requires servicing.