fix: do not expose internal error details to clients#1851
fix: do not expose internal error details to clients#1851lexwhiting wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
…tocol#1429) Tool handlers that throw unhandled errors now return a generic "Internal error" message instead of leaking the raw error.message to the client. This prevents exposing stack traces, connection strings, internal hostnames, or other sensitive information. - Add ToolError class for intentional client-visible errors - Add McpServerOptions.onToolError callback for server-side logging - ProtocolError messages (validation, invalid params) remain visible - Generic Error/unknown exceptions are sanitized to "Internal error" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
travisbreaks
left a comment
There was a problem hiding this comment.
Solid security improvement. The three-tier model (ToolError -> ProtocolError -> generic) is clean. A few items worth considering:
1. instanceof fragility across package boundaries
error instanceof ToolError will fail if the server author's copy of the SDK is a different instance (bundled, duplicated in node_modules, or loaded via different package manager hoisting). Their intentional ToolError would silently fall through to "Internal error". A Symbol-based brand check or static ToolError.isToolError(error) method would be more robust:
const TOOL_ERROR_BRAND = Symbol.for('mcp.ToolError');
export class ToolError extends Error {
readonly [Symbol.for('mcp.ToolError')] = true;
static isToolError(error: unknown): error is ToolError {
return typeof error === 'object' && error !== null && Symbol.for('mcp.ToolError') in error;
}
}Symbol.for is globally shared, so it survives package duplication.
2. ProtocolError assumed safe
The PR passes all ProtocolError messages through to clients. Worth verifying that no ProtocolError is ever constructed with user-supplied or system-internal data (file paths, connection strings) elsewhere in the SDK. If any are, those would leak through this path.
3. Silent loss without onToolError
Server authors who don't set onToolError lose all signal from internal errors. The client sees "Internal error" with no correlation ID, and the server has no log. Consider:
- Logging to
console.errorby default (not just when callback is set) - Including a unique error ID in the client response for log correlation
4. Breaking change for existing consumers
Any code that parses tool error messages (checking for specific substrings) will break since those messages are now "Internal error". The PR description doesn't mention this. Worth calling out in the changeset (which is also missing).
5. Missing changeset
The changeset-bot flagged this. This should be at least a patch bump so downstream consumers can pin to the fix.
Tests are well-structured. The connection-string-leak test is a particularly good demonstration of the security boundary.
|
Thanks for working on this! Closing as a duplicate of #1830 which is closer to being ship ready. |
Summary
Fixes #1429. Tool handlers that throw unhandled errors now return a generic "Internal error" message instead of leaking the raw
error.messageto the client.Before: Any thrown error (including stack traces, connection strings, internal hostnames) was serialized and sent to the client as a
CallToolResultwithisError: true.After:
ToolError— a new error class for intentional client-visible messages. Thrownew ToolError('Invalid input: ...')and the message is returned to the client.ProtocolError— SDK-internal errors (validation, invalid params) remain visible, as they contain no sensitive information."Internal error". The real error is passed to an optionalonToolErrorcallback for server-side logging.Changes
ToolErrorclass exported from@modelcontextprotocol/serverMcpServerOptionsinterface with optionalonToolErrorcallbacktools/callhandler to sanitize unhandled errors"Internal error"instead of raw messagesUsage
Test plan
ToolErrormessages are returned to the clientToolErrorexceptions return "Internal error" and triggeronToolErrorcallbackProtocolError) still return descriptive messagesUrlElicitationRequiredErrorstill re-throws correctly