fix(server): sanitize internal error details in tool error responses

[claygeo]

Summary

Closes #1429

When a tool handler throws an unexpected error, the full error message is sent to the MCP client, potentially leaking sensitive server internals (hostnames, connection strings, stack traces).

This PR:

• Adds a ToolError class that tool authors can throw when they want a specific message shown to the client
• Sanitizes all other unhandled errors to "Internal error"
• Preserves ProtocolError messages (SDK-generated validation errors are safe to expose)

Usage

import { McpServer, ToolError } from '@modelcontextprotocol/server';

server.registerTool('my-tool', {}, async () => {
    // This message WILL be shown to the client
    throw new ToolError('Invalid input: country not supported');

    // This message will NOT be shown — client sees "Internal error"
    throw new Error('DB connection failed at 10.0.0.5:5432');
});

Error handling behavior

Error type	Client sees	Rationale
ToolError	Developer's message	Intentional, developer-controlled
ProtocolError	SDK message	SDK-generated validation, safe to expose
Error	"Internal error"	May contain sensitive internals
Non-Error throw	"Internal error"	Unknown, sanitize defensively

Test plan

• Generic Error is sanitized to "Internal error"
• ToolError message passes through to client
• Non-Error throw (string) is sanitized to "Internal error"
• Existing ProtocolError validation tests still pass (129/129)
• Task cancellation error sanitized correctly

[changeset-bot]

🦋 Changeset detected

Latest commit: eafaf13

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@modelcontextprotocol/server	Minor
@modelcontextprotocol/express	Major
@modelcontextprotocol/fastify	Major
@modelcontextprotocol/hono	Major
@modelcontextprotocol/node	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@1830

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@1830

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@1830

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@1830

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@1830

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@1830

commit: eafaf13

[km-anthropic]

@claude review

[felixweinberger]

@claude review

[claude]

🔴 The instanceof ProtocolError check in the error sanitization catch block cannot distinguish SDK-generated errors from user-thrown ones, since ProtocolError is publicly exported from @modelcontextprotocol/server. Tool handler authors can throw new ProtocolError(someCode, sensitiveMessage) and bypass sanitization, causing sensitive internals to leak to clients despite the PR's intent to prevent this.

Extended reasoning...

Bug: ProtocolError bypass of sanitization

What the bug is and how it manifests

The error sanitization logic in packages/server/src/server/mcp.ts at lines 226-232 contains a catch block that checks instanceof ProtocolError and passes the error message through to the client unchanged. The comment reads: SDK-generated validation errors are safe to expose. However, this assumption is not enforced in code. ProtocolError is publicly exported from @modelcontextprotocol/core/public, which is re-exported by @modelcontextprotocol/server via export * from '@modelcontextprotocol/core/public'. Any tool handler author can import and throw ProtocolError directly.

The specific code path that triggers it

In the catch block at lines 226-232 of mcp.ts, the branch checking instanceof ProtocolError returns this.createToolError(error.message) — passing the message verbatim to the client. Since ProtocolError is publicly accessible, a tool handler can throw one directly. JavaScript's instanceof checks class identity, not origin, so this branch fires for both SDK-thrown and user-thrown ProtocolError instances.

Why existing code does not prevent it

There is no private symbol, internal flag, or sealed constructor that distinguishes an SDK-generated ProtocolError from one instantiated by user code. The ToolError class was introduced as the explicit, documented opt-in for passing messages to clients, but ProtocolError now functions as an undocumented second bypass path.

Impact

The PR's stated security goal — sanitizing all unexpected errors to 'Internal error' — is weakened. Any sensitive information placed into a ProtocolError message inside a tool handler (DB connection strings, hostnames, internal paths) will reach the client verbatim. This can happen via: (1) deliberate misuse, (2) a developer wrapping or rethrowing a caught ProtocolError with added sensitive context (a very common pattern), or (3) a developer unfamiliar with the ToolError/ProtocolError distinction using ProtocolError as a general-purpose error class.

Step-by-step proof

1. Tool handler author imports ProtocolError and ProtocolErrorCode from @modelcontextprotocol/server.
2. Handler throws: throw new ProtocolError(ProtocolErrorCode.InvalidParams, 'DB at postgres://admin:password@10.0.0.5:5432/prod')
3. Client calls tools/call with the tool name.
4. The catch block's instanceof ProtocolError branch matches.
5. this.createToolError(error.message) returns the sensitive message verbatim.
6. Client receives { isError: true, content: [{ type: 'text', text: 'DB at postgres://admin:password@10.0.0.5:5432/prod' }] }.

How to fix it

Use a private symbol or internal subclass to mark SDK-generated ProtocolError instances. For example, introduce an InternalProtocolError extends ProtocolError class used only by validateToolInput, validateToolOutput, and the task polling path, and replace the instanceof ProtocolError branch with instanceof InternalProtocolError. Alternatively, tag SDK-generated errors with a non-enumerable symbol at construction time.

Addressing the refutation

The refutation argues this requires deliberately non-idiomatic code and no third-party library throws ProtocolError accidentally. This is partially true, but misses two important real-world cases: developers wrapping/rethrowing SDK errors with additional sensitive context (a very common error-handling pattern), and developers unfamiliar with the ToolError/ProtocolError distinction who reach for ProtocolError as a general error class. The security guarantee of this PR should be robust against both cases.

[felixweinberger]

I would consider this user error - ProtocolError is a power user error class that might be used e.g. if you're building a Protocol Extension (hence it gets exported) but if using it for random userland errors that's just using errors wrong.

[claude]

🔴 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.

[claude]

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).

[felixweinberger]

This is quite literally the point of the PR.

[felixweinberger]

Thanks, secure-by-default makes sense here.

1. Need a changeset (minor) and a docs/migration.md entry — v1 authors who throw new Error('msg') expecting clients to see it will now get "Internal error", so they need to know to switch to ToolError
2. Move ToolError above McpServer's JSDoc block — it currently sits between the JSDoc and the class, which orphans McpServer in generated docs
3. Nit: cancellation test at :7080 could add expect(result.isError).toBe(true) to match the others

Also tagging @KKonstantinov for a look given recent work on error shapes in #1454 — wdyt? I think introducing ToolError is legit because tools have a specific error-in-result shape and this doesn't neatly fit either ProtocolError or SdkError, but just in case you had thoughts here.

[claygeo]

@felixweinberger All three addressed:

1. Changeset — added minor changeset for @modelcontextprotocol/server
2. ToolError placement — moved above the McpServer JSDoc block so generated docs aren't orphaned
3. Migration docs — added a "Tool error sanitization" section to docs/migration.md explaining the breaking change and how to use ToolError
4. Test nit — added expect(result.isError).toBe(true) to the cancellation test

[claygeo]

@felixweinberger All three items from your review are addressed — changeset added, ToolError moved above McpServer JSDoc, and migration docs entry included. Ready for another look when you get a chance.

[felixweinberger]

@claude review

[claude]

🟡 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 throw statements in the same async function body:

server.registerTool('my-tool', {}, async () => {
    // Client sees: "Internal error"
    throw new Error('DB connection failed at 10.0.0.5:5432');

    // Client sees: "Invalid country"
    throw new ToolError('Invalid country');
});

In JavaScript, throw is a statement that immediately terminates function execution. The first throw new Error(...) always fires and exits the function, making the second throw 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 Error and always receives "Internal error" from the client. They would never see ToolError have any effect, which is the opposite of what the example intends to demonstrate. This could lead to confusion about whether ToolError actually 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 second throw statement is never reached. (4) Client always receives "Internal error". (5) Developer wonders why throw 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.

[felixweinberger]

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.

[felixweinberger]

Thanks for the turnaround - can we update migration-SKILL.md as well please?