Handle OAuth errors and validate redirect URLs (#631)

Author: ghostwriternr

.changeset/happy-students-prove.md

@@ -0,0 +1,5 @@
+---
+"agents": patch
+---
+
+Handle OAuth errors and validate redirect URLs

packages/agents/src/index.ts

@@ -1669,17 +1669,31 @@ export class Agent<
 
     // Redirect to success URL if configured
     if (config?.successRedirect && result.authSuccess) {
-      return Response.redirect(
-        new URL(config.successRedirect, baseOrigin).href
-      );
+      try {
+        return Response.redirect(
+          new URL(config.successRedirect, baseOrigin).href
+        );
+      } catch (e) {
+        console.error(
+          "Invalid successRedirect URL:",
+          config.successRedirect,
+          e
+        );
+        return Response.redirect(baseOrigin);
+      }
     }
 
     // Redirect to error URL if configured
     if (config?.errorRedirect && !result.authSuccess) {
-      const errorUrl = `${config.errorRedirect}?error=${encodeURIComponent(
-        result.authError || "Unknown error"
-      )}`;
-      return Response.redirect(new URL(errorUrl, baseOrigin).href);
+      try {
+        const errorUrl = `${config.errorRedirect}?error=${encodeURIComponent(
+          result.authError || "Unknown error"
+        )}`;
+        return Response.redirect(new URL(errorUrl, baseOrigin).href);
+      } catch (e) {
+        console.error("Invalid errorRedirect URL:", config.errorRedirect, e);
+        return Response.redirect(baseOrigin);
+      }
     }
 
     // Default: redirect to base URL

packages/agents/src/mcp/client.ts

@@ -218,8 +218,20 @@ export class MCPClientManager {
     }
     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");
     const urlParams = urlMatch.split("/");
     const serverId = urlParams[urlParams.length - 1];
+
+    // Handle OAuth error responses from the provider
+    if (error) {
+      return {
+        serverId,
+        authSuccess: false,
+        authError: errorDescription || error
+      };
+    }
+
     if (!code) {
       throw new Error("Unauthorized: no code provided");
     }

packages/agents/src/tests/mcp/client-manager.test.ts

@@ -174,11 +174,26 @@ describe("MCPClientManager OAuth Integration", () => {
       ).rejects.toThrow("No callback URI match found");
     });
 
-    it("should throw error for callback without code", async () => {
+    it("should handle OAuth error response from provider", async () => {
       const callbackUrl = "http://localhost:3000/callback/server1";
       manager.registerCallbackUrl(callbackUrl);
 
-      const callbackRequest = new Request(`${callbackUrl}?error=access_denied`);
+      const callbackRequest = new Request(
+        `${callbackUrl}?error=access_denied&error_description=User%20denied%20access`
+      );
+
+      const result = await manager.handleCallbackRequest(callbackRequest);
+
+      expect(result.serverId).toBe("server1");
+      expect(result.authSuccess).toBe(false);
+      expect(result.authError).toBe("User denied access");
+    });
+
+    it("should throw error for callback without code or error", async () => {
+      const callbackUrl = "http://localhost:3000/callback/server1";
+      manager.registerCallbackUrl(callbackUrl);
+
+      const callbackRequest = new Request(`${callbackUrl}?state=test`);
 
       await expect(
         manager.handleCallbackRequest(callbackRequest)

packages/agents/src/tests/mcp/oauth2-mcp-client.test.ts

@@ -290,4 +290,92 @@ describe("OAuth2 MCP Client", () => {
       expect(response.status).toBeGreaterThanOrEqual(400);
     });
   });
+
+  describe("OAuth Redirect Behavior", () => {
+    async function setupOAuthTest(config: {
+      successRedirect?: string;
+      errorRedirect?: string;
+      origin?: string;
+    }) {
+      const agentId = env.TestOAuthAgent.newUniqueId();
+      const agentStub = env.TestOAuthAgent.get(agentId);
+      await agentStub.setName("default");
+      await agentStub.onStart();
+      await agentStub.configureOAuthForTest(config);
+
+      const serverId = nanoid(8);
+      const origin = config.origin || "http://example.com";
+      const callbackBaseUrl = `${origin}/agents/oauth/${agentId.toString()}/callback`;
+
+      agentStub.sql`
+        INSERT INTO cf_agents_mcp_servers (id, name, server_url, client_id, auth_url, callback_url, server_options)
+        VALUES (${serverId}, ${"test"}, ${"http://example.com/mcp"}, ${"client"}, ${"http://example.com/auth"}, ${callbackBaseUrl}, ${null})
+      `;
+
+      await agentStub.setupMockMcpConnection(
+        serverId,
+        "test",
+        "http://example.com/mcp",
+        callbackBaseUrl
+      );
+      await agentStub.setupMockOAuthState(serverId, "test-code", "test-state");
+
+      return { agentStub, serverId, callbackBaseUrl };
+    }
+
+    it("should return 302 redirect with Location header on successful OAuth callback", async () => {
+      const { agentStub, serverId, callbackBaseUrl } = await setupOAuthTest({
+        successRedirect: "/dashboard"
+      });
+
+      const response = await agentStub.fetch(
+        new Request(
+          `${callbackBaseUrl}/${serverId}?code=test-code&state=test-state`,
+          { method: "GET", redirect: "manual" }
+        )
+      );
+
+      expect(response.status).toBe(302);
+      expect(response.headers.get("Location")).toBe(
+        "http://example.com/dashboard"
+      );
+    });
+
+    it("should handle relative URLs in successRedirect", async () => {
+      const { agentStub, serverId, callbackBaseUrl } = await setupOAuthTest({
+        successRedirect: "/success",
+        origin: "http://test.local"
+      });
+
+      const response = await agentStub.fetch(
+        new Request(
+          `${callbackBaseUrl}/${serverId}?code=test-code&state=test-state`,
+          { method: "GET", redirect: "manual" }
+        )
+      );
+
+      expect(response.status).toBe(302);
+      expect(response.headers.get("Location")).toBe(
+        "http://test.local/success"
+      );
+    });
+
+    it("should redirect to errorRedirect with error parameter on OAuth failure", async () => {
+      const { agentStub, serverId, callbackBaseUrl } = await setupOAuthTest({
+        errorRedirect: "/error"
+      });
+
+      const response = await agentStub.fetch(
+        new Request(
+          `${callbackBaseUrl}/${serverId}?error=access_denied&state=test-state`,
+          { method: "GET", redirect: "manual" }
+        )
+      );
+
+      expect(response.status).toBe(302);
+      expect(response.headers.get("Location")).toMatch(
+        /^http:\/\/example\.com\/error\?error=/
+      );
+    });
+  });
 });

packages/agents/src/tests/worker.ts

@@ -191,6 +191,14 @@ export class TestOAuthAgent extends Agent<Env> {
     return new Response("Test OAuth Agent");
   }
 
+  // Allow tests to configure OAuth callback behavior
+  configureOAuthForTest(config: {
+    successRedirect?: string;
+    errorRedirect?: string;
+  }): void {
+    this.mcp.configureOAuthCallback(config);
+  }
+
   async setupMockMcpConnection(
     serverId: string,
     _serverName: string,