From 19bcd043d7b5aba029f582ddf776e8336e02a445 Mon Sep 17 00:00:00 2001
From: Asher <ash@coder.com>
Date: Thu, 30 Mar 2023 09:24:33 -0800
Subject: [PATCH] Add debug log for origin check (#6096)

Extracted host detection into a separate function to avoid multiple log
lines on each return and went with a thrown error to consolidate the
common log text.
---
 src/node/http.ts            | 54 +++++++++++++++++++++++--------------
 test/unit/node/http.test.ts | 21 ++++++++++-----
 2 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/src/node/http.ts b/src/node/http.ts
index 33575c0e1..0d9e2f6f1 100644
--- a/src/node/http.ts
+++ b/src/node/http.ts
@@ -323,35 +323,57 @@ function getFirstHeader(req: http.IncomingMessage, headerName: string): string |
 }
 
 /**
- * Throw an error if origin checks fail. Call `next` if provided.
+ * Throw a forbidden error if origin checks fail. Call `next` if provided.
  */
 export function ensureOrigin(req: express.Request, _?: express.Response, next?: express.NextFunction): void {
-  if (!authenticateOrigin(req)) {
+  try {
+    authenticateOrigin(req)
+    if (next) {
+      next()
+    }
+  } catch (error) {
+    logger.debug(`${error instanceof Error ? error.message : error}; blocking request to ${req.originalUrl}`)
     throw new HttpError("Forbidden", HttpCode.Forbidden)
   }
-  if (next) {
-    next()
-  }
 }
 
 /**
- * Authenticate the request origin against the host.
+ * Authenticate the request origin against the host.  Throw if invalid.
  */
-export function authenticateOrigin(req: express.Request): boolean {
+export function authenticateOrigin(req: express.Request): void {
   // A missing origin probably means the source is non-browser.  Not sure we
   // have a use case for this but let it through.
   const originRaw = getFirstHeader(req, "origin")
   if (!originRaw) {
-    return true
+    return
   }
 
   let origin: string
   try {
     origin = new URL(originRaw).host.trim().toLowerCase()
   } catch (error) {
-    return false // Malformed URL.
+    throw new Error(`unable to parse malformed origin "${originRaw}"`)
   }
 
+  const host = getHost(req)
+  if (typeof host === "undefined") {
+    // A missing host likely means the reverse proxy has not been configured to
+    // forward the host which means we cannot perform the check.  Emit an error
+    // so an admin can fix the issue.
+    logger.error("No host headers found")
+    logger.error("Are you behind a reverse proxy that does not forward the host?")
+    throw new Error("no host headers found")
+  }
+
+  if (host !== origin) {
+    throw new Error(`host "${host}" does not match origin "${origin}"`)
+  }
+}
+
+/**
+ * Get the host from headers.  It will be trimmed and lowercased.
+ */
+function getHost(req: express.Request): string | undefined {
   // Honor Forwarded if present.
   const forwardedRaw = getFirstHeader(req, "forwarded")
   if (forwardedRaw) {
@@ -359,7 +381,7 @@ export function authenticateOrigin(req: express.Request): boolean {
     for (let i = 0; i < parts.length; ++i) {
       const [key, value] = splitOnFirstEquals(parts[i])
       if (key.trim().toLowerCase() === "host" && value) {
-        return origin === value.trim().toLowerCase()
+        return value.trim().toLowerCase()
       }
     }
   }
@@ -367,17 +389,9 @@ export function authenticateOrigin(req: express.Request): boolean {
   // Honor X-Forwarded-Host if present.
   const xHost = getFirstHeader(req, "x-forwarded-host")
   if (xHost) {
-    return origin === xHost.trim().toLowerCase()
+    return xHost.trim().toLowerCase()
   }
 
-  // A missing host likely means the reverse proxy has not been configured to
-  // forward the host which means we cannot perform the check.  Emit a warning
-  // so an admin can fix the issue.
   const host = getFirstHeader(req, "host")
-  if (!host) {
-    logger.warn(`no host headers found; blocking request to ${req.originalUrl}`)
-    return false
-  }
-
-  return origin === host.trim().toLowerCase()
+  return host ? host.trim().toLowerCase() : undefined
 }
diff --git a/test/unit/node/http.test.ts b/test/unit/node/http.test.ts
index 9c4626cc2..d9f0271e3 100644
--- a/test/unit/node/http.test.ts
+++ b/test/unit/node/http.test.ts
@@ -24,32 +24,35 @@ describe("http", () => {
       {
         origin: "",
         host: "",
-        expected: true,
       },
       {
         origin: "http://localhost:8080",
         host: "",
-        expected: false,
+        expected: "no host headers",
+      },
+      {
+        origin: "http://localhost:8080",
+        host: " ",
+        expected: "does not match",
       },
       {
         origin: "http://localhost:8080",
         host: "localhost:8080",
-        expected: true,
       },
       {
         origin: "http://localhost:8080",
         host: "localhost:8081",
-        expected: false,
+        expected: "does not match",
       },
       {
         origin: "localhost:8080",
         host: "localhost:8080",
-        expected: false, // Gets parsed as host: localhost and path: 8080.
+        expected: "does not match", // Gets parsed as host: localhost and path: 8080.
       },
       {
         origin: "test.org",
         host: "localhost:8080",
-        expected: false, // Parsing fails completely.
+        expected: "malformed", // Parsing fails completely.
       },
     ].forEach((test) => {
       ;[
@@ -67,7 +70,11 @@ describe("http", () => {
               [key]: value,
             },
           })
-          expect(http.authenticateOrigin(req)).toBe(test.expected)
+          if (typeof test.expected === "string") {
+            expect(() => http.authenticateOrigin(req)).toThrow(test.expected)
+          } else {
+            expect(() => http.authenticateOrigin(req)).not.toThrow()
+          }
         })
       })
     })