fix: continue OAuth metadata discovery on 5xx responses#1632
fix: continue OAuth metadata discovery on 5xx responses#1632pcarleton merged 5 commits intomodelcontextprotocol:mainfrom
Conversation
`discoverAuthorizationServerMetadata()` threw immediately on 5xx responses instead of trying the next discovery URL. This breaks MCP servers behind reverse proxies that return 502/503 for well-known paths they don't route. Change both `discoverAuthorizationServerMetadata()` and `shouldAttemptFallback()` to treat any non-OK response as "try next" instead of only 4xx. Fixes modelcontextprotocol#1631
🦋 Changeset detectedLatest commit: 973f787 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Hi — just checking in. CI is all green (build, tests across Node 20/22/24, conformance, pkg-publish) and the changeset is included. Is there anything else needed from my side, or any design concerns with the approach? Happy to adjust. |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward resilience fix for metadata discovery behind reverse proxies, with good test coverage.
Extended reasoning...
Overview
This PR modifies three functions in packages/client/src/client/auth.ts: shouldAttemptFallback() is broadened from response.status >= 400 && response.status < 500 to !response.ok, discoverMetadataWithFallback() gets an updated comment, and discoverAuthorizationServerMetadata() is simplified by removing the 4xx-specific branch so all non-OK responses trigger continue to the next discovery URL. The test file adds 5 new/updated test cases covering 5xx scenarios for both the current and deprecated discovery paths.
Security risks
The change only affects OAuth metadata discovery — the process of finding the correct well-known endpoint URLs. It does not touch token handling, credential storage, authentication decisions, or authorization logic. Making discovery more resilient (try more URLs before giving up) does not weaken any security boundary. There are no injection vectors, auth bypasses, or data exposure risks introduced.
Level of scrutiny
While this file is in the auth module, the specific functions modified are metadata discovery helpers. The change is a net simplification (removing a branching condition), is well-tested, and follows patterns already established in the codebase (e.g., CORS error handling already returns undefined when all URLs fail). Low scrutiny is appropriate.
Other factors
- Both reported bugs are minor: one is a stale JSDoc on a deprecated function, the other is a pre-existing unconsumed response body issue that predates this PR.
- CI is green per the author's comment and the changeset is included.
- The PR has been open for 3 weeks with no design objections from maintainers.
packages/client/src/client/auth.ts
Outdated
| function shouldAttemptFallback(response: Response | undefined, pathname: string): boolean { | ||
| return !response || (response.status >= 400 && response.status < 500 && pathname !== '/'); | ||
| return !response || (!response.ok && pathname !== '/'); |
There was a problem hiding this comment.
🟡 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.
|
|
||
| // 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); |
There was a problem hiding this comment.
🟣 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.
|
@claude review |
|
@felixweinberger thx for reviewing. claude reviewed -- lmk if anything is needed form my end |
|
Hi, thanks for this PR. I want to acknowledge this is a pain point, but the intention behind this is to avoid hitting servers that are indicating that they are overloaded by sending a 5xx error. Continuing to probe other endpoints is an issue in that case, so while it causes problems for servers that return 5xx's on paths that are not found, unfortunately it causes worse problems for overloaded servers. nginx and traefik both default to 404's if the route isn't matched. It's possible Azure does route and give back a 502, but can you share what proxy you're using and what config it is? A current workaround is to explicitly specify the metadata URL via the "loadDiscoveryState". This is not ideal for a number of reasons (requires explicit config), but the main fix would be for the gateway to return a 404 on not found. I could potentially support a Would continuing on 502 be helpful for your use case? |
Per maintainer feedback — blanket 5xx fallback could hammer overloaded servers. 502 Bad Gateway is the most common reverse proxy routing error and covers the reported use case (Azure APIM). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Makes sense — 502 carveout works. That covers the main scenario from the issue (Azure Application Gateway returning 502 for unmatched routes). Updated the PR to scope it to 502 only. Same test coverage, just narrowed — added explicit tests for non-502 5xx still throwing. Thx for taking the time to review! |
3 participants
Summary
Fixes #1631
discoverAuthorizationServerMetadata()throws immediately on 5xx responses instead of continuing to the next discovery URL. This breaks MCP servers deployed behind reverse proxies (e.g., Azure Application Gateway, nginx, Traefik) that return 502/503 for well-known paths they don't route.Changes
discoverAuthorizationServerMetadata(): Any non-OK response nowcontinues to the next candidate URL instead of only continuing for 4xx. If all URLs fail, returnsundefined(consistent with existing CORS error handling).shouldAttemptFallback(): Broadened fromresponse.status >= 400 && response.status < 500to!response.ok, so 5xx triggers the root URL fallback in the deprecateddiscoverMetadataWithFallbackpath.discoverMetadataWithFallbackto reflect the broadened condition.Testing
undefined, no throwdiscoverOAuthProtectedResourceMetadataattempts root fallback on 5xxdiscoverOAuthMetadataattempts root fallback on 5xx