fix: global exception filter to stop leaking internal error detail to clients - #34
Merged
chonilius merged 8 commits intoJul 17, 2026
Conversation
…on boundary No global ExceptionFilter was registered anywhere, and several service-layer methods (SorobanClientService.invoke, most notably) throw raw Error objects with interpolated internal detail — a simulation-error string that can include contract execution traces and RPC endpoint detail, or a JSON-stringified transaction error result. Nothing caught and sanitized these before they'd reach an HTTP response (MergeFi#19). Adds GlobalExceptionFilter, catching everything at the HTTP boundary and drawing one principled line: HTTP status code, not exception class. HttpExceptions with status < 500 (BadRequestException, NotFoundException, UnauthorizedException, ValidationPipe failures, etc.) are intentional, validated, client-facing messages — authored specifically to be read by API callers — and pass through completely unmodified. Everything else (any non-HttpException, or an HttpException at 500+) is internal-only: the client gets a generic message plus a correlation `errorId` to quote when reporting the issue, while the full error (message + stack) is always logged server-side at `error` level. Safe client errors still get a `warn` log line for request correlation, without altering their response. This status-based rule is deliberately not an enumerated list of "trusted" exception classes — it covers any current or future HttpException by its actual severity, and categorically strips detail from raw Errors regardless of where in the call stack they originate, satisfying the "OR ensure the global filter categorically strips detail from any non-HttpException error" clause of the requirements without needing to touch every raw-Error throw site individually.
Wires up the filter added in the previous commit exactly as MergeFi#19's requirements specify (useGlobalFilters, applied after the existing ValidationPipe registration so validation failures — which throw BadRequestException, a safe client error — keep working exactly as before).
Covers the acceptance criteria's core assertion at the unit level: a raw Soroban-style error and a raw TypeORM/Postgres-style error both produce a sanitized response with no trace of the original message, while the full message and stack are logged server-side via Logger. Also covers: BadRequestException/NotFoundException responses pass through byte-for-byte unmodified (the "legitimate messages are unaffected" requirement); safe errors log at warn, sanitized ones at error; the same errorId appears in both the response and the log line for correlation; each exception gets a distinct errorId; and a non-Error thrown value (e.g. a thrown string) is still sanitized.
…rtest Complements the unit tests with a real Nest application (a throwaway controller wired up exactly as main.ts wires the real app — doesn't import AppModule, which needs a live Postgres connection this environment doesn't have). Verifies actual HTTP response bodies: a raw Soroban-shaped error and a raw DB-shaped error both come back sanitized with zero trace of the original content (asset keypair fragments, RPC hostnames, constraint/table names all asserted absent from the serialized response), while a legitimate NotFoundException comes back completely untouched, byte-for-byte, with no errorId injected.
Investigating MergeFi#19's "does the raw simulation error detail reach the HTTP response body" question for EscrowService.fund/release/etc. turned up a second, separate leak the global filter alone doesn't cover: EscrowService.fund's catch block persists the raw Soroban error message into escrow.metadata.error on failure, and EscrowController's endpoints return the raw Escrow entity — metadata included — directly as the HTTP response. A failed fund() throws (now sanitized by GlobalExceptionFilter), but a *subsequent* GET /escrow/:id for that same escrow returns the stored entity as a normal 200 response, with the raw internal error message sitting right there in the JSON body — no exception involved, so the filter never sees it. Add toPublicEscrow, applied at the controller boundary rather than inside EscrowService: BountiesService/MilestonesService consume the full Escrow entity returned by EscrowService.fund internally (e.g. to set bounty.escrow), so narrowing the service's own return type would break those callers for no benefit — the controller is the actual client-facing boundary.
Wires the mapper from the previous commit into fund, findOne (GET /:id), release, and refund — every EscrowController endpoint that returns a raw Escrow entity. splitRelease returns Payment[], which has no metadata field, so it's unaffected.
Confirms metadata is stripped (including both the failure shape,
{ error: ... }, and the success shape, { fund: result }) while every
other field on the entity is preserved unchanged.
Controller-level test with a mocked EscrowService returning an escrow whose metadata contains an internal RPC detail string standing in for a real leaked Soroban error message. Asserts fund, findOne, release, and refund all come back with metadata absent and the leaked string nowhere in the serialized response — the regression this whole fix exists to close.
|
@circleboyslimited is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
3 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes #19.
No global
ExceptionFilterwas registered anywhere in the app, and several service-layer methods (SorobanClientService.invoke, most notably) throw rawErrorobjects with interpolated internal detail — a Soroban simulation-error string (potentially including contract execution traces or RPC endpoint detail) and a JSON-stringified transaction error-result object. Nothing caught and sanitized these before they could reach an HTTP response.1.
GlobalExceptionFilter(src/common/filters/global-exception.filter.ts)Registered via
app.useGlobalFilters(...)inmain.ts, as the requirements specify. Draws one principled boundary — HTTP status code, not exception class:HttpExceptions with status< 500(BadRequestException,NotFoundException,UnauthorizedException,ValidationPipefailures, etc.) are intentional, validated, client-facing messages — pass through completely unmodified, response body and all.Error(Soroban/TypeORM/anything not anHttpException), or anHttpExceptionat 500+ — is sanitized: the client gets a generic message plus a correlationerrorIdto quote when reporting the issue, while the full error (message + stack) is always logged server-side aterrorlevel.This status-based rule covers any current or future
HttpExceptionby actual severity rather than an enumerated "trusted" class list, and categorically strips detail from raw Errors regardless of where in the call stack they originate — satisfying the requirements' explicit "OR ensure the global filter categorically strips detail from any non-HttpException error" clause without needing to touch every raw-Errorthrow site individually.2. A second leak the filter alone doesn't cover
Investigating "does the raw simulation error detail reach the HTTP response body" for
EscrowService.fundturned up something not exception-shaped at all:fund's catch block persists the raw Soroban error message intoescrow.metadata.erroron failure, andEscrowController's endpoints return the rawEscrowentity —metadataincluded — directly as the HTTP response body. A failedfund()call itself throws (now sanitized by the filter), but a subsequentGET /escrow/:idfor that same escrow returns the persisted entity as an ordinary 200 response, with the raw internal error message sitting in the JSON body — no exception involved, so the filter never sees it.Fixed with
toPublicEscrow(src/escrow/escrow-response.mapper.ts), applied at the controller boundary acrossfund/findOne/release/refund(splitReleasereturnsPayment[], unaffected — nometadatafield there). Deliberately not applied insideEscrowServiceitself:BountiesService/MilestonesServiceconsume the fullEscrowentity returned byEscrowService.fundinternally, so narrowing the service's own return type would break those callers for no benefit.Test plan
npx jest— 81/81 passing (the one pre-existing DB-integration spec from an earlier PR fails locally as expected — no live Postgres in this environment; it runs against CI's real Postgres service)npm run lint— cleannpx tsc --noEmit— cleannpm run build— succeedsglobal-exception.filter.spec.ts(unit) — raw Soroban-style and raw TypeORM-style errors both sanitized with zero trace of the original message in the response while fully logged server-side;BadRequestException/NotFoundExceptionpass through byte-for-byte; safe errors log atwarn, sanitized aterror; sameerrorIdin response and log line; distincterrorIdper exception; non-Errorthrown values also sanitizedglobal-exception.filter.integration.spec.ts— real Nest app + supertest, asserting actual HTTP response bodies (noAppModule/DB dependency)escrow-response.mapper.spec.ts+escrow.controller.spec.ts— confirmmetadatais stripped (both the failure and success shapes) and never reaches the client across everyEscrow-returning endpoint