Add ExceptionFilter to replace ExceptionInterceptor (v5) (#3244)

.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));
+```

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<void> {
         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,

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 });
+            });
+        });
+    });
+});

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<string, unknown> = {
+                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>();
+            response
+                .status(statusCode)
+                .json(
+                    "getResponse" in returnedError && typeof returnedError.getResponse === "function"
+                        ? returnedError.getResponse()
+                        : { statusCode, message: returnedError.message },
+                );
+        }
+    }
+}

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) {}
 

packages/api/cms-api/src/index.ts

@@ -46,6 +46,7 @@ export { SubjectEntity, SubjectEntityMeta, SubjectEntityOptions } from "./common
 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";