diff --git a/.changeset/fix-oauth-5xx-discovery.md b/.changeset/fix-oauth-5xx-discovery.md new file mode 100644 index 000000000..0b2e05ae7 --- /dev/null +++ b/.changeset/fix-oauth-5xx-discovery.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Continue OAuth metadata discovery on 502 (Bad Gateway) responses, matching the existing behavior for 4xx. This fixes MCP servers behind reverse proxies that return 502 for path-aware metadata URLs. Other 5xx errors still throw to avoid retrying against overloaded servers. diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 01e17dc0c..93a03ece6 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -1035,7 +1035,9 @@ async function tryMetadataDiscovery(url: URL, protocolVersion: string, fetchFn: * Determines if fallback to root discovery should be attempted */ function shouldAttemptFallback(response: Response | undefined, pathname: string): boolean { - return !response || (response.status >= 400 && response.status < 500 && pathname !== '/'); + if (!response) return true; // CORS error — always try fallback + if (pathname === '/') return false; // Already at root + return (response.status >= 400 && response.status < 500) || response.status === 502; } /** @@ -1062,7 +1064,7 @@ async function discoverMetadataWithFallback( let response = await tryMetadataDiscovery(url, protocolVersion, fetchFn); - // If path-aware discovery fails with 404 and we're not already at root, try fallback to root discovery + // If path-aware discovery fails (4xx or 502 Bad Gateway) and we're not already at root, try fallback to root discovery if (!opts?.metadataUrl && shouldAttemptFallback(response, issuer.pathname)) { const rootUrl = new URL(`/.well-known/${wellKnownType}`, issuer); response = await tryMetadataDiscovery(rootUrl, protocolVersion, fetchFn); @@ -1230,9 +1232,8 @@ export async function discoverAuthorizationServerMetadata( if (!response.ok) { await response.text?.().catch(() => {}); - // Continue looking for any 4xx response code. - if (response.status >= 400 && response.status < 500) { - continue; // Try next URL + if ((response.status >= 400 && response.status < 500) || response.status === 502) { + continue; // Try next URL for 4xx or 502 (Bad Gateway) } throw new Error( `HTTP ${response.status} trying to load ${type === 'oauth' ? 'OAuth' : 'OpenID provider'} metadata from ${endpointUrl}` diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 595ac6598..53263ad8c 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -338,19 +338,38 @@ describe('OAuth Authorization', () => { expect(calls.length).toBe(2); }); - it('throws error on 500 status and does not fallback', async () => { - // First call (path-aware) returns 500 + it('throws on 500 status without fallback', async () => { + // First call (path-aware) returns 500 (overloaded server) mockFetch.mockResolvedValueOnce({ ok: false, status: 500 }); - await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com/path/name')).rejects.toThrow(); + await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com/path/name')).rejects.toThrow('HTTP 500'); const calls = mockFetch.mock.calls; expect(calls.length).toBe(1); // Should not attempt fallback }); + it('falls back to root on 502 status for path URL', async () => { + // First call (path-aware) returns 502 (reverse proxy routing error) + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 502 + }); + + // Root fallback also returns 502 + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 502 + }); + + await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com/path/name')).rejects.toThrow('HTTP 502'); + + const calls = mockFetch.mock.calls; + expect(calls.length).toBe(2); // Should attempt root fallback for 502 + }); + it('does not fallback when the original URL is already at root path', async () => { // First call (path-aware for root) returns 404 mockFetch.mockResolvedValueOnce({ @@ -704,12 +723,54 @@ describe('OAuth Authorization', () => { expect(metadata).toBeUndefined(); }); - it('throws on non-404 errors', async () => { + it('throws on non-404 errors for root URL', async () => { mockFetch.mockResolvedValueOnce(new Response(null, { status: 500 })); await expect(discoverOAuthMetadata('https://auth.example.com')).rejects.toThrow('HTTP 500'); }); + it('falls back to root URL on 502 for path-aware discovery', async () => { + // Path-aware URL returns 502 (reverse proxy has no route for well-known path) + mockFetch.mockResolvedValueOnce(new Response(null, { status: 502 })); + + // Root fallback URL succeeds + mockFetch.mockResolvedValueOnce(Response.json(validMetadata, { status: 200 })); + + const metadata = await discoverOAuthMetadata('https://auth.example.com/tenant1', { + authorizationServerUrl: 'https://auth.example.com/tenant1' + }); + + expect(metadata).toEqual(validMetadata); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); + + it('does not fall back on non-502 5xx for path-aware discovery', async () => { + // Path-aware URL returns 500 (overloaded server — should not retry) + mockFetch.mockResolvedValueOnce(new Response(null, { status: 500 })); + + await expect( + discoverOAuthMetadata('https://auth.example.com/tenant1', { + authorizationServerUrl: 'https://auth.example.com/tenant1' + }) + ).rejects.toThrow('HTTP 500'); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + it('throws when root fallback also returns error for path-aware discovery', async () => { + // Path-aware URL returns 502 (gateway error — triggers fallback) + mockFetch.mockResolvedValueOnce(new Response(null, { status: 502 })); + + // Root fallback also returns 503 + mockFetch.mockResolvedValueOnce(new Response(null, { status: 503 })); + + await expect( + discoverOAuthMetadata('https://auth.example.com/tenant1', { + authorizationServerUrl: 'https://auth.example.com/tenant1' + }) + ).rejects.toThrow('HTTP 503'); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); + it('validates metadata schema', async () => { mockFetch.mockResolvedValueOnce( Response.json( @@ -862,13 +923,49 @@ describe('OAuth Authorization', () => { expect(metadata).toEqual(validOpenIdMetadata); }); - it('throws on non-4xx errors', async () => { + it('continues on 502 and tries next URL', async () => { + // First URL (OAuth) returns 502 (reverse proxy with no route) mockFetch.mockResolvedValueOnce({ ok: false, - status: 500 + status: 502, + text: async () => '' + }); + + // Second URL (OIDC) succeeds + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => validOpenIdMetadata + }); + + const metadata = await discoverAuthorizationServerMetadata('https://auth.example.com'); + + expect(metadata).toEqual(validOpenIdMetadata); + expect(mockFetch).toHaveBeenCalledTimes(2); + }); + + it('throws on non-502 5xx errors', async () => { + mockFetch.mockResolvedValueOnce({ + ok: false, + status: 500, + text: async () => '' }); - await expect(discoverAuthorizationServerMetadata('https://mcp.example.com')).rejects.toThrow('HTTP 500'); + await expect(discoverAuthorizationServerMetadata('https://auth.example.com')).rejects.toThrow('HTTP 500'); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + it('returns undefined when all URLs fail with 502', async () => { + // All URLs return 502 + mockFetch.mockResolvedValue({ + ok: false, + status: 502, + text: async () => '' + }); + + const metadata = await discoverAuthorizationServerMetadata('https://auth.example.com/tenant1'); + + expect(metadata).toBeUndefined(); }); it('handles CORS errors with retry (browser)', async () => {