Skip to content

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.

Sign up for GitHub

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

Open
claygeo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89b9adc
fix(server): sanitize internal error details in tool error responses
claygeo Mar 31, 2026
3e21c7c
address review: changeset, move ToolError, migration docs, test nit
claygeo Apr 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tool-error-sanitization.md
View file
Open in desktop
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.
20 changes: 20 additions & 0 deletions docs/migration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,26 @@ import { DefaultJsonSchemaValidator } from '@modelcontextprotocol/server/_shims'
import { AjvJsonSchemaValidator, CfWorkerJsonSchemaValidator } from '@modelcontextprotocol/server';
```

## Tool error sanitization

Tool handlers that `throw new Error('message')` will now return `"Internal error"` to clients instead of the raw error message. This prevents accidental leakage of server internals (hostnames, connection strings, stack traces).

To send a user-visible error message, use the new `ToolError` class:

```typescript
import { ToolError } from '@modelcontextprotocol/server';

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');
});
```

`ProtocolError` messages (SDK validation errors) are still passed through unchanged.

## Unchanged APIs

The following APIs are unchanged between v1 and v2 (only the import paths changed):
Expand Down
2 changes: 1 addition & 1 deletion packages/server/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Copy Markdown

@claude claude bot Apr 1, 2026

Choose a reason for hiding this comment

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

export type { HostHeaderValidationResult } from './server/middleware/hostHeaderValidation.js';
export { hostHeaderValidationResponse, localhostAllowedHostnames, validateHostHeader } from './server/middleware/hostHeaderValidation.js';
export type { ServerOptions } from './server/server.js';
Expand Down
26 changes: 25 additions & 1 deletion packages/server/src/server/mcp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Copy Markdown

@claude claude bot Apr 1, 2026

Choose a reason for hiding this comment

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

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

// 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');
}
});

Expand Down
112 changes: 109 additions & 3 deletions test/integration/test/server/mcp.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
UriTemplate,
UrlElicitationRequiredError
} from '@modelcontextprotocol/core';
import { completable, McpServer, ResourceTemplate } from '@modelcontextprotocol/server';
import { completable, McpServer, ResourceTemplate, ToolError } from '@modelcontextprotocol/server';
import { afterEach, beforeEach, describe, expect, test } from 'vitest';
import * as z from 'zod/v4';

Expand Down Expand Up @@ -1799,7 +1799,112 @@ describe('Zod v4', () => {
expect(result.content).toEqual([
{
type: 'text',
text: 'Tool execution failed'
text: 'Internal error'
}
]);
});

test('should pass through ToolError message to client', async () => {
const mcpServer = new McpServer({
name: 'test server',
version: '1.0'
});

const client = new Client({
name: 'test client',
version: '1.0'
});

mcpServer.registerTool('toolerror-test', {}, async () => {
throw new ToolError('Invalid input: country not supported');
});

const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();

await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);

const result = await client.request({
method: 'tools/call',
params: {
name: 'toolerror-test'
}
});

expect(result.isError).toBe(true);
expect(result.content).toEqual([
{
type: 'text',
text: 'Invalid input: country not supported'
}
]);
});

test('should sanitize generic Error to Internal error', async () => {
const mcpServer = new McpServer({
name: 'test server',
version: '1.0'
});

const client = new Client({
name: 'test client',
version: '1.0'
});

mcpServer.registerTool('internal-error-test', {}, async () => {
throw new Error('Connection failed at 10.0.0.5:5432');
});

const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();

await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);

const result = await client.request({
method: 'tools/call',
params: {
name: 'internal-error-test'
}
});

expect(result.isError).toBe(true);
expect(result.content).toEqual([
{
type: 'text',
text: 'Internal error'
}
]);
});

test('should sanitize non-Error throws to Internal error', async () => {
const mcpServer = new McpServer({
name: 'test server',
version: '1.0'
});

const client = new Client({
name: 'test client',
version: '1.0'
});

mcpServer.registerTool('string-throw-test', {}, async () => {
throw 'some raw string error';
});

const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();

await Promise.all([client.connect(clientTransport), mcpServer.server.connect(serverTransport)]);

const result = await client.request({
method: 'tools/call',
params: {
name: 'string-throw-test'
}
});

expect(result.isError).toBe(true);
expect(result.content).toEqual([
{
type: 'text',
text: 'Internal error'
}
]);
});
Expand Down Expand Up @@ -6971,8 +7076,9 @@ describe('Zod v4', () => {
});

// Should receive an error since cancelled tasks don't have results
expect(result.isError).toBe(true);
expect(result).toHaveProperty('content');
expect(result.content).toEqual([{ type: 'text' as const, text: expect.stringContaining('has no result stored') }]);
expect(result.content).toEqual([{ type: 'text' as const, text: 'Internal error' }]);

// Wait for async operations to complete
await waitForLatch();
Expand Down
Loading