Fix OAuth state parameter security vulnerability (#521)

* Fix OAuth state parameter security vulnerability

Replace clientId in state parameter with secure random tokens to
prevent potential security issues. Update callback handler to use
stored clientId instead of reading from state parameter.

Supports both static and dynamic client registration flows while
maintaining backward compatibility.

* Add changeset

* Remove redundant clientId storage

We already persist the clientId in sqlite and post hibernation, gets
picked up from the reconnect params where the clientId is stored.

Author: ghostwriternr

.changeset/mighty-badgers-vanish.md

@@ -0,0 +1,5 @@
+---
+"agents": patch
+---
+
+Fix OAuth state parameter security vulnerability by replacing client_id with secure random tokens

packages/agents/src/mcp/client.ts

@@ -201,13 +201,13 @@ export class MCPClientManager {
       );
     }
     const code = url.searchParams.get("code");
-    const clientId = url.searchParams.get("state");
+    const state = url.searchParams.get("state");
     const urlParams = urlMatch.split("/");
     const serverId = urlParams[urlParams.length - 1];
     if (!code) {
       throw new Error("Unauthorized: no code provided");
     }
-    if (!clientId) {
+    if (!state) {
       throw new Error("Unauthorized: no state provided");
     }
 
@@ -228,6 +228,9 @@ export class MCPClientManager {
       );
     }
 
+    // Get clientId from auth provider (stored during redirectToAuthorization) or fallback to state for backward compatibility
+    const clientId = conn.options.transport.authProvider.clientId || state;
+
     // Set the OAuth credentials
     conn.options.transport.authProvider.clientId = clientId;
     conn.options.transport.authProvider.serverId = serverId;

packages/agents/src/mcp/do-oauth-client-provider.ts

@@ -5,6 +5,7 @@ import type {
   OAuthClientMetadata,
   OAuthTokens
 } from "@modelcontextprotocol/sdk/shared/auth.js";
+import { nanoid } from "nanoid";
 
 // A slight extension to the standard OAuthClientProvider interface because `redirectToAuthorization` doesn't give us the interface we need
 // This allows us to track authentication for a specific server and associated dynamic client registration
@@ -122,12 +123,9 @@ export class DurableObjectOAuthClientProvider implements AgentsOAuthProvider {
    * and require user interact to initiate the redirect flow
    */
   async redirectToAuthorization(authUrl: URL): Promise<void> {
-    // We want to track the client ID in state here because the typescript SSE client sometimes does
-    // a dynamic client registration AFTER generating this redirect URL.
-    const client_id = authUrl.searchParams.get("client_id");
-    if (client_id) {
-      authUrl.searchParams.append("state", client_id);
-    }
+    // Generate secure random token for state parameter
+    const stateToken = nanoid();
+    authUrl.searchParams.set("state", stateToken);
     this._authUrl_ = authUrl.toString();
   }