fix(http): stop leaking internal error details in /mcp 500 responses - #36
Closed
andesyteoss wants to merge 1 commit into
Closed
fix(http): stop leaking internal error details in /mcp 500 responses#36andesyteoss wants to merge 1 commit into
andesyteoss wants to merge 1 commit into
Conversation
Interpolating the caught Error into the JSON response exposed stack traces and internal paths to unauthenticated HTTP callers. Return a generic message and keep full details in server logs only.
Author
|
Closing this as inactive — no maintainer response after 14 days. The security finding and fix remain valid. If this is still relevant, I'm happy to reopen, rebase, or re-submit against a different branch. Just drop a comment. |
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
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
The HTTP MCP server's three
/mcphandlers (POST, GET, DELETE) currently embed the rawerrorobject in the JSON response returned to the client on 500 responses:Because
${error}stringifies whatever exception was thrown by the underlying transport, viem, or Node itself, the response can contain stack traces, internal file paths, dependency version fingerprints, RPC endpoint URLs, or partially formed request state. This is a classic CWE-209 (Generation of Error Message Containing Sensitive Information) exposure. The HTTP server binds0.0.0.0:3001by default (seestartHttpServerinsrc/server/http-server.ts), so any network-adjacent caller can trigger it with a malformed request.Fix
Return a static
"Internal server error"string to the client and keep the full error inconsole.error, where operators can still see it. Three-line change, no behavioural change for successful requests, no new dependencies.Full details continue to be logged server-side via the existing
console.error(...)calls immediately above each response.Proof of concept
With the server running (
bun run start:http— mounts routes defined insrc/server/http-server.ts):Before the patch the response body contains a rendered error (e.g.
{"error":"Internal server error: SyntaxError: Unexpected end of JSON input\n at ..."}), leaking parser internals and file paths from the bundlednode_modules. After the patch the body is{"error":"Internal server error"}and the stack remains only in the server log.The
DELETE /mcpandGET /mcp(SSE) paths have the same pattern and are fixed identically; sending either without a validmcp-session-idheader reaches the same catch block.Security analysis
0.0.0.0:3001) and the ability to send a request that causes the handler to throw. No authentication is required at the transport layer.Adversarial review
Before submitting we tried to talk ourselves out of this one. The obvious counter-argument is "the error strings are usually harmless" — but
${error}on a NodeErrorrenders the full stack including absolute filesystem paths from the running host, and viem/StreamableHTTP errors include RPC URLs and request fragments. There is no framework-level filter in front of these responses (the handler writes directly withres.status(500).json(...)), and Express does not sanitize user-supplied error content. The fix is a strict subtraction — no user-visible behaviour changes for successful requests, and operators keep full detail in logs — so we don't see a downside to landing it independently of the larger auth/session work.Testing
bun installandbun run buildsucceed on the branch.tscis clean; the change is a pure string replacement with no type impact.bun run start:httponmainand confirmed the patched build returns the static message while the server log still shows the full stack.