-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(server): sanitize internal error details in tool error responses #1830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/server': minor | ||
| --- | ||
|
|
||
| Add ToolError class for secure-by-default tool error handling. Unhandled errors from tool handlers are now sanitized to "Internal error" instead of exposing raw messages. Use `throw new ToolError('message')` when you want clients to see a specific error message. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ export type { | |
| ResourceMetadata, | ||
| ToolCallback | ||
| } from './server/mcp.js'; | ||
| export { McpServer, ResourceTemplate } from './server/mcp.js'; | ||
| export { McpServer, ResourceTemplate, ToolError } from './server/mcp.js'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 This PR introduces a breaking behavioral change: generic Error messages from tool handlers are now sanitized to Internal error instead of being forwarded to clients. No changeset was added and neither docs/migration.md nor docs/migration-SKILL.md were updated as required by CLAUDE.md. Existing tool authors who relied on throw new Error messages reaching clients will be silently broken without migration guidance; they need to update their code to use the new ToolError class. Extended reasoning...What the bug is: This PR changes how tool handler errors are surfaced to MCP clients. Previously, any Error thrown from a tool handler had its .message forwarded to the client verbatim. After this PR, only ToolError and ProtocolError messages pass through; all other errors are sanitized to Internal error. This is a meaningful security improvement, but it is a breaking behavioral change for any existing tool author who relied on the previous behavior. The specific code path: In packages/server/src/server/mcp.ts, the catch block of the tools/call handler was changed from returning error.message for any Error, to a conditional that only passes through ProtocolError and ToolError messages, and maps everything else to Internal error. Why existing safeguards do not prevent the break: CLAUDE.md explicitly states When making breaking changes, document them in both docs/migration.md and docs/migration-SKILL.md. Neither file was updated. Grep for ToolError, sanitize, or Internal error returns zero matches in both files. Additionally, the changeset bot flagged this PR as having no changeset, confirming the versioning and documentation process was not followed. Impact: Any tool author who currently does throw new Error with a specific message expecting that message to reach their client will now receive Internal error instead. This breakage is silent. There is no compile-time or runtime warning, and without migration notes, tool authors have no way of knowing they need to switch to ToolError. How to fix: Add a .changeset file with at minimum a patch bump for the server package describing the sanitization behavior. Update docs/migration.md and docs/migration-SKILL.md with a section explaining the old behavior, the new behavior, the security rationale, and the migration path using ToolError for developer-controlled messages. Step-by-step proof: Before this PR, calling server.registerTool and throwing new Error with a database connection string causes the client to receive that full string as the error message. After this PR, the same code causes the client to receive Internal error. The integration test in mcp.test.ts was itself updated from checking for the specific message text to checking for Internal error, directly confirming the observable behavioral difference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changeset and docs/migration.md updates address most of the original concern, but docs/migration-SKILL.md was not updated. CLAUDE.md requires both files to be updated for breaking changes, and the original comment called this out explicitly. The migration-SKILL.md file is the LLM-optimized dense mapping table used by tools like Claude Code to mechanically apply migrations — it needs a corresponding entry for the ToolError/error sanitization change so that automated migration tools can correctly guide users. Please add a section to docs/migration-SKILL.md covering the old behavior (throw new Error message forwarded verbatim), the new behavior (sanitized to "Internal error"), and the migration path (use ToolError for developer-controlled messages).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is quite literally the point of the PR. |
||
| export type { HostHeaderValidationResult } from './server/middleware/hostHeaderValidation.js'; | ||
| export { hostHeaderValidationResponse, localhostAllowedHostnames, validateHostHeader } from './server/middleware/hostHeaderValidation.js'; | ||
| export type { ServerOptions } from './server/server.js'; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,21 @@ import { getCompleter, isCompletable } from './completable.js'; | |
| import type { ServerOptions } from './server.js'; | ||
| import { Server } from './server.js'; | ||
|
|
||
| /** | ||
| * Error class for tool handlers to throw when they want to send a | ||
| * user-visible error message to the client. Unlike regular errors, | ||
| * ToolError messages are passed through to the client as-is. | ||
| * | ||
| * Regular errors thrown from tool handlers are sanitized to "Internal | ||
| * error" to prevent leaking sensitive server internals. | ||
| */ | ||
| export class ToolError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = 'ToolError'; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * High-level MCP server that provides a simpler API for working with resources, tools, and prompts. | ||
| * For advanced usage (like sending notifications or setting custom request handlers), use the underlying | ||
|
|
@@ -209,7 +224,16 @@ export class McpServer { | |
| if (error instanceof ProtocolError && error.code === ProtocolErrorCode.UrlElicitationRequired) { | ||
| throw error; // Return the error to the caller without wrapping in CallToolResult | ||
| } | ||
| return this.createToolError(error instanceof Error ? error.message : String(error)); | ||
| if (error instanceof ProtocolError) { | ||
| // SDK-generated validation errors are safe to expose | ||
| return this.createToolError(error.message); | ||
| } | ||
| if (error instanceof ToolError) { | ||
|
Comment on lines
225
to
+231
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The Extended reasoning...Bug: ProtocolError bypass of sanitizationWhat the bug is and how it manifests The error sanitization logic in The specific code path that triggers it In the catch block at lines 226-232 of Why existing code does not prevent it There is no private symbol, internal flag, or sealed constructor that distinguishes an SDK-generated Impact The PR's stated security goal — sanitizing all unexpected errors to 'Internal error' — is weakened. Any sensitive information placed into a Step-by-step proof
How to fix it Use a private symbol or internal subclass to mark SDK-generated Addressing the refutation The refutation argues this requires deliberately non-idiomatic code and no third-party library throws
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would consider this user error - |
||
| // Developer intentionally wants this message shown to client | ||
| return this.createToolError(error.message); | ||
| } | ||
| // All other errors: sanitize to prevent leaking internals | ||
| return this.createToolError('Internal error'); | ||
| } | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The new '## Tool error sanitization' section in docs/migration.md is placed as a standalone top-level section after '## Enhancements', outside the '## Breaking Changes' block where it belongs — readers scanning that section will miss this significant behavioral change. Additionally, the code example contains two unconditional sequential throws in the same function body, making the second
throw new ToolError('Invalid country')permanently unreachable dead code; the example should be split into two separate snippets (or use conditional branching) to correctly illustrate both behaviors.Extended reasoning...
Section misplacement violates CLAUDE.md grouping rule
CLAUDE.md line 32 explicitly states: 'Search for related sections and group related changes together rather than adding new standalone sections.' The new '## Tool error sanitization' section is placed as a standalone H2 after '## Enhancements' and immediately before '## Unchanged APIs'. Every other breaking change in this document is correctly nested as an H3 subsection under the '## Breaking Changes' block (e.g., '### Package split', '### Error hierarchy refactoring'). The tool error sanitization change is a breaking behavioral change — existing tool handlers that threw
new Error('message')expecting that message to reach the client now silently receive"Internal error"instead. A developer scanning only the '## Breaking Changes' section will miss this entry entirely.The fix is to move the section inside '## Breaking Changes' as a '### Tool error sanitization' subsection, consistent with the document's existing structure. Placing it near the existing '### Error hierarchy refactoring' section would be especially logical since both deal with error handling changes.
Unreachable code in documentation example
The code example added at lines 882–888 of the diff contains two unconditional sequential
throwstatements in the same async function body:In JavaScript,
throwis a statement that immediately terminates function execution. The firstthrow new Error(...)always fires and exits the function, making the secondthrow new ToolError('Invalid country')permanently unreachable dead code that can never execute under any circumstance.Why existing code does not prevent this: This is a documentation quality issue with no runtime check — the code is syntactically valid JavaScript/TypeScript, so compilers and linters won't flag it as an error (though some static analysis tools may warn about unreachable code). The PR description itself uses the same shorthand pattern, suggesting this was an intentional stylistic choice, but in documentation examples, logical correctness matters because developers copy-paste these directly.
Impact of the unreachable code: A developer copying this example literally would get a tool handler that always throws a generic
Errorand always receives"Internal error"from the client. They would never seeToolErrorhave any effect, which is the opposite of what the example intends to demonstrate. This could lead to confusion about whetherToolErroractually works.Step-by-step proof: (1) Developer copies the example. (2) The tool handler always executes
throw new Error('DB connection failed at 10.0.0.5:5432')on the first line. (3) Function exits — the secondthrowstatement is never reached. (4) Client always receives"Internal error". (5) Developer wonders whythrow new ToolError('Invalid country')has no effect. (6) Developer wastes time debugging a documentation error.How to fix both issues: Move the section to '## Breaking Changes' as a '### Tool error sanitization' H3, and split the code example into two separate snippets clearly labeled 'Scenario 1' and 'Scenario 2', or use conditional branching (e.g.,
if (dbError) throw new Error(...); else throw new ToolError(...)) to make both throws reachable and independently demonstrable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is legit - the example code doesn't make sense as is, throwing twice in succession. Better to have 2 registerTools to have code that actually reaches all lines.