reduce logging (#3925)

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",

src/lib/error/bad-data-error.ts

@@ -1,4 +1,5 @@
 import { ErrorObject } from 'ajv';
+import { ValidationError } from 'joi';
 import getProp from 'lodash.get';
 import { ApiErrorSchema, UnleashError } from './unleash-error';
 
@@ -17,7 +18,7 @@ class BadDataError extends UnleashError {
         const topLevelMessage =
             'Request validation failed: your request body contains invalid data' +
             (errors
-                ? '. Refer to the `details` list to see what happened.'
+                ? '. Refer to the `details` list for more information.'
                 : `: ${message}`);
         super(topLevelMessage);
 
@@ -121,3 +122,29 @@ export const fromOpenApiValidationErrors = (
         [firstDetail, ...remainingDetails],
     );
 };
+
+export const fromJoiError = (err: ValidationError): BadDataError => {
+    const details = err.details.map((detail) => {
+        const messageEnd = detail.context?.value
+            ? `. You provided ${JSON.stringify(detail.context.value)}.`
+            : '.';
+        const description = detail.message + messageEnd;
+        return {
+            description,
+            message: description,
+        };
+    });
+
+    const [first, ...rest] = details;
+
+    if (first) {
+        return new BadDataError(
+            'A validation error occurred while processing your request data. Refer to the `details` property for more information.',
+            [first, ...rest],
+        );
+    } else {
+        return new BadDataError(
+            'A validation error occurred while processing your request data. Please make sure it conforms to the request data schema.',
+        );
+    }
+};

src/lib/error/from-legacy-error.ts

@@ -0,0 +1,54 @@
+import { fromJoiError } from './bad-data-error';
+import { ValidationError as JoiValidationError } from 'joi';
+import {
+    GenericUnleashError,
+    UnleashApiErrorName,
+    UnleashApiErrorTypes,
+    UnleashError,
+} from './unleash-error';
+
+export const fromLegacyError = (e: Error): UnleashError => {
+    if (e instanceof UnleashError) {
+        return e;
+    }
+    const name = UnleashApiErrorTypes.includes(e.name as UnleashApiErrorName)
+        ? (e.name as UnleashApiErrorName)
+        : 'UnknownError';
+
+    if (name === 'NoAccessError') {
+        return new GenericUnleashError({
+            name: 'NoAccessError',
+            message: e.message,
+        });
+    }
+
+    if (e instanceof JoiValidationError) {
+        return fromJoiError(e);
+    }
+
+    if (name === 'ValidationError' || name === 'BadDataError') {
+        return new GenericUnleashError({
+            name: 'BadDataError',
+            message: e.message,
+        });
+    }
+
+    if (name === 'OwaspValidationError') {
+        return new GenericUnleashError({
+            name: 'OwaspValidationError',
+            message: e.message,
+        });
+    }
+
+    if (name === 'AuthenticationRequired') {
+        return new GenericUnleashError({
+            name: 'AuthenticationRequired',
+            message: `You must be authenticated to view this content. Please log in.`,
+        });
+    }
+
+    return new GenericUnleashError({
+        name: name as UnleashApiErrorName,
+        message: e.message,
+    });
+};

src/lib/error/unleash-error.test.ts

@@ -1,7 +1,7 @@
 import owasp from 'owasp-password-strength-test';
 import { ErrorObject } from 'ajv';
 import AuthenticationRequired from '../types/authentication-required';
-import { ApiErrorSchema, fromLegacyError } from './unleash-error';
+import { ApiErrorSchema } from './unleash-error';
 import BadDataError, {
     fromOpenApiValidationError,
     fromOpenApiValidationErrors,
@@ -12,6 +12,8 @@ import IncompatibleProjectError from './incompatible-project-error';
 import PasswordUndefinedError from './password-undefined';
 import ProjectWithoutOwnerError from './project-without-owner-error';
 import NotFoundError from './notfound-error';
+import { validateString } from '../util/validators/constraint-types';
+import { fromLegacyError } from './from-legacy-error';
 
 describe('v5 deprecation: backwards compatibility', () => {
     it(`Adds details to error types that don't specify it`, () => {
@@ -352,6 +354,32 @@ describe('Error serialization special cases', () => {
             ],
         });
     });
+
+    it('Converts Joi errors in a sensible fashion', async () => {
+        // if the validation doesn't fail, this test does nothing, so ensure
+        // that an error is thrown.
+        let validationThrewAnError = false;
+        try {
+            await validateString([]);
+        } catch (e) {
+            validationThrewAnError = true;
+            const convertedError = fromLegacyError(e);
+
+            expect(convertedError.toJSON()).toMatchObject({
+                message:
+                    expect.stringContaining('validation error') &&
+                    expect.stringContaining('details'),
+                details: [
+                    {
+                        description:
+                            '"value" must contain at least 1 items. You provided [].',
+                    },
+                ],
+            });
+        }
+
+        expect(validationThrewAnError).toBeTruthy();
+    });
 });
 
 describe('Error serialization special cases', () => {

src/lib/error/unleash-error.ts

@@ -1,7 +1,7 @@
 import { v4 as uuidV4 } from 'uuid';
 import { FromSchema } from 'json-schema-to-ts';
 
-const UnleashApiErrorTypes = [
+export const UnleashApiErrorTypes = [
     'ContentTypeError',
     'DisabledError',
     'FeatureHasTagError',
@@ -32,7 +32,7 @@ const UnleashApiErrorTypes = [
     'InternalError',
 ] as const;
 
-type UnleashApiErrorName = typeof UnleashApiErrorTypes[number];
+export type UnleashApiErrorName = typeof UnleashApiErrorTypes[number];
 
 const statusCode = (errorName: string): number => {
     switch (errorName) {
@@ -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;
     }
@@ -127,7 +129,7 @@ export abstract class UnleashError extends Error {
     }
 }
 
-class GenericUnleashError extends UnleashError {
+export class GenericUnleashError extends UnleashError {
     constructor({
         name,
         message,
@@ -168,46 +170,4 @@ export const apiErrorSchema = {
     components: {},
 } as const;
 
-export const fromLegacyError = (e: Error): UnleashError => {
-    if (e instanceof UnleashError) {
-        return e;
-    }
-    const name = UnleashApiErrorTypes.includes(e.name as UnleashApiErrorName)
-        ? (e.name as UnleashApiErrorName)
-        : 'UnknownError';
-
-    if (name === 'NoAccessError') {
-        return new GenericUnleashError({
-            name: 'NoAccessError',
-            message: e.message,
-        });
-    }
-
-    if (name === 'ValidationError' || name === 'BadDataError') {
-        return new GenericUnleashError({
-            name: 'BadDataError',
-            message: e.message,
-        });
-    }
-
-    if (name === 'OwaspValidationError') {
-        return new GenericUnleashError({
-            name: 'OwaspValidationError',
-            message: e.message,
-        });
-    }
-
-    if (name === 'AuthenticationRequired') {
-        return new GenericUnleashError({
-            name: 'AuthenticationRequired',
-            message: `You must be authenticated to view this content. Please log in.`,
-        });
-    }
-
-    return new GenericUnleashError({
-        name: name as UnleashApiErrorName,
-        message: e.message,
-    });
-};
-
 export type ApiErrorSchema = FromSchema<typeof apiErrorSchema>;

src/lib/routes/admin-api/strategy.test.ts

@@ -68,7 +68,7 @@ test('require parameters array when creating a new strategy', async () => {
         .send({ name: 'TestStrat' })
         .expect(400)
         .expect((res) => {
-            expect(res.body.details[0].description).toEqual(
+            expect(res.body.details[0].description).toMatch(
                 '"parameters" is required',
             );
         });

src/lib/routes/util.ts

@@ -1,7 +1,9 @@
 import joi from 'joi';
 import { Response } from 'express';
 import { Logger } from '../logger';
-import { fromLegacyError, UnleashError } from '../error/unleash-error';
+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',
@@ -26,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);
 
@@ -39,15 +52,7 @@ export const handleErrors: (
         );
     }
 
-    logger.warn(
+    if (finalError.statusCode === 500) {
-        `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)) {
         logger.error(
             `Server failed executing request: ${format(error)}`,
             error,

src/test/e2e/api/admin/tag-types.e2e.test.ts

@@ -77,7 +77,7 @@ test('Invalid tag types gets rejected', async () => {
         .set('Content-Type', 'application/json')
         .expect(400)
         .expect((res) => {
-            expect(res.body.details[0].description).toBe(
+            expect(res.body.details[0].description).toMatch(
                 '"name" must be URL friendly',
             );
         });
@@ -151,7 +151,7 @@ test('Invalid tag-types get refused by validator', async () => {
         .set('Content-Type', 'application/json')
         .expect(400)
         .expect((res) => {
-            expect(res.body.details[0].description).toBe(
+            expect(res.body.details[0].description).toMatch(
                 '"name" must be URL friendly',
             );
         });

src/test/e2e/api/admin/tags.e2e.test.ts

@@ -86,7 +86,7 @@ test('Can validate a tag', async () =>
         .expect(400)
         .expect((res) => {
             expect(res.body.details.length).toBe(1);
-            expect(res.body.details[0].description).toBe(
+            expect(res.body.details[0].description).toMatch(
                 '"type" must be URL friendly',
             );
         }));

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==