confused deputy fix in example + supporting sdk changes #648
Conversation
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
Merged
061739d to
bdea6dd
Compare
pcarleton
commented
May 7, 2025
| @@ -218,6 +219,26 @@ async def error_response( | |||
| ) | |||
|
|
|||
| try: | |||
| # Check if client has already consented | |||
| has_consent = await self.provider.has_client_consent(client) | |||
There was a problem hiding this comment.
todo: check client_consent_required before doing this
src/mcp/server/auth/provider.py
Outdated
| @@ -174,6 +174,27 @@ async def authorize( | |||
| """ | |||
| ... | |||
|
|
|||
| async def has_client_consent(self, client: OAuthClientInformationFull) -> bool: | |||
There was a problem hiding this comment.
todo: if client_consent_required, these need to be required, but otherwise they should be optional. open to suggestions here. i think a default implementation of return True means we'd solve for being optional, but not for required.
ede25d3 to
48eb13d
Compare
felixweinberger
requested changes
Sep 5, 2025
There was a problem hiding this comment.
Hey @pcarleton just checking whether we're still planning to ship this? Marking it as "needs publish" for now so we can easily filter it from views.
I also assume we might still need some merge conflict resolution here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
See the confused deputy consideration here:
https://github.com/modelcontextprotocol/modelcontextprotocol/blob/c255349bd98d9dc73f3579c828d195bd09c0bee8/docs/specification/draft/basic/security_best_practices.mdx#21-confused-deputy-problem
In the example, we front the Github API, so we need to put a consent screen for each new client registration, otherwise the Github consent screen will be skipped for new clients, creating a path for token leakage.
Still TODO:
Here's what the consent screen looks like:
How Has This Been Tested?
Tested using inspector to run through auth flow
Breaking Changes
Types of changes
Checklist
Additional context