Repository navigation
Normalize registered OAuth redirect URL types #3682
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 |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| from typing import Any, Literal, cast | ||
| from typing import Annotated, Any, Literal, cast | ||
|
|
||
| from pydantic import AnyHttpUrl, AnyUrl, BaseModel, ConfigDict, Field, field_validator, model_validator | ||
| from pydantic import AfterValidator, AnyHttpUrl, AnyUrl, BaseModel, ConfigDict, Field, field_validator, model_validator | ||
|
|
||
| # RFC 7523 JWT bearer grant; SEP-990 leg 2 uses this to present the ID-JAG. | ||
| JWT_BEARER_GRANT_TYPE = "urn:ietf:params:oauth:grant-type:jwt-bearer" | ||
|
|
@@ -115,7 +115,8 @@ class OAuthClientMetadata(OAuthClientMetadataBase): | |
| job. See https://datatracker.ietf.org/doc/html/rfc7591#section-2 | ||
| """ | ||
|
|
||
| redirect_uris: list[AnyUrl] | None = Field(..., min_length=1) | ||
| # Pydantic retains URL subclasses, whose equality differs from AnyUrl. | ||
| redirect_uris: list[Annotated[AnyUrl, AfterValidator(AnyUrl)]] | None = Field(..., min_length=1) | ||
|
Comment on lines
+118
to
+119
Contributor
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. 🟡 nit (optional): AGENTS.md says v2's public API is a compatibility contract and any change to an existing API's observable behaviour is an explicit maintainer design decision that should generally be avoided. src/mcp/shared/auth.py changes the declared type of Why this was flagged…Same instruction at 2 sites (src/mcp/shared/auth.py:119, src/mcp/shared/auth.py:147). Nothing fails at runtime; this is a compatibility-contract note. Guarded against: unannounced changes to a released 2.x public API. Here, any caller that stored Verification: AGENTS.md (base commit) "Branching Model": "v2 is released; its public API is a compatibility contract for the 2.x line. Removals, renames, or any change to an existing API's signature or observable behaviour ... is a design decision a maintainer makes explicitly, and should generally be avoided." |
||
| # supported auth methods for the token endpoint | ||
| token_endpoint_auth_method: TokenEndpointAuthMethod | None = None | ||
| # supported grant_types of this implementation | ||
|
|
@@ -143,7 +144,7 @@ class OAuthClientInformationFull(OAuthClientMetadataBase): | |
| them against a client's requested `redirect_uri`. | ||
| """ | ||
|
|
||
| redirect_uris: list[AnyUrl] | None = None | ||
| redirect_uris: list[Annotated[AnyUrl, AfterValidator(AnyUrl)]] | None = None | ||
|
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.
When a caller constructs this model with an AGENTS.md reference: AGENTS.md:L14-L18 Useful? React with 👍 / 👎. |
||
| # RFC 7591 §3.2.1: the server may assign an auth method other than the one requested, | ||
| # including methods this SDK does not implement, or omit it. | ||
| token_endpoint_auth_method: str | None = None | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| """Tests for OAuth 2.0 shared code.""" | ||
|
|
||
| import pytest | ||
| from pydantic import AnyUrl, ValidationError | ||
| from pydantic import AnyHttpUrl, AnyUrl, ValidationError, create_model | ||
|
|
||
| from mcp.shared.auth import InvalidRedirectUriError, OAuthClientInformationFull, OAuthClientMetadata, OAuthMetadata | ||
|
|
||
|
|
@@ -218,6 +218,51 @@ def test_client_with_no_registered_redirect_uris_cannot_resolve_a_redirect( | |
| info.validate_redirect_uri(redirect_uri) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("model", [OAuthClientMetadata, OAuthClientInformationFull]) | ||
| @pytest.mark.parametrize("container", [list, tuple]) | ||
| def test_redirect_uri_subtypes_are_stored_as_any_url( | ||
| model: type[OAuthClientMetadata] | type[OAuthClientInformationFull], | ||
| container: type[list[AnyHttpUrl]] | type[tuple[AnyHttpUrl, ...]], | ||
| ) -> None: | ||
| """SDK-defined registration models compare a URL subclass with the incoming base URL.""" | ||
| url = "https://example.com/callback" | ||
| data: dict[str, object] = {"redirect_uris": container([AnyHttpUrl(url)])} | ||
|
Contributor
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. 🟡 nit (optional): the new tests type the validation payload as Why this was flaggedAGENTS.md:68 directs test code to conform to .claude/skills/test-quality/SKILL.md, whose Hygiene section says "In tests, narrow types with Verification: nit. SKILL.md:101-102 reads "never |
||
| if model is OAuthClientInformationFull: | ||
| data["client_id"] = "abc123" | ||
| info = model.model_validate(data, strict=container is list) | ||
|
|
||
| assert info.redirect_uris == [AnyUrl(url)] | ||
| assert info.model_dump(mode="json")["redirect_uris"] == [url] | ||
| if isinstance(info, OAuthClientInformationFull): | ||
| assert info.validate_redirect_uri(AnyUrl(url)) == AnyUrl(url) | ||
| with pytest.raises(InvalidRedirectUriError): | ||
| info.validate_redirect_uri(AnyUrl("https://example.com/other")) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("model", [OAuthClientMetadata, OAuthClientInformationFull]) | ||
| def test_redirect_uri_normalization_preserves_strict_list_validation( | ||
| model: type[OAuthClientMetadata] | type[OAuthClientInformationFull], | ||
| ) -> None: | ||
| """SDK-defined strict parsing still rejects non-list redirect URI containers.""" | ||
| data: dict[str, object] = {"redirect_uris": (AnyHttpUrl("https://example.com/callback"),)} | ||
| if model is OAuthClientInformationFull: | ||
| data["client_id"] = "abc123" | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| model.model_validate(data, strict=True) | ||
|
|
||
|
|
||
| def test_redeclared_redirect_uri_field_keeps_its_own_validation() -> None: | ||
| """SDK-defined URL normalization does not override a subclass's replacement field.""" | ||
|
|
||
| CustomClientInformation = create_model( | ||
| "CustomClientInformation", __base__=OAuthClientInformationFull, redirect_uris=(list[str] | None, None) | ||
| ) | ||
| info = CustomClientInformation.model_validate({"client_id": "abc123", "redirect_uris": ["custom redirect"]}) | ||
|
|
||
| assert info.redirect_uris == ["custom redirect"] | ||
|
|
||
|
|
||
| def test_request_metadata_restricts_application_type_to_the_values_the_sdk_sends(): | ||
| """What the SDK sends stays narrow even though what it accepts back is wide.""" | ||
| with pytest.raises(ValidationError): | ||
|
|
||
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.
🟡 (optional) MCP client authors who built
OAuthClientMetadatawithAnyHttpUrlelements and compare them in their callback handler now get False where the base branch gave True. src/mcp/shared/auth.py:119 rewrites each element to a plainAnyUrlat construction. Pydantic URL equality checks the exact class, soclient_metadata.redirect_uris[0] == AnyHttpUrl(url)orAnyHttpUrl(url) in client_metadata.redirect_urisfails after the upgrade. Fix: compare both sides after normalising toAnyUrlor tostr, and call out in the docs that stored elements no longer equal the subclass instance the caller passed in; the :191 finding only covers the server-sidevalidate_redirect_uriinput and does not fix client-side comparisons. [also at: src/mcp/shared/auth.py:147 - Authorization-server providers that compareparams.redirect_uriagainst their ownAnyHttpUrlvalues now get False where the base branch gave True. src/mcp/shared/auth.py:147 coerces every registered element to plainAnyUrl, and src/mcp/shared/auth.py:195 returnsself.redirect_uris[0]when the client omitsredirect_uri, so theAuthorizationParams.redirect_urihanded toauthorize()changes class.]Why this was flagged
A client application constructs
OAuthClientMetadata(redirect_uris=[AnyHttpUrl("http://localhost:3000/callback")]). On the base branch the model stores theAnyHttpUrlinstance unchanged (list[AnyUrl]with pydantic's isinstance pass-through). After src/mcp/shared/auth.py:119 theAfterValidator(AnyUrl)replaces it with a plainAnyUrl. In the application's redirect callback handler a check such asif received_url == self.client_metadata.redirect_uris[0]orif AnyHttpUrl(received) in self.client_metadata.redirect_urisreturns False because pydantic's_BaseUrl.__eq__requires identical classes. The application rejects its own callback or falls into an error path. docs/client/oauth-clients.md:28 states elements are stored asAnyUrlbut does not warn that equality with the passed subclass instance is lost. Remedy: document the equality change explicitly, or avoid rewriting the element class and instead normalise on the comparison side.Verification: Base declared
redirect_uris: list[AnyUrl] | None, so anAnyHttpUrlelement was retained asAnyHttpUrl. After the change, src/mcp/shared/auth.py:119 and :147 wrap each element inAfterValidator(AnyUrl), producing a plainAnyUrl. Pydantic implements_BaseUrl.__eq__as class-exact. HenceAnyHttpUrl(u) in m.redirect_urisis True on base and False on HEAD.