From e9369d652a11b025d70d578fcd23e09ec069ef23 Mon Sep 17 00:00:00 2001 From: Sean Sica <23294618+seansica@users.noreply.github.com> Date: Sun, 17 Nov 2024 15:47:16 -0500 Subject: [PATCH] fix: return 404 instead of 500 for missing collections - Replace string rejection with TaxiiNotFoundException in repository - Remove Promise constructor anti-pattern in findOne method - Add debug logging for collection lookups - Update JSDoc to document exception behavior The /collections/{id} endpoint now correctly returns a 404 status when a collection is not found in the database, rather than a 500 internal server error. --- .../exceptions/taxii-exception.filter.ts | 80 +++++++++---------- .../collection/collection.repository.ts | 20 +++-- 2 files changed, 51 insertions(+), 49 deletions(-) diff --git a/src/common/exceptions/taxii-exception.filter.ts b/src/common/exceptions/taxii-exception.filter.ts index 43f7a0a..97f4dc9 100644 --- a/src/common/exceptions/taxii-exception.filter.ts +++ b/src/common/exceptions/taxii-exception.filter.ts @@ -1,7 +1,6 @@ import { ArgumentsHost, ExceptionFilter, - HttpException, Logger, } from "@nestjs/common"; import { Response } from "express"; @@ -12,61 +11,60 @@ import { RequestContextModel, } from "../middleware/request-context"; -// @Catch(MyException) => can set to only catch specific exception types! export class TaxiiExceptionFilter implements ExceptionFilter { - private readonly logger: Logger = new Logger(); + private readonly logger: Logger = new Logger(TaxiiExceptionFilter.name); /** * Handles uncaught exceptions. Any exceptions which are caught that are not a subclass of TaxiiErrorException are * transformed to instances of TaxiiInternalServerErrorException and returned to the user. - * @param exception The uncaught exception instance that triggered the TaxiiExceptionFilter - * @param host Provides access to the HTTP context so that the Request and Response objects can be worked upon + * @param exception The uncaught exception instance that triggered the filter + * @param host Provides access to the HTTP context */ - catch(exception: HttpException, host: ArgumentsHost): any { + catch(exception: unknown, host: ArgumentsHost): any { const ctx = host.switchToHttp(); const response = ctx.getResponse(); const reqCtx: RequestContext = RequestContextModel.get(); + const requestId = reqCtx?.["x-request-id"] || "unknown"; + // Log the exception type and details this.logger.error( - `[${reqCtx["x-request-id"]}] The exception type is: ${ - exception.constructor.name - }. The exception is ${JSON.stringify(exception)}` + `[${requestId}] Exception occurred: ${exception?.constructor?.name || typeof exception}` ); - // Send a generic 500 error for any uncaught exceptions - if (!(exception instanceof TaxiiErrorException)) { - this.logger.error( - `[${reqCtx["x-request-id"]}] Received an unhandled exception. Please investigate the root cause of the exception triggered by request: ${reqCtx["x-request-id"]}` - ); - this.logger.error( - `[${reqCtx["x-request-id"]}] The error is ${exception.stack.toString()}` - ); - const internalServerError: TaxiiInternalServerErrorException = - new TaxiiInternalServerErrorException({ - title: "Internal Error", - description: - "An unexpected error has occurred. Please contact the TAXII server administrator.", - // errorId: request['x-request-id'].toString(), - errorId: reqCtx["x-request-id"].toString(), - }); + if (typeof exception === 'string') { + this.logger.error(`[${requestId}] String exception: ${exception}`); + } else if (exception instanceof Error) { + this.logger.error(`[${requestId}] Error details: ${exception.message}`); + if (exception.stack) { + this.logger.error(`[${requestId}] Stack trace: ${exception.stack}`); + } + } else { + try { + this.logger.error( + `[${requestId}] Exception details: ${JSON.stringify(exception)}` + ); + } catch (e) { + this.logger.error( + `[${requestId}] Could not stringify exception: ${e.message}` + ); + } + } - // Send the error - response - .status(TaxiiInternalServerErrorException.httpStatus) - .json(internalServerError); + // Handle TAXII exceptions + if (exception instanceof TaxiiErrorException) { + exception.errorId = requestId.toString(); + return response.status(exception.httpStatus).json(exception); } - // The else clause is triggered only if the exception type if one of the custom Taxii exception classes, in - // which case all we want to do is set the errorId field then return the exception. - else { - // Copy the request's unique ID onto the outgoing response body - exception.errorId = reqCtx["x-request-id"].toString(); + // For all other exceptions (including strings, non-HTTP errors, etc.) + const internalServerError = new TaxiiInternalServerErrorException({ + title: "Internal Error", + description: "An unexpected error has occurred. Please contact the TAXII server administrator.", + errorId: requestId.toString(), + }); - // Send the error - response - //.set('id', request['id']) - .status(exception.httpStatus) - .json(exception); - } + return response + .status(TaxiiInternalServerErrorException.httpStatus) + .json(internalServerError); } -} +} \ No newline at end of file diff --git a/src/taxii/providers/collection/collection.repository.ts b/src/taxii/providers/collection/collection.repository.ts index 99cb058..dfaedbd 100644 --- a/src/taxii/providers/collection/collection.repository.ts +++ b/src/taxii/providers/collection/collection.repository.ts @@ -7,7 +7,7 @@ import { TaxiiCollectionDocument, } from "src/hydrate/schema"; import { Model } from "mongoose"; -import { isNull } from "lodash"; +import { TaxiiNotFoundException } from "src/common/exceptions"; @Injectable() export class CollectionRepository { @@ -24,9 +24,11 @@ export class CollectionRepository { * * @param id TAXII/STIX ID of the collection * @returns Promise resolving to TaxiiCollectionDto - * @throws If collection is not found + * @throws TaxiiNotFoundException if collection is not found */ async findOne(id: string): Promise { + this.logger.debug(`Searching for collection with ID: ${id}`); + // Uses taxii_collection_lookup index const response: TaxiiCollectionEntity = await this.collectionModel .findOne({ @@ -35,12 +37,14 @@ export class CollectionRepository { }) .exec(); - return new Promise((resolve, reject) => { - if (!isNull(response)) { - resolve(new TaxiiCollectionDto({ ...response["_doc"] })); - } - reject(`Collection ID '${id}' not available in database`); - }); + if (!response) { + throw new TaxiiNotFoundException({ + title: "Collection Not Found", + description: `Collection ID '${id}' not available in database`, + }); + } + + return new TaxiiCollectionDto({ ...response["_doc"] }); } /**