From 1f566b8852a47988cd31229cc17cc4265cde9931 Mon Sep 17 00:00:00 2001 From: Ramiro Rivera Date: Fri, 28 Aug 2026 10:50:35 +0200 Subject: [PATCH 1/5] fix(mcp): surface OAuth reauthorization during discovery --- .../mcp-oauth-refresh-reauthorization.md | 12 +++ packages/core/sdk/src/executor.test.ts | 55 ++++++++++++- packages/core/sdk/src/executor.ts | 6 +- packages/core/sdk/src/plugin.ts | 4 + packages/plugins/mcp/src/sdk/connection.ts | 33 +++++++- packages/plugins/mcp/src/sdk/discover.ts | 5 ++ packages/plugins/mcp/src/sdk/errors.ts | 4 + packages/plugins/mcp/src/sdk/plugin.test.ts | 77 +++++++++++++++++++ packages/plugins/mcp/src/sdk/plugin.ts | 18 ++++- 9 files changed, 207 insertions(+), 7 deletions(-) create mode 100644 .changeset/mcp-oauth-refresh-reauthorization.md diff --git a/.changeset/mcp-oauth-refresh-reauthorization.md b/.changeset/mcp-oauth-refresh-reauthorization.md new file mode 100644 index 0000000000..57364ed8f2 --- /dev/null +++ b/.changeset/mcp-oauth-refresh-reauthorization.md @@ -0,0 +1,12 @@ +--- +"@executor-js/sdk": patch +"@executor-js/plugin-mcp": patch +--- + +**Rejected MCP OAuth grants now request reconnect without registering a disposable client** + +Remote MCP catalog discovery used the MCP SDK's interactive OAuth fallback when an upstream rejected Executor's stored bearer with `401`. A background refresh cannot finish that browser authorization, but the SDK first fetched OAuth metadata and dynamically registered another client. Executor then preserved the old catalog under a generic degraded health verdict, so clients saw zero or stale tools without a reliable reconnect signal. + +Executor now stops at the authenticated HTTP boundary for OAuth-backed MCP transports. A rejected stored bearer becomes a structured reauthorization result before OAuth discovery or Dynamic Client Registration runs. Catalog refresh still preserves the last authoritative tools, but records the connection as expired with a reconnect-required detail so the UI and API can direct the user through authorization again. + +API-key and unauthenticated MCP transports keep their existing `401` behavior, and ordinary incomplete discovery results remain degraded. diff --git a/packages/core/sdk/src/executor.test.ts b/packages/core/sdk/src/executor.test.ts index 19319061a5..377973a7fe 100644 --- a/packages/core/sdk/src/executor.test.ts +++ b/packages/core/sdk/src/executor.test.ts @@ -125,11 +125,20 @@ const demoPlugin = definePlugin(() => ({ const diagnosticsPlugin = definePlugin(() => ({ id: "diagnostics" as const, storage: () => ({}), - resolveTools: () => + resolveTools: ({ connection }) => Effect.succeed({ tools: [], incomplete: true, incompleteReason: "Schema introspection was rejected", + ...(String(connection.integration) === "diagnostics_expired" + ? { + health: { + status: "expired" as const, + checkedAt: Date.now(), + detail: "Reconnect the upstream OAuth grant", + }, + } + : {}), }), extension: (ctx) => ({ seed: () => @@ -138,6 +147,12 @@ const diagnosticsPlugin = definePlugin(() => ({ description: "Diagnostics", config: {}, }), + seedExpired: () => + ctx.core.integrations.register({ + slug: IntegrationSlug.make("diagnostics_expired"), + description: "Expired diagnostics", + config: {}, + }), }), }))(); @@ -470,6 +485,44 @@ describe("createExecutor", () => { }), ); + it.effect("preserves actionable health from an incomplete tool catalog", () => + Effect.gen(function* () { + const executor = yield* makeTestExecutor({ + plugins: [memoryCredentialsPlugin(), diagnosticsPlugin] as const, + coreTools: {}, + }); + yield* executor.diagnostics.seedExpired(); + + yield* executor.execute( + ToolAddress.make("executor.coreTools.connections.create"), + { + owner: "org", + name: "main", + integration: "diagnostics_expired", + template: "none", + }, + { onElicitation: "accept-all" }, + ); + + const refreshed = yield* executor.execute( + ToolAddress.make("executor.coreTools.connections.refresh"), + { + owner: "org", + name: "main", + integration: "diagnostics_expired", + }, + { onElicitation: "accept-all" }, + ); + expect(refreshed).toMatchObject({ + tools: [], + lastHealth: { + status: "expired", + detail: "Reconnect the upstream OAuth grant", + }, + }); + }), + ); + it.effect("hands pasted credential entry to the web UI", () => Effect.gen(function* () { const executor = yield* makeTestExecutor({ diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index b9a350cdc2..9aa241d3bd 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3020,12 +3020,12 @@ export const createExecutor = + const stampSyncedWithHealth = (reason: string, health?: HealthCheckResult) => core.updateMany("connection", { where: connectionWhere, set: { tools_synced_at: Date.now(), - last_health: toolSyncHealth(reason), + last_health: health ?? toolSyncHealth(reason), updated_at: new Date(), }, }); @@ -3092,7 +3092,7 @@ export const createExecutor = { diff --git a/packages/plugins/mcp/src/sdk/connection.ts b/packages/plugins/mcp/src/sdk/connection.ts index 2ee235de94..7f206974ff 100644 --- a/packages/plugins/mcp/src/sdk/connection.ts +++ b/packages/plugins/mcp/src/sdk/connection.ts @@ -49,6 +49,10 @@ export type RemoteConnectorInput = Omit< readonly headers?: Record; readonly queryParams?: Record; readonly authProvider?: OAuthClientProvider; + /** This provider only replays a resolved bearer. A 401 cannot be recovered + * inside the MCP SDK and must return to core as reconnect-required before + * the SDK attempts discovery or Dynamic Client Registration. */ + readonly staticOAuthBearer?: boolean; readonly httpClientLayer?: Layer.Layer; }; @@ -153,6 +157,18 @@ const nestedMcpHttpTransportError = (cause: unknown): Option.Option { + let current: unknown = cause; + for (let depth = 0; depth < 8; depth += 1) { + if (Predicate.isTagged(current, "McpOAuthReauthorizationRequired")) return true; + const decodedCause = decodeExternalTransportCause(current); + if (Option.isNone(decodedCause)) return false; + current = decodedCause.value.cause ?? decodedCause.value.data?.cause; + if (current === undefined) return false; + } + return false; +}; + const externalTransportCodes = (cause: unknown): ReadonlySet => { const codes = new Set(); let current: unknown = cause; @@ -230,6 +246,7 @@ const awaitAbort = (signal: AbortSignal): Effect.Effect => const fetchFromHttpClientLayer = ( httpClientLayer: Layer.Layer, + staticOAuthBearer: boolean, ): FetchLike => { const execute: FetchLike = async (url, init) => { const headers = headersFrom(init?.headers); @@ -262,6 +279,10 @@ const fetchFromHttpClientLayer = ( headers: responseHeaders, }); }).pipe(Effect.mapError(normalizeHttpClientFailure), Effect.provide(httpClientLayer)); + // Executor resolves and refreshes OAuth credentials before constructing + // this transport. If that stored bearer is rejected, the MCP SDK cannot + // complete its interactive fallback in a catalog refresh and would perform + // avoidable DCR first. Stop at the authenticated HTTP boundary instead. // A 403 carrying an RFC 6750 insufficient_scope challenge is intercepted // HERE, below the SDK: with an authProvider the SDK would consume the // challenge and re-run auth ("upscoping"), which our static-token @@ -271,6 +292,12 @@ const fetchFromHttpClientLayer = ( // consumes promise rejections) so it reaches the invoke/connect catch // sites verbatim. const promise = Effect.runPromise(effect).then((response) => { + if (staticOAuthBearer && response.status === 401) { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: Fetch-compatible adapter can only signal through a rejected promise + throw new McpOAuthReauthorizationRequired({ + message: "MCP OAuth re-authorization required", + }); + } if (response.status === 403) { const challenge = response.headers.get("www-authenticate"); if ( @@ -335,7 +362,7 @@ const connectionFailure = ( message: string, cause: unknown, ): McpConnectionError | McpOAuthReauthorizationRequired => { - if (Predicate.isTagged(cause, "McpOAuthReauthorizationRequired")) { + if (hasNestedOAuthReauthorization(cause)) { return new McpOAuthReauthorizationRequired({ message: "MCP OAuth re-authorization required" }); } if (Predicate.isTagged(cause, "McpInsufficientScopeError")) { @@ -487,7 +514,9 @@ export const createMcpConnector = (input: ConnectorInput): McpConnector => { const headers = input.headers ?? {}; const remoteTransport = input.remoteTransport ?? "auto"; const requestInit = Object.keys(headers).length > 0 ? { headers } : undefined; - const fetch = input.httpClientLayer ? fetchFromHttpClientLayer(input.httpClientLayer) : undefined; + const fetch = input.httpClientLayer + ? fetchFromHttpClientLayer(input.httpClientLayer, input.staticOAuthBearer === true) + : undefined; const endpoint = buildEndpointUrl(input.endpoint, input.queryParams ?? {}); diff --git a/packages/plugins/mcp/src/sdk/discover.ts b/packages/plugins/mcp/src/sdk/discover.ts index 5e965b9f44..31ba3863bc 100644 --- a/packages/plugins/mcp/src/sdk/discover.ts +++ b/packages/plugins/mcp/src/sdk/discover.ts @@ -114,10 +114,15 @@ export const discoverTools = ( const httpStatus = Predicate.isTagged(failure, "McpConnectionError") ? failure.httpStatus : undefined; + const reauthorizationRequired = Predicate.isTagged( + failure, + "McpOAuthReauthorizationRequired", + ); return new McpToolDiscoveryError({ stage: "connect", message: `Failed connecting to MCP server: ${failure.message}`, ...(httpStatus !== undefined ? { httpStatus } : {}), + ...(reauthorizationRequired ? { reauthorizationRequired: true } : {}), }); }), ), diff --git a/packages/plugins/mcp/src/sdk/errors.ts b/packages/plugins/mcp/src/sdk/errors.ts index a56a2c63bb..6d5a2501ea 100644 --- a/packages/plugins/mcp/src/sdk/errors.ts +++ b/packages/plugins/mcp/src/sdk/errors.ts @@ -42,6 +42,10 @@ export class McpToolDiscoveryError extends Schema.TaggedErrorClass }), ); +const rejectedOAuthDiscoveryLayer = (endpoint: string) => { + const issuer = new URL(endpoint).origin; + const requests: string[] = []; + const layer = Layer.succeed(HttpClient.HttpClient)( + HttpClient.make((request: HttpClientRequest.HttpClientRequest) => { + requests.push(request.url); + const url = new URL(request.url); + const response = + request.url === endpoint + ? new Response("", { status: 401 }) + : url.pathname === "/.well-known/oauth-protected-resource/mcp" + ? Response.json({ resource: endpoint, authorization_servers: [issuer] }) + : url.pathname === "/.well-known/oauth-authorization-server" + ? Response.json({ + issuer, + authorization_endpoint: `${issuer}/authorize`, + token_endpoint: `${issuer}/token`, + registration_endpoint: `${issuer}/register`, + response_types_supported: ["code"], + code_challenge_methods_supported: ["S256"], + }) + : url.pathname === "/register" + ? Response.json( + { + client_id: "replacement-client", + redirect_uris: ["http://localhost/oauth/callback"], + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + token_endpoint_auth_method: "none", + }, + { status: 201 }, + ) + : new Response("unexpected request", { status: 500 }); + return Effect.succeed(HttpClientResponse.fromWeb(request, response)); + }), + ); + return { layer, requests }; +}; + // `tools/call` responders. Both embed a "do-not-leak" sentinel the assertions // confirm never reaches the caller-facing failure. const httpStatusCallTool = @@ -296,6 +335,44 @@ describe("joinToolPath", () => { // --------------------------------------------------------------------------- describe("mcpPlugin", () => { + it.effect("surfaces OAuth reauthorization from resolveTools as expired health", () => + Effect.gen(function* () { + const endpoint = "https://mcp.example.test/mcp"; + const plugin = mcpPlugin(); + const ledger = rejectedOAuthDiscoveryLayer(endpoint); + const result = yield* plugin.resolveTools!({ + config: { + transport: "remote", + endpoint, + remoteTransport: "streamable-http", + authenticationTemplate: [{ slug: "oauth2", kind: "oauth2" }], + }, + connection: { + owner: "org", + integration: IntegrationSlug.make("oauth_mcp"), + name: ConnectionName.make("main"), + }, + template: AuthTemplateSlug.make("oauth2"), + getValues: () => Effect.succeed({ token: "rejected-token" }), + getValue: () => Effect.succeed("rejected-token"), + httpClientLayer: ledger.layer, + ctx: null as never, + integration: null as never, + storage: {}, + }); + + expect(result).toMatchObject({ + tools: [], + incomplete: true, + health: { + status: "expired", + detail: expect.stringContaining("reauthorization"), + }, + }); + expect(ledger.requests.filter((url) => new URL(url).pathname === "/register")).toEqual([]); + }), + ); + it.effect("creates executor with mcp plugin", () => Effect.gen(function* () { const executor = yield* createExecutor( diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index f4c55383ec..bab0ad2e3d 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -628,6 +628,7 @@ const buildConnectorInput = ( queryParams: Object.keys(queryParams).length > 0 ? queryParams : undefined, headers: Object.keys(headers).length > 0 ? headers : undefined, authProvider, + ...(authProvider === undefined ? {} : { staticOAuthBearer: true }), httpClientLayer, }); }; @@ -1298,10 +1299,20 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { }), ); if (Result.isFailure(discovered)) { + const reauthorizationRequired = discovered.failure.reauthorizationRequired === true; return { tools: [] as readonly ToolDef[], incomplete: true, incompleteReason: discovered.failure.message, + ...(reauthorizationRequired + ? { + health: { + status: "expired" as const, + checkedAt: Date.now(), + detail: "MCP OAuth reauthorization required", + }, + } + : {}), }; } return { tools: discovered.success.tools.map(toToolDef) }; @@ -1310,7 +1321,12 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { attributes: { "mcp.connection.name": String(connection.name) }, }), ) as Effect.Effect< - { readonly tools: readonly ToolDef[]; readonly incomplete?: boolean }, + { + readonly tools: readonly ToolDef[]; + readonly incomplete?: boolean; + readonly incompleteReason?: string; + readonly health?: HealthCheckResult; + }, StorageFailure >, From d5961817825d69ca5ff6b64c33611c6351209aef Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:39:42 -0700 Subject: [PATCH 2/5] e2e: cover MCP OAuth reauthorization during tool refresh --- .../mcp-oauth-tool-refresh-reauth.test.ts | 290 ++++++++++++++++++ 1 file changed, 290 insertions(+) create mode 100644 e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts diff --git a/e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts b/e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts new file mode 100644 index 0000000000..0ac03b4fd8 --- /dev/null +++ b/e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts @@ -0,0 +1,290 @@ +// Selfhost repros for #1816: a tool catalog refresh that meets an OAuth +// reauthorization condition must surface it as an actionable expired verdict, +// preserve the previously synced catalog, and must never dynamically register +// a fresh OAuth client — the saved connection already references one. +// +// Two variants: +// 1. The upstream MCP endpoint rejects a bearer executor still considers +// unexpired (the live report): the refresh dials with the stored token, +// gets 401, and must come back as reconnect-required without the MCP SDK's +// interactive OAuth fallback registering a disposable client. +// 2. The token is expired locally and the refresh-token grant is rejected +// with `invalid_grant` during the sync's credential resolution: the +// recorded dead grant must present as expired on the connection read, not +// be buried under a generic tool-sync verdict. +import { randomBytes } from "node:crypto"; + +import { Effect } from "effect"; +import { expect } from "@effect/vitest"; +import type { HttpApiClient } from "effect/unstable/httpapi"; +import { composePluginApi } from "@executor-js/api/server"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { makeGreetingMcpServer, serveMcpServer } from "@executor-js/plugin-mcp/testing"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, +} from "@executor-js/sdk/shared"; +import { serveOAuthTestServer, type OAuthTestServerShape } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Target } from "../src/services"; + +const api = composePluginApi([mcpHttpPlugin()] as const); +type Client = HttpApiClient.ForApi; + +const name = ConnectionName.make("main"); +const template = AuthTemplateSlug.make("oauth2"); + +const freshSlug = (prefix: string): string => `${prefix}-${randomBytes(4).toString("hex")}`; + +/** A real MCP server (serves `tools/list` for a one-tool catalog) that only + * accepts bearers the OAuth test server issued and still honours. */ +const serveTokenGatedMcpServer = (oauth: OAuthTestServerShape) => + serveMcpServer(() => makeGreetingMcpServer(), { + auth: { + validateAuthorization: oauth.acceptsAuthorizationHeader, + authorizationServerUrls: [oauth.issuerUrl], + scopes: ["channels:history", "users:read"], + }, + }); + +const requiredRedirect = (response: Response, from: string): string => { + const location = response.headers.get("location"); + if (!location) { + throw new Error(`Expected redirect from ${from}, got HTTP ${response.status}`); + } + return new URL(location, from).toString(); +}; + +/** The test server's login page is plain text with Basic-auth POST — nothing a + * browser can click. Complete it out of band and hand back the callback URL. */ +const submitProviderLogin = async (loginUrl: string): Promise => { + const credentials = Buffer.from("alice:password").toString("base64"); + const response = await fetch(loginUrl, { + method: "POST", + redirect: "manual", + headers: { authorization: `Basic ${credentials}` }, + }); + const location = response.headers.get("location"); + if (response.status !== 302 || !location) { + throw new Error(`provider login did not redirect (${response.status})`); + } + return new URL(location, loginUrl).toString(); +}; + +const completeAuthorization = (authorizationUrl: string) => + Effect.promise(async () => { + const login = await fetch(authorizationUrl, { redirect: "manual" }); + const loginUrl = requiredRedirect(login, authorizationUrl); + const callbackUrl = await submitProviderLogin(loginUrl); + const parsed = new URL(callbackUrl); + const code = parsed.searchParams.get("code"); + if (!code) throw new Error(`OAuth callback did not include a code: ${callbackUrl}`); + return { code }; + }); + +const seedDcrMcpOAuthConnection = ( + client: Client, + prefix: string, + oauth: OAuthTestServerShape, + endpoint: string, +) => + Effect.gen(function* () { + const slug = IntegrationSlug.make(freshSlug(prefix)); + const clientSlug = OAuthClientSlug.make(freshSlug(`${prefix}-client`)); + + yield* client.mcp.addServer({ + payload: { + transport: "remote", + name: `OAuth refresh repro ${String(slug)}`, + endpoint, + slug: String(slug), + authenticationTemplate: [{ kind: "oauth2" }], + }, + }); + yield* Effect.addFinalizer(() => + client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore), + ); + + const probe = yield* client.oauth.probe({ payload: { url: endpoint } }); + if (!probe.registrationEndpoint) { + return yield* Effect.die("OAuth probe did not discover a DCR registration endpoint"); + } + + const registered = yield* client.oauth.registerDynamic({ + payload: { + owner: "org", + slug: clientSlug, + issuer: probe.issuer ?? null, + registrationEndpoint: probe.registrationEndpoint, + authorizationUrl: probe.authorizationUrl, + tokenUrl: probe.tokenUrl, + resource: probe.resource ?? endpoint, + scopes: probe.scopesSupported ?? [], + tokenEndpointAuthMethodsSupported: probe.tokenEndpointAuthMethodsSupported, + clientName: "Executor e2e MCP OAuth refresh repro", + originIntegration: slug, + }, + }); + yield* Effect.addFinalizer(() => + client.oauth + .removeClient({ params: { slug: registered.client }, payload: { owner: "org" } }) + .pipe(Effect.ignore), + ); + + const started = yield* client.oauth.start({ + payload: { + owner: "org", + client: registered.client, + clientOwner: "org", + name, + integration: slug, + template, + }, + }); + expect(started.status, "DCR MCP OAuth starts an authorization-code redirect").toBe("redirect"); + if (started.status !== "redirect") return yield* Effect.die("OAuth start did not redirect"); + + const callback = yield* completeAuthorization(started.authorizationUrl); + yield* client.oauth.complete({ payload: { state: started.state, code: callback.code } }); + yield* Effect.addFinalizer(() => + client.connections + .remove({ params: { owner: "org", integration: slug, name } }) + .pipe(Effect.ignore), + ); + yield* oauth.clearRequests; + + return { slug }; + }); + +const registrationRequests = (oauth: OAuthTestServerShape) => + Effect.map(oauth.requests, (requests) => + requests + .filter((request) => request.path === "/register") + .map((request) => `${request.method} ${request.path}`), + ); + +scenario( + "MCP OAuth · tool refresh on an upstream-rejected bearer surfaces reconnect without re-registering the DCR client", + { + timeout: 180_000, + }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeApiClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + + // Long-lived tokens: executor's stored expiry stays in the future, so + // the refresh dials the MCP endpoint with the stored bearer. + const oauth = yield* serveOAuthTestServer({ + scopes: ["channels:history", "users:read"], + }); + const mcp = yield* serveTokenGatedMcpServer(oauth); + const { slug } = yield* seedDcrMcpOAuthConnection( + client, + "mcp-refresh-401", + oauth, + mcp.endpoint, + ); + + // Baseline: with the bearer honoured, the refresh syncs the real catalog. + const synced = yield* client.connections.refresh({ + params: { owner: "org", integration: slug, name }, + }); + expect( + synced.map((tool) => String(tool.name)), + "the healthy connection syncs the server's catalog", + ).toEqual(["simple_echo"]); + + // The provider revokes the grant server-side; executor has no idea and + // still considers the stored token unexpired. + const issued = yield* oauth.issuedAccessTokens; + expect(issued.length, "the completed OAuth flow minted a bearer").toBeGreaterThan(0); + yield* Effect.forEach(issued, (token) => oauth.revokeAccessToken(token)); + yield* oauth.clearRequests; + + const refreshed = yield* client.connections.refresh({ + params: { owner: "org", integration: slug, name }, + }); + + const registers = yield* registrationRequests(oauth); + expect( + registers, + "a noninteractive tool refresh must not dynamically register a fresh OAuth client", + ).toEqual([]); + + expect( + refreshed.map((tool) => String(tool.name)), + "the previously synced catalog is preserved through the failed refresh", + ).toEqual(["simple_echo"]); + + const reread = yield* client.connections.get({ + params: { owner: "org", integration: slug, name }, + }); + console.info(`[BUG repro] post-refresh health: ${JSON.stringify(reread.lastHealth ?? null)}`); + expect( + reread.lastHealth?.status, + "an upstream-rejected bearer is a reauthorization condition, not an anonymous degraded sync", + ).toBe("expired"); + }), + ), +); + +scenario( + "MCP OAuth · invalid_grant during tool refresh presents expired, not a buried sync failure", + { + timeout: 180_000, + }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeApiClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + + // Every minted token is already expired and the refresh grant is dead: + // the sync's own credential resolution meets `invalid_grant`. + const oauth = yield* serveOAuthTestServer({ + scopes: ["channels:history", "users:read"], + supportRefresh: false, + tokenExpiresInSeconds: 0, + invalidRefreshTokenDescription: "Grant not found", + }); + const mcp = yield* serveTokenGatedMcpServer(oauth); + const { slug } = yield* seedDcrMcpOAuthConnection( + client, + "mcp-refresh-dead", + oauth, + mcp.endpoint, + ); + yield* oauth.clearRequests; + + yield* client.connections.refresh({ + params: { owner: "org", integration: slug, name }, + }); + + const registers = yield* registrationRequests(oauth); + expect( + registers, + "a dead-grant tool refresh must not dynamically register a fresh OAuth client", + ).toEqual([]); + + const reread = yield* client.connections.get({ + params: { owner: "org", integration: slug, name }, + }); + console.info(`[BUG repro] post-refresh health: ${JSON.stringify(reread.lastHealth ?? null)}`); + expect( + reread.lastHealth?.status, + "the recorded dead grant presents as expired on the connection read", + ).toBe("expired"); + expect( + reread.lastHealth?.detail, + "the provider rejection detail survives to the user", + ).toContain("Grant not found"); + }), + ), +); From caf38b1d2eef3a4903f90d029d2db865e232f2f8 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:13:06 -0700 Subject: [PATCH 3/5] Surface reauthorization from tools/list failures and guard the sync verdict write --- packages/core/sdk/src/executor.ts | 44 ++++++++++++++++------ packages/plugins/mcp/src/sdk/connection.ts | 40 +++++++++++++++----- packages/plugins/mcp/src/sdk/discover.ts | 34 ++++++++++++++--- packages/plugins/mcp/src/sdk/errors.ts | 3 +- 4 files changed, 93 insertions(+), 28 deletions(-) diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 32c6ae07f9..75536b0417 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3232,21 +3232,41 @@ export const createExecutor = findConnectionRow(ref).pipe( Effect.flatMap((fresh) => - core.updateMany("connection", { - where: connectionWhere, - set: - fresh !== null && - oauthReauthRequiredFromProviderState(fresh.provider_state) !== null - ? { tools_synced_at: Date.now() } - : { - tools_synced_at: Date.now(), - last_health: health ?? toolSyncHealth(reason), - updated_at: new Date(), - }, - }), + fresh === null + ? Effect.void + : core + .updateMany("connection", { + where: (b: AnyCb) => + b.and( + connectionWhere(b), + b("updated_at", "=", fresh.updated_at), + fresh.tools_synced_at == null + ? b.isNull("tools_synced_at") + : b("tools_synced_at", "=", fresh.tools_synced_at), + ), + set: + oauthReauthRequiredFromProviderState(fresh.provider_state) !== null + ? { tools_synced_at: Date.now() } + : { + tools_synced_at: Date.now(), + last_health: health ?? toolSyncHealth(reason), + updated_at: new Date(), + }, + }) + .pipe(Effect.asVoid), ), ); diff --git a/packages/plugins/mcp/src/sdk/connection.ts b/packages/plugins/mcp/src/sdk/connection.ts index 7f206974ff..6d9a980b78 100644 --- a/packages/plugins/mcp/src/sdk/connection.ts +++ b/packages/plugins/mcp/src/sdk/connection.ts @@ -157,7 +157,11 @@ const nestedMcpHttpTransportError = (cause: unknown): Option.Option { +/** Walks a (possibly SDK-wrapped) failure cause for the fetch adapter's + * `McpOAuthReauthorizationRequired`. Shared with tool discovery: the same + * interception fires during `tools/list`, where the SDK wraps the rejection + * before it reaches the listing's catch site. */ +export const hasNestedOAuthReauthorization = (cause: unknown): boolean => { let current: unknown = cause; for (let depth = 0; depth < 8; depth += 1) { if (Predicate.isTagged(current, "McpOAuthReauthorizationRequired")) return true; @@ -291,15 +295,31 @@ const fetchFromHttpClientLayer = ( // tagged error from the fetch adapter (a true runtime edge: the SDK // consumes promise rejections) so it reaches the invoke/connect catch // sites verbatim. - const promise = Effect.runPromise(effect).then((response) => { - if (staticOAuthBearer && response.status === 401) { - // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: Fetch-compatible adapter can only signal through a rejected promise - throw new McpOAuthReauthorizationRequired({ - message: "MCP OAuth re-authorization required", - }); + const promise = Effect.runPromise(effect).then(async (response) => { + let settled = response; + if (staticOAuthBearer && settled.status === 401) { + // One immediate replay before classifying: a lone 401 can be a + // transient upstream blip (a proxy hiccup, a racing key rotation on + // the server), and stamping reauthorization-required from a single + // sample forces a needless reconnect. Replaying is safe — a 401 + // refused the request before processing it, and every body this + // adapter builds is buffered (`applyBody`), never a one-shot stream. + // Retrying is preferred over demanding a `WWW-Authenticate` challenge + // because a headerless 401 (noncompliant server; the MCP auth spec + // requires the challenge) must STILL stop at this boundary — falling + // through would hand the 401 to the SDK, whose interactive fallback + // performs exactly the avoidable discovery/DCR this interception + // exists to prevent. + settled = await Effect.runPromise(effect); + if (settled.status === 401) { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: Fetch-compatible adapter can only signal through a rejected promise + throw new McpOAuthReauthorizationRequired({ + message: "MCP OAuth re-authorization required", + }); + } } - if (response.status === 403) { - const challenge = response.headers.get("www-authenticate"); + if (settled.status === 403) { + const challenge = settled.headers.get("www-authenticate"); if ( challenge !== null && detectInsufficientScope({ headers: { "www-authenticate": challenge } }) !== null @@ -311,7 +331,7 @@ const fetchFromHttpClientLayer = ( }); } } - return response; + return settled; }); // Mark the request promise observed (a no-op handler on the ORIGINAL // promise; callers still see the rejection). The MCP SDK fires some diff --git a/packages/plugins/mcp/src/sdk/discover.ts b/packages/plugins/mcp/src/sdk/discover.ts index 31ba3863bc..5448e76d94 100644 --- a/packages/plugins/mcp/src/sdk/discover.ts +++ b/packages/plugins/mcp/src/sdk/discover.ts @@ -4,8 +4,9 @@ import { Duration, Effect, Option, Predicate } from "effect"; -import type { McpConnection, McpConnector } from "./connection"; +import { hasNestedOAuthReauthorization, type McpConnection, type McpConnector } from "./connection"; import { McpToolDiscoveryError } from "./errors"; +import { httpStatusFromCause } from "./http-status"; import { decodeListToolsPage, extractManifestFromListToolsResult, @@ -49,11 +50,34 @@ const listAllTools = ( const params: { cursor?: string } | undefined = cursor === undefined ? undefined : { cursor }; const listResult = yield* Effect.tryPromise({ try: () => connection.client.listTools(params), - catch: () => - new McpToolDiscoveryError({ + // A bearer accepted at the handshake can be rejected by the time the + // listing runs (revocation landing mid-discovery). The fetch adapter's + // reauthorization interception fires here exactly as during connect, + // so keep the structural signals — the reauthorization tag and the + // HTTP status — instead of collapsing every listing failure into a + // generic discovery error; discarding them left the connection on a + // sticky degraded verdict instead of reconnect-required. Statuses come + // from `httpStatusFromCause` only: the connection-only numeric-code + // decode must stay out, since JSON-RPC error codes are not HTTP + // statuses. + catch: (cause) => { + if (hasNestedOAuthReauthorization(cause)) { + return new McpToolDiscoveryError({ + stage: "list_tools", + message: "Failed listing MCP tools: MCP OAuth re-authorization required", + reauthorizationRequired: true, + }); + } + const httpStatus = httpStatusFromCause(cause); + return new McpToolDiscoveryError({ stage: "list_tools", - message: "Failed listing MCP tools", - }), + message: + httpStatus === undefined + ? "Failed listing MCP tools" + : `Failed listing MCP tools (HTTP ${httpStatus})`, + ...(httpStatus === undefined ? {} : { httpStatus }), + }); + }, }); const decoded = decodeListToolsPage(listResult); diff --git a/packages/plugins/mcp/src/sdk/errors.ts b/packages/plugins/mcp/src/sdk/errors.ts index 6d5a2501ea..f1e6d78a5e 100644 --- a/packages/plugins/mcp/src/sdk/errors.ts +++ b/packages/plugins/mcp/src/sdk/errors.ts @@ -40,7 +40,8 @@ export class McpToolDiscoveryError extends Schema.TaggedErrorClass Date: Sat, 29 Aug 2026 00:13:06 -0700 Subject: [PATCH 4/5] Cover post-handshake tools/list 401, lone-401 retry, and the sync verdict swap --- .../mcp-oauth-tool-refresh-reauth.test.ts | 73 +++++++- packages/core/sdk/src/connections.test.ts | 47 ++++- packages/plugins/mcp/src/sdk/plugin.test.ts | 166 ++++++++++++++++++ packages/plugins/mcp/src/testing/server.ts | 62 +++++-- 4 files changed, 335 insertions(+), 13 deletions(-) diff --git a/e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts b/e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts index 0ac03b4fd8..298d812904 100644 --- a/e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts +++ b/e2e/selfhost/mcp-oauth-tool-refresh-reauth.test.ts @@ -3,7 +3,7 @@ // preserve the previously synced catalog, and must never dynamically register // a fresh OAuth client — the saved connection already references one. // -// Two variants: +// Three variants: // 1. The upstream MCP endpoint rejects a bearer executor still considers // unexpired (the live report): the refresh dials with the stored token, // gets 401, and must come back as reconnect-required without the MCP SDK's @@ -12,6 +12,10 @@ // with `invalid_grant` during the sync's credential resolution: the // recorded dead grant must present as expired on the connection read, not // be buried under a generic tool-sync verdict. +// 3. The bearer is honoured at the handshake and revoked by the time +// `tools/list` runs: the reauthorization condition surfaces from the +// LISTING failure, which the connect-path classification never sees, and +// must reach the same expired verdict. import { randomBytes } from "node:crypto"; import { Effect } from "effect"; @@ -288,3 +292,70 @@ scenario( }), ), ); + +scenario( + "MCP OAuth · a bearer rejected during tools/list after a successful handshake surfaces reconnect without re-registering", + { + timeout: 180_000, + }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const { client: makeApiClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + + const oauth = yield* serveOAuthTestServer({ + scopes: ["channels:history", "users:read"], + }); + const mcp = yield* serveTokenGatedMcpServer(oauth); + const { slug } = yield* seedDcrMcpOAuthConnection( + client, + "mcp-refresh-list-401", + oauth, + mcp.endpoint, + ); + + // Baseline: with the bearer honoured, the refresh syncs the real catalog. + const synced = yield* client.connections.refresh({ + params: { owner: "org", integration: slug, name }, + }); + expect( + synced.map((tool) => String(tool.name)), + "the healthy connection syncs the server's catalog", + ).toEqual(["simple_echo"]); + + // Revocation landing between the handshake and the listing: the server + // keeps honouring the bearer for `initialize` but answers every + // `tools/list` with the auth wall. The connect-path 401 classification + // never fires — the reauthorization signal must survive the listing + // failure instead. + yield* mcp.rejectSessionMethod("tools/list", 401); + yield* oauth.clearRequests; + + const refreshed = yield* client.connections.refresh({ + params: { owner: "org", integration: slug, name }, + }); + + const registers = yield* registrationRequests(oauth); + expect( + registers, + "a noninteractive tool refresh must not dynamically register a fresh OAuth client", + ).toEqual([]); + + expect( + refreshed.map((tool) => String(tool.name)), + "the previously synced catalog is preserved through the failed refresh", + ).toEqual(["simple_echo"]); + + const reread = yield* client.connections.get({ + params: { owner: "org", integration: slug, name }, + }); + console.info(`[BUG repro] post-refresh health: ${JSON.stringify(reread.lastHealth ?? null)}`); + expect( + reread.lastHealth?.status, + "a post-handshake 401 during listing is a reauthorization condition, not an anonymous degraded sync", + ).toBe("expired"); + }), + ), +); diff --git a/packages/core/sdk/src/connections.test.ts b/packages/core/sdk/src/connections.test.ts index eb8a52ba88..ac254ceb79 100644 --- a/packages/core/sdk/src/connections.test.ts +++ b/packages/core/sdk/src/connections.test.ts @@ -28,7 +28,7 @@ import { ConnectionAlreadyExistsError } from "./errors"; import { createExecutor } from "./executor"; import { StorageError, type FumaDb } from "./fuma-runtime"; import { HealthCheckResult } from "./health-check"; -import { definePlugin } from "./plugin"; +import { definePlugin, type ResolveToolsResult } from "./plugin"; import type { CredentialProvider } from "./provider"; import { makeTestConfig, makeTestExecutor } from "./testing"; import { ToolResult } from "./tool-result"; @@ -2326,6 +2326,10 @@ const makeHealthHarness = (options?: { // Cleared before it runs, so the conflicting write it performs (through // the unwrapped `config.db`) is not intercepted again. beforeHealthPersist: null as Effect.Effect | null, + // Replaces the plugin's `resolveTools` outcome, so a test can drive the + // incomplete-catalog path that persists a plugin-supplied health verdict + // (`stampSyncedWithHealth`). + resolveTools: null as Effect.Effect | null, }; // Wraps the executor's FumaDb handle so `beforeHealthPersist` can commit a // conflicting write in the exact window the write guards must close. @@ -2378,7 +2382,11 @@ const makeHealthHarness = (options?: { credentialProviders: [countingProvider], storage: () => ({}), resolveTools: () => - Effect.succeed({ tools: [{ name: ToolName.make("deploy"), description: "deploy" }] }), + Effect.suspend( + () => + hooks.resolveTools ?? + Effect.succeed({ tools: [{ name: ToolName.make("deploy"), description: "deploy" }] }), + ), invokeTool: ({ toolRow, credential, args }) => Effect.as( hooks.onInvoke, @@ -2970,6 +2978,41 @@ describe("verdict write guards close the check-to-write window", () => { expect(row?.lastHealth).toMatchObject({ status: "degraded", detail: SYNC_DETAIL }); }), ); + + it.effect("tool-sync verdict persist: a reconnect landing inside the window wins", () => + Effect.gen(function* () { + const { executor, stamp, persisted, hooks } = yield* makeHealthHarness(); + // Age the stamp so the reconnect's bump lands in a different granule. + yield* stamp({ updated_at: new Date(Date.now() - STALE_MS) }); + // The refresh's discovery meets a reauthorization condition on the OLD + // credential and reports an actionable expired verdict... + hooks.resolveTools = Effect.succeed({ + tools: [], + incomplete: true, + incompleteReason: "MCP OAuth re-authorization required", + health: { + status: "expired" as const, + checkedAt: Date.now(), + detail: "MCP OAuth reauthorization required", + }, + }); + // ...while a reconnect commits inside the check-to-write window: the + // replacement grant clears the verdict and any dead-grant state and + // bumps the stamp. The read-side dead-grant guard cannot refuse the + // stale verdict — the reconnected row has nothing for it to observe — + // so only the compare-and-swap can. + hooks.beforeHealthPersist = stamp({ + provider_state: null, + last_health: null, + updated_at: new Date(), + }).pipe(Effect.asVoid); + + yield* executor.connections.refresh(REF); + + const row = yield* persisted(); + expect(row?.lastHealth ?? null).toBeNull(); + }), + ); }); describe("credential-only health path", () => { diff --git a/packages/plugins/mcp/src/sdk/plugin.test.ts b/packages/plugins/mcp/src/sdk/plugin.test.ts index fbe99ab9c0..3983d28755 100644 --- a/packages/plugins/mcp/src/sdk/plugin.test.ts +++ b/packages/plugins/mcp/src/sdk/plugin.test.ts @@ -142,6 +142,93 @@ const rejectedOAuthDiscoveryLayer = (endpoint: string) => { return { layer, requests }; }; +const clientRpcOf = (request: HttpClientRequest.HttpClientRequest): JsonRpcRequest | undefined => + Predicate.isTagged(request.body, "Uint8Array") + ? Option.getOrUndefined(decodeJsonRpcRequest(new TextDecoder().decode(request.body.body))) + : undefined; + +/** Streamable-http fixture for the post-handshake auth wall: the handshake + * methods succeed and `tools/list` answers 401 for the first + * `revokedListResponses` calls (Infinity = the bearer stays revoked). The + * OAuth discovery + DCR endpoints are served so an SDK fallback that DID see + * the 401 could register — the ledger proves it never gets there. Entries are + * `pathname` or `pathname#jsonRpcMethod`. */ +const listRejectionFixtureLayer = (endpoint: string, revokedListResponses: number) => { + const issuer = new URL(endpoint).origin; + const requests: string[] = []; + let remaining = revokedListResponses; + const jsonRpc = (rpc: JsonRpcRequest, result: unknown) => + Response.json({ jsonrpc: "2.0", id: rpc.id ?? null, result }); + const layer = Layer.succeed(HttpClient.HttpClient)( + HttpClient.make((request: HttpClientRequest.HttpClientRequest) => { + const url = new URL(request.url); + const rpc = request.url === endpoint ? clientRpcOf(request) : undefined; + requests.push(rpc === undefined ? url.pathname : `${url.pathname}#${rpc.method}`); + const respond = (response: Response) => + Effect.succeed(HttpClientResponse.fromWeb(request, response)); + if (request.url === endpoint) { + if (rpc === undefined) return respond(new Response("SSE disabled", { status: 405 })); + if (rpc.method === "initialize") { + return respond( + jsonRpc(rpc, { + protocolVersion: "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "list-reject-fixture", version: "1.0.0" }, + }), + ); + } + if (rpc.method === "notifications/initialized") { + return respond(new Response("", { status: 202 })); + } + if (rpc.method === "tools/list") { + if (remaining > 0) { + remaining -= 1; + return respond(new Response("", { status: 401 })); + } + return respond( + jsonRpc(rpc, { + tools: [ + { + name: "echo_back", + description: "Echoes", + inputSchema: { type: "object", properties: {} }, + }, + ], + }), + ); + } + return respond(new Response("Unexpected JSON-RPC method", { status: 400 })); + } + const response = + url.pathname === "/.well-known/oauth-protected-resource/mcp" + ? Response.json({ resource: endpoint, authorization_servers: [issuer] }) + : url.pathname === "/.well-known/oauth-authorization-server" + ? Response.json({ + issuer, + authorization_endpoint: `${issuer}/authorize`, + token_endpoint: `${issuer}/token`, + registration_endpoint: `${issuer}/register`, + response_types_supported: ["code"], + code_challenge_methods_supported: ["S256"], + }) + : url.pathname === "/register" + ? Response.json( + { + client_id: "replacement-client", + redirect_uris: ["http://localhost/oauth/callback"], + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + token_endpoint_auth_method: "none", + }, + { status: 201 }, + ) + : new Response("unexpected request", { status: 500 }); + return respond(response); + }), + ); + return { layer, requests }; +}; + // `tools/call` responders. Both embed a "do-not-leak" sentinel the assertions // confirm never reaches the caller-facing failure. const httpStatusCallTool = @@ -412,6 +499,85 @@ describe("mcpPlugin", () => { }), ); + // The connect path above classifies a handshake 401. This covers the other + // half of the window: the bearer is honoured at `initialize` and revoked by + // the time `tools/list` runs, so the reauthorization signal surfaces from + // the LISTING failure, not the connect failure. + it.effect( + "surfaces OAuth reauthorization when tools/list rejects a bearer the handshake accepted", + () => + Effect.gen(function* () { + const endpoint = "https://mcp.example.test/mcp"; + const plugin = mcpPlugin(); + const ledger = listRejectionFixtureLayer(endpoint, Number.POSITIVE_INFINITY); + const result = yield* plugin.resolveTools!({ + config: { + transport: "remote", + endpoint, + remoteTransport: "streamable-http", + authenticationTemplate: [{ slug: "oauth2", kind: "oauth2" }], + }, + connection: { + owner: "org", + integration: IntegrationSlug.make("oauth_mcp"), + name: ConnectionName.make("main"), + }, + template: AuthTemplateSlug.make("oauth2"), + getValues: () => Effect.succeed({ token: "revoked-after-handshake" }), + getValue: () => Effect.succeed("revoked-after-handshake"), + httpClientLayer: ledger.layer, + ctx: null as never, + integration: null as never, + storage: {}, + }); + + expect(result).toMatchObject({ + tools: [], + incomplete: true, + health: { + status: "expired", + detail: expect.stringContaining("reauthorization"), + }, + }); + expect(ledger.requests.filter((entry) => entry === "/register")).toEqual([]); + }), + ); + + // A LONE 401 is not evidence of a revoked bearer: transient upstream blips + // must not stamp reauthorization-required. The adapter replays the request + // once and only a repeated 401 classifies. + it.effect("retries a lone tools/list 401 instead of demanding reauthorization", () => + Effect.gen(function* () { + const endpoint = "https://mcp.example.test/mcp"; + const plugin = mcpPlugin(); + const ledger = listRejectionFixtureLayer(endpoint, 1); + const result = yield* plugin.resolveTools!({ + config: { + transport: "remote", + endpoint, + remoteTransport: "streamable-http", + authenticationTemplate: [{ slug: "oauth2", kind: "oauth2" }], + }, + connection: { + owner: "org", + integration: IntegrationSlug.make("oauth_mcp"), + name: ConnectionName.make("main"), + }, + template: AuthTemplateSlug.make("oauth2"), + getValues: () => Effect.succeed({ token: "blipped-token" }), + getValue: () => Effect.succeed("blipped-token"), + httpClientLayer: ledger.layer, + ctx: null as never, + integration: null as never, + storage: {}, + }); + + expect(result.incomplete).not.toBe(true); + expect(result.tools.map((tool) => String(tool.name))).toEqual(["echo_back"]); + expect(ledger.requests.filter((entry) => entry === "/mcp#tools/list")).toHaveLength(2); + }), + ); + it.effect("creates executor with mcp plugin", () => Effect.gen(function* () { const executor = yield* createExecutor( diff --git a/packages/plugins/mcp/src/testing/server.ts b/packages/plugins/mcp/src/testing/server.ts index c1fdca98b5..7c47d3fbe2 100644 --- a/packages/plugins/mcp/src/testing/server.ts +++ b/packages/plugins/mcp/src/testing/server.ts @@ -19,6 +19,12 @@ export type McpTestServer = { readonly forgetSessions: Effect.Effect; /** Rejects the next request carrying an MCP session id with this status. */ readonly rejectNextSessionRequest: (status: number) => Effect.Effect; + /** From now on, rejects every session-carrying POST whose JSON-RPC body + * names this method with the given status (401 answers with the same + * WWW-Authenticate shape as the auth gate). Models a bearer revoked right + * after the handshake: `initialize` succeeds, the named request meets the + * auth wall. */ + readonly rejectSessionMethod: (method: string, status: number) => Effect.Effect; }; export type McpTestRequest = { @@ -92,6 +98,29 @@ export const serveMcpServer = (factory: () => McpServer, options: McpTestServerO const path = options.path ?? "/"; let sessions = 0; let nextSessionRequestStatus: number | undefined; + let sessionMethodRejection: { readonly method: string; readonly status: number } | undefined; + + const writeUnauthorized = (response: http.ServerResponse, origin: string) => + writeJson( + response, + 401, + { error: "invalid_token" }, + { + "www-authenticate": + options.auth?.wwwAuthenticate ?? + `Bearer resource_metadata="${origin}${protectedResourcePath}${path}", error="invalid_token"`, + }, + ); + + const namesJsonRpcMethod = (parsedBody: unknown, method: string): boolean => { + const messages = Array.isArray(parsedBody) ? parsedBody : [parsedBody]; + return messages.some( + (message) => + typeof message === "object" && + message !== null && + (message as { readonly method?: unknown }).method === method, + ); + }; const handleMcpRequest = ( request: http.IncomingMessage, @@ -141,16 +170,7 @@ export const serveMcpServer = (factory: () => McpServer, options: McpTestServerO if (options.auth) { const accepted = yield* options.auth.validateAuthorization(authorization); if (!accepted) { - writeJson( - response, - 401, - { error: "invalid_token" }, - { - "www-authenticate": - options.auth.wwwAuthenticate ?? - `Bearer resource_metadata="${origin}${protectedResourcePath}${path}", error="invalid_token"`, - }, - ); + writeUnauthorized(response, origin); return; } } @@ -169,6 +189,24 @@ export const serveMcpServer = (factory: () => McpServer, options: McpTestServerO } if (existingTransport) { + const rejection = sessionMethodRejection; + if (rejection !== undefined && request.method === "POST") { + const body = yield* readRequestBody(request); + const parsedBody = Option.getOrUndefined(decodeJsonBody(body)); + if (namesJsonRpcMethod(parsedBody, rejection.method)) { + if (rejection.status === 401 && options.auth) { + writeUnauthorized(response, origin); + } else { + writeText(response, rejection.status, `Forced HTTP ${rejection.status}`); + } + return; + } + yield* Effect.tryPromise({ + try: () => existingTransport.handleRequest(request, response, parsedBody), + catch: (cause) => new McpTestServerError({ cause }), + }); + return; + } yield* Effect.tryPromise({ try: () => existingTransport.handleRequest(request, response), catch: (cause) => new McpTestServerError({ cause }), @@ -268,6 +306,10 @@ export const serveMcpServer = (factory: () => McpServer, options: McpTestServerO Effect.sync(() => { nextSessionRequestStatus = status; }), + rejectSessionMethod: (method: string, status: number) => + Effect.sync(() => { + sessionMethodRejection = { method, status }; + }), close: Effect.gen(function* () { for (const transport of allTransports) { yield* Effect.tryPromise({ From bcf32441f866f77bac1ed87511b1eb0ea8155281 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 29 Aug 2026 00:25:41 -0700 Subject: [PATCH 5/5] Restrict the 401 replay to read-only JSON-RPC methods --- packages/plugins/mcp/src/sdk/connection.ts | 76 +++++++++++++++++---- packages/plugins/mcp/src/sdk/plugin.test.ts | 38 +++++++++++ 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/packages/plugins/mcp/src/sdk/connection.ts b/packages/plugins/mcp/src/sdk/connection.ts index eeb480a5ef..6c8f4e38ad 100644 --- a/packages/plugins/mcp/src/sdk/connection.ts +++ b/packages/plugins/mcp/src/sdk/connection.ts @@ -248,6 +248,50 @@ const awaitAbort = (signal: AbortSignal): Effect.Effect => signal.addEventListener("abort", () => resume(Effect.void), { once: true }); }); +/** JSON-RPC methods the 401 replay below may re-send. An HTTP 401 does not + * guarantee the server did no work before rejecting, so replay is limited to + * methods that are read-only or handshake-only: `initialize` and + * `notifications/initialized` (handshake), `ping` (side-effect-free by + * spec), and `tools/list` (discovery). That is every method this codebase + * sends except `tools/call`, which may have executed its side effect before + * the 401 and must NEVER run twice. The set is deliberately closed: an + * unlisted or unparseable method does not replay either. */ +const REPLAYABLE_JSONRPC_METHODS: ReadonlySet = new Set([ + "initialize", + "notifications/initialized", + "ping", + "tools/list", +]); + +const JsonRpcMethodOnly = Schema.Struct({ method: Schema.String }); +const decodeJsonRpcMethods = Schema.decodeUnknownOption( + Schema.fromJsonString(Schema.Union([JsonRpcMethodOnly, Schema.Array(JsonRpcMethodOnly)])), +); + +/** Whether a 401-rejected request is safe to replay once. Only requests whose + * buffered JSON-RPC body consists entirely of allowlisted read-only methods + * qualify (a batch replays only when EVERY element is allowlisted). The one + * bodyless exception is `GET`, the streamable-http server->client stream + * open, which carries no JSON-RPC request and is read-only by HTTP + * semantics. Everything else — `tools/call` above all — fails closed. */ +const isReplaySafeRequest = (init: RequestInit | undefined): boolean => { + const body = init?.body; + if (body == null) return httpMethodFrom(init?.method) === "GET"; + // Both remote SDK transports send JSON-RPC bodies as `JSON.stringify` + // strings. Any other body shape cannot be verified read-only here. + if (typeof body !== "string") return false; + return Option.match(decodeJsonRpcMethods(body), { + onNone: () => false, + onSome: (parsed) => { + const messages = Array.isArray(parsed) ? parsed : [parsed]; + return ( + messages.length > 0 && + messages.every((message) => REPLAYABLE_JSONRPC_METHODS.has(message.method)) + ); + }, + }); +}; + const fetchFromHttpClientLayer = ( httpClientLayer: Layer.Layer, staticOAuthBearer: boolean, @@ -298,19 +342,27 @@ const fetchFromHttpClientLayer = ( const promise = Effect.runPromise(effect).then(async (response) => { let settled = response; if (staticOAuthBearer && settled.status === 401) { - // One immediate replay before classifying: a lone 401 can be a - // transient upstream blip (a proxy hiccup, a racing key rotation on - // the server), and stamping reauthorization-required from a single - // sample forces a needless reconnect. Replaying is safe — a 401 - // refused the request before processing it, and every body this + // One immediate replay before classifying — but only for requests the + // allowlist proves read-only (`isReplaySafeRequest`). A lone 401 on + // discovery/handshake traffic can be a transient upstream blip (a + // proxy hiccup, a racing key rotation on the server), and stamping + // reauthorization-required from a single sample forces a needless + // reconnect; the replay itself is possible because every body this // adapter builds is buffered (`applyBody`), never a one-shot stream. - // Retrying is preferred over demanding a `WWW-Authenticate` challenge - // because a headerless 401 (noncompliant server; the MCP auth spec - // requires the challenge) must STILL stop at this boundary — falling - // through would hand the 401 to the SDK, whose interactive fallback - // performs exactly the avoidable discovery/DCR this interception - // exists to prevent. - settled = await Effect.runPromise(effect); + // A side-effectful method (`tools/call`) never replays: a 401 does + // not guarantee the server did no work first, so re-sending could + // execute the action twice. Its single 401 classifies directly — + // the invocation has already failed either way, and the + // transient-blip concern only justified the retry on read-only + // paths. Retrying/classifying here is preferred over demanding a + // `WWW-Authenticate` challenge because a headerless 401 + // (noncompliant server; the MCP auth spec requires the challenge) + // must STILL stop at this boundary — falling through would hand the + // 401 to the SDK, whose interactive fallback performs exactly the + // avoidable discovery/DCR this interception exists to prevent. + if (isReplaySafeRequest(init)) { + settled = await Effect.runPromise(effect); + } if (settled.status === 401) { // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: Fetch-compatible adapter can only signal through a rejected promise throw new McpOAuthReauthorizationRequired({ diff --git a/packages/plugins/mcp/src/sdk/plugin.test.ts b/packages/plugins/mcp/src/sdk/plugin.test.ts index 3983d28755..fc8ce7f120 100644 --- a/packages/plugins/mcp/src/sdk/plugin.test.ts +++ b/packages/plugins/mcp/src/sdk/plugin.test.ts @@ -540,6 +540,11 @@ describe("mcpPlugin", () => { }, }); expect(ledger.requests.filter((entry) => entry === "/register")).toEqual([]); + // Exactly 2: the original tools/list plus the adapter's single + // read-only replay. A third request would mean the replay loops; a + // single one would mean a lone 401 classified without the + // transient-blip re-sample. + expect(ledger.requests.filter((entry) => entry === "/mcp#tools/list")).toHaveLength(2); }), ); @@ -990,6 +995,39 @@ describe("mcpPlugin", () => { ); } + // The lone-401 replay above is restricted to read-only methods. A + // `tools/call` may have executed its side effect before the server answered + // 401 (HTTP gives no such guarantee), so the adapter must never re-send it + // — the single 401 classifies reauthorization directly instead. + it.effect("never replays a tools/call 401: the action must not run twice", () => + Effect.scoped( + Effect.gen(function* () { + let callToolRequests = 0; + const { executor, toolAddress } = yield* seedCallToolExecutor({ + slug: "call_replay_401", + // oauth: the transport gets an authProvider, which is the + // staticOAuthBearer path where the adapter's 401 replay lives. + oauth: true, + callTool: () => { + callToolRequests += 1; + return HttpServerResponse.text("do-not-leak: revoked mid-session", { status: 401 }); + }, + }); + + const result = yield* executor.execute(toolAddress, {}, { onElicitation: "accept-all" }); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "oauth_reauth_required", + details: { category: "authentication" }, + }, + }); + expect(callToolRequests, "a 401 tools/call must reach the server exactly once").toBe(1); + }), + ), + ); + it.effect( "classifies a scope-insufficient 403 as oauth_scope_insufficient, not connection_rejected", () =>