fix: continue OAuth metadata discovery on 5xx responses (#1632)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com> Co-authored-by: Paul Carleton <paulcarletonjr@gmail.com>
This commit is contained in:
parent
045c62a1e0
commit
d99f3ee527
3 changed files with 115 additions and 12 deletions
5
.changeset/fix-oauth-5xx-discovery.md
Normal file
5
.changeset/fix-oauth-5xx-discovery.md
Normal file
|
|
@ -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.
|
||||
|
|
@ -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}`
|
||||
|
|
|
|||
|
|
@ -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 () => ''
|
||||
});
|
||||
|
||||
await expect(discoverAuthorizationServerMetadata('https://mcp.example.com')).rejects.toThrow('HTTP 500');
|
||||
// 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://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 () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue