fix: sanitize error #2269
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| // Copyright (c) 2025-2026 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import type { NextFunction, Request, Response } from 'express' | ||
| import { pino } from 'pino' | ||
| import { sink } from 'pino-test' | ||
| import { providerErrors, rpcErrors } from '@canton-network/core-rpc-errors' | ||
| import { errorHandler } from './errorHandler.js' | ||
|
|
||
| describe('errorHandler', () => { | ||
| const logger = pino({ level: 'silent' }, sink()) | ||
| const isApiPath = (path: string) => path.startsWith('/api/') | ||
|
|
||
| let next: NextFunction | ||
| let status: ReturnType<typeof vi.fn> | ||
| let json: ReturnType<typeof vi.fn> | ||
|
|
||
| beforeEach(() => { | ||
| next = vi.fn() as NextFunction | ||
| status = vi.fn().mockReturnThis() | ||
| json = vi.fn() | ||
| }) | ||
|
|
||
| function makeReq(partial: Partial<Request> = {}): Request { | ||
| return { | ||
| path: '/api/v0/user', | ||
| body: { id: 1 }, | ||
| ...partial, | ||
| } as Request | ||
| } | ||
|
|
||
| function makeRes(headersSent = false): Response { | ||
| return { status, json, headersSent } as unknown as Response | ||
| } | ||
|
|
||
| it('maps a JsonRpcError to its HTTP status and keeps the message', () => { | ||
| const err = providerErrors.unauthorized({ | ||
| message: 'User is not connected', | ||
| }) | ||
|
|
||
| errorHandler(logger, isApiPath)(err, makeReq(), makeRes(), next) | ||
|
|
||
| expect(status).toHaveBeenCalledWith(401) | ||
| expect(json).toHaveBeenCalledWith({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| error: { | ||
| code: providerErrors.unauthorized().code, | ||
| message: 'User is not connected', | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| it('replaces an unexpected error with a generic JSON-RPC 500 on API paths', () => { | ||
| const err = new Error('connect ECONNREFUSED 127.0.0.1:5432') | ||
|
|
||
| errorHandler(logger, isApiPath)(err, makeReq(), makeRes(), next) | ||
|
|
||
| expect(status).toHaveBeenCalledWith(500) | ||
| expect(json).toHaveBeenCalledWith({ | ||
| jsonrpc: '2.0', | ||
| id: 1, | ||
| error: { | ||
| code: rpcErrors.internal().code, | ||
| message: 'Something went wrong', | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| it('never sends the stack trace to the client', () => { | ||
| const err = new Error('internal detail') | ||
|
|
||
| errorHandler(logger, isApiPath)(err, makeReq(), makeRes(), next) | ||
|
|
||
| const body = JSON.stringify(json.mock.calls[0][0]) | ||
| expect(body).not.toContain('internal detail') | ||
| expect(body).not.toContain('at ') // stack trace | ||
| }) | ||
|
|
||
| it('keeps the 413 from express.json() for err.status', () => { | ||
| const err = Object.assign(new Error('request too large'), { | ||
| status: 413, | ||
| }) | ||
|
|
||
| errorHandler(logger, isApiPath)(err, makeReq(), makeRes(), next) | ||
|
|
||
| expect(status).toHaveBeenCalledWith(413) | ||
| expect(json).toHaveBeenCalledWith({ error: 'Payload Too Large' }) | ||
| }) | ||
|
|
||
| it('keeps the 413 from express.json() for err.statusCode', () => { | ||
| const err = Object.assign(new Error('request too large'), { | ||
| statusCode: 413, | ||
| }) | ||
|
|
||
| errorHandler(logger, isApiPath)(err, makeReq(), makeRes(), next) | ||
|
|
||
| expect(status).toHaveBeenCalledWith(413) | ||
| expect(json).toHaveBeenCalledWith({ error: 'Payload Too Large' }) | ||
| }) | ||
|
|
||
| it('leaves an error without a 413 status as a generic 500', () => { | ||
| const err = Object.assign(new Error('not a 413 error'), { | ||
| status: 404, | ||
| }) | ||
|
|
||
| errorHandler(logger, isApiPath)(err, makeReq(), makeRes(), next) | ||
|
|
||
| expect(status).toHaveBeenCalledWith(500) | ||
| }) | ||
|
|
||
| it('returns a generic error body for non-API paths', () => { | ||
| const req = makeReq({ path: '/login' }) | ||
|
|
||
| errorHandler(logger, isApiPath)( | ||
| new Error('internal detail'), | ||
| req, | ||
| makeRes(), | ||
| next | ||
| ) | ||
|
|
||
| expect(status).toHaveBeenCalledWith(500) | ||
| expect(json).toHaveBeenCalledWith({ error: 'Internal Server Error' }) | ||
| }) | ||
|
|
||
| it('delegates to express when the response has already started', () => { | ||
| const err = new Error( | ||
| 'error that some middleware started res on, but still passed error down' | ||
| ) | ||
|
|
||
| errorHandler(logger, isApiPath)(err, makeReq(), makeRes(true), next) | ||
|
|
||
| expect(next).toHaveBeenCalledWith(err) | ||
| expect(status).not.toHaveBeenCalled() | ||
| expect(json).not.toHaveBeenCalled() | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| // Copyright (c) 2025-2026 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import type { NextFunction, Request, Response } from 'express' | ||
| import { Logger } from 'pino' | ||
| import { | ||
| JsonRpcError, | ||
| rpcErrors, | ||
| toHttpErrorCode, | ||
| } from '@canton-network/core-rpc-errors' | ||
| import { jsonRpcResponse } from '@canton-network/core-rpc-transport' | ||
|
|
||
| const isPayloadTooLargeError = (err: unknown): boolean => { | ||
| if (typeof err !== 'object' || err === null) { | ||
| return false | ||
| } | ||
|
|
||
| const { status, statusCode } = err as { | ||
| status?: unknown | ||
| statusCode?: unknown | ||
| } | ||
|
|
||
| return status === 413 || statusCode === 413 | ||
| } | ||
|
|
||
| // Catches unhandled errors and prevents internal details like stack trace from reaching end user | ||
| export function errorHandler( | ||
| logger: Logger, | ||
| isApiPath: (path: string) => boolean | ||
| ) { | ||
| return ( | ||
| err: unknown, | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction | ||
| ): void => { | ||
| // Full error with stack goes to logs only. | ||
| logger.error({ err }, 'Unhandled request error') | ||
|
|
||
| // If the response has already started, we can't safely send an error response. | ||
| if (res.headersSent) { | ||
| next(err) | ||
| return | ||
| } | ||
|
|
||
| if (isPayloadTooLargeError(err)) { | ||
| res.status(413).json({ error: 'Payload Too Large' }) | ||
| return | ||
| } | ||
|
|
||
| // jsonRpcHandler already maps controllers errors via handleRpcError. | ||
| // This only runs for errors that escape earlier middlewares (e.g. auth/session checks). | ||
| if (isApiPath(req.path)) { | ||
| const id = req.body?.id ?? null | ||
|
|
||
| if (err instanceof JsonRpcError) { | ||
| res.status(toHttpErrorCode(err.code)).json( | ||
| jsonRpcResponse(id, { | ||
| error: { code: err.code, message: err.message }, | ||
| }) | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| res.status(500).json( | ||
| jsonRpcResponse(id, { | ||
| error: { | ||
| code: rpcErrors.internal().code, | ||
| message: 'Something went wrong', | ||
| }, | ||
| }) | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| res.status(500).json({ error: 'Internal Server Error' }) | ||
|
Contributor
There was a problem hiding this comment. Is it not possible to tie this to the request somehow?
Contributor
Author
There was a problem hiding this comment. I don't think we can fully rely on logs from all potential places error can come from as a way to correlate error with request. I am inclined to add request info next to the error in
What do you think? It could guide us to api and method that caused the unhandled error, but unfortunately not exactly what was in the payload. |
||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this separate from
handleRpcErrorinjsonRpcHandler.ts?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
jsonRpcHandlerwouldn't know about errors thrown anywhere else but controller. Issue this PR originated from reports a runtime error from sessionHandler middleware. When an error is thrown (or next(err) is called) along the journey of request through middlewares, express looks for closest next error middleware (a middleware that has four args: err, req, res, next). We didn't have one, so the error went straight to default express error handler and it responded with a verbose JS error in body.I think it's a standard pattern to have one general error handler at the end of chain that would catch unexpected errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense to me, thanks. I think still think it would be good for @alexmatson-da to have a look here, given that he is the author of
jsonRpcHandler