From cfdfede82a907829604fade265014508c369dfd2 Mon Sep 17 00:00:00 2001 From: Gjermund Garaba Date: Sat, 20 Jun 2026 14:17:00 +0200 Subject: [PATCH 1/2] mcp: surface auth-gated failures Entire-Checkpoint: be2e6e9d7167 --- e2e/selfhost/mcp-auth-required-add.test.ts | 88 ++++++ packages/plugins/mcp/src/sdk/connection.ts | 39 ++- packages/plugins/mcp/src/sdk/errors.ts | 30 +- packages/plugins/mcp/src/sdk/invoke.test.ts | 207 +++++++++++++ packages/plugins/mcp/src/sdk/invoke.ts | 47 ++- packages/plugins/mcp/src/sdk/plugin.test.ts | 309 ++++++++++++++++++-- packages/plugins/mcp/src/sdk/plugin.ts | 127 ++++++-- 7 files changed, 765 insertions(+), 82 deletions(-) create mode 100644 e2e/selfhost/mcp-auth-required-add.test.ts create mode 100644 packages/plugins/mcp/src/sdk/invoke.test.ts diff --git a/e2e/selfhost/mcp-auth-required-add.test.ts b/e2e/selfhost/mcp-auth-required-add.test.ts new file mode 100644 index 0000000000..78fb59cee9 --- /dev/null +++ b/e2e/selfhost/mcp-auth-required-add.test.ts @@ -0,0 +1,88 @@ +// Regression guard for the add-MCP dead-end on a server that gates on auth +// without a spec-compliant MCP challenge: the user should reach the auth-method +// editor, not a "requires authentication, add credentials below" error with no +// editor rendered below it. +// +// Selfhost-only because the probe must shape-probe a loopback server: the +// selfhost instance runs with EXECUTOR_ALLOW_LOCAL_NETWORK so its outbound +// probe can reach the loopback test server. Video is the artifact. +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { HttpServerResponse } from "effect/unstable/http"; +import { composePluginApi } from "@executor-js/api/server"; +import { deriveMcpNamespace } from "@executor-js/plugin-mcp"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { IntegrationSlug } from "@executor-js/sdk/shared"; +import { serveTestHttpApp } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +scenario( + "Auth methods · a non-spec-compliant 401 still gets the auth editor (no dead-end)", + {}, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + // Auth-gated shape: a 401 with no Bearer WWW-Authenticate, no RFC 9728 + // protected-resource metadata (the .well-known probe 404s), and a body + // that is neither JSON-RPC nor an OAuth error envelope. + const server = yield* serveTestHttpApp((request) => + Effect.succeed( + (request.url ?? "").includes("/.well-known/") + ? HttpServerResponse.text("missing", { status: 404 }) + : HttpServerResponse.jsonUnsafe({ message: "Unauthorized" }, { status: 401 }), + ), + ); + const endpoint = server.url("/mcp"); + // The raw 401 server reports no server name, so the probe can't seed a + // unique identity. Selfhost identities share one tenant, so name the + // integration uniquely to keep the derived slug from colliding across + // runs. + const name = `auth-gated-401-${randomBytes(3).toString("hex")}`; + const slug = IntegrationSlug.make(deriveMcpNamespace({ name })); + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + + yield* Effect.gen(function* () { + yield* browser.session(identity, async ({ page, step }) => { + await step("Open the add-MCP flow pointed at the auth-gated server", async () => { + await page.goto(`/integrations/add/mcp?url=${encodeURIComponent(endpoint)}`, { + waitUntil: "networkidle", + }); + // Before the fix this dead-ended on a red "add credentials below" + // error with no editor. Now the auth-method editor renders. + await page.getByText("How does this server authenticate?").waitFor(); + }); + + await step("The probe seeded a detected Bearer-header method", async () => { + await page.getByText("Method 1 · Detected").waitFor(); + // The preview card flags the gate rather than failing the probe. + await page.getByText("Auth required").first().waitFor(); + }); + + await step("Add the source with the declared method", async () => { + await page.getByPlaceholder("e.g. Linear").fill(name); + await page.getByRole("button", { name: "Add source" }).click(); + // onComplete routes to the new integration's detail hub. + await page.waitForURL(/\/integrations\/(?!add\b)[^/?]+$/, { timeout: 30_000 }); + const landedSlug = new URL(page.url()).pathname.split("/").filter(Boolean).at(-1); + expect(landedSlug, "the add flow lands on the created integration").toBe(String(slug)); + await page.getByText("Connections").first().waitFor(); + }); + + await step("The declared API key method is connectable", async () => { + await page.getByRole("button", { name: "Add connection" }).first().click(); + await page.getByRole("tab", { name: "API key (Authorization)" }).waitFor(); + }); + }); + }).pipe(Effect.ensuring(client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore))); + }), + ), +); diff --git a/packages/plugins/mcp/src/sdk/connection.ts b/packages/plugins/mcp/src/sdk/connection.ts index 208bc2f306..f629c594d9 100644 --- a/packages/plugins/mcp/src/sdk/connection.ts +++ b/packages/plugins/mcp/src/sdk/connection.ts @@ -3,7 +3,7 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { CfWorkerJsonSchemaValidator } from "@modelcontextprotocol/sdk/validation/cfworker"; -import { Effect } from "effect"; +import { Effect, Predicate } from "effect"; // NOTE: `StdioClientTransport` is NOT imported eagerly. The upstream module // (`@modelcontextprotocol/sdk/client/stdio.js`) touches `node:child_process` @@ -14,7 +14,7 @@ import { Effect } from "effect"; // stdio branch of `createMcpConnector`. import type { McpRemoteIntegrationConfig, McpStdioIntegrationConfig } from "./types"; -import { McpConnectionError } from "./errors"; +import { McpConnectionError, McpOAuthReauthorizationRequired } from "./errors"; // --------------------------------------------------------------------------- // Connection type @@ -25,7 +25,10 @@ export type McpConnection = { readonly close: () => Promise; }; -export type McpConnector = Effect.Effect; +export type McpConnector = Effect.Effect< + McpConnection, + McpConnectionError | McpOAuthReauthorizationRequired +>; // --------------------------------------------------------------------------- // Connector input — extends stored source data with resolved auth @@ -77,21 +80,29 @@ const connectionFromClient = (client: Client): McpConnection => ({ close: () => client.close(), }); +const connectionFailure = ( + transport: string, + message: string, + cause: unknown, +): McpConnectionError | McpOAuthReauthorizationRequired => { + if (Predicate.isTagged(cause, "McpOAuthReauthorizationRequired")) { + return new McpOAuthReauthorizationRequired({ message: "MCP OAuth re-authorization required" }); + } + return new McpConnectionError({ transport, message }); +}; + const connectClient = (input: { transport: string; createTransport: () => Parameters[0]; -}): Effect.Effect => +}): Effect.Effect => Effect.gen(function* () { const client = createClient(); const transportInstance = input.createTransport(); yield* Effect.tryPromise({ try: () => client.connect(transportInstance), - catch: () => - new McpConnectionError({ - transport: input.transport, - message: `Failed connecting via ${input.transport}`, - }), + catch: (cause) => + connectionFailure(input.transport, `Failed connecting via ${input.transport}`, cause), }).pipe( Effect.withSpan("plugin.mcp.connection.handshake", { attributes: { "plugin.mcp.transport": input.transport }, @@ -170,6 +181,12 @@ export const createMcpConnector = (input: ConnectorInput): McpConnector => { if (remoteTransport === "streamable-http") return connectStreamableHttp; if (remoteTransport === "sse") return connectSse; - // auto — try streamable-http first, fall back to SSE - return connectStreamableHttp.pipe(Effect.catch(() => connectSse)); + // auto: try streamable-http first, fall back to SSE for transport failures. + return connectStreamableHttp.pipe( + Effect.catch((error) => + Predicate.isTagged(error, "McpOAuthReauthorizationRequired") + ? Effect.fail(error) + : connectSse, + ), + ); }; diff --git a/packages/plugins/mcp/src/sdk/errors.ts b/packages/plugins/mcp/src/sdk/errors.ts index 4e64584c42..2c36dda243 100644 --- a/packages/plugins/mcp/src/sdk/errors.ts +++ b/packages/plugins/mcp/src/sdk/errors.ts @@ -1,9 +1,7 @@ -// MCP plugin tagged errors. Each carries an `HttpApiSchema` annotation so -// it can be `.addError(...)` directly on the API group — handlers return -// these and HttpApi encodes them as 4xx responses with a typed body. No -// per-handler sanitisation step. +// MCP plugin tagged errors. API-facing errors carry `HttpApiSchema` +// annotations so they can be `.addError(...)` directly on the API group. -import { Schema } from "effect"; +import { Data, Schema } from "effect"; export class McpConnectionError extends Schema.TaggedErrorClass()( "McpConnectionError", @@ -23,14 +21,20 @@ export class McpToolDiscoveryError extends Schema.TaggedErrorClass()( - "McpInvocationError", - { - toolName: Schema.String, - message: Schema.String, - }, - { httpApiStatus: 400 }, -) {} +// Internal only: core wraps non-auth failures as ToolInvocationError.cause, so +// this must carry only sanitized invocation metadata. Raw SDK causes can contain +// upstream bodies/challenges and should not leave the invoke catch block. +export class McpInvocationError extends Data.TaggedError("McpInvocationError")<{ + readonly toolName: string; + readonly message: string; + readonly status?: number; +}> {} + +export class McpOAuthReauthorizationRequired extends Data.TaggedError( + "McpOAuthReauthorizationRequired", +)<{ + readonly message: string; +}> {} export class McpOAuthError extends Schema.TaggedErrorClass()( "McpOAuthError", diff --git a/packages/plugins/mcp/src/sdk/invoke.test.ts b/packages/plugins/mcp/src/sdk/invoke.test.ts new file mode 100644 index 0000000000..3b5aaae66f --- /dev/null +++ b/packages/plugins/mcp/src/sdk/invoke.test.ts @@ -0,0 +1,207 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Predicate } from "effect"; +import { HttpServerResponse } from "effect/unstable/http"; + +import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js"; +import { StreamableHTTPError } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; +import { McpError } from "@modelcontextprotocol/sdk/types.js"; +import { ElicitationResponse } from "@executor-js/sdk"; +import { serveTestHttpApp } from "@executor-js/sdk/testing"; + +import { createMcpConnector, type McpConnection, type McpConnector } from "./connection"; +import { McpInvocationError, McpOAuthReauthorizationRequired } from "./errors"; +import { invokeMcpTool } from "./invoke"; + +const acceptAll = () => Effect.succeed(ElicitationResponse.make({ action: "accept" })); + +const rejectingConnector = (cause: unknown): McpConnector => + Effect.succeed({ + // oxlint-disable-next-line executor/no-double-cast -- boundary: minimal fake MCP client implements only the methods invokeMcpTool calls + client: { + setRequestHandler: () => undefined, + // oxlint-disable-next-line executor/no-promise-reject -- boundary: fake MCP client rejects to exercise invocation error wrapping + callTool: () => Promise.reject(cause), + } as unknown as McpConnection["client"], + close: () => Promise.resolve(), + }); + +const reauthorizationProvider: OAuthClientProvider = { + get redirectUrl() { + return "http://localhost/oauth/callback"; + }, + get clientMetadata() { + return { + redirect_uris: ["http://localhost/oauth/callback"], + grant_types: ["authorization_code", "refresh_token"] as string[], + response_types: ["code"] as string[], + token_endpoint_auth_method: "none" as const, + client_name: "Executor", + }; + }, + clientInformation: () => ({ client_id: "test-client" }), + saveClientInformation: () => undefined, + tokens: () => ({ access_token: "expired-token", token_type: "Bearer" }), + saveTokens: () => undefined, + redirectToAuthorization: async () => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: MCP SDK OAuthClientProvider callback can only signal reauthorization by throwing + throw new McpOAuthReauthorizationRequired({ message: "reauthorization required" }); + }, + saveCodeVerifier: () => undefined, + codeVerifier: () => "unused", + saveDiscoveryState: () => undefined, + discoveryState: () => undefined, +}; + +const serveReauthorizationChallengeServer = () => + serveTestHttpApp((request) => + Effect.sync(() => { + const origin = `http://${request.headers.host ?? "127.0.0.1"}`; + const requestUrl = new URL(request.url, origin); + + if (requestUrl.pathname.startsWith("/.well-known/oauth-protected-resource")) { + return HttpServerResponse.jsonUnsafe({ + resource: `${origin}/mcp`, + authorization_servers: [origin], + bearer_methods_supported: ["header"], + scopes_supported: ["read"], + }); + } + + if ( + requestUrl.pathname === "/.well-known/oauth-authorization-server" || + requestUrl.pathname === "/.well-known/openid-configuration" + ) { + return HttpServerResponse.jsonUnsafe({ + issuer: origin, + authorization_endpoint: `${origin}/authorize`, + token_endpoint: `${origin}/token`, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + scopes_supported: ["read"], + }); + } + + if (requestUrl.pathname === "/mcp" && request.method === "GET") { + return HttpServerResponse.text("SSE disabled", { status: 405 }); + } + + return HttpServerResponse.jsonUnsafe( + { error: "invalid_token" }, + { + status: 401, + headers: { + "www-authenticate": `Bearer resource_metadata="${origin}/.well-known/oauth-protected-resource/mcp", error="invalid_token"`, + }, + }, + ); + }), + ); + +// The status-extraction cases share one shape: dial a connector that rejects +// with `cause`, then assert the surfaced failure is a sanitized +// McpInvocationError carrying the expected HTTP status (or none) and never the +// upstream body. Each `cause` embeds a "do-not-leak" sentinel. +const invocationRejectionCases = [ + { + name: "wraps callTool rejection with a stable message and status", + toolId: "blocked", + transport: "streamable-http", + cause: new StreamableHTTPError(401, "token=do-not-leak"), + expectedStatus: 401 as number | undefined, + }, + { + name: "does not treat MCP protocol error codes as HTTP statuses", + toolId: "protocol_error", + transport: "streamable-http", + cause: new McpError(401, "application-level do-not-leak"), + expectedStatus: undefined, + }, + { + name: "does not invent a status from non-HTTP rejection shapes", + toolId: "network", + transport: "streamable-http", + cause: { code: -1, message: "socket said do-not-leak" }, + expectedStatus: undefined, + }, + { + name: "extracts the status from the SDK SSE POST error prefix without leaking the body", + toolId: "sse_blocked", + transport: "sse", + cause: { + message: "Error POSTing to endpoint (HTTP 403): do-not-leak: upstream auth challenge", + }, + expectedStatus: 403, + }, +]; + +describe("invokeMcpTool", () => { + for (const testCase of invocationRejectionCases) { + it.effect(testCase.name, () => + Effect.gen(function* () { + const error = yield* invokeMcpTool({ + toolId: testCase.toolId, + toolName: testCase.toolId, + args: {}, + transport: testCase.transport, + connector: rejectingConnector(testCase.cause), + elicit: acceptAll, + }).pipe(Effect.flip); + + expect(Predicate.isTagged(error, "McpInvocationError")).toBe(true); + const invocation = error as McpInvocationError; + expect(invocation.toolName).toBe(testCase.toolId); + expect(invocation).toMatchObject({ + message: `MCP tool call failed for ${testCase.toolId}`, + }); + expect(invocation).toMatchObject({ + message: expect.not.stringContaining("do-not-leak"), + }); + expect(invocation.status).toBe(testCase.expectedStatus); + expect("cause" in invocation).toBe(false); + }), + ); + } + + it.effect("marks OAuth reauthorization rejections without leaking SDK details", () => + Effect.gen(function* () { + const error = yield* invokeMcpTool({ + toolId: "oauth_scope", + toolName: "oauth_scope", + args: {}, + transport: "streamable-http", + connector: rejectingConnector( + new McpOAuthReauthorizationRequired({ message: "redirect to do-not-leak" }), + ), + elicit: acceptAll, + }).pipe(Effect.flip); + + expect(Predicate.isTagged(error, "McpOAuthReauthorizationRequired")).toBe(true); + expect(error).toMatchObject({ message: expect.not.stringContaining("do-not-leak") }); + expect("cause" in error).toBe(false); + }), + ); + + it.effect("preserves OAuth reauthorization required during auto connection setup", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveReauthorizationChallengeServer(); + const error = yield* invokeMcpTool({ + toolId: "oauth_scope", + toolName: "oauth_scope", + args: {}, + transport: "auto", + connector: createMcpConnector({ + transport: "remote", + endpoint: server.url("/mcp"), + authProvider: reauthorizationProvider, + }), + elicit: acceptAll, + }).pipe(Effect.flip); + + expect(Predicate.isTagged(error, "McpOAuthReauthorizationRequired")).toBe(true); + }), + ), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/invoke.ts b/packages/plugins/mcp/src/sdk/invoke.ts index 5e6e83d0cc..3e11adc58d 100644 --- a/packages/plugins/mcp/src/sdk/invoke.ts +++ b/packages/plugins/mcp/src/sdk/invoke.ts @@ -13,6 +13,7 @@ import { Cause, Effect, Exit, Option, Predicate, Schema } from "effect"; +import { StreamableHTTPError } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { ElicitRequestSchema } from "@modelcontextprotocol/sdk/types.js"; import { @@ -23,7 +24,7 @@ import { type ElicitationRequest, } from "@executor-js/sdk"; -import { McpConnectionError, McpInvocationError } from "./errors"; +import { McpConnectionError, McpInvocationError, McpOAuthReauthorizationRequired } from "./errors"; import type { McpConnection, McpConnector } from "./connection"; // --------------------------------------------------------------------------- @@ -36,6 +37,29 @@ const decodeArgsRecord = Schema.decodeUnknownOption(ArgsRecord); const argsRecord = (value: unknown): Record => Option.getOrElse(decodeArgsRecord(value), () => ({})); +const SsePostErrorCause = Schema.Struct({ message: Schema.String }); +const decodeSsePostErrorCause = Schema.decodeUnknownOption(SsePostErrorCause); + +const statusFromSsePostError = (cause: unknown): number | undefined => + Option.match(decodeSsePostErrorCause(cause), { + onNone: () => undefined, + onSome: ({ message }) => { + const match = /^Error POSTing to endpoint \(HTTP ([1-5][0-9]{2})\):/.exec(message); + if (!match) return undefined; + return Number(match[1]); + }, + }); + +const statusFromStreamableHttpError = (cause: unknown): number | undefined => { + // oxlint-disable-next-line executor/no-instanceof-tagged-error -- boundary: MCP SDK exposes transport HTTP failures as this Error subclass; protocol errors can carry the same numeric code + if (!(cause instanceof StreamableHTTPError)) return undefined; + const code = cause.code; + return code !== undefined && code >= 100 && code <= 599 ? code : undefined; +}; + +const httpStatusFromCause = (cause: unknown): number | undefined => + statusFromStreamableHttpError(cause) ?? statusFromSsePostError(cause); + // --------------------------------------------------------------------------- // Elicitation bridge — decode incoming MCP ElicitRequest, route through // the host's elicit function, marshal the response back to MCP shape. @@ -110,16 +134,24 @@ const useConnection = ( toolName: string, args: Record, elicit: Elicit, -): Effect.Effect => +): Effect.Effect => Effect.gen(function* () { installElicitationHandler(connection.client, elicit); return yield* Effect.tryPromise({ try: () => connection.client.callTool({ name: toolName, arguments: args }), - catch: () => - new McpInvocationError({ + catch: (cause) => { + if (Predicate.isTagged(cause, "McpOAuthReauthorizationRequired")) { + return new McpOAuthReauthorizationRequired({ + message: "MCP OAuth re-authorization required", + }); + } + const status = httpStatusFromCause(cause); + return new McpInvocationError({ toolName, message: `MCP tool call failed for ${toolName}`, - }), + ...(status === undefined ? {} : { status }), + }); + }, }).pipe( Effect.withSpan("plugin.mcp.client.call_tool", { attributes: { "mcp.tool.name": toolName }, @@ -144,7 +176,10 @@ export interface InvokeMcpToolInput { export const invokeMcpTool = ( input: InvokeMcpToolInput, -): Effect.Effect => +): Effect.Effect< + unknown, + McpConnectionError | McpInvocationError | McpOAuthReauthorizationRequired +> => Effect.gen(function* () { const args = argsRecord(input.args); diff --git a/packages/plugins/mcp/src/sdk/plugin.test.ts b/packages/plugins/mcp/src/sdk/plugin.test.ts index 1ee613ae17..9d0b0cb3de 100644 --- a/packages/plugins/mcp/src/sdk/plugin.test.ts +++ b/packages/plugins/mcp/src/sdk/plugin.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; -import { Effect } from "effect"; +import { Effect, Option, Predicate, Schema } from "effect"; import { HttpServerResponse } from "effect/unstable/http"; import { @@ -18,6 +18,7 @@ import { } from "@executor-js/sdk/testing"; import { mcpPlugin, userFacingProbeMessage } from "./plugin"; +import { McpInvocationError } from "./errors"; import { extractManifestFromListToolsResult, deriveMcpNamespace, joinToolPath } from "./manifest"; import { makeAnnotationsMcpServer, serveMcpServer } from "../testing"; @@ -31,6 +32,122 @@ import { makeAnnotationsMcpServer, serveMcpServer } from "../testing"; const TEMPLATE = AuthTemplateSlug.make("none"); +const JsonRpcId = Schema.Union([Schema.String, Schema.Number, Schema.Null]); +const JsonRpcRequest = Schema.Struct({ + id: Schema.optional(JsonRpcId), + method: Schema.String, +}); +type JsonRpcRequest = typeof JsonRpcRequest.Type; + +const decodeJsonRpcRequest = Schema.decodeUnknownOption(Schema.fromJsonString(JsonRpcRequest)); + +const jsonRpcResult = (request: JsonRpcRequest, result: unknown) => + HttpServerResponse.jsonUnsafe({ + jsonrpc: "2.0", + id: request.id ?? null, + result, + }); + +// The call-tool fixtures share one JSON-RPC scaffold (handshake, tool listing, +// unknown-method rejection); only the `tools/call` response varies. Each +// scenario supplies that branch via a `CallToolResponder`. +type CallToolResponder = (rpc: JsonRpcRequest) => ReturnType; + +const callToolFixtureResponse = (rpc: JsonRpcRequest, callTool: CallToolResponder) => { + if (rpc.method === "initialize") { + return jsonRpcResult(rpc, { + protocolVersion: "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "call-tool-fixture", version: "1.0.0" }, + }); + } + if (rpc.method === "notifications/initialized") { + return HttpServerResponse.text("", { status: 202 }); + } + if (rpc.method === "tools/list") { + return jsonRpcResult(rpc, { + tools: [ + { + name: "explode", + description: "Returns a failure from tools/call", + inputSchema: { type: "object", properties: {} }, + }, + ], + }); + } + if (rpc.method === "tools/call") { + return callTool(rpc); + } + return HttpServerResponse.text("Unexpected JSON-RPC method", { status: 400 }); +}; + +const serveCallToolServer = (callTool: CallToolResponder) => + serveTestHttpApp((request) => + Effect.gen(function* () { + if (request.method === "GET") { + return HttpServerResponse.text("SSE disabled", { status: 405 }); + } + + const body = yield* request.text.pipe(Effect.orDie); + return Option.match(decodeJsonRpcRequest(body), { + onNone: () => HttpServerResponse.text("Invalid JSON-RPC fixture request", { status: 400 }), + onSome: (rpc) => callToolFixtureResponse(rpc, callTool), + }); + }), + ); + +// `tools/call` responders. Both embed a "do-not-leak" sentinel the assertions +// confirm never reaches the caller-facing failure. +const httpStatusCallTool = + (status: number): CallToolResponder => + () => + HttpServerResponse.text("do-not-leak: upstream auth challenge", { status }); + +const jsonRpcErrorCallTool = + (code: number): CallToolResponder => + (rpc) => + HttpServerResponse.jsonUnsafe({ + jsonrpc: "2.0", + id: rpc.id ?? null, + error: { code, message: "application-level do-not-leak" }, + }); + +const seedCallToolExecutor = (input: { slug: string; callTool: CallToolResponder }) => + Effect.acquireRelease( + Effect.gen(function* () { + const server = yield* serveCallToolServer(input.callTool); + const config = makeTestConfig({ + plugins: [memoryCredentialsPlugin(), mcpPlugin()] as const, + }); + const executor = yield* createExecutor(config); + + yield* executor.mcp.addServer({ + name: "Call tool fixture", + endpoint: server.url("/mcp"), + slug: input.slug, + remoteTransport: "streamable-http", + }); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: IntegrationSlug.make(input.slug), + template: TEMPLATE, + value: "", + }); + + return { + config, + executor, + toolAddress: ToolAddress.make(`tools.${input.slug}.org.main.explode`), + } as const; + }), + ({ config, executor }) => + Effect.gen(function* () { + yield* executor.close().pipe(Effect.ignore); + yield* Effect.promise(() => config.testDb.close()).pipe(Effect.ignore); + }), + ); + // --------------------------------------------------------------------------- // Manifest extraction // --------------------------------------------------------------------------- @@ -410,6 +527,93 @@ describe("mcpPlugin", () => { }), ); + for (const status of [401, 403] as const) { + it.effect(`returns an auth tool failure when tools/call responds HTTP ${status}`, () => + Effect.scoped( + Effect.gen(function* () { + const slug = `call_status_${status}`; + const { executor, toolAddress } = yield* seedCallToolExecutor({ + slug, + callTool: httpStatusCallTool(status), + }); + + const result = yield* executor.execute(toolAddress, {}, { onElicitation: "accept-all" }); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "connection_rejected", + status, + retryable: false, + details: { + category: "authentication", + source: { id: slug }, + credential: { kind: "upstream", label: "main" }, + upstream: { status }, + }, + }, + }); + + const failure = result as { + readonly ok: false; + readonly error: { readonly message: string }; + }; + expect(failure.error).toMatchObject({ + message: expect.not.stringContaining("do-not-leak"), + }); + }), + ), + ); + } + + it.effect("does not classify non-auth tools/call HTTP failures as auth failures", () => + Effect.scoped( + Effect.gen(function* () { + const { executor, toolAddress } = yield* seedCallToolExecutor({ + slug: "call_status_500", + callTool: httpStatusCallTool(500), + }); + + const failure = yield* executor + .execute(toolAddress, {}, { onElicitation: "accept-all" }) + .pipe(Effect.flip); + expect(Predicate.isTagged(failure, "ToolInvocationError")).toBe(true); + + const error = failure as { readonly message: string; readonly cause?: unknown }; + expect(error).toMatchObject({ message: "MCP tool call failed for explode" }); + expect(error).toMatchObject({ message: expect.not.stringContaining("do-not-leak") }); + expect(Predicate.isTagged(error.cause, "McpInvocationError")).toBe(true); + const cause = error.cause as McpInvocationError; + expect(cause.status).toBe(500); + expect(cause).toMatchObject({ message: expect.not.stringContaining("do-not-leak") }); + expect("cause" in cause).toBe(false); + }), + ), + ); + + it.effect("does not classify JSON-RPC error codes as auth failures", () => + Effect.scoped( + Effect.gen(function* () { + const { executor, toolAddress } = yield* seedCallToolExecutor({ + slug: "call_jsonrpc_401", + callTool: jsonRpcErrorCallTool(401), + }); + + const failure = yield* executor + .execute(toolAddress, {}, { onElicitation: "accept-all" }) + .pipe(Effect.flip); + expect(Predicate.isTagged(failure, "ToolInvocationError")).toBe(true); + + const error = failure as { readonly message: string; readonly cause?: unknown }; + expect(error).toMatchObject({ message: "MCP tool call failed for explode" }); + expect(error).toMatchObject({ message: expect.not.stringContaining("do-not-leak") }); + expect(Predicate.isTagged(error.cause, "McpInvocationError")).toBe(true); + const cause = error.cause as McpInvocationError; + expect(cause.status).toBeUndefined(); + }), + ), + ); + it.effect("probeEndpoint returns manual auth when MCP requires auth without OAuth metadata", () => Effect.scoped( Effect.gen(function* () { @@ -445,6 +649,84 @@ describe("mcpPlugin", () => { }), ), ); + + it.effect( + "probeEndpoint treats a non-spec-compliant 401 as requires-auth instead of dead-ending", + () => + Effect.scoped( + Effect.gen(function* () { + // Auth-gated shape: a 401 with no Bearer WWW-Authenticate, no + // RFC 9728 protected-resource metadata, and a non-JSON-RPC body. + // probeMcpEndpointShape classifies this `not-mcp`/auth-required, but + // the user should still get the auth editor (not a dead-end error) + // so they can declare a method and connect an account afterward. + const server = yield* serveTestHttpApp((request) => + Effect.succeed( + (request.url ?? "").includes("/.well-known/") + ? HttpServerResponse.text("missing", { status: 404 }) + : HttpServerResponse.jsonUnsafe({ message: "Unauthorized" }, { status: 401 }), + ), + ); + const config = makeTestConfig({ plugins: [mcpPlugin()] as const }); + const executor = yield* createExecutor(config); + + const result = yield* executor.mcp.probeEndpoint(server.url("/mcp")); + + expect(result).toMatchObject({ + connected: false, + requiresAuthentication: true, + requiresOAuth: false, + toolCount: null, + }); + + yield* executor.close(); + yield* Effect.promise(() => config.testDb.close()); + }), + ), + ); + + it.effect("probeEndpoint keeps auth-gated non-MCP OAuth services on manual auth", () => + Effect.scoped( + Effect.gen(function* () { + const server = yield* serveTestHttpApp((request) => + Effect.sync(() => { + const origin = `http://${request.headers.host ?? "127.0.0.1"}`; + const requestUrl = new URL(request.url, origin); + + if ( + requestUrl.pathname === "/.well-known/oauth-authorization-server" || + requestUrl.pathname === "/.well-known/openid-configuration" + ) { + return HttpServerResponse.jsonUnsafe({ + issuer: origin, + authorization_endpoint: `${origin}/authorize`, + token_endpoint: `${origin}/token`, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code"], + }); + } + + return HttpServerResponse.jsonUnsafe({ message: "Unauthorized" }, { status: 401 }); + }), + ); + const config = makeTestConfig({ plugins: [mcpPlugin()] as const }); + const executor = yield* createExecutor(config); + + const result = yield* executor.mcp.probeEndpoint(server.url("/mcp")); + + expect(result).toMatchObject({ + connected: false, + requiresAuthentication: true, + requiresOAuth: false, + supportsDynamicRegistration: false, + toolCount: null, + }); + + yield* executor.close(); + yield* Effect.promise(() => config.testDb.close()); + }), + ), + ); }); // --------------------------------------------------------------------------- @@ -508,16 +790,6 @@ describe("MCP destructiveHint → requiresApproval", () => { }); describe("userFacingProbeMessage", () => { - it("turns auth-required into a credentials-asking message", () => { - const message = userFacingProbeMessage({ - kind: "not-mcp", - category: "auth-required", - reason: "401 without Bearer WWW-Authenticate — not an MCP auth challenge", - }); - expect(message).toMatch(/requires authentication/i); - expect(message).toMatch(/credentials/i); - }); - it("turns wrong-shape into a 'not an MCP server' message", () => { const message = userFacingProbeMessage({ kind: "not-mcp", @@ -536,18 +808,9 @@ describe("userFacingProbeMessage", () => { }); it("never surfaces the raw probe reason verbatim", () => { - const reasons = [ - "401 without Bearer WWW-Authenticate — not an MCP auth challenge", - "2xx POST body is not a JSON-RPC envelope", - "GET response is not an SSE stream", - "unexpected status 418 for initialize", - ] as const; - for (const reason of reasons) { - const auth = userFacingProbeMessage({ kind: "not-mcp", category: "auth-required", reason }); - const wrong = userFacingProbeMessage({ kind: "not-mcp", category: "wrong-shape", reason }); - expect(auth).not.toContain(reason); - expect(wrong).not.toContain(reason); - } + const reason = "2xx POST body is not a JSON-RPC envelope"; + const message = userFacingProbeMessage({ kind: "not-mcp", category: "wrong-shape", reason }); + expect(message).not.toContain(reason); }); }); diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index 4ac059ba56..dd0228da2b 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -1,4 +1,4 @@ -import { Effect, Layer, Match, Option, Result, Schema } from "effect"; +import { Effect, Layer, Option, Result, Schema } from "effect"; import type { HttpClient } from "effect/unstable/http"; import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js"; @@ -38,7 +38,11 @@ import { import { createMcpConnector, type ConnectorInput, type McpConnector } from "./connection"; import { discoverTools } from "./discover"; -import { McpConnectionError, McpToolDiscoveryError } from "./errors"; +import { + McpConnectionError, + McpOAuthReauthorizationRequired, + McpToolDiscoveryError, +} from "./errors"; import { invokeMcpTool } from "./invoke"; import { deriveMcpNamespace, type McpToolManifestEntry } from "./manifest"; import { mcpPresets } from "./presets"; @@ -249,6 +253,34 @@ const mcpToolFailure = (code: string, message: string, details?: unknown) => ...(details === undefined ? {} : { details }), }); +const mcpInvocationAuthFailure = (input: { + readonly status: 401 | 403; + readonly integration: string; + readonly connection: string; +}) => + authToolFailure({ + code: "connection_rejected", + message: + input.status === 403 + ? `MCP server rejected connection "${input.connection}" with HTTP 403. The credential may lack access or required scope; re-authenticate or update the connection before retrying this tool.` + : `MCP server rejected connection "${input.connection}" with HTTP 401. Re-authenticate or update the connection before retrying this tool.`, + source: { id: input.integration }, + credential: { kind: "upstream", label: input.connection }, + status: input.status, + upstream: { status: input.status }, + }); + +const mcpInvocationOAuthReauthFailure = (input: { + readonly integration: string; + readonly connection: string; +}) => + authToolFailure({ + code: "oauth_reauth_required", + message: `OAuth connection "${input.connection}" requires reauthorization before retrying this MCP tool.`, + source: { id: input.integration }, + credential: { kind: "oauth", label: input.connection }, + }); + // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- @@ -359,28 +391,16 @@ const urlMatchesToken = (url: URL, token: string): boolean => { return re.test(url.hostname) || re.test(url.pathname); }; -/** Translate a non-MCP probe outcome into a message a user can act on. +/** Translate a hard-stop probe outcome into a message a user can act on. + * Auth-required shapes are routed to the auth editor upstream, so they never + * reach here; the only outcomes are an unreachable endpoint or a non-MCP one. * Exported for tests. */ export const userFacingProbeMessage = ( - shape: Extract, -): string => { - if (shape.kind === "unreachable") { - return "Couldn't reach this URL. Check the address, your network, and that the server is running."; - } - return Match.value(shape.category).pipe( - Match.when( - "auth-required", - () => - "This server requires authentication. Add credentials (Authorization header, query parameter, or API key) below and retry.", - ), - Match.when( - "wrong-shape", - () => - "This URL doesn't appear to host an MCP server. Double-check the address, including the path.", - ), - Match.exhaustive, - ); -}; + shape: Extract, +): string => + shape.kind === "unreachable" + ? "Couldn't reach this URL. Check the address, your network, and that the server is running." + : "This URL doesn't appear to host an MCP server. Double-check the address, including the path."; // --------------------------------------------------------------------------- // MCP-SDK OAuth provider adapter — wraps a pre-resolved access token so the @@ -407,8 +427,10 @@ const makeOAuthProvider = (accessToken: string): OAuthClientProvider => ({ tokens: () => ({ access_token: accessToken, token_type: "Bearer" }), saveTokens: () => undefined, redirectToAuthorization: async () => { - // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: MCP SDK OAuthClientProvider callback can only signal reauthorization by throwing - throw new Error("MCP OAuth re-authorization required"); + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: MCP SDK OAuthClientProvider callback can only signal reauthorization by throwing + throw new McpOAuthReauthorizationRequired({ + message: "MCP OAuth re-authorization required", + }); }, saveCodeVerifier: () => undefined, codeVerifier: () => { @@ -644,13 +666,40 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { headers: probeHeaders, queryParams: probeQueryParams, }); - if (shape.kind !== "mcp") { + + // A `not-mcp`/auth-required shape only proves the endpoint returned + // 401, but the add-flow recovery is the same as a spec-compliant MCP + // auth challenge: declare auth and connect an account afterward. + // Only an unreachable endpoint or a confirmed wrong-shape is a hard + // stop. + if (shape.kind === "unreachable") { return yield* new McpConnectionError({ transport: "remote", message: userFacingProbeMessage(shape), }); } + if (shape.kind === "not-mcp") { + if (shape.category === "wrong-shape") { + return yield* new McpConnectionError({ + transport: "remote", + message: userFacingProbeMessage(shape), + }); + } + + return { + connected: false, + requiresAuthentication: true, + requiresOAuth: false, + supportsDynamicRegistration: false, + name, + slug, + toolCount: null, + serverName: null, + instructions: null, + } satisfies McpProbeResult; + } + const probeResult = yield* ctx.oauth.probe({ url: trimmed }).pipe( Effect.map((oauth) => ({ ok: true as const, oauth })), Effect.catch(() => Effect.succeed({ ok: false as const, oauth: null })), @@ -985,16 +1034,36 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { } return ToolResult.ok(raw); }).pipe( - Effect.catchTag("McpConnectionError", ({ message }) => + Effect.catchTag("McpOAuthReauthorizationRequired", () => Effect.succeed( + mcpInvocationOAuthReauthFailure({ + integration: String(credential.integration), + connection: String(credential.connection), + }), + ), + ), + Effect.catchTag("McpConnectionError", (error) => { + return Effect.succeed( authToolFailure({ code: "connection_rejected", - message, + message: error.message, source: { id: String(credential.integration) }, credential: { kind: "upstream", label: String(credential.connection) }, }), - ), - ), + ); + }), + Effect.catchTag("McpInvocationError", (error) => { + if (error.status === 401 || error.status === 403) { + return Effect.succeed( + mcpInvocationAuthFailure({ + status: error.status, + integration: String(credential.integration), + connection: String(credential.connection), + }), + ); + } + return Effect.fail(error); + }), Effect.withSpan("mcp.plugin.invoke_tool", { attributes: { "mcp.tool.name": String(toolRow.name), From ef5d388975222455add3d23273277e39c45cb916 Mon Sep 17 00:00:00 2001 From: Gjermund Garaba Date: Sun, 21 Jun 2026 18:01:18 +0200 Subject: [PATCH 2/2] pr fixes Entire-Checkpoint: cbacb8ff478d --- packages/plugins/mcp/src/sdk/index.ts | 8 ++------ packages/plugins/mcp/src/sdk/invoke.ts | 2 ++ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/plugins/mcp/src/sdk/index.ts b/packages/plugins/mcp/src/sdk/index.ts index cf0991cd41..9003a06f97 100644 --- a/packages/plugins/mcp/src/sdk/index.ts +++ b/packages/plugins/mcp/src/sdk/index.ts @@ -30,11 +30,7 @@ export { migrateMcpAuthConfig } from "./migrate-config"; // Request-shaped authoring: `headers: { Authorization: ["Bearer ", variable("token")] }`. export { variable, type ApiKeyAuthTemplate } from "@executor-js/sdk/http-auth"; -export { - McpConnectionError, - McpToolDiscoveryError, - McpInvocationError, - McpOAuthError, -} from "./errors"; +// Only the API-facing errors; the internal Data.TaggedError ones stay private. +export { McpConnectionError, McpToolDiscoveryError, McpOAuthError } from "./errors"; export { deriveMcpNamespace, joinToolPath, extractManifestFromListToolsResult } from "./manifest"; diff --git a/packages/plugins/mcp/src/sdk/invoke.ts b/packages/plugins/mcp/src/sdk/invoke.ts index 3e11adc58d..7f0f6c5974 100644 --- a/packages/plugins/mcp/src/sdk/invoke.ts +++ b/packages/plugins/mcp/src/sdk/invoke.ts @@ -40,6 +40,8 @@ const argsRecord = (value: unknown): Record => const SsePostErrorCause = Schema.Struct({ message: Schema.String }); const decodeSsePostErrorCause = Schema.decodeUnknownOption(SsePostErrorCause); +// Matches the SDK's SSEClientTransport POST-failure message (sse.js); re-verify +// on SDK bumps. A format drift just yields undefined (generic error, no crash). const statusFromSsePostError = (cause: unknown): number | undefined => Option.match(decodeSsePostErrorCause(cause), { onNone: () => undefined,