-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(auth): add token issuer validation for MCP spec compliance #1447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helps with catching incorrect tokens sent to the MCP Server, preventing a class of MITIM attack. This will only help with JWT-based tokens, leaving the issue unsolved for opaque tokens. A more complete solution would involve e.g. requiring AS's send an ?issuer=
param on their callback, and checking that. However, that would require a specification change, so this is still an improvement in the interim.
So, overall 👍 to this approach.
I left a few comments, and then please add a test, both for JWT tokens, opaque tokens, and an opaque token that has .
's but is not a JWT.
return False | ||
|
||
# If no expected issuer is configured, behave as before | ||
if not getattr(self, "expected_issuer", None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not getattr(self, "expected_issuer", None): | |
if not self.expected_issuer: |
no need for get_attr
here afaict
logger.exception("Failed to validate token issuer") | ||
return False | ||
|
||
def _token_issuer_matches(self, token: str) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather we use a library for JWT parsing rather than do it partially here, e.g. PyJWT
@pcarleton Thanks for the feedback. I’ll update to use direct access for expected_issuer, switch to PyJWT for parsing, and add the requested tests for JWT and opaque tokens. I’ll push the changes and update the PR soon. |
Summary
This PR implements token issuer validation in the MCP Python SDK client to ensure compliance with the MCP specification requirement:
Previously, the SDK only checked for token existence and expiration.
This change adds verification that tokens were issued by the expected authorization server before they are used.
What I Changed
auth.py
expected_issuer: str | None = None
toOAuthContext
.is_token_valid()
to:expected_issuer
is not set, preserve the original behavior.expected_issuer
is set, decode the access token (assuming JWT format), extract theiss
claim, and verify that it matchesself.expected_issuer
.False
for missing/mismatched issuers or parsing errors.Why This Is Safe and Backward-Compatible
expected_issuer
is not provided, behavior is identical to previous versions.is_token_valid
and one private helper were modified.False
return value (fail closed).Validation Performed
iss
claims.is_token_valid()
correctly respects token expiry and issuer matching.🔗 Related Issue
Closes #1442
Type of Change