From 51d73f67a3511c1c920c7bc8902db3abc364c27c Mon Sep 17 00:00:00 2001
From: Thomas Heartman <thomas@getunleash.ai>
Date: Thu, 8 Jun 2023 13:14:53 +0200
Subject: [PATCH] fix: propagate http-errors as they are (#3922)

This PR aims to handle the increased log alarm volume seen by the SREs.

It appears that we get a large number of alarms because a client
disconnects early from the front-end API. These errors are then
converted into 500s because of missing handling. These errors appear to
be caused by the `http-errors` library in a dependency.

We also introduced a log line whenever we see errors now a while back,
and I don't think we need this logging (I was also the one who
introduced it).

The changes in this PR are specifically:
- When converting from arbitrary errors, give `BadRequestError` a 400
status code, not a 500.
- Add a dependency on `http-errors` (which is already a transitive
dependency because of the body parser) and use that to check whether an
error is an http-error.
- If an error is an http error, then propagate it to the user with the
status code and message.
- Remove warning logs when an error occurs. This was introduced to make
it easier to correlate an API error and the logs, but the system hasn't
been set up for that (yet?), so it's just noise now.
- When logging errors as errors, only do that if their status code would
be 500.
---
 package.json                   |  1 +
 src/lib/error/unleash-error.ts |  2 ++
 src/lib/routes/util.ts         | 22 +++++++++++++---------
 yarn.lock                      |  2 +-
 4 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/package.json b/package.json
index 338c63a014..52034f53b6 100644
--- a/package.json
+++ b/package.json
@@ -127,6 +127,7 @@
     "gravatar-url": "^3.1.0",
     "hash-sum": "^2.0.0",
     "helmet": "^6.0.0",
+    "http-errors": "^2.0.0",
     "ip": "^1.1.8",
     "joi": "^17.3.0",
     "js-sha256": "^0.9.0",
diff --git a/src/lib/error/unleash-error.ts b/src/lib/error/unleash-error.ts
index d8ccfafa54..5673958ae3 100644
--- a/src/lib/error/unleash-error.ts
+++ b/src/lib/error/unleash-error.ts
@@ -86,6 +86,8 @@ const statusCode = (errorName: string): number => {
             return 403;
         case 'AuthenticationRequired':
             return 401;
+        case 'BadRequestError': //thrown by express; do not remove
+            return 400;
         default:
             return 500;
     }
diff --git a/src/lib/routes/util.ts b/src/lib/routes/util.ts
index af8b81b5f3..9d29437731 100644
--- a/src/lib/routes/util.ts
+++ b/src/lib/routes/util.ts
@@ -3,6 +3,7 @@ import { Response } from 'express';
 import { Logger } from '../logger';
 import { UnleashError } from '../error/unleash-error';
 import { fromLegacyError } from '../error/from-legacy-error';
+import createError from 'http-errors';
 
 export const customJoi = joi.extend((j) => ({
     type: 'isUrlFriendly',
@@ -27,6 +28,17 @@ export const handleErrors: (
     logger: Logger,
     error: Error,
 ) => void = (res, logger, error) => {
+    if (createError.isHttpError(error)) {
+        return (
+            res
+                // @ts-expect-error http errors all have statuses, but there are no
+                // types provided
+                .status(error.status ?? 400)
+                .json({ message: error.message })
+                .end()
+        );
+    }
+
     const finalError =
         error instanceof UnleashError ? error : fromLegacyError(error);
 
@@ -40,15 +52,7 @@ export const handleErrors: (
         );
     }
 
-    logger.warn(
-        `Original error message: ${error.message}. Processed error message: "${
-            finalError.message
-        }" Error ID: "${finalError.id}". Full, serialized error: ${format(
-            finalError.toJSON(),
-        )}`,
-    );
-
-    if (['InternalError', 'UnknownError'].includes(finalError.name)) {
+    if (finalError.statusCode === 500) {
         logger.error(
             `Server failed executing request: ${format(error)}`,
             error,
diff --git a/yarn.lock b/yarn.lock
index a196679acb..d095e07e17 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -3851,7 +3851,7 @@ http-cache-semantics@^4.1.0, http-cache-semantics@^4.1.1:
   resolved "https://registry.yarnpkg.com/http-cache-semantics/-/http-cache-semantics-4.1.1.tgz#abe02fcb2985460bf0323be664436ec3476a6d5a"
   integrity sha512-er295DKPVsV82j5kw1Gjt+ADA/XYHsajl82cGNQG2eyoPkvgUhX+nDIyelzhIWbbsXP39EHcI6l5tYs2FYqYXQ==
 
-http-errors@2.0.0:
+http-errors@2.0.0, http-errors@^2.0.0:
   version "2.0.0"
   resolved "https://registry.yarnpkg.com/http-errors/-/http-errors-2.0.0.tgz#b7774a1486ef73cf7667ac9ae0858c012c57b9d3"
   integrity sha512-FtwrG/euBzaEjYeRqOgly7G0qviiXoJWnvEH2Z1plBdXgbyjv34pHTSb9zoeHMyDy33+DWy5Wt9Wo+TURtOYSQ==