fix: surface MCP OAuth errors to browser instead of silently swallowing

[Muhammad-Bin-Ali]

Surface MCP OAuth errors instead of swallowing them

Previously, when an OAuth callback failed (e.g. user denied access, invalid state, missing code), handleCallbackRequest would throw an unhandled exception that bubbled up as a raw 500 in the browser with no useful information.

Problem

handleCallbackRequest mixed two patterns: returning MCPOAuthCallbackResult objects for some paths and throwing exceptions for others. The thrown errors were never caught by the Agent's callback handler, so they became opaque 500s. OAuth errors from the authorization server (like access_denied) also weren't routed through failConnection(), leaving the connection stuck in "authenticating" instead of properly transitioning to "failed".

What changed

• Extracted a validateCallbackRequest() method in MCPClientManager that returns structured results instead of throwing. Validation errors with an active connection now route through failConnection() so state properly transitions to "failed".
• The default OAuth callback handler redirects back to the origin instead of rendering a hardcoded HTML error page. Auth errors are surfaced to clients via WebSocket broadcast (onMcpUpdate / server.error).
• MCPOAuthCallbackResult and MCPClientOAuthResult types updated so serverId is optional on error results.
• Updated the mcp-client example to handle auth failures in customHandler and display errors inline in the UI.

---

Generated by OpenCode

[changeset-bot]

🦋 Changeset detected

Latest commit: e01ff7a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
agents	Patch

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]

Claude Code Review

Issues Found:

1. XSS vulnerability in React error display (examples/mcp-client/src/client.tsx:173)

   • {server.error} is rendered without sanitization
   • If server.error contains malicious HTML/scripts, React will render it as text (safe), but the error message originates from OAuth providers and could potentially contain unsafe content
   • Consider using dangerouslySetInnerHTML with a sanitizer if HTML is needed, or ensure error messages are plain text

2. Potential double-escaping issue (packages/agents/src/index.ts:3313, 3321)

   • escapeHtml() is called on both result.authError and baseOrigin
   • The PR description acknowledges this: "escapeHtml is applied at both data and presentation layers, causing potential double-escaping"
   • While safe, this could result in displaying HTML entities instead of readable text (e.g., < instead of <)
   • The PR notes this as a future consideration, which is reasonable

3. Missing HTML sanitization in example (examples/mcp-client/src/server.ts:22)

   • ${error} is directly interpolated into HTML without escaping
   • While error is assigned from result.authError || "Unknown error", there's no guarantee about its content
   • Should use escapeHtml() or equivalent: ${escapeHtml(error)}

Testing:

• Excellent test coverage: 7 new integration tests covering the full config × outcome matrix
• Tests verify proper error surfacing, HTTP status codes, and all configuration scenarios

Overall:
The core fix is solid - properly catching thrown errors and returning meaningful HTTP responses instead of raw 500s. However, there are XSS concerns in the example code that should be addressed before merging.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/agents@850

commit: e01ff7a

[whoiskatrin]

cc @mattzcarey

[whoiskatrin]

/bonk review this PR

[ask-bonk]

Now I have a complete picture. Here's my review:

---

PR Review: fix: surface MCP OAuth errors to browser instead of silently swallowing

Overall Assessment

This is a well-motivated fix. The core problem -- OAuth errors being silently swallowed via a redirect to origin -- is a real usability issue, and the approach of catching thrown errors + rendering a visible error page is the right one. The test coverage is thorough. There are a few issues worth addressing before merge.

Issues

1. XSS in the example customHandler (High)

examples/mcp-client/src/server.ts:22 -- the error variable is interpolated directly into HTML via a template literal:

const error = result.authError || "Unknown error";
// ...
<p style="color:#666;">${error}</p>

result.authError can originate from external OAuth providers (the error_description query parameter). While escapeHtml is applied at the data layer in mcp/client.ts:740, the example should not rely on that implementation detail. If the escaping in client.ts were ever removed or bypassed, this becomes a direct XSS sink. The example should apply its own escapeHtml() call here since it's constructing raw HTML. Examples get copy-pasted.

2. Confirmed double-escaping (Medium)

The PR description acknowledges this, but to be concrete about the chain:

• mcp/client.ts:740: throw new Error(escapeHtml(errorDescription || error))
• index.ts:3313: const safeError = escapeHtml(result.authError || "Unknown error")

If the OAuth provider returns error_description=token<expired, the user will see token<expired on the default error page. This is safe but visually broken. The fix is straightforward: remove escapeHtml from mcp/client.ts:740 (the data layer) and keep it only in index.ts:3313 (the presentation layer). Error messages stored in result.authError should be plain text; escaping belongs at render time.

3. broadcastMcpServers() called regardless of success/failure (Low, Nit)

index.ts:3256 calls this.broadcastMcpServers() unconditionally. On the error path from the catch block (lines 3235-3244), the server state hasn't actually changed -- the error was that we couldn't even find the connection. Broadcasting in this case is harmless but unnecessary. Consider gating it on result.authSuccess || !err or just leave as-is if you want the broadcast to surface the error state to connected clients.

Good things

• The try/catch in handleMcpOAuthCallback (index.ts:3231-3244) cleanly converts thrown errors into structured MCPClientOAuthResult objects, preventing raw 500s. The serverId extraction from the state parameter is a reasonable fallback.
• The default error page (index.ts:3314-3328) is simple, informative, and uses proper escapeHtml on both the error message and the origin URL.
• The handleOAuthCallbackResponse method (index.ts:3268-3333) has a clear priority chain: customHandler > errorRedirect > default HTML page > successRedirect > redirect to origin. Easy to reason about.
• The example customHandler (server.ts:7-33) now properly inspects result.authSuccess instead of blindly closing the popup. The auto-close-after-3s pattern for errors is a nice UX touch.
• The React client (client.tsx:165-175) rendering server.error on failure state is a good addition. React's JSX {server.error} auto-escapes, so no XSS risk there despite the Claude review comment.
• Test coverage is excellent: 7 new tests covering no-config, successRedirect-only, errorRedirect, customHandler, missing in-memory connection, success redirect to origin -- essentially the full config x outcome matrix.

Summary

The core fix is solid and the test coverage is thorough. The main actionable item is the XSS in the example's customHandler (issue 1). The double-escaping (issue 2) is a real bug that will produce garbled error messages for certain OAuth errors and should also be fixed, ideally by moving escaping to the presentation layer only.

github run

[claude]

Potential XSS vulnerability: baseOrigin is being HTML-escaped here but it comes from new URL(request.url).origin which should already be safe. However, the double-escaping could cause display issues. More importantly, if this is truly necessary for security, it suggests baseOrigin might contain user-controlled data, which would be concerning.

Consider either:

1. Remove the escaping if baseOrigin is guaranteed safe (recommended)
2. Or document why user input could affect the origin

[claude]

State parsing could be fragile: The code assumes state?.split(".")[1] will always contain the server ID, but if the state format changes or is malformed, this could break. Consider adding validation or using a more robust parsing method.

const serverId = (() => {
  try {
    const parts = state?.split(".");
    return parts && parts.length > 1 ? parts[1] : "unknown";
  } catch {
    return "unknown";
  }
})();

[claude]

Missing HTML escaping in error display: The error message is being directly interpolated into HTML without escaping, which could lead to XSS if the error message contains user-controlled content.

const safeError = escapeHtml(error);

You'll need to import escapeHtml from 'escape-html' at the top of the file.

[claude]

Review Summary

This PR effectively fixes the critical issue of silently swallowed MCP OAuth errors. The implementation is solid with comprehensive test coverage.

Key improvements:

• Proper error handling with try/catch around handleCallbackRequest
• Default HTML error page instead of silent redirects
• Comprehensive test matrix covering all config scenarios
• Proper changeset included

Issues to address:

• Security: XSS vulnerability in mcp-client example (see inline comment)
• Robustness: State parsing could be more defensive (see inline comment)
• Clarification: Double HTML-escaping of baseOrigin may be unnecessary (see inline comment)

The core logic and architecture align well with project conventions. Once the security issue in the example is fixed, this will be ready to merge.

[mattzcarey]

/bonk review this. Understand the previous review comments and explain the fixes. Explain the design requirements of this fix

[ask-bonk]

Now I have a complete picture. Here's my review:

---

Review: fix/surface-oauth-errors-to-browser

What the PR does

This PR fixes a real bug where MCP OAuth callback errors were silently swallowed, producing opaque 500 errors in the browser with no useful information.

The original problem

On main, handleCallbackRequest in packages/agents/src/mcp/client.ts:695 used throw new Error(...) for early validation failures (no state, no serverId, no server in DB, no in-memory connection). These throws were never caught by handleMcpOAuthCallback in packages/agents/src/index.ts:3589, which called handleCallbackRequest without a try/catch. The result: unhandled exceptions bubbled up as raw 500s. OAuth provider errors (like access_denied) were also thrown inside the try/catch but only routed through failConnection() after passing state validation first, meaning the connection stayed stuck in "authenticating" if validation hadn't passed yet.

What changed and how it addresses the reviews

1. Extracted validateCallbackRequest() — returns structured results instead of throwing

packages/agents/src/mcp/client.ts:693-761 — A new private method that performs all upfront validation (state presence, serverId extraction, OAuth error param, code presence, server existence, in-memory connection check) and returns a discriminated union { valid: true, serverId, code, state } | { valid: false, serverId?, error }. This is the core architectural fix: no more thrown exceptions for expected error paths.

handleCallbackRequest at client.ts:763 now consumes this validation result. When validation fails and there's an active in-memory connection (client.ts:767-768), it routes through failConnection(), which properly transitions the connection state to "failed" and sets connectionError. This means the error is surfaced to WebSocket clients via the onServerStateChanged event and broadcastMcpServers().

2. Moved OAuth error/error_description check into validation

Previously at client.ts:751-754 (main), the OAuth provider error check (if (error) { throw ... }) sat inside the try/catch after authProvider.checkState(). The fix moves it to client.ts:721-727 in validateCallbackRequest(), before any auth provider interaction. This is correct — if the OAuth provider says access_denied, there's no need to validate state first.

3. Removed the hardcoded HTML error page from the default path

The previous review (bonk) flagged that the default error page approach had double-escaping issues. The current code at index.ts:3666-3668 just redirects back to origin for all default error cases. The comment explains that errors are surfaced via WebSocket broadcast (onMcpUpdate / server.error), not a server-rendered HTML page. This eliminates both the XSS concern and the double-escaping problem the reviewers called out.

4. MCPOAuthCallbackResult and MCPClientOAuthResult types updated

client.ts:53 — serverId is now optional on error results (serverId?: string). This is necessary because some validation failures (no state, unparseable state) genuinely can't identify which server the callback was for.

client.ts:104-106 — MCPClientOAuthResult is now a proper discriminated union with authSuccess: true | false instead of a flat { authSuccess: boolean; authError?: string }. This gives customHandler authors type-safe branching on result.authSuccess.

5. Example updated to handle auth failures

examples/mcp-client/src/server.ts:7-20 — The customHandler now inspects result.authSuccess and returns a plain-text error response (status 400) on failure instead of blindly returning window.close().

examples/mcp-client/src/client.tsx:167-177 — The React client renders server.error when server.state === "failed". React's JSX {server.error} auto-escapes, so there's no XSS risk here.

How it addresses previous review issues

Review issue	Resolution
XSS in example customHandler (bonk #1, claude)	The <script> tag approach was removed from the error path. Error is now plain text: Authentication Failed: ${error}. No HTML interpolation of the error.
Double-escaping (bonk #2)	The hardcoded HTML error page was removed entirely. Default error path is a redirect, so there's no presentation-layer escaping needed. escapeHtml remains only at the data layer in validateCallbackRequest (client.ts:725) for the OAuth error params, which is the single correct place for it.
broadcastMcpServers() called unconditionally (bonk #3)	Not addressed — still called unconditionally at index.ts:3612. This is harmless; it just means an extra broadcast on error paths where server state hasn't changed.
mattzcarey: don't show error in script tag	Fixed — error path no longer uses <script> tags.
mattzcarey: prefer react hook for error access	Fixed — client.tsx:167-177 renders server.error from the MCP server state, which comes through the WebSocket/react hook path.
mattzcarey: put try/catch inside handleCallbackRequest	Fixed — handleCallbackRequest (client.ts:763-825) now handles all its own errors internally. The validation returns structured results; the post-validation try/catch at client.ts:781-824 catches any remaining exceptions and routes them through failConnection(). No exceptions escape to index.ts.
claude: state parsing fragility	Partially addressed — extractServerIdFromState is called in validateCallbackRequest (client.ts:712) and the result is checked (client.ts:713-719). The split is simple (state.split(".")[1]), and a missing serverId returns a structured error instead of throwing.

Remaining concern

escapeHtml at the data layer (client.ts:725): escapeHtml(errorDescription || error) is applied to the OAuth error string before it's stored in the result. This means authError contains HTML-escaped text. If a developer renders this in a context that also escapes (like React JSX, or another escapeHtml call), the user sees < instead of <. The convention should be: store raw text, escape at render time. The current code is safe but may produce garbled display for certain error messages. This is the one remaining item from bonk's review (#2) that wasn't fully resolved — the escaping was moved to validation instead of being removed entirely.

Design requirements

The fix satisfies these design constraints:

1. No unhandled exceptions from OAuth callbacks — All paths return structured MCPOAuthCallbackResult objects.
2. Proper state machine transitions — Errors with an active connection transition to "failed" via failConnection(), not left stuck in "authenticating".
3. Errors surfaced via WebSocket — broadcastMcpServers() pushes the failed state (including server.error) to all connected clients, so React/hook-based UIs can display errors without relying on server-rendered HTML.
4. Configurable response handling — The priority chain (customHandler > errorRedirect > successRedirect > redirect to origin) gives developers flexibility while providing a safe default.
5. Examples are copy-paste safe — The example customHandler uses plain text for errors, and the React client uses JSX auto-escaping.

github run