From 40a7c11ce3f0d9c5416eb7d54ab529b80a4847dc Mon Sep 17 00:00:00 2001
From: Anmol Sethi <hi@nhooyr.io>
Date: Fri, 13 Nov 2020 15:32:47 -0500
Subject: [PATCH] node/routes: Fix error handling

We should always send HTML if the user agent expects it.

If they do not, they should clearly indicate such via the Accept header.

Closes #2297
---
 src/node/routes/domainProxy.ts |  4 ++--
 src/node/routes/index.ts       | 18 +++++++++++-------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/node/routes/domainProxy.ts b/src/node/routes/domainProxy.ts
index 25745955e..6b527255a 100644
--- a/src/node/routes/domainProxy.ts
+++ b/src/node/routes/domainProxy.ts
@@ -48,8 +48,8 @@ router.all("*", (req, res, next) => {
     // Assume anything that explicitly accepts text/html is a user browsing a
     // page (as opposed to an xhr request). Don't use `req.accepts()` since
     // *every* request that I've seen (in Firefox and Chromium at least)
-    // includes `*/*` making it always truthy.
-    if (typeof req.headers.accepts === "string" && req.headers.accepts.split(",").includes("text/html")) {
+    // includes `*/*` making it always truthy. Even for css/javascript.
+    if (req.headers.accept && req.headers.accept.includes("text/html")) {
       // Let the login through.
       if (/\/login\/?/.test(req.path)) {
         return next()
diff --git a/src/node/routes/index.ts b/src/node/routes/index.ts
index 2c54917da..5bb62d1ee 100644
--- a/src/node/routes/index.ts
+++ b/src/node/routes/index.ts
@@ -125,7 +125,7 @@ export const register = async (
     throw new HttpError("Not Found", HttpCode.NotFound)
   })
 
-  const errorHandler: express.ErrorRequestHandler = async (err, req, res) => {
+  const errorHandler: express.ErrorRequestHandler = async (err, req, res, next) => {
     if (err.code === "ENOENT" || err.code === "EISDIR") {
       err.status = HttpCode.NotFound
     }
@@ -133,12 +133,11 @@ export const register = async (
     const status = err.status ?? err.statusCode ?? 500
     res.status(status)
 
-    if (req.accepts("application/json")) {
-      res.json({
-        error: err.message,
-        ...(err.details || {}),
-      })
-    } else {
+    // Assume anything that explicitly accepts text/html is a user browsing a
+    // page (as opposed to an xhr request). Don't use `req.accepts()` since
+    // *every* request that I've seen (in Firefox and Chromium at least)
+    // includes `*/*` making it always truthy. Even for css/javascript.
+    if (req.headers.accept && req.headers.accept.includes("text/html")) {
       const resourcePath = path.resolve(rootPath, "src/browser/pages/error.html")
       res.set("Content-Type", getMediaMime(resourcePath))
       const content = await fs.readFile(resourcePath, "utf8")
@@ -148,6 +147,11 @@ export const register = async (
           .replace(/{{ERROR_HEADER}}/g, status)
           .replace(/{{ERROR_BODY}}/g, err.message),
       )
+    } else {
+      res.json({
+        error: err.message,
+        ...(err.details || {}),
+      })
     }
   }