fix(cactus-common): coerceUnknownToError() now uses HTML sanitize

1. This is a security fix so that the exception serialization does not accidentally XSS anybody who is looking at crash logs through some admin GUI that is designed to show logs that are considered trusted. Related discussion about `1)` can be seen at this other pull request: https://github.com/hyperledger/cacti/pull/2893 Signed-off-by: Peter Somogyvari <[email protected]>
petermetz · Jan 4, 2024 · a760d00 · a760d00
1 parent 1fb2551
commit a760d00
Show file tree

Hide file tree

Showing 10 changed files with 289 additions and 27 deletions.
diff --git a/packages/cactus-common/src/main/typescript/exception/coerce-unknown-to-error.ts b/packages/cactus-common/src/main/typescript/exception/coerce-unknown-to-error.ts
@@ -1,4 +1,5 @@
 import stringify from "fast-safe-stringify";
+import sanitizeHtml from "sanitize-html";
 import { ErrorFromUnknownThrowable } from "./error-from-unknown-throwable";
 import { ErrorFromSymbol } from "./error-from-symbol";
 
@@ -26,10 +27,11 @@ export function coerceUnknownToError(x: unknown): Error {
   } else if (x instanceof Error) {
     return x;
   } else {
-    const xAsJson = stringify(x, (_, value) =>
+    const xAsJsonUnsafe = stringify(x, (_, value) =>
       typeof value === "bigint" ? value.toString() + "n" : value,
     );
-    return new ErrorFromUnknownThrowable(xAsJson);
+    const xAsJsonSanitized = sanitizeHtml(xAsJsonUnsafe);
+    return new ErrorFromUnknownThrowable(xAsJsonSanitized);
   }
 }
 

diff --git a/packages/cactus-core/package.json b/packages/cactus-core/package.json
@@ -56,13 +56,15 @@
     "express-jwt-authz": "2.4.1",
     "express-openapi-validator": "5.0.4",
     "http-errors": "2.0.0",
+    "http-errors-enhanced": "1.1.2",
     "run-time-error-cjs": "1.4.0",
     "safe-stable-stringify": "2.4.3",
     "typescript-optional": "2.0.1"
   },
   "devDependencies": {
     "@types/express": "4.17.19",
     "@types/http-errors": "2.0.2",
+    "node-mocks-http": "1.14.0",
     "uuid": "8.3.2"
   },
   "engines": {

diff --git a/packages/cactus-core/src/main/typescript/web-services/handle-rest-endpoint-exception.ts b/packages/cactus-core/src/main/typescript/web-services/handle-rest-endpoint-exception.ts
@@ -1,12 +1,13 @@
 import {
   Logger,
   createRuntimeErrorWithCause,
+  safeStringifyException,
 } from "@hyperledger/cactus-common";
 import type { Response } from "express";
 import createHttpError from "http-errors";
 
 /**
- * An interface describing the object contaiing the contextual information needed by the
+ * An interface describing the object containing the contextual information needed by the
  * `#handleRestEndpointException()` method to perform its duties.
  *
  * @param ctx - An object containing options for handling the REST endpoint exception.
@@ -35,39 +36,46 @@ export interface IHandleRestEndpointExceptionOptions {
  *
  * @param ctx - An object containing options for handling the REST endpoint exception.
  */
-export function handleRestEndpointException(
+export async function handleRestEndpointException(
   ctx: Readonly<IHandleRestEndpointExceptionOptions>,
-): void {
+): Promise<void> {
+  const errorAsSanitizedJson = safeStringifyException(ctx.error);
+
+  const { identifierByCodes, INTERNAL_SERVER_ERROR } = await import(
+    "http-errors-enhanced"
+  );
+
   if (createHttpError.isHttpError(ctx.error)) {
     ctx.res.status(ctx.error.statusCode);
 
     // Log either an error or a debug message depending on what the statusCode is
     // For 5xx errors we treat it as a production bug that needs to be fixed on
     // our side and for everything else we treat it a user error and debug log it.
-    if (ctx.error.statusCode >= 500) {
-      ctx.log.debug(ctx.errorMsg, ctx.error);
+    if (ctx.error.statusCode >= INTERNAL_SERVER_ERROR) {
+      ctx.log.debug(ctx.errorMsg, errorAsSanitizedJson);
     } else {
-      ctx.log.error(ctx.errorMsg, ctx.error);
+      ctx.log.error(ctx.errorMsg, errorAsSanitizedJson);
     }
 
     // If the `expose` property is set to true it implies that we can safely
     // expose the contents of the exception to the calling client.
     if (ctx.error.expose) {
       ctx.res.json({
-        message: ctx.error.message,
-        error: ctx.error,
+        message: identifierByCodes[ctx.error.statusCode],
+        error: errorAsSanitizedJson,
       });
     }
   } else {
     // If the exception was not an http-error then we assume it was an internal
     // error (e.g. same behavior as if we had received an HTTP 500 statusCode)
-    ctx.log.error(ctx.errorMsg, ctx.error);
+    ctx.log.error(ctx.errorMsg, errorAsSanitizedJson);
 
     const rex = createRuntimeErrorWithCause(ctx.errorMsg, ctx.error);
+    const sanitizedJsonRex = safeStringifyException(rex);
 
-    ctx.res.status(500).json({
-      message: "Internal Server Error",
-      error: rex,
+    ctx.res.status(INTERNAL_SERVER_ERROR).json({
+      message: identifierByCodes[INTERNAL_SERVER_ERROR],
+      error: sanitizedJsonRex,
     });
   }
 }
diff --git a/packages/cactus-core/src/test/typescript/unit/handle-rest-endpoint-exception.test.ts b/packages/cactus-core/src/test/typescript/unit/handle-rest-endpoint-exception.test.ts
@@ -0,0 +1,211 @@
+import "jest-extended";
+import createHttpError from "http-errors";
+import { createResponse } from "node-mocks-http";
+
+import { safeStringifyException } from "@hyperledger/cactus-common";
+
+import {
+  handleRestEndpointException,
+  IHandleRestEndpointExceptionOptions,
+} from "../../../main/typescript/public-api"; // replace with the correct path to your module
+
+import { LoggerProvider } from "@hyperledger/cactus-common";
+
+const log = LoggerProvider.getOrCreate({
+  label: "handle-rest-endpoint-exception.test.ts",
+  level: "DEBUG",
+});
+
+describe("handleRestEndpointException", () => {
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it("should handle HttpError with statusCode >= 500", async () => {
+    const mockResponse = createResponse();
+
+    const mockOptions: IHandleRestEndpointExceptionOptions = {
+      errorMsg: "Test error message",
+      log: jest.mocked(log), // Provide a mock logger if needed
+      error: new Error("Test error"), // Provide appropriate error objects for testing
+      res: mockResponse,
+    };
+
+    const mockHttpError = createHttpError(500, "Test HTTP error", {
+      expose: true,
+    });
+
+    const errorAsSanitizedJson = safeStringifyException(mockHttpError);
+    const spyLogDebug = jest.spyOn(mockOptions.log, "debug");
+    const spyStatus = jest.spyOn(mockResponse, "status");
+    const spyJson = jest.spyOn(mockResponse, "json");
+
+    await handleRestEndpointException({ ...mockOptions, error: mockHttpError });
+
+    expect(spyStatus).toHaveBeenCalledWith(500);
+
+    expect(spyLogDebug).toHaveBeenCalledWith(
+      mockOptions.errorMsg,
+      errorAsSanitizedJson,
+    );
+
+    expect(spyJson).toHaveBeenCalledWith({
+      message: "InternalServerError",
+      error: errorAsSanitizedJson,
+    });
+  });
+
+  it("should handle HttpError with statusCode < 500", async () => {
+    const mockResponse = createResponse();
+
+    const mockOptions: IHandleRestEndpointExceptionOptions = {
+      errorMsg: "Test error message",
+      log: jest.mocked(log), // Provide a mock logger if needed
+      error: new Error("Test error"), // Provide appropriate error objects for testing
+      res: mockResponse,
+    };
+
+    const mockHttpError = createHttpError(404, "Test HTTP error", {
+      expose: true,
+    });
+
+    const errorAsSanitizedJson = safeStringifyException(mockHttpError);
+    const spyLogError = jest.spyOn(mockOptions.log, "error");
+    const spyStatus = jest.spyOn(mockResponse, "status");
+    const spyJson = jest.spyOn(mockResponse, "json");
+    await handleRestEndpointException({ ...mockOptions, error: mockHttpError });
+
+    expect(spyStatus).toHaveBeenCalledWith(404);
+    expect(spyLogError).toHaveBeenCalledWith(
+      mockOptions.errorMsg,
+      errorAsSanitizedJson,
+    );
+
+    expect(spyJson).toHaveBeenCalledWith({
+      message: "NotFound",
+      error: errorAsSanitizedJson,
+    });
+  });
+
+  it("should handle non-HttpError", async () => {
+    const mockResponse = createResponse();
+
+    const mockError = new Error("An unexpected exception. Ha!");
+
+    const mockOptions: IHandleRestEndpointExceptionOptions = {
+      errorMsg: "Test error message",
+      log: jest.mocked(log), // Provide a mock logger if needed
+      error: mockError,
+      res: mockResponse,
+    };
+
+    const mockErrorJson = safeStringifyException(mockError);
+    const spyLoggerFn = jest.spyOn(mockOptions.log, "error");
+    const spyStatus = jest.spyOn(mockResponse, "status");
+    const spyJson = jest.spyOn(mockResponse, "json");
+
+    await handleRestEndpointException({ ...mockOptions, error: mockError });
+
+    expect(spyStatus).toHaveBeenCalledWith(500);
+
+    expect(spyLoggerFn).toHaveBeenCalledWith(
+      mockOptions.errorMsg,
+      mockErrorJson,
+    );
+
+    expect(spyJson).toHaveBeenCalledOnce();
+
+    const mostRecentCall = spyJson.mock.lastCall;
+    expect(mostRecentCall).toBeTruthy();
+
+    expect(mostRecentCall?.[0].message).toBeString();
+    expect(mostRecentCall?.[0].message).toEqual("InternalServerError");
+    expect(mostRecentCall?.[0].error).toMatch(
+      /RuntimeError: Test error message(.*)An unexpected exception. Ha!/,
+    );
+  });
+
+  it("should escape malicious payloads in exception messages", async () => {
+    const mockResponse = createResponse();
+
+    const dummyXssPayload = `<script>stealAndUploadPrivateKeys();</script>`;
+    const mockError = new Error(dummyXssPayload);
+
+    const mockOptions: IHandleRestEndpointExceptionOptions = {
+      errorMsg: "Test error message",
+      log: jest.mocked(log), // Provide a mock logger if needed
+      error: mockError,
+      res: mockResponse,
+    };
+
+    const mockErrorJson = safeStringifyException(mockError);
+    const spyLoggerFn = jest.spyOn(mockOptions.log, "error");
+    const spyStatus = jest.spyOn(mockResponse, "status");
+    const spyJson = jest.spyOn(mockResponse, "json");
+
+    await handleRestEndpointException({ ...mockOptions, error: mockError });
+
+    expect(spyStatus).toHaveBeenCalledWith(500);
+
+    expect(spyLoggerFn).toHaveBeenCalledWith(
+      mockOptions.errorMsg,
+      mockErrorJson,
+    );
+
+    expect(spyJson).toHaveBeenCalledOnce();
+
+    const mostRecentCall = spyJson.mock.lastCall;
+    expect(mostRecentCall).toBeTruthy();
+
+    expect(mostRecentCall?.[0].message).toBeString();
+    expect(mostRecentCall?.[0].message).toEqual("InternalServerError");
+    expect(mostRecentCall?.[0].error).toMatch(
+      /RuntimeError: Test error message(.*)/,
+    );
+    expect(mostRecentCall?.[0].error).not.toMatch(
+      /.*stealAndUploadPrivateKeys.*/,
+    );
+  });
+
+  it("should escape malicious payloads in strings thrown", async () => {
+    const mockResponse = createResponse();
+
+    const dummyXssPayload = `<script>stealAndUploadPrivateKeys();</script>`;
+    const mockError = dummyXssPayload;
+
+    const mockOptions: IHandleRestEndpointExceptionOptions = {
+      errorMsg: "Test error message",
+      log: jest.mocked(log), // Provide a mock logger if needed
+      error: mockError,
+      res: mockResponse,
+    };
+
+    const mockErrorJson = safeStringifyException(mockError);
+    const spyLoggerFn = jest.spyOn(mockOptions.log, "error");
+    const spyStatus = jest.spyOn(mockResponse, "status");
+    const spyJson = jest.spyOn(mockResponse, "json");
+
+    await handleRestEndpointException({ ...mockOptions, error: mockError });
+
+    expect(spyStatus).toHaveBeenCalledWith(500);
+
+    expect(spyLoggerFn).toHaveBeenCalledWith(
+      mockOptions.errorMsg,
+      mockErrorJson,
+    );
+
+    expect(spyJson).toHaveBeenCalledOnce();
+
+    const mostRecentCall = spyJson.mock.lastCall;
+    expect(mostRecentCall).toBeTruthy();
+
+    expect(mostRecentCall?.[0].message).toBeString();
+    expect(mostRecentCall?.[0].message).toEqual("InternalServerError");
+    expect(mostRecentCall?.[0].error).toMatch(
+      /RuntimeError: Test error message(.*)/,
+    );
+    expect(mostRecentCall?.[0].error).not.toMatch(
+      /.*stealAndUploadPrivateKeys.*/,
+    );
+  });
+});
diff --git a/...gin-keychain-memory/src/main/typescript/web-services/delete-keychain-entry-endpoint-v1.ts b/...gin-keychain-memory/src/main/typescript/web-services/delete-keychain-entry-endpoint-v1.ts
@@ -87,6 +87,7 @@ export class DeleteKeychainEntryV1Endpoint implements IWebServiceEndpoint {
   }
 
   public async handleRequest(req: Request, res: Response): Promise<void> {
+    const { log } = this;
     const fnTag = `${this.className}#handleRequest()`;
     const reqTag = `${this.getVerbLowerCase()} - ${this.getPath()}`;
     this.log.debug(reqTag);
@@ -96,7 +97,7 @@ export class DeleteKeychainEntryV1Endpoint implements IWebServiceEndpoint {
       res.json(resBody);
     } catch (ex) {
       const errorMsg = `${reqTag} ${fnTag} Failed to deploy contract:`;
-      handleRestEndpointException({ errorMsg, log: this.log, error: ex, res });
+      await handleRestEndpointException({ errorMsg, log, error: ex, res });
     }
   }
 }
diff --git a/...plugin-keychain-memory/src/main/typescript/web-services/get-keychain-entry-endpoint-v1.ts b/...plugin-keychain-memory/src/main/typescript/web-services/get-keychain-entry-endpoint-v1.ts
@@ -87,6 +87,7 @@ export class GetKeychainEntryV1Endpoint implements IWebServiceEndpoint {
   }
 
   public async handleRequest(req: Request, res: Response): Promise<void> {
+    const { log } = this;
     const fnTag = `${this.className}#handleRequest()`;
     const reqTag = `${this.getVerbLowerCase()} - ${this.getPath()}`;
     this.log.debug(reqTag);
@@ -96,7 +97,7 @@ export class GetKeychainEntryV1Endpoint implements IWebServiceEndpoint {
       res.json({ key, value });
     } catch (ex) {
       const errorMsg = `${reqTag} ${fnTag} Failed to deploy contract:`;
-      handleRestEndpointException({ errorMsg, log: this.log, error: ex, res });
+      await handleRestEndpointException({ errorMsg, log, error: ex, res });
     }
   }
 }
diff --git a/...plugin-keychain-memory/src/main/typescript/web-services/has-keychain-entry-endpoint-v1.ts b/...plugin-keychain-memory/src/main/typescript/web-services/has-keychain-entry-endpoint-v1.ts
@@ -87,6 +87,7 @@ export class HasKeychainEntryV1Endpoint implements IWebServiceEndpoint {
   }
 
   public async handleRequest(req: Request, res: Response): Promise<void> {
+    const { log } = this;
     const fnTag = `${this.className}#handleRequest()`;
     const reqTag = `${this.getVerbLowerCase()} - ${this.getPath()}`;
     this.log.debug(reqTag);
@@ -103,7 +104,7 @@ export class HasKeychainEntryV1Endpoint implements IWebServiceEndpoint {
       res.json(resBody);
     } catch (ex) {
       const errorMsg = `${reqTag} ${fnTag} Failed to deploy contract:`;
-      handleRestEndpointException({ errorMsg, log: this.log, error: ex, res });
+      await handleRestEndpointException({ errorMsg, log, error: ex, res });
     }
   }
 }
diff --git a/...plugin-keychain-memory/src/main/typescript/web-services/set-keychain-entry-endpoint-v1.ts b/...plugin-keychain-memory/src/main/typescript/web-services/set-keychain-entry-endpoint-v1.ts
@@ -87,6 +87,7 @@ export class SetKeychainEntryV1Endpoint implements IWebServiceEndpoint {
   }
 
   public async handleRequest(req: Request, res: Response): Promise<void> {
+    const { log } = this;
     const fnTag = `${this.className}#handleRequest()`;
     const reqTag = `${this.getVerbLowerCase()} - ${this.getPath()}`;
     this.log.debug(reqTag);
@@ -96,7 +97,7 @@ export class SetKeychainEntryV1Endpoint implements IWebServiceEndpoint {
       res.json(resBody);
     } catch (ex) {
       const errorMsg = `${reqTag} ${fnTag} Failed to deploy contract:`;
-      handleRestEndpointException({ errorMsg, log: this.log, error: ex, res });
+      await handleRestEndpointException({ errorMsg, log, error: ex, res });
     }
   }
 }