Skip to content

fix: continue OAuth metadata discovery on 5xx responses #1632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pcarleton merged 5 commits into from
Mar 30, 2026
+115 −12
Merged

fix: continue OAuth metadata discovery on 5xx responses #1632

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d98845
fix: continue OAuth metadata discovery on 5xx responses
matantsach Mar 5, 2026
b5b0360
Add changeset for OAuth 5xx discovery fix
matantsach Mar 5, 2026
bdcc107
Merge branch 'main' into fix/1631-oauth-5xx-fallback
felixweinberger Mar 27, 2026
c9a6012
fix: scope discovery fallback to 502 only, not all 5xx
matantsach Mar 30, 2026
973f787
Merge branch 'main' into fix/1631-oauth-5xx-fallback
pcarleton Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-oauth-5xx-discovery.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@modelcontextprotocol/client': patch
---

Continue OAuth metadata discovery on 5xx responses instead of throwing, matching the existing behavior for 4xx. This fixes MCP servers behind reverse proxies that return 502/503 for path-aware metadata URLs.
12 changes: 3 additions & 9 deletions packages/client/src/client/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,8 +1001,8 @@
/**
* 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 !== '/');
return !response || (!response.ok && pathname !== '/');

Check warning on line 1005 in packages/client/src/client/auth.ts

View check run for this annotation

Claude / Claude Code Review

Stale JSDoc on deprecated discoverOAuthMetadata 

The JSDoc for the deprecated `discoverOAuthMetadata()` (lines 1044-1045) says "Any other errors will be thrown as exceptions," but after broadening `shouldAttemptFallback` to `!response.ok`, a 5xx on a path-aware URL now triggers root fallback — and if the root returns 404, the function silently returns `undefined` instead of throwing. The doc should note that non-OK responses on path-aware URLs may trigger fallback before throwing.
Copy link
Copy Markdown

@claude claude Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The JSDoc for the deprecated discoverOAuthMetadata() (lines 1044-1045) says "Any other errors will be thrown as exceptions," but after broadening shouldAttemptFallback to !response.ok, a 5xx on a path-aware URL now triggers root fallback — and if the root returns 404, the function silently returns undefined instead of throwing. The doc should note that non-OK responses on path-aware URLs may trigger fallback before throwing.

Extended reasoning...

The shouldAttemptFallback() function was changed from response.status >= 400 && response.status < 500 to !response.ok, broadening when discoverMetadataWithFallback() attempts root fallback. This creates a new code path in the caller discoverOAuthMetadata() where the JSDoc becomes inaccurate.

The JSDoc at lines 1044-1045 states: "If the server returns a 404 for the well-known endpoint, this function will return undefined. Any other errors will be thrown as exceptions." This implies that non-404 errors (including 5xx) will always throw. However, this is no longer true for path-aware URLs.

Step-by-step proof: Consider calling discoverOAuthMetadata("https://auth.example.com/tenant1", { authorizationServerUrl: "https://auth.example.com/tenant1" }). (1) discoverMetadataWithFallback constructs the path-aware well-known URL and calls tryMetadataDiscovery, which returns a 502 response. (2) shouldAttemptFallback(response, "/tenant1") evaluates !response || (!response.ok && pathname !== "/") → !false || (!false && true) → true. (3) Fallback to root URL /.well-known/oauth-authorization-server is attempted, which returns 404. (4) Back in discoverOAuthMetadata, the response is now the 404 from root. (5) The check !response || response.status === 404 is true, so the function returns undefined. Pre-PR, step 2 would have evaluated response.status >= 400 && response.status < 500 → false for 502, so no fallback would occur, and the function would hit !response.ok → throw new Error("HTTP 502 ...").

The impact is minimal: discoverOAuthMetadata is deprecated in favor of discoverAuthorizationServerMetadata, and the behavioral change (falling back on 5xx) is the intended purpose of this PR. The issue is purely that the documentation was not updated to reflect the new behavior.

The fix is straightforward: update the JSDoc to note that for path-aware URLs, non-OK responses (including 5xx) trigger a fallback to root discovery before throwing, and the function may return undefined if the root also fails with 404.

}

/**
Expand All @@ -1028,11 +1028,11 @@
}

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 (any non-OK status) 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);

Check notice on line 1035 in packages/client/src/client/auth.ts

View check run for this annotation

Claude / Claude Code Review

Response body not consumed before fallback in discoverMetadataWithFallback 

Nit: In `discoverMetadataWithFallback()`, when the fallback path is taken (line 1033-1035), the first response is overwritten without consuming its body. Compare with `discoverAuthorizationServerMetadata()` (line 1199) which correctly calls `response.text?.().catch(() => {})` before continuing. This is a pre-existing issue (same pattern existed for 4xx before this PR), but adding `await response?.text?.().catch(() => {})` before the fallback assignment would be a trivial consistency fix.
Comment on lines 1066 to 1070
Copy link
Copy Markdown

@claude claude Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Nit: In discoverMetadataWithFallback(), when the fallback path is taken (line 1033-1035), the first response is overwritten without consuming its body. Compare with discoverAuthorizationServerMetadata() (line 1199) which correctly calls response.text?.().catch(() => {}) before continuing. This is a pre-existing issue (same pattern existed for 4xx before this PR), but adding await response?.text?.().catch(() => {}) before the fallback assignment would be a trivial consistency fix.

Extended reasoning...

When discoverMetadataWithFallback() performs path-aware discovery and the response triggers a fallback (via shouldAttemptFallback), the code at lines 1031-1035 overwrites the response variable with the root URL response without first consuming the body of the original response:

let response = await tryMetadataDiscovery(url, protocolVersion, fetchFn);
if (!opts?.metadataUrl && shouldAttemptFallback(response, issuer.pathname)) {
    const rootUrl = new URL(`/.well-known/${wellKnownType}`, issuer);
    response = await tryMetadataDiscovery(rootUrl, protocolVersion, fetchFn);
}

This is inconsistent with discoverAuthorizationServerMetadata() at line 1199, which correctly drains the body before moving on: await response.text?.().catch(() => {}). The callers of discoverMetadataWithFallback (e.g., discoverOAuthProtectedResourceMetadata at line 920 and discoverOAuthMetadata at line 1076) do consume the returned response, but the discarded first response is never drained.

Concrete example: An MCP server at https://example.com/tenant1 is behind a reverse proxy that returns 502 for /.well-known/oauth-protected-resource/tenant1. The code calls tryMetadataDiscovery which returns a 502 response. shouldAttemptFallback returns true (since !response.ok && pathname !== "/"). The code then overwrites response with a new fetch to /.well-known/oauth-protected-resource at root — but never calls .text() on the 502 response. The original response body remains unconsumed.

Unconsumed response bodies can prevent HTTP/1.1 connection reuse because the connection cannot be returned to the pool until the body stream is fully read or the connection is closed. In practice, the impact is minimal here: this is a one-time discovery call (not a hot path), well-known endpoints typically return small responses, and modern Node.js runtimes (undici) will eventually GC unconsumed bodies. Still, it is an inconsistency worth fixing for correctness.

This is a pre-existing issue. Before this PR, the same code path was triggered for 4xx responses (the old condition was response.status >= 400 && response.status < 500). The PR only broadens shouldAttemptFallback to !response.ok, extending the same pattern to 5xx. The fix is trivial — add await response?.text?.().catch(() => {}) before the fallback reassignment, matching the pattern used elsewhere in the file.

}

return response;
Expand Down Expand Up @@ -1197,13 +1197,7 @@

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
}
throw new Error(
`HTTP ${response.status} trying to load ${type === 'oauth' ? 'OAuth' : 'OpenID provider'} metadata from ${endpointUrl}`
);
continue; // Try next URL for any non-OK response (4xx, 5xx)
}

// Parse and validate based on type
Expand Down
77 changes: 69 additions & 8 deletions packages/client/test/client/auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,17 +337,23 @@ 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('falls back to root on 500 status for path URL', async () => {
// First call (path-aware) returns 500 (reverse proxy)
mockFetch.mockResolvedValueOnce({
ok: false,
status: 500
});

await expect(discoverOAuthProtectedResourceMetadata('https://resource.example.com/path/name')).rejects.toThrow();
// Root fallback also returns 500
mockFetch.mockResolvedValueOnce({
ok: false,
status: 500
});

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
expect(calls.length).toBe(2); // Should attempt root fallback
});

it('does not fallback when the original URL is already at root path', async () => {
Expand Down Expand Up @@ -703,12 +709,42 @@ 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 5xx 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('throws when root fallback also returns 5xx for path-aware discovery', async () => {
// Path-aware URL returns 502
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(
Expand Down Expand Up @@ -861,13 +897,38 @@ describe('OAuth Authorization', () => {
expect(metadata).toEqual(validOpenIdMetadata);
});

it('throws on non-4xx errors', async () => {
it('continues on 5xx errors and tries next URL', async () => {
// First URL (OAuth) returns 502 (e.g. 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('returns undefined when all URLs fail with 5xx', async () => {
// All URLs return 5xx
mockFetch.mockResolvedValue({
ok: false,
status: 502,
text: async () => ''
});

await expect(discoverAuthorizationServerMetadata('https://mcp.example.com')).rejects.toThrow('HTTP 500');
const metadata = await discoverAuthorizationServerMetadata('https://auth.example.com/tenant1');

expect(metadata).toBeUndefined();
});

it('handles CORS errors with retry (browser)', async () => {
Expand Down
Loading