fix: escape html in external oauth error message#841
Conversation
🦋 Changeset detectedLatest commit: b636122 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🦋 Changeset detectedLatest commit: c3ff8bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Claude Code ReviewSecurity Fix: XSS Prevention in OAuth Error Messages This PR addresses a critical XSS vulnerability in OAuth error handling. The implementation is solid with good test coverage. Issues Found:
Strengths:
Minor:
|
commit: |
|
happy with this now :) |
…ion 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.
Updates documentation to reflect security improvements in OAuth error handling: - Remove MCPClientOAuthResult from customHandler signature (no longer receives error info) - Document new `error` field in MCPServer type that stores connection errors - Update examples to display errors from connection state instead of script alerts - Add note that error messages are automatically escaped to prevent XSS attacks - Simplify customHandler examples to only close popup (errors handled separately) Related to cloudflare/agents#841 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
| status: 200 | ||
| }); | ||
| } else { | ||
| const safeError = JSON.stringify(result.authError || "Unknown error"); |
There was a problem hiding this comment.
remove this hot garbage
| export class MCPClientConnection { | ||
| client: Client; | ||
| connectionState: MCPConnectionState = MCPConnectionState.CONNECTING; | ||
| connectionError: string | null = null; |
There was a problem hiding this comment.
add this string as a first class thing which can be bubbled up
| @@ -1,4 +1,5 @@ | |||
| import type { Client } from "@modelcontextprotocol/sdk/client/index.js"; | |||
| import escapeHtml from "escape-html"; | |||
There was a problem hiding this comment.
this library is already in the bundle as a transient dep so thats handy
| /** | ||
| * Result of an OAuth callback request | ||
| */ | ||
| export type MCPOAuthCallbackResult = |
There was a problem hiding this comment.
this type didnt exist but it was nice to define it
| ); | ||
| } | ||
|
|
||
| private failConnection( |
There was a problem hiding this comment.
this standardises what happens when we have a connection so we know which one to mark as failed. extracting this logic means I dont have to repeat it for the 3 occasions this happens in
| @@ -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}`); | |||
| } | |||
There was a problem hiding this comment.
in these cases we cant get a connection id to mark the connection as failed so not much choice other to error out. In future this might change.
|
|
||
| const authProvider = conn.options.transport.authProvider; | ||
| authProvider.serverId = serverId; | ||
| try { |
There was a problem hiding this comment.
from here we have a connection id so any errors are wrapped in this try catch and we can fail the connection gracefully.
| } | ||
| if (error) { | ||
| // Escape external OAuth error params to prevent XSS | ||
| throw new Error(escapeHtml(errorDescription || error)); |
There was a problem hiding this comment.
the big baddie xss
| // Already authenticated - just return success | ||
| if ( | ||
| conn.connectionState === MCPConnectionState.READY || | ||
| conn.connectionState === MCPConnectionState.CONNECTED | ||
| ) { | ||
| this.clearServerAuthUrl(serverId); | ||
| return { serverId, authSuccess: true }; | ||
| } |
There was a problem hiding this comment.
happy path making sure to clear the auth url. Maybe we also need to clear the auth error also but it shouldnt be set.
| await authProvider.consumeState(state); | ||
| await conn.completeAuthorization(code); | ||
| await authProvider.deleteCodeVerifier(); | ||
| this.clearServerAuthUrl(serverId); | ||
| conn.connectionError = null; | ||
| this._onServerStateChanged.fire(); |
There was a problem hiding this comment.
other happy path.
| return { serverId, authSuccess: true }; | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| return this.failConnection(serverId, message); |
| }); | ||
|
|
||
| it("should throw error for callback without code or error", async () => { | ||
| it("should fail connection for callback without code or error", async () => { |
There was a problem hiding this comment.
this makes sense since we can tie it to a connection id. This would be the result of a server misbehaving so we wouldnt want to get stuck in authentication
| }); | ||
|
|
||
| it("should error when callback received for connection in failed state", async () => { | ||
| it("should fail connection when callback received for connection in failed state", async () => { |
There was a problem hiding this comment.
same here, developer should manually flip the connection state if they want to initiate a retry
2 participants
No description provided.