MCP OAuth connect: fix missing state param (CL-6371)

packages/connections/src/mcp-oauth-routes.test.ts

@@ -60,7 +60,13 @@ function mountAs(routes: Hono<TenantEnv>): Hono<TenantEnv> {
  * 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<McpProbeResult> => ({
+          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<McpProbeResult> => ({
+          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({

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=<nonce>` 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

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