Skip to content

confused deputy fix in example + supporting sdk changes - #648

Closed
pcarleton wants to merge 8 commits into
mainfrom
pcarleton/auth-confused-deputy
Closed

confused deputy fix in example + supporting sdk changes#648
pcarleton wants to merge 8 commits into
mainfrom
pcarleton/auth-confused-deputy

Conversation

@pcarleton

@pcarletonpcarleton commented May 7, 2025

Copy link
Copy Markdown
Member

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:

  • make the has_consent functions optional (currently every provider needs to implement them)
  • allow overriding consent handler (currently always uses the default one)
  • consider keeping consent handler abstract in the SDK, and putting example impl into the example directory

Here's what the consent screen looks like:
CleanShot 2025-05-07 at 11 19 05@2x

How Has This Been Tested?

Tested using inspector to run through auth flow

  • w/ consent required
  • w/o consent required

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

@pcarletonpcarleton mentioned this pull request May 7, 2025
@pcarleton
pcarletonforce-pushed the pcarleton/auth-confused-deputy branch from 061739d to bdea6ddCompareMay 7, 2025 10:13

try:
# Check if client has already consented
has_consent = await self.provider.has_client_consent(client)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

todo: check client_consent_required before doing this

Comment threadsrc/mcp/server/auth/provider.py Outdated
"""
...

async def has_client_consent(self, client: OAuthClientInformationFull) -> bool:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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.

Base automatically changed from ihrpr/auth-example to mainMay 7, 2025 16:52
@pcarleton
pcarletonforce-pushed the pcarleton/auth-confused-deputy branch from ede25d3 to 48eb13dCompareMay 9, 2025 11:21
@felixweinbergerfelixweinberger added needs more work Not ready to be merged yet, needs additional follow-up from the author(s). needs publish labels Sep 5, 2025

@felixweinbergerfelixweinberger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@maxisbey
maxisbey deleted the pcarleton/auth-confused-deputy branch March 17, 2026 19:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs more workNot ready to be merged yet, needs additional follow-up from the author(s).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pcarleton@felixweinberger@maxisbey