Skip to content

Conversation

chipgpt
Copy link

@chipgpt chipgpt commented Oct 10, 2025

Updates the OAuth authorization flow to prefer to use the token_endpoint_auth_method result from the Dynamic Client Registration endpoint, if provided.

#951

Motivation and Context

When using dynamic client registration, the registration endpoint may return the token_endpoint_auth_method value to be used when exchanging tokens for access tokens. The current oauth implementation ignores this field and only uses the methods from the oauth authorization server metadata.

How Has This Been Tested?

This has not been tested in a real application.

Breaking Changes

No breaking changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Previous PR #982 is no longer showing changes after rebase for some reason and was automatically closed, reopening new PR

@chipgpt chipgpt requested review from a team as code owners October 10, 2025 17:35
* server.
*/
clientInformation(): OAuthClientInformation | undefined | Promise<OAuthClientInformation | undefined>;
clientInformation(): OAuthClientInformationFull | undefined | Promise<OAuthClientInformationFull | undefined>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type signature change worries me a bit. The OAuthClientInformation is the response from the DCR endpoint, listed here:
https://datatracker.ietf.org/doc/html/rfc7591#section-3.2.1

All the fields are optional except for redirect_uris (which is what I think is what required a bunch of changes in tests).

this means this change will be backwards incompatible for implementations of the provider that directly return the DCR response rather than the full metadata.

saveClientInformation takes the full information type, so this might be a non issue.

to land this, we'll either want to:

  • check around for implementations to see if they commonly return the reduced type, to see the estimated impact of the breaking change
  • OR: make this backwards compatible

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I can make it backwards compatible. My only argument for changing the type is that it seems like the current type is wrong. As you pointed out, saveClientInformation() takes the OAuthClientInformationFull type and is the only way clientInformation() is set.

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