-
Notifications
You must be signed in to change notification settings - Fork 63
fix: sanitize error #2269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
fix: sanitize error #2269
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
bfafec3
middleware catching unhandled errors and returning generic message
pawelstepien-da 5f25459
fix user and dapp paths breaking wg when not starting with /api
pawelstepien-da f46a116
fix leaking error stack when no access token provided
pawelstepien-da 837d784
fix missing error messages
pawelstepien-da 92c7523
hide error messages when unexpected and show when custom error
pawelstepien-da 321ad60
consistent naming of userApiUrl in /wallet-gateway-config
pawelstepien-da a9500ed
unit tests
pawelstepien-da 755b293
unit tests
pawelstepien-da 7682934
Merge remote-tracking branch 'refs/remotes/origin/main' into pawel/sa…
pawelstepien-da 41b918c
unit tests
pawelstepien-da d3082eb
log full error in jsonRpcHandler while keeping response sanitized
pawelstepien-da 6c40e14
missed /api/ fix part
pawelstepien-da a1488e9
fix req too large becoming 500 instead of 413
pawelstepien-da 70d2cff
Merge remote-tracking branch 'refs/remotes/origin/main' into pawel/sa…
pawelstepien-da 1d3e909
Merge remote-tracking branch 'refs/remotes/origin/main' into pawel/sa…
pawelstepien-da 46584a1
Merge remote-tracking branch 'refs/remotes/origin/main' into pawel/sa…
pawelstepien-da 3a40ef0
Merge branch 'main' into pawel/sanitize-error
pawelstepien-da File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
138 changes: 138 additions & 0 deletions
138
wallet-gateway/remote/src/middleware/errorHandler.test.ts
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' }) | ||
|
pawelstepien-da marked this conversation as resolved.
|
||
| } | ||
| } | ||
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.
Uh oh!
There was an error while loading. Please reload this page.