-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Description
Question
Question
Does the current validate_redirect_uri()
implementation in OAuthClientMetadata
fully comply with OAuth 2.1 Section 7.12.2 requirement that "Authorization servers MUST take precautions to prevent redirecting user agents to untrusted URI's"?
Current Implementation Analysis
File: src/mcp/shared/auth.py
# Lines 82-91: validate_redirect_uri()
def validate_redirect_uri(self, redirect_uri: AnyUrl | None) -> AnyUrl:
if redirect_uri is not None:
# Validate redirect_uri against client's registered redirect URIs
if redirect_uri not in self.redirect_uris:
raise InvalidRedirectUriError(f"Redirect URI '{redirect_uri}' not registered for client")
return redirect_uri
elif len(self.redirect_uris) == 1:
return self.redirect_uris[0]
else:
raise InvalidRedirectUriError("redirect_uri must be specified when client has multiple registered URIs")
Current Security Measures
- ✅ Validates redirect URI against client's registered URIs
- ✅ Uses Pydantic's AnyUrl for basic URL format validation
- ✅ Handles single vs. multiple registered URIs correctly
Key Security Concern
What if the registered URIs themselves are untrusted?
The current implementation only checks if a redirect URI is in the client's registered list, but does not validate whether those registered URIs are themselves trustworthy. For example:
- A client could register
http://evil.com/callback
as a valid redirect URI - The current validation would allow this redirect
- This could lead to open redirect attacks or phishing
Potential Security Gaps
- ❌ No validation of URI protocol (HTTP vs HTTPS)
- ❌ No domain whitelist validation
- ❌ No STRONG restrictions protection against open redirect attacks?
- ❌ No validation of URI path components
- ❌ No check for internal/local network redirects
- ❌ No validation that registered URIs themselves are trustworthy
Question
Is the current implementation sufficient for OAuth 2.1 Section 7.12.2 compliance, or should additional security measures be added to prevent redirection to untrusted URIs beyond just checking against registered URIs?
P.S. I am not very confident that it is a real issue, which seems just a "implementation choice". I will be very appreciative if you maintainers, have any time to review it and tell me your opinion.
Additional Context
No response