From 89de0df0712caab67f079c31fea83b3d1d502217 Mon Sep 17 00:00:00 2001 From: Basil Hosmer Date: Thu, 26 Feb 2026 14:44:24 -0500 Subject: [PATCH 1/2] Don't swallow fetch TypeError as CORS in non-browser environments 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. --- packages/client/src/client/auth.ts | 42 +++++++++++-- packages/client/test/client/auth.test.ts | 79 +++++++++++++++++++++--- 2 files changed, 106 insertions(+), 15 deletions(-) diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index cfba29d85..f7fa6ab58 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -761,18 +761,48 @@ 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 { + // CORS only exists in browsers. In Node.js/Workers/etc., a TypeError from fetch is always a + // real network or configuration error, never CORS. + const corsIsPossible = (globalThis as { document?: unknown }).document !== undefined; 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) || !corsIsPossible) { + 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; } } diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 742dbc143..709425dc1 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -33,6 +33,25 @@ vi.mock('pkce-challenge', () => ({ const mockFetch = vi.fn(); globalThis.fetch = mockFetch; +/** + * fetchWithCorsRetry gates its CORS-swallowing heuristic on running in a browser (where `document` + * exists). CORS doesn't apply in Node.js, so there a fetch TypeError is always a real network error + * and is thrown instead of swallowed. Tests that specifically exercise the CORS retry logic use this + * helper to simulate a browser environment. + */ +function withBrowserLikeEnvironment(): { restore: () => void } { + const g = globalThis as { document?: unknown }; + const had = 'document' in g; + const prev = g.document; + g.document = {}; + return { + restore: () => { + if (had) g.document = prev; + else delete g.document; + } + }; +} + describe('OAuth Authorization', () => { beforeEach(() => { mockFetch.mockReset(); @@ -131,7 +150,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 () => { + const env = withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -157,9 +177,11 @@ describe('OAuth Authorization', () => { // Verify first call had MCP header expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version'); + env.restore(); }); - 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 () => { + const env = withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -175,6 +197,19 @@ describe('OAuth Authorization', () => { // Verify both calls were made expect(mockFetch).toHaveBeenCalledTimes(2); + env.restore(); + }); + + 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 () => { @@ -348,7 +383,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 () => { + const env = withBrowserLikeEnvironment(); // First call (path-aware) fails with TypeError (CORS) mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -377,6 +413,7 @@ describe('OAuth Authorization', () => { expect(lastOptions.headers).toEqual({ 'MCP-Protocol-Version': LATEST_PROTOCOL_VERSION }); + env.restore(); }); it('does not fallback when resourceMetadataUrl is provided', async () => { @@ -560,7 +597,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 () => { + const env = withBrowserLikeEnvironment(); // First call (path-aware) fails with TypeError (CORS) mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -589,9 +627,11 @@ describe('OAuth Authorization', () => { expect(lastOptions.headers).toEqual({ 'MCP-Protocol-Version': LATEST_PROTOCOL_VERSION }); + env.restore(); }); - 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 () => { + const env = withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -617,9 +657,11 @@ describe('OAuth Authorization', () => { // Verify first call had MCP header expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version'); + env.restore(); }); - 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 () => { + const env = withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -635,9 +677,11 @@ describe('OAuth Authorization', () => { // Verify both calls were made expect(mockFetch).toHaveBeenCalledTimes(2); + env.restore(); }); - it('returns undefined when both CORS requests fail in fetchWithCorsRetry', async () => { + it('returns undefined when both CORS requests fail in fetchWithCorsRetry (browser)', async () => { + const env = 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(() => { @@ -648,6 +692,7 @@ describe('OAuth Authorization', () => { // This should return undefined (the desired behavior after the fix) const metadata = await discoverOAuthMetadata('https://auth.example.com/path'); expect(metadata).toBeUndefined(); + env.restore(); }); it('returns undefined when discovery endpoint returns 404', async () => { @@ -827,7 +872,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 () => { + const env = withBrowserLikeEnvironment(); // First call fails with CORS mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -849,6 +895,7 @@ describe('OAuth Authorization', () => { // Second call should not have headers (CORS retry) expect(calls[1]![1]?.headers).toBeUndefined(); + env.restore(); }); it('supports custom fetch function', async () => { @@ -883,7 +930,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 () => { + const env = withBrowserLikeEnvironment(); // All fetch attempts fail with CORS errors (TypeError) mockFetch.mockImplementation(() => Promise.reject(new TypeError('CORS error'))); @@ -893,6 +941,19 @@ describe('OAuth Authorization', () => { // Verify that all discovery URLs were attempted expect(mockFetch).toHaveBeenCalledTimes(6); // 3 URLs × 2 attempts each (with and without headers) + env.restore(); + }); + + 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); }); }); From b8a2e610d4e231924f889f08ef1d7549bf3e6072 Mon Sep 17 00:00:00 2001 From: Basil Hosmer Date: Thu, 26 Feb 2026 15:09:17 -0500 Subject: [PATCH 2/2] Use afterEach to restore document stub in CORS tests 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. --- packages/client/test/client/auth.test.ts | 51 ++++++++++-------------- 1 file changed, 20 insertions(+), 31 deletions(-) diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 709425dc1..7bcc1aaf4 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -36,26 +36,24 @@ globalThis.fetch = mockFetch; /** * fetchWithCorsRetry gates its CORS-swallowing heuristic on running in a browser (where `document` * exists). CORS doesn't apply in Node.js, so there a fetch TypeError is always a real network error - * and is thrown instead of swallowed. Tests that specifically exercise the CORS retry logic use this - * helper to simulate a browser environment. + * and is thrown instead of swallowed. Tests that specifically exercise the CORS retry logic call + * this helper to simulate a browser environment. Cleanup is done by `restoreBrowserLikeEnvironment` + * in an `afterEach` hook so a failed assertion can't leak the `document` stub into later tests. */ -function withBrowserLikeEnvironment(): { restore: () => void } { - const g = globalThis as { document?: unknown }; - const had = 'document' in g; - const prev = g.document; - g.document = {}; - return { - restore: () => { - if (had) g.document = prev; - else delete g.document; - } - }; +function withBrowserLikeEnvironment(): void { + (globalThis as { document?: unknown }).document = {}; +} +function restoreBrowserLikeEnvironment(): void { + delete (globalThis as { document?: unknown }).document; } describe('OAuth Authorization', () => { beforeEach(() => { mockFetch.mockReset(); }); + afterEach(() => { + restoreBrowserLikeEnvironment(); + }); describe('extractWWWAuthenticateParams', () => { it('returns resource metadata url when present', async () => { @@ -151,7 +149,7 @@ describe('OAuth Authorization', () => { }); it('returns metadata when first fetch fails but second without MCP header succeeds (browser CORS retry)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -177,11 +175,10 @@ describe('OAuth Authorization', () => { // Verify first call had MCP header expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version'); - env.restore(); }); it('throws an error when all fetch attempts fail (browser, retry throws non-TypeError)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -197,7 +194,6 @@ describe('OAuth Authorization', () => { // Verify both calls were made expect(mockFetch).toHaveBeenCalledTimes(2); - env.restore(); }); it('throws TypeError immediately in non-browser environments without retrying', async () => { @@ -384,7 +380,7 @@ describe('OAuth Authorization', () => { }); it('falls back when path-aware discovery encounters CORS error (browser)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // First call (path-aware) fails with TypeError (CORS) mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -413,7 +409,6 @@ describe('OAuth Authorization', () => { expect(lastOptions.headers).toEqual({ 'MCP-Protocol-Version': LATEST_PROTOCOL_VERSION }); - env.restore(); }); it('does not fallback when resourceMetadataUrl is provided', async () => { @@ -598,7 +593,7 @@ describe('OAuth Authorization', () => { }); it('falls back when path-aware discovery encounters CORS error (browser)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // First call (path-aware) fails with TypeError (CORS) mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -627,11 +622,10 @@ describe('OAuth Authorization', () => { expect(lastOptions.headers).toEqual({ 'MCP-Protocol-Version': LATEST_PROTOCOL_VERSION }); - env.restore(); }); it('returns metadata when first fetch fails but second without MCP header succeeds (browser CORS retry)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -657,11 +651,10 @@ describe('OAuth Authorization', () => { // Verify first call had MCP header expect(mockFetch.mock.calls[0]![1]?.headers).toHaveProperty('MCP-Protocol-Version'); - env.restore(); }); it('throws an error when all fetch attempts fail (browser, retry throws non-TypeError)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // Set up a counter to control behavior let callCount = 0; @@ -677,11 +670,10 @@ describe('OAuth Authorization', () => { // Verify both calls were made expect(mockFetch).toHaveBeenCalledTimes(2); - env.restore(); }); it('returns undefined when both CORS requests fail in fetchWithCorsRetry (browser)', async () => { - const env = withBrowserLikeEnvironment(); + 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(() => { @@ -692,7 +684,6 @@ describe('OAuth Authorization', () => { // This should return undefined (the desired behavior after the fix) const metadata = await discoverOAuthMetadata('https://auth.example.com/path'); expect(metadata).toBeUndefined(); - env.restore(); }); it('returns undefined when discovery endpoint returns 404', async () => { @@ -873,7 +864,7 @@ describe('OAuth Authorization', () => { }); it('handles CORS errors with retry (browser)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // First call fails with CORS mockFetch.mockImplementationOnce(() => Promise.reject(new TypeError('CORS error'))); @@ -895,7 +886,6 @@ describe('OAuth Authorization', () => { // Second call should not have headers (CORS retry) expect(calls[1]![1]?.headers).toBeUndefined(); - env.restore(); }); it('supports custom fetch function', async () => { @@ -931,7 +921,7 @@ describe('OAuth Authorization', () => { }); it('returns undefined when all URLs fail with CORS errors (browser)', async () => { - const env = withBrowserLikeEnvironment(); + withBrowserLikeEnvironment(); // All fetch attempts fail with CORS errors (TypeError) mockFetch.mockImplementation(() => Promise.reject(new TypeError('CORS error'))); @@ -941,7 +931,6 @@ describe('OAuth Authorization', () => { // Verify that all discovery URLs were attempted expect(mockFetch).toHaveBeenCalledTimes(6); // 3 URLs × 2 attempts each (with and without headers) - env.restore(); }); it('throws TypeError in non-browser environments instead of silently falling through (network failure)', async () => {