fix!: preserve OAuth discovery transport errors - #1069
Merged
Conversation
github-actions
Bot
added
T-documentation
9 tasks
Let custom OAuth HTTP clients preserve native error types and source chains while keeping the type-erased boundary explicit in the public API. BREAKING CHANGE: OAuthHttpClientError is now a boxed error alias; custom clients should return native errors with .into() or box them directly.
DaleSeo
force-pushed
the
fix/oauth-discovery-box-error
branch
from
July 28, 2026 14:21
a7918b5 to
86a3c63
Compare
DaleSeo
marked this pull request as ready for review
July 28, 2026 14:25
DaleSeo
changed the title
alexhancock
reviewed
Jul 28, 2026
| @@ -2525,37 +2541,33 @@ impl AuthorizationManager { | |||
| discovery_url.set_fragment(None); | |||
| discovery_url.set_path(&candidate_path); | |||
| if let Some(resource_metadata_url) = | |||
Contributor
There was a problem hiding this comment.
Do we need to handle the error case here?
Member
Author
There was a problem hiding this comment.
The error case is intentionally propagated by ?. I had the same question in #1028 (comment).
alexhancock
approved these changes
Jul 28, 2026
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
OAuth metadata discovery previously converted transport failures into a successful fallback path, hiding actionable causes such as connection and certificate errors. This PR reports those failures with the discovery URL and complete source chain while retaining fallback behavior for valid non-matching discovery responses.
PR #1028 identified this through a concrete failure: using the Linear MCP server from a
node:22-bookworm-slimcontainer without CA certificates made the server appear not to support OAuth, while the underlying TLS error was discarded. This PR retains the original intended semantics: transport failures and HTTP 5xx responses stop discovery and surface the cause, while recognized non-success responses can still fall through to other discovery candidates.Because this work already changes error propagation and v3 is approaching, this is the right point to settle the public API before it becomes stable. Rather than expand the public
OAuthHttpClientErrorwrapper with separate constructors for messages and source errors, it becomes a domain-specific alias forBox<dyn Error + Send + Sync>. This matches existing SDK error aliases, lets custom clients propagate native errors with?, and confines the concrete compatibility wrapper required byoauth2to the private implementation.How Has This Been Tested?
Added coverage
Breaking Changes
OAuthHttpClientErrorchanges from a public struct with anOAuthHttpClientError::newconstructor toBox<dyn Error + Send + Sync>. CustomOAuthHttpClientimplementations should return their native errors with.into()or box them directly.Types of changes
Checklist