-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Validates registered redirect_uris for DCR are a secure schema with no fragments #3206
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,32 @@ | |
| from mcp.shared.inbound import MCP_PROTOCOL_VERSION_HEADER | ||
|
|
||
|
|
||
| def validate_redirect_uri(url: AnyHttpUrl): | ||
| """Validate a registered redirect_uri for DCR. | ||
|
|
||
| RFC 9700 section 4.1.1 and RFC 7591 section 2 require HTTPS for | ||
| redirect_uris, with an HTTP loopback exception for local development. | ||
|
|
||
| Args: | ||
| url: The redirect URI to validate. | ||
|
|
||
| Raises: | ||
| ValueError: If the redirect URI uses an unsafe scheme or contains | ||
| a fragment. | ||
| """ | ||
| if url.scheme != "https" and url.host not in ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Non-HTTPS schemes on a loopback hostname are still accepted. For example, Prompt for AI agents |
||
| "localhost", | ||
| "127.0.0.1", | ||
| "[::1]", | ||
| ): | ||
| raise ValueError( | ||
| "Redirect URI must use HTTPS (or HTTP loopback for local development)" | ||
| ) | ||
|
|
||
| if url.fragment is not None: | ||
| raise ValueError("Redirect URI must not contain a fragment") | ||
|
|
||
|
|
||
| def validate_issuer_url(url: AnyHttpUrl): | ||
| """Validate that the issuer URL meets OAuth 2.0 requirements. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import pytest | ||
| from pydantic import AnyHttpUrl | ||
|
|
||
| from mcp.server.auth.routes import build_metadata, validate_issuer_url | ||
| from mcp.server.auth.routes import build_metadata, validate_issuer_url, validate_redirect_uri | ||
| from mcp.server.auth.settings import AuthSettings, ClientRegistrationOptions, RevocationOptions | ||
|
|
||
|
|
||
|
|
@@ -70,3 +70,43 @@ def test_build_metadata_serves_issuer_without_trailing_slash(): | |
| assert served["issuer"] == "https://as.example.com" | ||
| assert served["authorization_endpoint"] == "https://as.example.com/authorize" | ||
| assert served["token_endpoint"] == "https://as.example.com/token" | ||
|
|
||
| def test_validate_redirect_uri_https_allowed(): | ||
| validate_redirect_uri(AnyHttpUrl('https://example.com/cb')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_http_localhost_allowed(): | ||
| validate_redirect_uri(AnyHttpUrl('http://localhost:3000/cb')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_http_127_0_0_1_allowed(): | ||
| validate_redirect_uri(AnyHttpUrl('http://127.0.0.1:8080/cb')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_http_ipv6_loopback_allowed(): | ||
| validate_redirect_uri(AnyHttpUrl('http://[::1]:9090/cb')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_javascript_scheme_rejected(): | ||
| with pytest.raises(ValueError, match='Redirect URI must use HTTPS'): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: test_validate_redirect_uri_javascript_scheme_rejected will fail: AnyHttpUrl('javascript:alert(1)') raises a pydantic ValidationError at construction time — before validate_redirect_uri is even called — with message "URL scheme should be 'http' or 'https'", which does not match the expected pattern 'Redirect URI must use HTTPS'. Either pass a mock or skip the AnyHttpUrl wrapper for non-HTTP scheme inputs. Prompt for AI agents |
||
| validate_redirect_uri(AnyHttpUrl('javascript:alert(1)')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_file_scheme_rejected(): | ||
| with pytest.raises(ValueError, match='Redirect URI must use HTTPS'): | ||
| validate_redirect_uri(AnyHttpUrl('file:///etc/passwd')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_http_non_loopback_rejected(): | ||
| with pytest.raises(ValueError, match='Redirect URI must use HTTPS'): | ||
| validate_redirect_uri(AnyHttpUrl('http://evil.com/cb')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_fragment_rejected(): | ||
| with pytest.raises(ValueError, match='Redirect URI must not contain a fragment'): | ||
| validate_redirect_uri(AnyHttpUrl('https://example.com/cb#frag')) | ||
|
|
||
|
|
||
| def test_validate_redirect_uri_empty_fragment_rejected(): | ||
| with pytest.raises(ValueError, match='Redirect URI must not contain a fragment'): | ||
| validate_redirect_uri(AnyHttpUrl('https://example.com/cb#')) | ||
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.
P1: Importing the auth routes now fails with a circular import:
routesimportsRegistrationHandlerbefore it definesvalidate_redirect_uri, while this new module-level import asks for that symbol from the partially initializedroutesmodule. Move the shared validator to a module that neither side imports (or otherwise break the top-level cycle) so route construction and the route tests can load.Prompt for AI agents