diff --git a/package.json b/package.json index f931ac5442..60b26fe914 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/bad-data-error.ts b/src/lib/error/bad-data-error.ts index b9c2ff358f..2c1f61aec9 100644 --- a/src/lib/error/bad-data-error.ts +++ b/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.', + ); + } +}; diff --git a/src/lib/error/from-legacy-error.ts b/src/lib/error/from-legacy-error.ts new file mode 100644 index 0000000000..c90e09b076 --- /dev/null +++ b/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, + }); +}; diff --git a/src/lib/error/unleash-error.test.ts b/src/lib/error/unleash-error.test.ts index 3369d10840..5f6a03f4cd 100644 --- a/src/lib/error/unleash-error.test.ts +++ b/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', () => { diff --git a/src/lib/error/unleash-error.ts b/src/lib/error/unleash-error.ts index 8356c87a82..5673958ae3 100644 --- a/src/lib/error/unleash-error.ts +++ b/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; diff --git a/src/lib/routes/admin-api/strategy.test.ts b/src/lib/routes/admin-api/strategy.test.ts index f1d13402f3..62542d33bb 100644 --- a/src/lib/routes/admin-api/strategy.test.ts +++ b/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', ); }); diff --git a/src/lib/routes/util.ts b/src/lib/routes/util.ts index d12099332d..9d29437731 100644 --- a/src/lib/routes/util.ts +++ b/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( - `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/src/test/e2e/api/admin/tag-types.e2e.test.ts b/src/test/e2e/api/admin/tag-types.e2e.test.ts index 6f1173a1ce..926d5baa7a 100644 --- a/src/test/e2e/api/admin/tag-types.e2e.test.ts +++ b/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', ); }); diff --git a/src/test/e2e/api/admin/tags.e2e.test.ts b/src/test/e2e/api/admin/tags.e2e.test.ts index 0e76428cce..fc13372ff0 100644 --- a/src/test/e2e/api/admin/tags.e2e.test.ts +++ b/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', ); })); diff --git a/yarn.lock b/yarn.lock index e84874ac2c..7fa4ba65d0 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==