Skip to content

Validate OAuth issuers and registration metadata - #293

Open
EhsanAzish80 wants to merge 2 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/july-oauth-issuer-registration
Open

EhsanAzish80 wants to merge 2 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/july-oauth-issuer-registration

Conversation

@EhsanAzish80

Copy link
Copy Markdown

Checks OAuth issuer metadata and callback issuers before using an authorization code. It also sends the client application type during registration and starts a new registration when the authorization server changes. Related to #231 and #242.

Tests: 553 Swift tests passed locally. AI was used to write the tests. I ran them locally.

Copy link
Copy Markdown

The issuer check still accepts Authorization Server Metadata that omits issuer entirely.

fetchAuthorizationServerMetadata() only rejects when:

if let issuer = asMetadata.issuer,
   issuer.absoluteString != candidateServer.absoluteString

so a document with no issuer is returned as valid. RFC 8414 defines issuer as REQUIRED metadata and requires it to be identical to the issuer identifier used to construct the well-known request.

That matters even more in this PR because later authorization-response validation falls back to selectedAuthorizationServer when asMetadata.issuer is nil, which can make an invalid metadata document look internally consistent instead of failing at discovery.

Could discovery reject a missing issuer as well as a mismatched issuer, with a regression for a 200 metadata document that has endpoints but no issuer?

AI-assisted review; checked the current PR head against RFC 8414 issuer requirements before posting.

@EhsanAzish80

Copy link
Copy Markdown
Author

Fixed the missing issuer case in c5374db. A 200 metadata response without an issuer now fails discovery. All 554 Swift tests passed locally. AI helped write the new test; I ran it. Could a maintainer review this and approve the CI workflow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants