Skip to content

fix(server): sanitize error responses to prevent stack trace exposure#1187

Open
cliffhall wants to merge 1 commit intomodelcontextprotocol:mainfrom
cliffhall:fix-info-exposure-alerts
Open

fix(server): sanitize error responses to prevent stack trace exposure#1187
cliffhall wants to merge 1 commit intomodelcontextprotocol:mainfrom
cliffhall:fix-info-exposure-alerts

Conversation

@cliffhall
Copy link
Copy Markdown
Member

Summary

Resolves the 11 open CodeQL js/stack-trace-exposure alerts in server/src/index.ts. Each was a spot where a raw caught error object was handed to res.json(...), which can leak internal error details (including stack-trace-derived data) to clients.

The fix introduces a small sendErrorResponse(res, status, message) helper that returns a generic sanitized JSON body, and replaces every flagged call site with it. Full error objects continue to be logged server-side via the existing console.error calls, so debuggability is unchanged.

Alerts addressed

Incidental fix

The /sse handler's ECONNREFUSED branch was missing a return, so after sending its 500 it fell through to the generic Error in /sse route branch and attempted a second response on the already-sent headers. Added the missing return since the sanitization pass touched those lines anyway.

Behavior changes

  • Clients no longer receive raw serialized Error objects on 4xx/5xx failures from /mcp, /stdio, /sse, /message, or /config. They now receive { "error": "<generic message>" }.
  • sendProxiedUnauthorized no longer takes an error parameter — the upstream-capture path was already ignoring it, and the fallback path now returns a plain { error: "Unauthorized" } instead of JSON-encoding the caught exception.
  • Server-side logs are unchanged — every site still logs the full error via console.error before responding.

Test plan

  • npm run build-server passes (tsc)
  • npm run prettier-fix clean
  • Manually verify an error path (e.g. connect /mcp to an unreachable URL) returns { "error": "Internal server error" } rather than a serialized exception, and that console.error still logs full details server-side
  • Manually verify OAuth 401 forwarding from an upstream MCP server still returns the captured WWW-Authenticate header and body (the non-fallback path in sendProxiedUnauthorized)

🤖 Generated with Claude Code

Replace raw error objects passed to res.json() with generic sanitized
messages so internal error details are not leaked to clients. Full
errors continue to be logged server-side via console.error.

Also adds a missing return in the /sse ECONNREFUSED branch, which
previously fell through and attempted a second response after headers
had already been sent.

Resolves CodeQL js/stack-trace-exposure alerts in server/src/index.ts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses CodeQL js/stack-trace-exposure findings in the server by ensuring Express routes no longer serialize raw caught Error objects into HTTP JSON responses, reducing the risk of leaking stack traces/internal details to clients.

Changes:

  • Added a sendErrorResponse(res, status, message) helper to return sanitized { error: "<message>" } responses.
  • Replaced multiple res.status(...).json(error) call sites across /mcp, /stdio, /sse, /message, and /config with sanitized responses.
  • Fixed a /sse error-path fallthrough by adding a missing return after responding in the ECONNREFUSED branch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants