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({ 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; },