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: 10bc7f5

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: 10bc7f5

[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.