fix: escape html in external oauth error message (#841)

* changeset

* feat: Add XSS protection for OAuth error messages

* feat: display server errors inline in ai playground

* fix: types

* refactor: fail connection helper

* refactor: restructure handleCallbackRequest to separate early validation errors (throw) from connection-specific errors (fail connection)

Move all connection-related error handling into try-catch block to ensure consistent error reporting. Escape OAuth error params to prevent XSS. Update tests to expect failed connections instead of thrown errors for connection-specific validation failures.

Author: mattzcarey

.changeset/rare-windows-bathe.md

@@ -0,0 +1,5 @@
+---
+"agents": patch
+---
+
+Escape authError to prevent XSS attacks and store it in the connection state to avoid needing script tags to display error.

examples/mcp-client/src/client.tsx

@@ -69,6 +69,7 @@ function App() {
         placeholder: {
           auth_url: null,
           capabilities: null,
+          error: null,
           instructions: null,
           name: serverName,
           server_url: serverUrl,

examples/mcp-client/src/server.ts

@@ -1,26 +1,14 @@
 import { Agent, routeAgentRequest } from "agents";
-import type { MCPClientOAuthResult } from "agents/mcp";
 
 export class MyAgent extends Agent {
   onStart() {
     // Optionally configure OAuth callback. Here we use popup-closing behavior since we're opening a window on the client
     this.mcp.configureOAuthCallback({
-      customHandler: (result: MCPClientOAuthResult) => {
-        if (result.authSuccess) {
-          return new Response("<script>window.close();</script>", {
-            headers: { "content-type": "text/html" },
-            status: 200
-          });
-        } else {
-          const safeError = JSON.stringify(result.authError || "Unknown error");
-          return new Response(
-            `<script>alert('Authentication failed: ' + ${safeError}); window.close();</script>`,
-            {
-              headers: { "content-type": "text/html" },
-              status: 200
-            }
-          );
-        }
+      customHandler: () => {
+        return new Response("<script>window.close();</script>", {
+          headers: { "content-type": "text/html" },
+          status: 200
+        });
       }
     });
   }

package-lock.json

@@ -27,6 +27,7 @@
     "@cfworker/json-schema": "^4.1.1",
     "@modelcontextprotocol/sdk": "1.25.2",
     "cron-schedule": "^6.0.0",
+    "escape-html": "^1.0.3",
     "json-schema": "^0.4.0",
     "json-schema-to-typescript": "^15.0.4",
     "mimetext": "^3.0.28",
@@ -39,6 +40,7 @@
     "@ai-sdk/openai": "^3.0.23",
     "@ai-sdk/react": "^3.0.66",
     "@cloudflare/workers-oauth-provider": "^0.2.2",
+    "@types/escape-html": "^1.0.4",
     "@types/react": "^19.2.10",
     "@types/yargs": "^17.0.35",
     "ai": "^6.0.64",

packages/agents/package.json

@@ -259,6 +259,7 @@ export type MCPServer = {
   // Scope outside of that can't be relied upon because when the DO sleeps, there's no way
   // to communicate a change to a non-ready state.
   state: MCPConnectionState;
+  error: string | null;
   instructions: string | null;
   capabilities: ServerCapabilities | null;
 };
@@ -3182,6 +3183,7 @@ export class Agent<
         mcpState.servers[server.id] = {
           auth_url: server.auth_url,
           capabilities: serverConn?.serverCapabilities ?? null,
+          error: serverConn?.connectionError ?? null,
           instructions: serverConn?.instructions ?? null,
           name: server.name,
           server_url: server.server_url,

packages/agents/src/index.ts

@@ -95,6 +95,7 @@ export type MCPDiscoveryResult = {
 export class MCPClientConnection {
   client: Client;
   connectionState: MCPConnectionState = MCPConnectionState.CONNECTING;
+  connectionError: string | null = null;
   lastConnectedTransport: BaseTransportType | undefined;
   instructions?: string;
   tools: Tool[] = [];

packages/agents/src/mcp/client-connection.ts

@@ -1,4 +1,5 @@
 import type { Client } from "@modelcontextprotocol/sdk/client/index.js";
+import escapeHtml from "escape-html";
 import type { RequestOptions } from "@modelcontextprotocol/sdk/shared/protocol.js";
 import type {
   CallToolRequest,
@@ -44,6 +45,13 @@ export type MCPServerOptions = {
   };
 };
 
+/**
+ * Result of an OAuth callback request
+ */
+export type MCPOAuthCallbackResult =
+  | { serverId: string; authSuccess: true; authError?: undefined }
+  | { serverId: string; authSuccess: false; authError: string };
+
 /**
  * Options for registering an MCP server
  */
@@ -187,6 +195,19 @@ export class MCPClientManager {
     );
   }
 
+  private failConnection(
+    serverId: string,
+    error: string
+  ): MCPOAuthCallbackResult {
+    this.clearServerAuthUrl(serverId);
+    if (this.mcpConnections[serverId]) {
+      this.mcpConnections[serverId].connectionState = MCPConnectionState.FAILED;
+      this.mcpConnections[serverId].connectionError = error;
+    }
+    this._onServerStateChanged.fire();
+    return { serverId, authSuccess: false, authError: error };
+  }
+
   jsonSchema: typeof import("ai").jsonSchema | undefined;
 
   /**
@@ -663,19 +684,19 @@ export class MCPClientManager {
     return servers.some((server) => server.id === serverId);
   }
 
-  async handleCallbackRequest(req: Request) {
+  async handleCallbackRequest(req: Request): Promise<MCPOAuthCallbackResult> {
     const url = new URL(req.url);
     const code = url.searchParams.get("code");
     const state = url.searchParams.get("state");
     const error = url.searchParams.get("error");
     const errorDescription = url.searchParams.get("error_description");
 
+    // Early validation - these throw because we can't identify the connection
     if (!state) {
       throw new Error("Unauthorized: no state provided");
     }
 
     const serverId = this.extractServerIdFromState(state);
-
     if (!serverId) {
       throw new Error(
         "No serverId found in state parameter. Expected format: {nonce}.{serverId}"
@@ -684,7 +705,6 @@ export class MCPClientManager {
 
     const servers = this.getServersFromStorage();
     const serverExists = servers.some((server) => server.id === serverId);
-
     if (!serverExists) {
       throw new Error(
         `No server found with id "${serverId}". Was the request matched with \`isCallbackRequest()\`?`
@@ -695,89 +715,61 @@ export class MCPClientManager {
       throw new Error(`Could not find serverId: ${serverId}`);
     }
 
+    // We have a valid connection - all errors from here should fail the connection
     const conn = this.mcpConnections[serverId];
-    if (!conn.options.transport.authProvider) {
-      throw new Error(
-        "Trying to finalize authentication for a server connection without an authProvider"
-      );
-    }
 
-    const authProvider = conn.options.transport.authProvider;
-    authProvider.serverId = serverId;
+    try {
+      if (!conn.options.transport.authProvider) {
+        throw new Error(
+          "Trying to finalize authentication for a server connection without an authProvider"
+        );
+      }
 
-    // Two-phase state validation: check first (non-destructive), consume later
-    // This prevents DoS attacks where attacker consumes valid state before legitimate callback
-    const stateValidation = await authProvider.checkState(state);
-    if (!stateValidation.valid) {
-      this.clearServerAuthUrl(serverId);
-      if (this.mcpConnections[serverId]) {
-        this.mcpConnections[serverId].connectionState =
-          MCPConnectionState.FAILED;
+      const authProvider = conn.options.transport.authProvider;
+      authProvider.serverId = serverId;
+
+      // Two-phase state validation: check first (non-destructive), consume later
+      // This prevents DoS attacks where attacker consumes valid state before legitimate callback
+      const stateValidation = await authProvider.checkState(state);
+      if (!stateValidation.valid) {
+        throw new Error(stateValidation.error || "Invalid state");
       }
-      this._onServerStateChanged.fire();
-      return {
-        serverId,
-        authSuccess: false,
-        authError: stateValidation.error || "Invalid state"
-      };
-    }
 
-    if (error) {
-      return {
-        serverId,
-        authSuccess: false,
-        authError: errorDescription || error
-      };
-    }
+      if (error) {
+        // Escape external OAuth error params to prevent XSS
+        throw new Error(escapeHtml(errorDescription || error));
+      }
 
-    if (!code) {
-      throw new Error("Unauthorized: no code provided");
-    }
+      if (!code) {
+        throw new Error("Unauthorized: no code provided");
+      }
 
-    if (
-      this.mcpConnections[serverId].connectionState ===
-        MCPConnectionState.READY ||
-      this.mcpConnections[serverId].connectionState ===
-        MCPConnectionState.CONNECTED
-    ) {
-      this.clearServerAuthUrl(serverId);
-      return {
-        serverId,
-        authSuccess: true
-      };
-    }
+      // Already authenticated - just return success
+      if (
+        conn.connectionState === MCPConnectionState.READY ||
+        conn.connectionState === MCPConnectionState.CONNECTED
+      ) {
+        this.clearServerAuthUrl(serverId);
+        return { serverId, authSuccess: true };
+      }
 
-    if (
-      this.mcpConnections[serverId].connectionState !==
-      MCPConnectionState.AUTHENTICATING
-    ) {
-      throw new Error(
-        `Failed to authenticate: the client is in "${this.mcpConnections[serverId].connectionState}" state, expected "authenticating"`
-      );
-    }
+      if (conn.connectionState !== MCPConnectionState.AUTHENTICATING) {
+        throw new Error(
+          `Failed to authenticate: the client is in "${conn.connectionState}" state, expected "authenticating"`
+        );
+      }
 
-    try {
       await authProvider.consumeState(state);
       await conn.completeAuthorization(code);
       await authProvider.deleteCodeVerifier();
       this.clearServerAuthUrl(serverId);
+      conn.connectionError = null;
       this._onServerStateChanged.fire();
 
-      return {
-        serverId,
-        authSuccess: true
-      };
-    } catch (authError) {
-      const errorMessage =
-        authError instanceof Error ? authError.message : String(authError);
-
-      this._onServerStateChanged.fire();
-
-      return {
-        serverId,
-        authSuccess: false,
-        authError: errorMessage
-      };
+      return { serverId, authSuccess: true };
+    } catch (err) {
+      const message = err instanceof Error ? err.message : String(err);
+      return this.failConnection(serverId, message);
     }
   }