fix(gateway): don't leak error details on unhandled 500 - #104
Conversation
The catch-all in the gateway HTTP handler returned `err.message` directly to the client, which CodeQL flags as `js/stack-trace-exposure` (alert #4 on main). The gateway listens on 127.0.0.1 only so the practical attack surface is small, but error messages can still surface to logs/transcripts that flow further out. - Log the full error (with stack) server-side via logError(). - Return a generic "Internal server error" to the client. The structured handler-level error path (handleAction) is unchanged — those errors are part of the action protocol and tests assert their content. Only the truly-unexpected outer 500 is sanitized. All 1635 tests pass; the existing test that triggers this path (`returns 500 when handleAction result cannot be JSON-serialized`) only asserts `body.error` is truthy, which still holds.
There was a problem hiding this comment.
Pull request overview
This PR addresses a CodeQL js/stack-trace-exposure alert by sanitizing the gateway’s catch-all HTTP 500 error response so unexpected server errors no longer return internal error details to the client.
Changes:
- Log unexpected request-lifecycle errors server-side via
logError("gateway", ...). - Replace the client-facing 500 JSON response error with a generic
"Internal server error"message.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `Unhandled error on ${req.method} ${req.url}: ${detail}`, | ||
| err, |
Address Copilot review feedback on PR #104: logError previously only captured err.message, dropping the stack. Enhance logError to include err.stack as a separate structured field when err is an Error. - pino-pretty renders the stack field on its own line for readability - JSON log consumers can read err (concise) or stack (full trace) - Benefits all 234 logError callsites, not just gateway Simplify the gateway catch block accordingly: drop the manual detail = err.stack ?? err.message interpolation in the message string, since the structured stack field now carries it. Updated log.test.ts assertion to expect both err and stack fields. 1635/1635 tests pass, tsc clean, lint 0 errors, prettier clean.
|
Good catch from Copilot — Pushed if (err instanceof Error) {
logger.error({ component, err: err.message, stack: err.stack }, message);
}
Updated Verified locally:
Diff vs |
Summary
Fixes CodeQL alert #4 (
js/stack-trace-exposure, medium) onsrc/core/gateway.ts.The catch-all handler at the bottom of the request lifecycle was returning
err.messagedirectly to the HTTP client. The gateway only listens on 127.0.0.1 so the real attack surface is small, but errors can still propagate into logs/transcripts that flow further out, and CodeQL is flagging it on every main commit.Changes
logError("gateway", ...)."Internal server error".handleActionis unchanged — those errors are part of the action protocol (e.g."No active chat context","send_message: ...") and tests assert their content. Only the truly-unexpected outer 500 is sanitized.Test plan
npx vitest run src/__tests__/gateway-http.test.ts— 24 passednpx vitest run— full suite, 1635/1635 passednpx tsc --noEmit— cleannpm run lint— 0 errors (9 pre-existing warnings)returns 500 when handleAction result cannot be JSON-serialized) only assertsbody.erroris truthy — still holds.🤖 Heartbeat agent — picked up from #45's "next time: investigate vulnerabilities Dylan flagged on main"