diff --git a/.changeset/tame-camels-greet.md b/.changeset/tame-camels-greet.md new file mode 100644 index 000000000..5f9c1d1c5 --- /dev/null +++ b/.changeset/tame-camels-greet.md @@ -0,0 +1,9 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Don't swallow fetch `TypeError` as CORS in non-browser environments. Network errors +(DNS resolution failure, connection refused, invalid URL) in Node.js and Cloudflare +Workers now propagate from OAuth discovery instead of being silently misattributed +to CORS and returning `undefined`. This surfaces the real error to callers rather +than masking it as "metadata not found." diff --git a/packages/client/package.json b/packages/client/package.json index a8cd73c3b..e205903b8 100644 --- a/packages/client/package.json +++ b/packages/client/package.json @@ -30,8 +30,8 @@ "import": "./dist/shimsWorkerd.mjs" }, "browser": { - "types": "./dist/shimsWorkerd.d.mts", - "import": "./dist/shimsWorkerd.mjs" + "types": "./dist/shimsBrowser.d.mts", + "import": "./dist/shimsBrowser.mjs" }, "node": { "types": "./dist/shimsNode.d.mts", diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 58ec23ddd..bedfd8743 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -1,3 +1,4 @@ +import { CORS_IS_POSSIBLE } from '@modelcontextprotocol/client/_shims'; import type { AuthorizationServerMetadata, FetchLike, @@ -512,7 +513,12 @@ async function authInternal( { resourceMetadataUrl: effectiveResourceMetadataUrl }, fetchFn ); - } catch { + } catch (error) { + // Network failures (DNS, connection refused) surface as TypeError — propagate + // those rather than masking a transient reachability problem. + if (error instanceof TypeError) { + throw error; + } // RFC 9728 not available — selectResourceURL will handle undefined } } @@ -826,18 +832,45 @@ export async function discoverOAuthProtectedResourceMetadata( } /** - * Helper function to handle fetch with CORS retry logic + * Fetch with a retry heuristic for CORS errors caused by custom headers. + * + * In browsers, adding a custom header (e.g. `MCP-Protocol-Version`) triggers a CORS preflight. + * If the server doesn't allow that header, the browser throws a `TypeError` before any response + * is received. Retrying without custom headers often succeeds because the request becomes + * "simple" (no preflight). If the server sends no CORS headers at all, the retry also fails + * with `TypeError` and we return `undefined` so callers can fall through to an alternate URL. + * + * However, `fetch()` also throws `TypeError` for non-CORS failures (DNS resolution, connection + * refused, invalid URL). Swallowing those and returning `undefined` masks real errors and can + * cause callers to silently fall through to a different discovery URL. CORS is a browser-only + * concept, so in non-browser runtimes (Node.js, Workers) a `TypeError` from `fetch` is never a + * CORS error — there we propagate the error instead of swallowing it. + * + * In browsers, we cannot reliably distinguish CORS `TypeError` from network `TypeError` from the + * error object alone, so the swallow-and-fallthrough heuristic is preserved there. */ async function fetchWithCorsRetry(url: URL, headers?: Record, fetchFn: FetchLike = fetch): Promise { try { return await fetchFn(url, { headers }); } catch (error) { - if (error instanceof TypeError) { - // CORS errors come back as TypeError, retry without headers - // We're getting CORS errors on retry too, return undefined - return headers ? fetchWithCorsRetry(url, undefined, fetchFn) : undefined; + if (!(error instanceof TypeError) || !CORS_IS_POSSIBLE) { + throw error; } - throw error; + if (headers) { + // Could be a CORS preflight rejection caused by our custom header. Retry as a simple + // request: if that succeeds, we've sidestepped the preflight. + try { + return await fetchFn(url, {}); + } catch (retryError) { + if (!(retryError instanceof TypeError)) { + throw retryError; + } + // Retry also got CORS-blocked (server sends no CORS headers at all). + // Return undefined so the caller tries the next discovery URL. + return undefined; + } + } + return undefined; } } @@ -1146,7 +1179,13 @@ export async function discoverOAuthServerInfo( if (resourceMetadata.authorization_servers && resourceMetadata.authorization_servers.length > 0) { authorizationServerUrl = resourceMetadata.authorization_servers[0]; } - } catch { + } catch (error) { + // Network failures (DNS, connection refused) surface as TypeError from fetch. Those are + // transient reachability problems, not "server doesn't support PRM" — propagate so the + // caller sees the real error instead of silently falling back to a different auth server. + if (error instanceof TypeError) { + throw error; + } // RFC 9728 not supported -- fall back to treating the server URL as the authorization server } diff --git a/packages/client/src/shimsBrowser.ts b/packages/client/src/shimsBrowser.ts new file mode 100644 index 000000000..bfaa67a2e --- /dev/null +++ b/packages/client/src/shimsBrowser.ts @@ -0,0 +1,13 @@ +/** + * Browser runtime shims for client package + * + * This file is selected via package.json export conditions when running in a browser. + */ +export { CfWorkerJsonSchemaValidator as DefaultJsonSchemaValidator } from '@modelcontextprotocol/core'; + +/** + * Whether `fetch()` may throw `TypeError` due to CORS. Only true in browser contexts + * (including Web Workers / Service Workers). In Node.js and Cloudflare Workers, a + * `TypeError` from `fetch` is always a real network/configuration error. + */ +export const CORS_IS_POSSIBLE = true; diff --git a/packages/client/src/shimsNode.ts b/packages/client/src/shimsNode.ts index 1ad16bedc..00b80abe0 100644 --- a/packages/client/src/shimsNode.ts +++ b/packages/client/src/shimsNode.ts @@ -4,3 +4,10 @@ * This file is selected via package.json export conditions when running in Node.js. */ export { AjvJsonSchemaValidator as DefaultJsonSchemaValidator } from '@modelcontextprotocol/core'; + +/** + * Whether `fetch()` may throw `TypeError` due to CORS. CORS is a browser-only concept — + * in Node.js, a `TypeError` from `fetch` is always a real network/configuration error + * (DNS resolution, connection refused, invalid URL), never a CORS error. + */ +export const CORS_IS_POSSIBLE = false; diff --git a/packages/client/src/shimsWorkerd.ts b/packages/client/src/shimsWorkerd.ts index 14380d41b..f8374597e 100644 --- a/packages/client/src/shimsWorkerd.ts +++ b/packages/client/src/shimsWorkerd.ts @@ -4,3 +4,10 @@ * This file is selected via package.json export conditions when running in workerd. */ export { CfWorkerJsonSchemaValidator as DefaultJsonSchemaValidator } from '@modelcontextprotocol/core'; + +/** + * Whether `fetch()` may throw `TypeError` due to CORS. CORS is a browser-only concept — + * in Cloudflare Workers, a `TypeError` from `fetch` is always a real network/configuration + * error, never a CORS error. + */ +export const CORS_IS_POSSIBLE = false; diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 9d8f5cf6b..8178df906 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -33,10 +33,34 @@ vi.mock('pkce-challenge', () => ({ const mockFetch = vi.fn(); globalThis.fetch = mockFetch; +/** + * fetchWithCorsRetry gates its CORS-swallowing heuristic on the `CORS_IS_POSSIBLE` shim constant. + * Tests run under the Node shim (`false`), so a fetch TypeError is treated as a real network error + * and thrown instead of swallowed. Tests that specifically exercise the browser CORS retry path + * call `withBrowserLikeEnvironment()` to flip the mocked constant to `true`. The `afterEach` hook + * resets it so a failed assertion can't leak the override into later tests. + */ +let mockedCorsIsPossible = false; +vi.mock('@modelcontextprotocol/client/_shims', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + get CORS_IS_POSSIBLE() { + return mockedCorsIsPossible; + } + }; +}); +function withBrowserLikeEnvironment(): void { + mockedCorsIsPossible = true; +} + describe('OAuth Authorization', () => { beforeEach(() => { mockFetch.mockReset(); }); + afterEach(() => { + mockedCorsIsPossible = false; + }); describe('extractWWWAuthenticateParams', () => { it('returns resource metadata url when present', async () => { @@ -131,7 +155,8 @@ describe('OAuth Authorization', () => { expect(url.toString()).toBe('https://resource.example.com/.well-known/oauth-protected-resource'); }); - it('returns metadata when first fetch fails but second without MCP header succeeds', async () => { + it('returns metadata when first fetch fails but second without MCP header succeeds (browser CORS retry)', async () => { + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -159,7 +184,8 @@ describe('OAuth Authorization', () => { expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version'); }); - it('throws an error when all fetch attempts fail', async () => { + it('throws an error when all fetch attempts fail (browser, retry throws non-TypeError)', async () => { + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -177,6 +203,18 @@ describe('OAuth Authorization', () => { expect(mockFetch).toHaveBeenCalledTimes(2); }); + it('throws TypeError immediately in non-browser environments without retrying', async () => { + // In Node.js/Workers, CORS doesn't exist — a TypeError from fetch is a real + // network/config error (DNS failure, connection refused, invalid URL) and + // should propagate rather than being silently swallowed. + mockFetch.mockImplementation(() => Promise.reject(new TypeError('getaddrinfo ENOTFOUND resource.example.com'))); + + await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com')).rejects.toThrow(TypeError); + + // Only one call — no CORS retry attempted + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + it('throws on 404 errors', async () => { mockFetch.mockResolvedValueOnce({ ok: false, @@ -348,7 +386,8 @@ describe('OAuth Authorization', () => { expect(url.toString()).toBe('https://resource.example.com/.well-known/oauth-protected-resource'); }); - it('falls back when path-aware discovery encounters CORS error', async () => { + it('falls back when path-aware discovery encounters CORS error (browser)', async () => { + withBrowserLikeEnvironment(); // First call (path-aware) fails with TypeError (CORS) mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -560,7 +599,8 @@ describe('OAuth Authorization', () => { expect(url.toString()).toBe('https://auth.example.com/.well-known/oauth-authorization-server'); }); - it('falls back when path-aware discovery encounters CORS error', async () => { + it('falls back when path-aware discovery encounters CORS error (browser)', async () => { + withBrowserLikeEnvironment(); // First call (path-aware) fails with TypeError (CORS) mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -591,7 +631,8 @@ describe('OAuth Authorization', () => { }); }); - it('returns metadata when first fetch fails but second without MCP header succeeds', async () => { + it('returns metadata when first fetch fails but second without MCP header succeeds (browser CORS retry)', async () => { + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -619,7 +660,8 @@ describe('OAuth Authorization', () => { expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version'); }); - it('throws an error when all fetch attempts fail', async () => { + it('throws an error when all fetch attempts fail (browser, retry throws non-TypeError)', async () => { + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -637,7 +679,8 @@ describe('OAuth Authorization', () => { expect(mockFetch).toHaveBeenCalledTimes(2); }); - it('returns undefined when both CORS requests fail in fetchWithCorsRetry', async () => { + it('returns undefined when both CORS requests fail in fetchWithCorsRetry (browser)', async () => { + withBrowserLikeEnvironment(); // fetchWithCorsRetry tries with headers (fails with CORS), then retries without headers (also fails with CORS) // simulating a 404 w/o headers set. We want this to return undefined, not throw TypeError mockFetch.mockImplementation(() => { @@ -827,7 +870,8 @@ describe('OAuth Authorization', () => { await expect(discoverAuthorizationServerMetadata('https://mcp.example.com')).rejects.toThrow('HTTP 500'); }); - it('handles CORS errors with retry', async () => { + it('handles CORS errors with retry (browser)', async () => { + withBrowserLikeEnvironment(); // First call fails with CORS mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -883,7 +927,8 @@ describe('OAuth Authorization', () => { }); }); - it('returns undefined when all URLs fail with CORS errors', async () => { + it('returns undefined when all URLs fail with CORS errors (browser)', async () => { + withBrowserLikeEnvironment(); // All fetch attempts fail with CORS errors (TypeError) mockFetch.mockImplementation(() => Promise.reject(new TypeError('CORS error'))); @@ -894,6 +939,18 @@ describe('OAuth Authorization', () => { // Verify that all discovery URLs were attempted expect(mockFetch).toHaveBeenCalledTimes(6); // 3 URLs × 2 attempts each (with and without headers) }); + + it('throws TypeError in non-browser environments instead of silently falling through (network failure)', async () => { + // In Node.js, a TypeError from fetch is a real error (DNS/connection), not CORS. + // Swallowing it and returning undefined would cause the caller to silently fall + // through to the next discovery URL, masking the actual network failure. + mockFetch.mockImplementation(() => Promise.reject(new TypeError('getaddrinfo ENOTFOUND auth.example.com'))); + + await expect(discoverAuthorizationServerMetadata('https://auth.example.com/tenant1')).rejects.toThrow(TypeError); + + // Only one call — no CORS retry attempted in non-browser environments + expect(mockFetch).toHaveBeenCalledTimes(1); + }); }); describe('discoverOAuthServerInfo', () => { @@ -1006,6 +1063,15 @@ describe('OAuth Authorization', () => { // Verify the override URL was used instead of the default well-known path expect(mockFetch.mock.calls[0]![0].toString()).toBe(overrideUrl.toString()); }); + + it('propagates network failures instead of silently falling back (non-browser)', async () => { + // PRM discovery hits a DNS/connection failure. That's a transient reachability problem, + // not "server doesn't support RFC 9728" — the caller should see the real error rather + // than silently falling back to treating the MCP server URL as the auth server. + mockFetch.mockImplementation(() => Promise.reject(new TypeError('getaddrinfo ENOTFOUND resource.example.com'))); + + await expect(discoverOAuthServerInfo('https://resource.example.com')).rejects.toThrow(TypeError); + }); }); describe('auth with provider authorization server URL caching', () => { diff --git a/packages/client/tsdown.config.ts b/packages/client/tsdown.config.ts index 5ab07eecd..c6b89247a 100644 --- a/packages/client/tsdown.config.ts +++ b/packages/client/tsdown.config.ts @@ -3,7 +3,7 @@ import { defineConfig } from 'tsdown'; export default defineConfig({ // 1. Entry Points // Directly matches package.json include/exclude globs - entry: ['src/index.ts', 'src/shimsNode.ts', 'src/shimsWorkerd.ts'], + entry: ['src/index.ts', 'src/shimsNode.ts', 'src/shimsWorkerd.ts', 'src/shimsBrowser.ts'], // 2. Output Configuration format: ['esm'],