From 5c31d5a560eee31f144cc68955a9838c5a8075a6 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 6 Feb 2026 09:27:04 -0800 Subject: [PATCH 1/2] fix: Redact config error details in HTTP transport (#781) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConfigurationError, LLMProviderError, and APICallError 4xx now route differently based on transport. In STDIO mode, detailed messages are preserved so users can fix their own config. In HTTP mode (Cloudflare), errors are logged to Sentry via logIssue() and a generic "Feature Unavailable" message is returned—users can't fix server-side config, but operators get visibility through Sentry. Co-Authored-By: Claude Opus 4.6 --- .../src/server/lib/mcp-handler.ts | 1 + .../src/internal/error-handling.test.ts | 121 ++++++++++++++++++ .../mcp-core/src/internal/error-handling.ts | 107 +++++++++++----- packages/mcp-core/src/server.ts | 4 +- packages/mcp-core/src/types.ts | 4 + packages/mcp-server/src/index.ts | 1 + 6 files changed, 208 insertions(+), 30 deletions(-) create mode 100644 packages/mcp-core/src/internal/error-handling.test.ts diff --git a/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts b/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts index af7078dc0..d3cd1a0b2 100644 --- a/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts +++ b/packages/mcp-cloudflare/src/server/lib/mcp-handler.ts @@ -173,6 +173,7 @@ const mcpHandler: ExportedHandler = { mcpUrl: env.MCP_URL, agentMode: isAgentMode, experimentalMode: isExperimentalMode, + transport: "http", }; // Create and configure MCP server with tools filtered by context diff --git a/packages/mcp-core/src/internal/error-handling.test.ts b/packages/mcp-core/src/internal/error-handling.test.ts new file mode 100644 index 000000000..d1f2282d4 --- /dev/null +++ b/packages/mcp-core/src/internal/error-handling.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { formatErrorForUser } from "./error-handling"; +import { + UserInputError, + ConfigurationError, + LLMProviderError, +} from "../errors"; +import { APICallError } from "ai"; + +vi.mock("../telem/logging", () => ({ + logIssue: vi.fn(() => "mock-event-id"), +})); + +import { logIssue } from "../telem/logging"; + +describe("formatErrorForUser", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe("ConfigurationError", () => { + const error = new ConfigurationError("OPENAI_API_KEY is not set"); + + it("returns detailed message for stdio transport", async () => { + const result = await formatErrorForUser(error, { transport: "stdio" }); + expect(result).toContain("OPENAI_API_KEY is not set"); + expect(result).toContain("**Configuration Error**"); + expect(result).not.toContain("Feature Unavailable"); + expect(logIssue).not.toHaveBeenCalled(); + }); + + it("returns generic message for http transport", async () => { + const result = await formatErrorForUser(error, { transport: "http" }); + expect(result).toContain("**Feature Unavailable**"); + expect(result).not.toContain("OPENAI_API_KEY is not set"); + expect(logIssue).toHaveBeenCalledWith(error); + }); + + it("returns detailed message when transport is undefined (backward compat)", async () => { + const result = await formatErrorForUser(error); + expect(result).toContain("OPENAI_API_KEY is not set"); + expect(result).toContain("**Configuration Error**"); + expect(logIssue).not.toHaveBeenCalled(); + }); + }); + + describe("LLMProviderError", () => { + const error = new LLMProviderError("Region not supported by OpenAI"); + + it("returns detailed message for stdio transport", async () => { + const result = await formatErrorForUser(error, { transport: "stdio" }); + expect(result).toContain("Region not supported by OpenAI"); + expect(result).toContain("**AI Provider Error**"); + expect(result).not.toContain("Feature Unavailable"); + expect(logIssue).not.toHaveBeenCalled(); + }); + + it("returns generic message for http transport", async () => { + const result = await formatErrorForUser(error, { transport: "http" }); + expect(result).toContain("**Feature Unavailable**"); + expect(result).not.toContain("Region not supported by OpenAI"); + expect(logIssue).toHaveBeenCalledWith(error); + }); + + it("returns detailed message when transport is undefined", async () => { + const result = await formatErrorForUser(error); + expect(result).toContain("Region not supported by OpenAI"); + expect(logIssue).not.toHaveBeenCalled(); + }); + }); + + describe("APICallError 4xx", () => { + const error = new APICallError({ + message: "Invalid API key provided", + url: "https://api.openai.com/v1/chat/completions", + requestBodyValues: {}, + statusCode: 401, + isRetryable: false, + }); + + it("returns detailed message for stdio transport", async () => { + const result = await formatErrorForUser(error, { transport: "stdio" }); + expect(result).toContain("Invalid API key provided"); + expect(result).toContain("**AI Provider Error**"); + expect(result).not.toContain("Feature Unavailable"); + expect(logIssue).not.toHaveBeenCalled(); + }); + + it("returns generic message for http transport", async () => { + const result = await formatErrorForUser(error, { transport: "http" }); + expect(result).toContain("**Feature Unavailable**"); + expect(result).not.toContain("Invalid API key provided"); + expect(logIssue).toHaveBeenCalledWith(error); + }); + + it("returns detailed message when transport is undefined", async () => { + const result = await formatErrorForUser(error); + expect(result).toContain("Invalid API key provided"); + expect(logIssue).not.toHaveBeenCalled(); + }); + }); + + describe("UserInputError", () => { + const error = new UserInputError("Invalid issue ID format"); + + it("returns detailed message for http transport (user can fix input)", async () => { + const result = await formatErrorForUser(error, { transport: "http" }); + expect(result).toContain("Invalid issue ID format"); + expect(result).toContain("**Input Error**"); + expect(result).not.toContain("Feature Unavailable"); + expect(logIssue).not.toHaveBeenCalled(); + }); + + it("returns detailed message for stdio transport", async () => { + const result = await formatErrorForUser(error, { transport: "stdio" }); + expect(result).toContain("Invalid issue ID format"); + expect(result).toContain("**Input Error**"); + expect(logIssue).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/mcp-core/src/internal/error-handling.ts b/packages/mcp-core/src/internal/error-handling.ts index 003307574..87f188d97 100644 --- a/packages/mcp-core/src/internal/error-handling.ts +++ b/packages/mcp-core/src/internal/error-handling.ts @@ -6,6 +6,7 @@ import { import { ApiError, ApiClientError, ApiServerError } from "../api-client"; import { logIssue } from "../telem/logging"; import { APICallError } from "ai"; +import type { TransportType } from "../types"; /** * Type guard to identify user input validation errors. @@ -58,56 +59,104 @@ export function isAPICallError(error: unknown): error is APICallError { return APICallError.isInstance(error); } +const GENERIC_CONFIG_ERROR_MESSAGE = [ + "**Feature Unavailable**", + "This feature is temporarily unavailable due to a server configuration issue.", + "The service operator has been notified. Please try again later.", +].join("\n\n"); + +/** + * Check whether this is an HTTP transport where server-side config errors + * should be hidden from the user. When true, the error is logged to Sentry + * and a generic message is returned instead of the detailed error. + */ +function isHttpTransport(options?: { transport?: TransportType }): boolean { + return options?.transport === "http"; +} + +/** + * For HTTP transport, log a server-side config/provider error to Sentry and + * return a generic message (the user cannot fix server-side configuration). + * Returns undefined for stdio/unset transport so the caller can fall through + * to the detailed error message. + */ +function redactForHttpTransport( + error: Error, + options?: { transport?: TransportType }, +): string | undefined { + if (!isHttpTransport(options)) { + return undefined; + } + logIssue(error); + return GENERIC_CONFIG_ERROR_MESSAGE; +} + /** * Format an error for user display with markdown formatting. * This is used by tool handlers to format errors for MCP responses. * + * When transport is "http", config/provider errors are logged to Sentry + * and a generic message is returned (users can't fix server-side config). + * When transport is "stdio" or undefined, detailed messages are returned + * (users can fix their own config). + * * SECURITY: Only return trusted error messages to prevent prompt injection vulnerabilities. * We trust: Sentry API errors, our own UserInputError/ConfigurationError messages, and system templates. */ -export async function formatErrorForUser(error: unknown): Promise { +export async function formatErrorForUser( + error: unknown, + options?: { transport?: TransportType }, +): Promise { if (isUserInputError(error)) { return [ "**Input Error**", "It looks like there was a problem with the input you provided.", error.message, - `You may be able to resolve the issue by addressing the concern and trying again.`, + "You may be able to resolve the issue by addressing the concern and trying again.", ].join("\n\n"); } if (isConfigurationError(error)) { - return [ - "**Configuration Error**", - "There appears to be a configuration issue with your setup.", - error.message, - `Please check your environment configuration and try again.`, - ].join("\n\n"); + return ( + redactForHttpTransport(error, options) ?? + [ + "**Configuration Error**", + "There appears to be a configuration issue with your setup.", + error.message, + "Please check your environment configuration and try again.", + ].join("\n\n") + ); } if (isLLMProviderError(error)) { - return [ - "**AI Provider Error**", - "The AI provider service is not available for this request.", - error.message, - `This is a service availability issue that cannot be resolved by retrying.`, - ].join("\n\n"); + return ( + redactForHttpTransport(error, options) ?? + [ + "**AI Provider Error**", + "The AI provider service is not available for this request.", + error.message, + "This is a service availability issue that cannot be resolved by retrying.", + ].join("\n\n") + ); } - // Handle AI SDK APICallError that wasn't converted to LLMProviderError - // This is a defensive layer - ideally callEmbeddedAgent converts these + // Handle AI SDK APICallError that wasn't converted to LLMProviderError. + // This is a defensive layer - ideally callEmbeddedAgent converts these. if (isAPICallError(error)) { const statusCode = error.statusCode; // 4xx errors are user-facing (account issues, rate limits, invalid keys) - // These should NOT be logged to Sentry if (statusCode && statusCode >= 400 && statusCode < 500) { - return [ - "**AI Provider Error**", - "The AI provider service returned an error.", - error.message, - "This may be a configuration or account issue. Please check your AI provider settings.", - ].join("\n\n"); + return ( + redactForHttpTransport(error, options) ?? + [ + "**AI Provider Error**", + "The AI provider service returned an error.", + error.message, + "This may be a configuration or account issue. Please check your AI provider settings.", + ].join("\n\n") + ); } - // 5xx errors - log to Sentry + // 5xx errors - always log to Sentry regardless of transport const eventId = logIssue(error); const parts = [ "**AI Provider Error**", @@ -131,7 +180,7 @@ export async function formatErrorForUser(error: unknown): Promise { "**Input Error**", statusText, error.toUserMessage(), - `You may be able to resolve the issue by addressing the concern and trying again.`, + "You may be able to resolve the issue by addressing the concern and trying again.", ].join("\n\n"); } @@ -142,11 +191,11 @@ export async function formatErrorForUser(error: unknown): Promise { ? `There was an HTTP ${error.status} server error with the Sentry API.` : "There was a server error."; - const parts = ["**Error**", statusText, `${error.message}`]; + const parts = ["**Error**", statusText, error.message]; if (eventId) { parts.push(`**Event ID**: ${eventId}`); } - parts.push(`Please contact support if the problem persists.`); + parts.push("Please contact support if the problem persists."); return parts.join("\n\n"); } @@ -159,8 +208,8 @@ export async function formatErrorForUser(error: unknown): Promise { return [ "**Error**", statusText, - `${error.message}`, - `You may be able to resolve the issue by addressing the concern and trying again.`, + error.message, + "You may be able to resolve the issue by addressing the concern and trying again.", ].join("\n\n"); } diff --git a/packages/mcp-core/src/server.ts b/packages/mcp-core/src/server.ts index 9d2164284..841207ef5 100644 --- a/packages/mcp-core/src/server.ts +++ b/packages/mcp-core/src/server.ts @@ -373,7 +373,9 @@ function configureServer({ content: [ { type: "text" as const, - text: await formatErrorForUser(error), + text: await formatErrorForUser(error, { + transport: context.transport, + }), }, ], isError: true, diff --git a/packages/mcp-core/src/types.ts b/packages/mcp-core/src/types.ts index 34cfad2a0..a2cf6f1fd 100644 --- a/packages/mcp-core/src/types.ts +++ b/packages/mcp-core/src/types.ts @@ -38,6 +38,8 @@ export const CONSTRAINT_PARAMETER_KEYS = new Set([ "regionUrl", ]); +export type TransportType = "stdio" | "http"; + export type ServerContext = { sentryHost?: string; mcpUrl?: string; @@ -53,4 +55,6 @@ export type ServerContext = { agentMode?: boolean; /** Whether experimental tools are enabled */ experimentalMode?: boolean; + /** Transport type - affects error message formatting */ + transport?: TransportType; }; diff --git a/packages/mcp-server/src/index.ts b/packages/mcp-server/src/index.ts index 7a771d60f..049edd149 100644 --- a/packages/mcp-server/src/index.ts +++ b/packages/mcp-server/src/index.ts @@ -284,6 +284,7 @@ const context = { openaiBaseUrl: cfg.openaiBaseUrl, agentMode: cli.agent, experimentalMode: cli.experimental, + transport: "stdio" as const, }; // Build server with context to filter tools based on granted skills From ea66e0189e11e6163b73b41abc9e6b6efcddea8a Mon Sep 17 00:00:00 2001 From: David Cramer Date: Fri, 6 Feb 2026 09:53:06 -0800 Subject: [PATCH 2/2] refactor: Replace redactForHttpTransport with formatServerConfigError helper The `redactForHttpTransport() ??` pattern obscured control flow by returning undefined to trigger fallthrough. Replace with a single `formatServerConfigError()` helper that takes the detailed message parts and returns the correct response directly for both transports. Co-Authored-By: Claude Opus 4.6 --- .../mcp-core/src/internal/error-handling.ts | 53 +++++++++---------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/packages/mcp-core/src/internal/error-handling.ts b/packages/mcp-core/src/internal/error-handling.ts index 87f188d97..8e4b9b79e 100644 --- a/packages/mcp-core/src/internal/error-handling.ts +++ b/packages/mcp-core/src/internal/error-handling.ts @@ -66,29 +66,23 @@ const GENERIC_CONFIG_ERROR_MESSAGE = [ ].join("\n\n"); /** - * Check whether this is an HTTP transport where server-side config errors - * should be hidden from the user. When true, the error is logged to Sentry - * and a generic message is returned instead of the detailed error. - */ -function isHttpTransport(options?: { transport?: TransportType }): boolean { - return options?.transport === "http"; -} - -/** - * For HTTP transport, log a server-side config/provider error to Sentry and - * return a generic message (the user cannot fix server-side configuration). - * Returns undefined for stdio/unset transport so the caller can fall through - * to the detailed error message. + * Format a server-side configuration error for user display. + * + * For HTTP transport, the detailed message is hidden (the user cannot fix + * server-side configuration) — the error is logged to Sentry and a generic + * message is returned instead. + * For stdio/unset transport, the detailed parts are returned as-is. */ -function redactForHttpTransport( +function formatServerConfigError( error: Error, + detailedParts: string[], options?: { transport?: TransportType }, -): string | undefined { - if (!isHttpTransport(options)) { - return undefined; +): string { + if (options?.transport === "http") { + logIssue(error); + return GENERIC_CONFIG_ERROR_MESSAGE; } - logIssue(error); - return GENERIC_CONFIG_ERROR_MESSAGE; + return detailedParts.join("\n\n"); } /** @@ -117,26 +111,28 @@ export async function formatErrorForUser( } if (isConfigurationError(error)) { - return ( - redactForHttpTransport(error, options) ?? + return formatServerConfigError( + error, [ "**Configuration Error**", "There appears to be a configuration issue with your setup.", error.message, "Please check your environment configuration and try again.", - ].join("\n\n") + ], + options, ); } if (isLLMProviderError(error)) { - return ( - redactForHttpTransport(error, options) ?? + return formatServerConfigError( + error, [ "**AI Provider Error**", "The AI provider service is not available for this request.", error.message, "This is a service availability issue that cannot be resolved by retrying.", - ].join("\n\n") + ], + options, ); } @@ -146,14 +142,15 @@ export async function formatErrorForUser( const statusCode = error.statusCode; // 4xx errors are user-facing (account issues, rate limits, invalid keys) if (statusCode && statusCode >= 400 && statusCode < 500) { - return ( - redactForHttpTransport(error, options) ?? + return formatServerConfigError( + error, [ "**AI Provider Error**", "The AI provider service returned an error.", error.message, "This may be a configuration or account issue. Please check your AI provider settings.", - ].join("\n\n") + ], + options, ); } // 5xx errors - always log to Sentry regardless of transport