Implement RFC 9207 issuer validation in ClientOAuthProvider#1605
Implement RFC 9207 issuer validation in ClientOAuthProvider#1605mikekistler wants to merge 7 commits into
Conversation
| /// the redirect URI callback and return them in an <see cref="AuthorizationResult"/>. | ||
| /// </para> | ||
| /// </remarks> | ||
| public Func<Uri, Uri, CancellationToken, Task<AuthorizationResult?>>? AuthorizationCallbackHandler { get; set; } |
There was a problem hiding this comment.
Given that this is going to be new in new major version, 2.0, I'd almost take a breaking change to AuthorizationRedirectDelegate over a mutually exclusive new callback. If we like the new name better, maybe just Obsolete the AuthorizationRedirectDelegate property and type?
Also, while I usually lean towards liking Funcs over custom delegate types, I think it's useful when there's multiple parameters of the same type like Uri. I'm also wondering if we shouldn't take a context object instead to avoid future breaking changes. Then maybe we could stick with the Func.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@tarekgh @PranavSenthilnathan I updated this to make the breaking API change I suggested in my earlier comment. Let me know what you think. |
Resolves the semantic merge conflict: main's step-up scope tests used the removed AuthorizationRedirectDelegate API. Migrated them to the new AuthorizationCallbackHandler / AuthorizationCallbackContext API.
|
|
||
| return metadata; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
These validations (the issuer mismatch here, plus the authorization_endpoint checks above) throw McpException via ThrowFailedToHandleUnauthorizedResponse, but they run inside the try, so this catch swallows them and moves on to the next well-known endpoint. A real RFC 8414 issuer mismatch is a security signal and should surface to the caller rather than being logged and retried against other endpoints.
Suggest letting it propagate, e.g. add a typed catch (McpException) { throw; } ahead of this catch, or move the metadata validation out after the loop.
…elegate The AuthorizationRedirectDelegate type and ClientOAuthOptions.AuthorizationRedirectDelegate property were removed in favor of the new AuthorizationCallbackHandler API. Add baseline suppressions (CP0001/CP0002) so the Release pack's package validation against 1.3.0 passes.
| /// <param name="authServerMetadata">The authorization server metadata containing the expected issuer.</param> | ||
| private void ValidateIssuerResponse(string? iss, AuthorizationServerMetadata authServerMetadata) | ||
| { | ||
| var expectedIssuer = authServerMetadata.Issuer?.OriginalString; |
There was a problem hiding this comment.
nit: expectedIssuer can be null when the server metadata omits issuer. If the server advertises iss support (or returns an iss without advertising it), the comparison below runs against null and always fails with a confusing expected issuer '' message. Worth handling the null-metadata-issuer case explicitly.
| // so we never reach this point with resourceUri == null in non-legacy flows. | ||
| if (resourceUri is not null && | ||
| metadata.Issuer is not null && | ||
| !string.Equals(metadata.Issuer.OriginalString, authServerUri.OriginalString, StringComparison.Ordinal)) |
There was a problem hiding this comment.
nit: OriginalString + Ordinal is spec-correct for RFC 8414's exact match, but comparing raw original strings can reject otherwise-compliant servers over trailing-slash, case, or percent-encoding differences. Worth considering light normalization, or at least a note about the strictness.
| { | ||
| var expectedIssuer = authServerMetadata.Issuer?.OriginalString; | ||
|
|
||
| if (authServerMetadata.AuthorizationResponseIssParameterSupported) |
There was a problem hiding this comment.
nit: iss validation is gated entirely on the server advertising AuthorizationResponseIssParameterSupported. A mix-up authorization server can skip validation by omitting both the advertisement and the iss value. This is inherent to RFC 9207, but a short comment documenting the limitation would help.
| @@ -492,14 +511,28 @@ private async Task<string> InitiateAuthorizationCodeFlowAsync( | |||
| var codeChallenge = GenerateCodeChallenge(codeVerifier); | |||
|
|
|||
| var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge); | |||
There was a problem hiding this comment.
nit (pre-existing, out of scope): the authorization URL built here carries no state parameter, which is the usual CSRF/binding defense for the redirect. Not introduced by this PR, but flagging since it is adjacent to this auth work.
@halter73 I have left a few comments, but in general LGTM. |
4 participants
Summary
Implements SEP-2468 — RFC 9207 issuer (
iss) parameter validation in the OAuth authorization flow.Closes #1571
Changes
AuthorizationResult— New class that returns both the authorization code and the validated issuer URI from the authorization redirect.ClientOAuthProvider— Validates theissparameter in authorization responses per RFC 9207, and validates that the authorization server metadataissuerfield matches the expected URI per RFC 8414 Section 3.3.AuthorizationRedirectDelegate— Updated signature to returnAuthorizationResult(containing issuer) instead of just a string code.AuthorizationServerMetadata— AddedIssuerproperty.ClientOAuthOptions— Updated to accommodate the new authorization result type.AuthorizationResultreturn type.RFC 9207 Behavior
issparameter in the authorization response, the client validates it matches the expected authorization server issuer.issueris validated against the expected URI per RFC 8414 Section 3.3.Known Issue
The
auth/2025-03-26-oauth-metadata-backcompatconformance test currently fails because it expects the client to tolerate an issuer mismatch in legacy metadata discovery. This is an intentional strictness choice per RFC 8414 — we may need to relax validation for this specific backward-compatibility scenario depending on spec discussion.