fix(auth): preserve OAuth discovery transport errors#1028
Conversation
github-actions
Bot
added
T-core
| let response = self | ||
| .discovery_get(discovery_url) | ||
| .await | ||
| .map_err(|error| Self::discovery_failed(discovery_url, error))?; |
There was a problem hiding this comment.
Previously, if one candidate failed, the code moved on to the next well-known URL. Now it returns Err, and the existing ? in try_discover_oauth_server's candidate loop propagates the error and skips all remaining candidates. Is this intended?
There was a problem hiding this comment.
Yeah, I think so. What's happening is that now we immediately bubble up errors during the request itself or if the response returns a 5xx (the latter is converted to an error in the new discovery_request method). The other status codes still return the same response and fall into the next checks for whether that response is a 200, 403, or 404.
I feel this is a good compromise, where if requesting one of the candidates returns an unexpected error, we still surface that to the user, but something like 200, 403, or 404 we know how to handle.
2 participants
Motivation and Context
OAuth discovery currently treats network and server failures as missing metadata, which can incorrectly make a server appear not to support OAuth even when the root cause could be a client-side issue or a server-side server error.
This change propagates such errors to clients so that they can be displayed. I think it aligns more with how this SDK in other languages (I checked Go) behave.
How Has This Been Tested?
Currently, in Codex, adding Linear MCP server in a node:22-bookworm-slim container without ca-certificates installed shows the Linear MCP server as being unavailable even though the root cause is missing CA certificates on the system. With this change, the error is now clearer (even though Codex needs some change too in order to surface it properly).
Breaking Changes
N/A
Types of changes
Checklist
Additional context