From 433aed547c74d3a32c867ae23b2c462f8cd96038 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 06:27:05 -0700 Subject: [PATCH 1/2] Add tests for MCP OAuth state round-trip (CL-6371) Cover the authorize URL carrying a CSRF-binding state param, a full connect where the provider echoes it back, and a provider that omits state we sent -- rejected as a CSRF failure via the consumer envelope, never a raw provider error string. --- .../connections/src/mcp-oauth-routes.test.ts | 78 ++++++++++++++++++- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/packages/connections/src/mcp-oauth-routes.test.ts b/packages/connections/src/mcp-oauth-routes.test.ts index a6186fd78..a1b576381 100644 --- a/packages/connections/src/mcp-oauth-routes.test.ts +++ b/packages/connections/src/mcp-oauth-routes.test.ts @@ -60,7 +60,13 @@ function mountAs(routes: Hono): Hono { * token response also issues a refresh token — an authorization server * that doesn't (the Hugging-Face-precedent case) is the default. */ function startStubAuthorizationServer( - tokenGrant: { refreshToken?: string; expiresIn?: number } = {}, + tokenGrant: { + refreshToken?: string; + expiresIn?: number; + /** CL-6371 red-path fixture: a provider that never echoes `state` + * back on the authorize redirect, even though we sent one. */ + echoState?: boolean; + } = {}, ): { origin: string; resourcePath: string; @@ -119,7 +125,9 @@ function startStubAuthorizationServer( issuedCodes.set(code, { codeChallenge, clientId }); const redirect = new URL(redirectUri); redirect.searchParams.set("code", code); - redirect.searchParams.set("state", state); + if (tokenGrant.echoState !== false) { + redirect.searchParams.set("state", state); + } return Response.redirect(redirect.toString(), 302); } if (url.pathname === "/token" && req.method === "POST") { @@ -313,6 +321,11 @@ describe("MCP OAuth connect flow", () => { const params = new URL(location).searchParams; expect(params.get("code_challenge_method")).toBe("S256"); expect(params.get("client_id")).not.toBeNull(); + // CL-6371: the authorize URL must carry a CSRF-binding `state` -- + // omitting it is what made PostHog's real MCP authorization server + // reject the redirect with "Missing state parameter." + expect(params.get("state")).not.toBeNull(); + expect(params.get("state")).not.toBe(""); expect(response.headers.get("set-cookie") ?? "").toContain( "workbench_mcp_oauth_exa=", ); @@ -444,6 +457,67 @@ describe("MCP OAuth connect flow", () => { expect(response.status).toBe(404); }); + test("CL-6371: a provider that echoes state back completes the round trip", async () => { + const as = startStubAuthorizationServer({ echoState: true }); + try { + const hub = fakeHub(); + const routes = createMcpOAuthRoutes({ + hubUrl: "http://hub.test", + requireGrant: allowAll, + log: () => {}, + credentialCipher: createNoopCredentialCipher(), + apiCall: hub.apiCall, + probe: async (): Promise => ({ + ok: true, + toolCount: 1, + }), + }); + const app = mountAs(routes); + + const callbackResponse = await runConnectFlow(app, as); + + expect(callbackResponse.status).toBe(302); + expect(callbackResponse.headers.get("location") ?? "").toContain( + "outcome=connected", + ); + } finally { + as.stop(); + } + }); + + test("CL-6371: a provider that omits state we sent is rejected as a CSRF failure, not a raw error", async () => { + const as = startStubAuthorizationServer({ echoState: false }); + try { + const hub = fakeHub(); + const routes = createMcpOAuthRoutes({ + hubUrl: "http://hub.test", + requireGrant: allowAll, + log: () => {}, + credentialCipher: createNoopCredentialCipher(), + apiCall: hub.apiCall, + probe: async (): Promise => ({ + ok: true, + toolCount: 1, + }), + }); + const app = mountAs(routes); + + const callbackResponse = await runConnectFlow(app, as); + + expect(callbackResponse.status).toBe(302); + const location = callbackResponse.headers.get("location") ?? ""; + expect(location).toContain("outcome=error"); + expect(location).toContain("code=state_mismatch"); + // The consumer envelope idiom (CL-6360): the redirect carries a + // machine code the UI maps to copy, never the raw provider/SDK + // error text. + expect(location).not.toContain("Missing state parameter"); + expect(hub.credentials).toHaveLength(0); + } finally { + as.stop(); + } + }); + test("a callback with no cookie redirects with a state_expired error", async () => { const hub = fakeHub(); const routes = createMcpOAuthRoutes({ From 7b81b2fe9ec147e7e33385b83208656583b9122c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 20 Aug 2026 06:27:16 -0700 Subject: [PATCH 2/2] MCP OAuth connect: send and validate the CSRF state param (CL-6371) The MCP SDK's auth() only appends ?state= to the authorize URL when the OAuthClientProvider implements an optional state() method; ours never did, so every MCP OAuth connect (PostHog and every other preset, all sharing this one route) sent authorize requests with no state at all. PostHog's authorization server rejects those with "Missing state parameter." The route already minted a nonce and sealed it into the connect cookie, but never plumbed it into the SDK provider or checked it back on the callback -- CSRF protection that looked wired but did nothing. Fix: mint the nonce before calling auth() on /start, hand it to the provider via session.state so the SDK sends it as state=, and on /callback require the provider's returned state to match exactly. Missing or mismatched state now redirects with a `state_mismatch` error code through the same outcome/code envelope every other failure here uses, never the raw provider message. --- packages/connections/src/mcp-oauth-routes.ts | 26 ++++++++++++++++++-- packages/connections/src/mcp-oauth.ts | 15 +++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/packages/connections/src/mcp-oauth-routes.ts b/packages/connections/src/mcp-oauth-routes.ts index d7ab394e0..6bedbc051 100644 --- a/packages/connections/src/mcp-oauth-routes.ts +++ b/packages/connections/src/mcp-oauth-routes.ts @@ -153,7 +153,13 @@ export function createMcpOAuthRoutes( c.req.path.replace(/\/start$/, "/callback"), deps.hubUrl, ).toString(); - const session: McpOAuthSession = {}; + // Minted before `auth()` runs, not after: `auth()` reads it via + // `provider.state()` while building the authorize URL, so it must + // already be on the session by the time `redirectToAuthorization` + // fires. The same value is what `/callback` requires the provider's + // `?state=` to match. + const nonce = randomNonce(); + const session: McpOAuthSession = { state: nonce }; const provider = createMcpOAuthProvider({ callbackUrl, clientName: "Corbits Workbench", @@ -195,7 +201,7 @@ export function createMcpOAuthRoutes( name: target.name, url: target.url, returnPath, - nonce: randomNonce(), + nonce, expiresAt: Date.now() + OAUTH_STATE_TTL_MS, ...(session.codeVerifier !== undefined ? { codeVerifier: session.codeVerifier } @@ -286,6 +292,22 @@ export function createMcpOAuthRoutes( ); } + // CSRF check: `/start` always sends `state=` on the + // authorize URL (see `mcp-oauth.ts`'s `state()`), so the provider + // must echo that exact value back -- never optional-when-absent. + // A missing or mismatched `state` means this callback did not + // originate from the authorize redirect this session minted. + if (c.req.query("state") !== payload.nonce) { + return c.redirect( + redirectPath(returnPath, { + mcpOauth: payload.slug, + outcome: "error", + code: "state_mismatch", + }), + 302, + ); + } + const callbackUrl = new URL(c.req.path, deps.hubUrl).toString(); const session: McpOAuthSession = { ...(payload.codeVerifier !== undefined diff --git a/packages/connections/src/mcp-oauth.ts b/packages/connections/src/mcp-oauth.ts index 68ff6a0bf..2c239da52 100644 --- a/packages/connections/src/mcp-oauth.ts +++ b/packages/connections/src/mcp-oauth.ts @@ -28,6 +28,15 @@ import type { export type McpOAuthSession = { clientInformation?: OAuthClientInformationMixed; codeVerifier?: string; + /** The CSRF-binding OAuth `state` value: minted by the `/start` route + * before `auth()` runs (so it's known before `redirectToAuthorization` + * fires) and sealed into the same cookie as `codeVerifier`. `auth()` + * reads it through `state()` below and appends it to the authorize URL + * it sends the provider; `/callback` re-derives it from the cookie and + * requires the provider's `?state=` to match exactly -- CSRF protection + * that was previously minted (`nonce`) but never actually sent to the + * provider or checked back, so every connect silently omitted `state`. */ + state?: string; tokens?: OAuthTokens; }; @@ -59,6 +68,12 @@ export function createMcpOAuthProvider(args: { token_endpoint_auth_method: "none", }; }, + state(): string { + if (session.state === undefined) { + throw new Error("No OAuth state minted for this MCP OAuth session"); + } + return session.state; + }, clientInformation(): OAuthClientInformationMixed | undefined { return session.clientInformation; },