From 2fd22e7db78579f8716a32c1487078d4162ab6dd Mon Sep 17 00:00:00 2001 From: Thomas Dax Date: Tue, 21 Jan 2025 16:12:08 +0100 Subject: [PATCH] Add `ExceptionFilter` to replace `ExceptionInterceptor` (v6) (#3243) Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com> --- .changeset/tasty-news-change.md | 18 ++ demo/api/src/main.ts | 4 +- .../common/errors/exception.filter.spec.ts | 167 ++++++++++++++++++ .../src/common/errors/exception.filter.ts | 65 +++++++ .../common/errors/exception.interceptor.ts | 3 + packages/api/cms-api/src/index.ts | 1 + 6 files changed, 256 insertions(+), 2 deletions(-) create mode 100644 .changeset/tasty-news-change.md create mode 100644 packages/api/cms-api/src/common/errors/exception.filter.spec.ts create mode 100644 packages/api/cms-api/src/common/errors/exception.filter.ts diff --git a/.changeset/tasty-news-change.md b/.changeset/tasty-news-change.md new file mode 100644 index 0000000000..7d153b4cf1 --- /dev/null +++ b/.changeset/tasty-news-change.md @@ -0,0 +1,18 @@ +--- +"@comet/cms-api": minor +--- + +Add `ExceptionFilter` to replace `ExceptionInterceptor` + +The main motivation for this change was that the `ExceptionInterceptor` didn't capture exceptions thrown in guards. This could lead to information leaks, e.g., details about the database schema or the underlying code. This is considered a security risk. + +The `ExceptionFilter` also catches error within guards. The error format remains unchanged. + +Switching from the `ExceptionInterceptor` to the `ExceptionFilter` must be done in the project: + +```diff +// main.ts + +- app.useGlobalInterceptors(new ExceptionInterceptor(config.debug)); ++ app.useGlobalFilters(new ExceptionFilter(config.debug)); +``` diff --git a/demo/api/src/main.ts b/demo/api/src/main.ts index caf70212bd..4a285148b6 100644 --- a/demo/api/src/main.ts +++ b/demo/api/src/main.ts @@ -4,7 +4,7 @@ if (process.env.TRACING_ENABLED) { require("./tracing"); } -import { ExceptionInterceptor, ValidationExceptionFactory } from "@comet/cms-api"; +import { ExceptionFilter, ValidationExceptionFactory } from "@comet/cms-api"; import { ValidationPipe } from "@nestjs/common"; import { NestFactory } from "@nestjs/core"; import { NestExpressApplication } from "@nestjs/platform-express"; @@ -31,7 +31,7 @@ async function bootstrap(): Promise { origin: config.corsAllowedOrigins.map((val: string) => new RegExp(val)), }); - app.useGlobalInterceptors(new ExceptionInterceptor(config.debug)); + app.useGlobalFilters(new ExceptionFilter(config.debug)); app.useGlobalPipes( new ValidationPipe({ exceptionFactory: ValidationExceptionFactory, diff --git a/packages/api/cms-api/src/common/errors/exception.filter.spec.ts b/packages/api/cms-api/src/common/errors/exception.filter.spec.ts new file mode 100644 index 0000000000..d8b17a32a2 --- /dev/null +++ b/packages/api/cms-api/src/common/errors/exception.filter.spec.ts @@ -0,0 +1,167 @@ +import { ArgumentsHost, BadRequestException, HttpException, InternalServerErrorException } from "@nestjs/common"; + +import { CometEntityNotFoundException } from "./entity-not-found.exception"; +import { ExceptionFilter } from "./exception.filter"; +import { CometValidationException } from "./validation.exception"; +import Mock = jest.Mock; + +const graphQLHost = { + getType: () => "graphql", +} as ArgumentsHost; + +jest.spyOn(console, "error").mockImplementation(() => { + // noop +}); + +describe("ExceptionFilter", () => { + describe("catch", () => { + describe("graphql", () => { + it("returns BadRequestException for CometException", () => { + const exceptionFilter = new ExceptionFilter(false); + + const returnError = exceptionFilter.catch(new CometEntityNotFoundException("Not found"), graphQLHost); + + expect(returnError).toBeInstanceOf(BadRequestException); + expect((returnError as BadRequestException).getResponse()).toEqual({ + statusCode: 400, + message: "Not found", + error: "CometEntityNotFoundException", + validationErrors: [], + }); + }); + + it("returns BadRequestException for CometValidationException", () => { + const exceptionFilter = new ExceptionFilter(false); + + const returnError = exceptionFilter.catch(new CometValidationException("Invalid", [{ property: "prop1" }]), graphQLHost); + + expect(returnError).toBeInstanceOf(BadRequestException); + expect((returnError as BadRequestException).getResponse()).toEqual({ + statusCode: 400, + message: "Invalid", + error: "CometValidationException", + validationErrors: [ + { + property: "prop1", + }, + ], + }); + }); + + it("returns HttpException for HttpException", () => { + const exceptionFilter = new ExceptionFilter(false); + + const returnError = exceptionFilter.catch(new HttpException("Unauthorized", 401), graphQLHost); + + expect(returnError).toBeInstanceOf(HttpException); + expect((returnError as HttpException).getResponse()).toBe("Unauthorized"); + }); + + it("returns InternalServerErrorException for normal Error in non-debug mode", () => { + const exceptionFilter = new ExceptionFilter(false); + + const returnError = exceptionFilter.catch(new Error("Other error"), graphQLHost); + + expect(returnError).toBeInstanceOf(InternalServerErrorException); + expect((returnError as InternalServerErrorException).getResponse()).toEqual({ + message: "Internal Server Error", + statusCode: 500, + }); + }); + + it("returns real Error for normal Error in debug mode", () => { + const exceptionFilter = new ExceptionFilter(true); + + const returnError = exceptionFilter.catch(new Error("Other error"), graphQLHost); + + expect(returnError).toBeInstanceOf(Error); + expect((returnError as Error).message).toBe("Other error"); + }); + }); + + describe("http", () => { + let httpHost: ArgumentsHost; + let statusMock: Mock; + let jsonMock: Mock; + + beforeEach(() => { + statusMock = jest.fn(); + jsonMock = jest.fn(); + + httpHost = { + getType: () => "http", + switchToHttp: jest.fn(() => ({ + getResponse: jest.fn(() => ({ + status: statusMock.mockReturnThis(), + json: jsonMock, + })), + })), + } as unknown as ArgumentsHost; + }); + + it("response status is 400 and json is correct for CometException", () => { + const exceptionFilter = new ExceptionFilter(false); + + exceptionFilter.catch(new CometEntityNotFoundException("Not found"), httpHost); + + const responseMock = httpHost.switchToHttp().getResponse(); + expect(responseMock.status).toHaveBeenCalledWith(400); + expect(responseMock.json).toHaveBeenCalledWith({ + statusCode: 400, + message: "Not found", + error: "CometEntityNotFoundException", + validationErrors: [], + }); + }); + + it("response status is 400 and json is correct for CometValidationException", () => { + const exceptionFilter = new ExceptionFilter(false); + + exceptionFilter.catch(new CometValidationException("Invalid", [{ property: "prop1" }]), httpHost); + + const responseMock = httpHost.switchToHttp().getResponse(); + expect(responseMock.status).toHaveBeenCalledWith(400); + expect(responseMock.json).toHaveBeenCalledWith({ + statusCode: 400, + message: "Invalid", + error: "CometValidationException", + validationErrors: [ + { + property: "prop1", + }, + ], + }); + }); + + it("response status is 401 and message is correct for HttpException", () => { + const exceptionFilter = new ExceptionFilter(false); + + exceptionFilter.catch(new HttpException("Unauthorized", 401), httpHost); + + const responseMock = httpHost.switchToHttp().getResponse(); + expect(responseMock.status).toHaveBeenCalledWith(401); + expect(responseMock.json).toHaveBeenCalledWith("Unauthorized"); + }); + + it("response status is 500 and json is correct for normal Error in non-debug mode", () => { + const exceptionFilter = new ExceptionFilter(false); + + exceptionFilter.catch(new Error("Other error"), httpHost); + + const responseMock = httpHost.switchToHttp().getResponse(); + expect(responseMock.status).toHaveBeenCalledWith(500); + expect(responseMock.json).toHaveBeenCalledWith({ message: "Internal Server Error", statusCode: 500 }); + }); + + it("response status is 500 and json contains real error for normal Error in debug mode", () => { + const exceptionFilter = new ExceptionFilter(true); + + exceptionFilter.catch(new Error("Other error"), httpHost); + + const responseMock = httpHost.switchToHttp().getResponse(); + expect(responseMock.status).toHaveBeenCalledWith(500); + expect(responseMock.json).toHaveBeenCalledWith({ message: "Other error", statusCode: 500 }); + }); + }); + }); +}); diff --git a/packages/api/cms-api/src/common/errors/exception.filter.ts b/packages/api/cms-api/src/common/errors/exception.filter.ts new file mode 100644 index 0000000000..05538be753 --- /dev/null +++ b/packages/api/cms-api/src/common/errors/exception.filter.ts @@ -0,0 +1,65 @@ +import { + ArgumentsHost, + BadRequestException, + Catch, + ExceptionFilter as NestExceptionFilter, + HttpException, + InternalServerErrorException, + Logger, +} from "@nestjs/common"; +import { ErrorHttpStatusCode } from "@nestjs/common/utils/http-error-by-code.util"; +import { Response } from "express"; + +import { CometException } from "./comet.exception"; +import { CometValidationException } from "./validation.exception"; + +@Catch() +export class ExceptionFilter implements NestExceptionFilter { + protected readonly logger = new Logger(ExceptionFilter.name); + + constructor(private readonly debug: boolean) {} + + catch(exception: Error, host: ArgumentsHost) { + let statusCode: ErrorHttpStatusCode; + let returnedError: Error; + + if (exception instanceof CometException) { + const errorObject: Record = { + statusCode: 400, + message: exception.message, + error: exception.constructor.name, + validationErrors: [], + }; + + if (exception instanceof CometValidationException) { + errorObject.validationErrors = exception.errors; + } + + statusCode = 400; + returnedError = new BadRequestException(errorObject); + } else if (exception instanceof HttpException) { + statusCode = exception.getStatus(); + returnedError = exception; + } else { + returnedError = this.debug ? exception : new InternalServerErrorException(); + statusCode = "getStatus" in returnedError && typeof returnedError.getStatus === "function" ? returnedError.getStatus() : 500; + this.logger.error(exception, exception.stack); // Log for debugging + } + + const ctxType = host.getType<"http" | "graphql">(); // Check if it's an HTTP or GraphQL request + + if (ctxType === "graphql") { + return returnedError; + } else { + const ctx = host.switchToHttp(); + const response = ctx.getResponse(); + response + .status(statusCode) + .json( + "getResponse" in returnedError && typeof returnedError.getResponse === "function" + ? returnedError.getResponse() + : { statusCode, message: returnedError.message }, + ); + } + } +} diff --git a/packages/api/cms-api/src/common/errors/exception.interceptor.ts b/packages/api/cms-api/src/common/errors/exception.interceptor.ts index 77f406fa68..bb394b6e7a 100644 --- a/packages/api/cms-api/src/common/errors/exception.interceptor.ts +++ b/packages/api/cms-api/src/common/errors/exception.interceptor.ts @@ -15,6 +15,9 @@ import { CometValidationException } from "./validation.exception"; // Inspired by https://docs.nestjs.com/interceptors#more-operators @Injectable() +/** + * @deprecated Use `ExceptionFilter` instead + */ export class ExceptionInterceptor implements NestInterceptor { constructor(private readonly debug: boolean) {} diff --git a/packages/api/cms-api/src/index.ts b/packages/api/cms-api/src/index.ts index d64b97e9ff..ab378e4240 100644 --- a/packages/api/cms-api/src/index.ts +++ b/packages/api/cms-api/src/index.ts @@ -44,6 +44,7 @@ export { getRequestContextHeadersFromRequest, RequestContext, RequestContextInte export { getRequestFromExecutionContext } from "./common/decorators/utils"; export { CometException } from "./common/errors/comet.exception"; export { CometEntityNotFoundException } from "./common/errors/entity-not-found.exception"; +export { ExceptionFilter } from "./common/errors/exception.filter"; export { ExceptionInterceptor } from "./common/errors/exception.interceptor"; export { CometValidationException } from "./common/errors/validation.exception"; export { ValidationExceptionFactory } from "./common/errors/validation.exception-factory";