Skip to content

fix: surface real server error details to client console#1748

Open
MrDirkelz wants to merge 2 commits into
mainfrom
1667-app-server-errors-not-giving-good-enough-details-in-console
Open

fix: surface real server error details to client console#1748
MrDirkelz wants to merge 2 commits into
mainfrom
1667-app-server-errors-not-giving-good-enough-details-in-console

Conversation

@MrDirkelz

Copy link
Copy Markdown
Collaborator

The API's exception filter stripped 5xx errors down to a generic message before the client ever saw them, so the app/cms console.error calls had nothing useful to log. Send the real exception.message (never .stack) to clients in all environments; the friendly toast copy is now purely a client-side display choice, unrelated to what the server sends.

@MrDirkelz MrDirkelz linked an issue Jul 2, 2026 that may be closed by this pull request
@MrDirkelz MrDirkelz self-assigned this Jul 2, 2026
@ivanslabbert

Copy link
Copy Markdown
Contributor

Review summary — ✅ Approve

  • Ticket fulfilled: AllExceptionsFilter now returns the real exception.message for 5xx/unknown errors instead of the generic string (api/src/exceptions/allExceptions.filter.ts:55,64), and both clients now console.error it (app/src/main.ts:136, cms/src/main.ts:108). The message already flows client-side via shared/src/api/http.ts:40, so no shared change needed.
  • Correct end-to-end: actualMessage = exception instanceof Error ? exception.message : String(exception) handles Error, HttpException, and non-Error throws; spec updates cover all three. 4xx passthrough untouched.
  • Sound security tradeoff: only .message is sent, never .stack (documented in the filter JSDoc allExceptions.filter.ts:14-18). Residual risk that a raw Error(...) message contains internals is explicitly accepted.
  • Good placement: console.error sits before the serverErrorTimeout debounce guard, so every error is logged while toast/Sentry stay debounced; the error.message ? ... guard handles undefined safely.
  • No cross-package contract impact: response shape {statusCode, message} unchanged; no DTO/sync-query/FTS/auth surface touched.

🤖 AI-assisted review summary (Claude Code). Read-only analysis of the PR diff — not a substitute for human review.

@ivanslabbert

Copy link
Copy Markdown
Contributor
  • Sound security tradeoff: only .message is sent, never .stack (documented in the filter JSDoc allExceptions.filter.ts:14-18). Residual risk that a raw Error(...) message contains internals is explicitly accepted.

Think we need to ensure that raw error messages are not sent to the client - only extracted string messages.

MrDirkelz added a commit that referenced this pull request Jul 6, 2026
Only forward .message when the exception is an HttpException we threw
ourselves — that text is deliberately authored and safe. Anything else
(a bare Error from nano, the S3 SDK, or a bug) gets a generic message
instead, since its .message was never vetted for client exposure and
could leak internals such as the CouchDB connection string. The real
cause is still logged server-side either way.

Addresses review feedback on PR #1748.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@johan-bell

Copy link
Copy Markdown
Collaborator

Passing through the real message for all HttpExceptions regardless of status (not just <500) removes the filter's safety net for 5xx errors. Today every HttpException in the codebase uses a hardcoded 4xx string, so this isn't exploitable yet, but nothing stops a future 5xx throw like new InternalServerErrorException(dbErr.message) from leaking internals straight to the client — this filter would now consider that message "ours" and forward it verbatim. Given storageStatus.controller.ts already has a similar pattern (interpolating error.message into a response), this seems likely to happen again. Worth keeping 5xx responses generic regardless of exception type, or restricting the passthrough to an explicit allowlist of exception classes known to carry only safe, hardcoded text.

MrDirkelz and others added 2 commits July 16, 2026 08:07
The API's exception filter stripped 5xx errors down to a generic
message before the client ever saw them, so the app/cms console.error
calls had nothing useful to log. Send the real exception.message
(never .stack) to clients in all environments; the friendly toast
copy is now purely a client-side display choice, unrelated to what
the server sends.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Only forward .message when the exception is an HttpException we threw
ourselves — that text is deliberately authored and safe. Anything else
(a bare Error from nano, the S3 SDK, or a bug) gets a generic message
instead, since its .message was never vetted for client exposure and
could leak internals such as the CouchDB connection string. The real
cause is still logged server-side either way.

Addresses review feedback on PR #1748.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@MrDirkelz
MrDirkelz force-pushed the 1667-app-server-errors-not-giving-good-enough-details-in-console branch from 4db9f70 to 0f58c32 Compare July 16, 2026 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

APP: Server errors not giving good enough details in console

3 participants