Don't swallow fetch TypeError as CORS in non-browser environments#1595
Open
bhosmer-ant wants to merge 2 commits intomainfrom
Open
Don't swallow fetch TypeError as CORS in non-browser environments#1595bhosmer-ant wants to merge 2 commits intomainfrom
bhosmer-ant wants to merge 2 commits intomainfrom
Conversation
fetchWithCorsRetry previously caught all TypeErrors from fetch() and either retried without headers or silently returned undefined. The intent was to work around CORS preflight failures triggered by the custom MCP-Protocol-Version header in browsers. However, fetch() also throws TypeError for network failures (DNS resolution, connection refused, invalid URL). Swallowing those causes OAuth discovery to silently fall through to the next URL, masking the real error and giving users a misleading 'metadata not found' instead of the actual network failure. CORS is a browser-only concept. In Node.js, Workers, and other non-browser runtimes, a TypeError from fetch is never a CORS error. This change gates the CORS retry/swallow heuristic on running in a browser (detected via globalThis.document). In non-browser environments, TypeErrors now propagate immediately so callers see the underlying network failure. In browsers, the existing behavior is preserved: we cannot reliably distinguish CORS TypeError from network TypeError from the error object alone, so the swallow-and-fallthrough heuristic still applies there. Tests that exercise CORS retry logic now stub globalThis.document to simulate a browser environment.
Ensures the browser-environment stub is cleaned up even if a CORS test throws an assertion error before reaching the explicit restore() call, preventing the stubbed document from leaking into subsequent tests.
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
Contributor
|
Hey, thanks for your contribution. For platform specific functionality we should use shims rather than an api heuristic. This works for browser, node and workerd runtimes and is the pattern across the repo.There is an example of this in the MCP server package for validators. On my phone so hard to provide specific links :)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fetchWithCorsRetrypreviously caught allTypeErrors fromfetch()and either retried without headers or silently returnedundefined. The intent was to work around CORS preflight failures triggered by the customMCP-Protocol-Versionheader in browsers.However,
fetch()also throwsTypeErrorfor network failures (DNS resolution, connection refused, invalid URL). Swallowing those causes OAuth discovery to silently fall through to the next URL, masking the real error — users see a misleading "metadata not found" instead of the actual network failure.CORS is a browser-only concept. In Node.js, Cloudflare Workers, and other non-browser runtimes, a
TypeErrorfromfetchis never a CORS error. This PR gates the CORS retry/swallow heuristic on running in a browser (detected viaglobalThis.document). In non-browser environments,TypeErrors now propagate immediately so callers see the underlying network failure.In browsers, the existing behavior is preserved — we cannot reliably distinguish CORS
TypeErrorfrom networkTypeErrorfrom the error object alone, so the swallow-and-fallthrough heuristic still applies there.Motivation and Context
The previous behavior made OAuth discovery failures in Node.js indistinguishable from "endpoint doesn't exist." A DNS outage, connection refused, or firewall block would all appear as if the auth server simply wasn't at that URL. This is bad for debuggability and could also lead callers to fall through to alternate discovery URLs when the intended server is temporarily unreachable.
How Has This Been Tested?
globalThis.documentto simulate a browser environment (viawithBrowserLikeEnvironment()helper, cleaned up inafterEach)TypeErrorpropagates immediately in non-browser environments without any CORS retry attemptsAll 132 auth tests pass, 382 integration tests pass.
Breaking Changes
Behavioral change in non-browser environments:
discoverOAuthProtectedResourceMetadata,discoverOAuthMetadata, anddiscoverAuthorizationServerMetadatawill now throwTypeErroron network failures instead of returningundefined. This surfaces real errors that were previously swallowed. Callers that were relying on the silent fallthrough behavior will now see the underlying network error.In practice this is what users want — they want to know their auth server is unreachable, not silently fall through to a fallback URL.
Types of changes
Checklist
Additional context
Related to #545, #764, #1450 — users debugging auth failures get misleading errors because the underlying network problem is swallowed.
Known limitation: Web Workers and Service Workers run in browser contexts where CORS applies, but
documentis not defined there. With this change, a CORS error in a Worker would throw instead of falling through. OAuth discovery from within a Worker is an unusual use case, and the tradeoff seems right — Workers are more likely to hit genuine network errors than CORS issues with well-known OAuth endpoints.