Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
📚 Documentation preview
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93f3bfcda9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
|
|
||
| redirect_uris: list[AnyUrl] | None = None | ||
| redirect_uris: list[Annotated[AnyUrl, AfterValidator(AnyUrl)]] | None = None |
There was a problem hiding this comment.
When a caller constructs this model with an AnyHttpUrl and later passes that same AnyHttpUrl to validate_redirect_uri(), this validator stores the registered value as an exact AnyUrl while the method compares the argument unchanged. Because Pydantic URL equality requires matching concrete classes, the identical URI is now rejected; before this change both values remained AnyHttpUrl and validation succeeded. Normalize the supplied URI before the membership check, or compare canonical URI values.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/mcp/shared/auth.py— nit, pre-existing: a server author who passes an AnyHttpUrl (or any other AnyUrl subclass) to validate_redirect_uri still gets InvalidRedirectUriError for a registered URI. The PR normalizes only the registered side; the incoming redirect_uri at src/mcp/shared/auth.py:191 is compared within, and pydantic's URL eq requires identical classes. Fix: normalize both sides of the comparison, e.g.redirect_uri = AnyUrl(redirect_uri)before the membership test (or comparestr()forms), so the match no longer depends on which AnyUrl subclass either side carries. The SDK's own callers in src/mcp/server/auth/handlers/authorize.py:109 and :176 pass plain AnyUrl, so only external callers hit this.Why this was flagged
A custom OAuth server implementation (outside this repository) calls OAuthClientInformationFull.validate_redirect_uri with an AnyHttpUrl instance, for example built from its own request model typed AnyHttpUrl. At src/mcp/shared/auth.py:191 the check is
redirect_uri not in self.redirect_uris; after this PR every element of self.redirect_uris is a plain AnyUrl, and pydantic 2.12's _BaseUrl.eq returns False when the two classes differ, so the membership test fails and InvalidRedirectUriError is raised for a URI that is registered. The base branch fails the same way for this input, so this is pre-existing; the PR fixes only the stored-side mismatch (issue #2687) and leaves the argument-side mismatch in place. The in-repo callers at src/mcp/server/auth/handlers/authorize.py:109 (via _ANY_URL_ADAPTER) and :176 (AuthorizationRequest.redirect_uri typed AnyUrl) pass plain AnyUrl and are unaffected; no other callers of validate_redirect_uri exist in the repository, so the affected population could not be enumerated from this checkout.Verification: pre-existing. Trigger: code outside the SDK calls
validate_redirect_uriwith anAnyUrlsubclass such asAnyHttpUrl. The PR'sAfterValidator(AnyUrl)normalizes only the stored list; the membership check at src/mcp/shared/auth.py:191 uses pydantic's_BaseUrl.__eq__, which requires class identity, soInvalidRedirectUriErroris raised for a registered URI. On the base branch an incomingAnyHttpUrlalready failed by the same route.
| # Pydantic retains URL subclasses, whose equality differs from AnyUrl. | ||
| redirect_uris: list[Annotated[AnyUrl, AfterValidator(AnyUrl)]] | None = Field(..., min_length=1) |
There was a problem hiding this comment.
🟡 (optional) MCP client authors who built OAuthClientMetadata with AnyHttpUrl elements 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 plain AnyUrl at construction. Pydantic URL equality checks the exact class, so client_metadata.redirect_uris[0] == AnyHttpUrl(url) or AnyHttpUrl(url) in client_metadata.redirect_uris fails after the upgrade. Fix: compare both sides after normalising to AnyUrl or to str, 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-side validate_redirect_uri input and does not fix client-side comparisons. [also at: src/mcp/shared/auth.py:147 - Authorization-server providers that compare params.redirect_uri against their own AnyHttpUrl values now get False where the base branch gave True. src/mcp/shared/auth.py:147 coerces every registered element to plain AnyUrl, and src/mcp/shared/auth.py:195 returns self.redirect_uris[0] when the client omits redirect_uri, so the AuthorizationParams.redirect_uri handed to authorize() 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 the AnyHttpUrl instance unchanged (list[AnyUrl] with pydantic's isinstance pass-through). After src/mcp/shared/auth.py:119 the AfterValidator(AnyUrl) replaces it with a plain AnyUrl. In the application's redirect callback handler a check such as if received_url == self.client_metadata.redirect_uris[0] or if AnyHttpUrl(received) in self.client_metadata.redirect_uris returns 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 as AnyUrl but 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 an AnyHttpUrl element was retained as AnyHttpUrl. After the change, src/mcp/shared/auth.py:119 and :147 wrap each element in AfterValidator(AnyUrl), producing a plain AnyUrl. Pydantic implements _BaseUrl.__eq__ as class-exact. Hence AnyHttpUrl(u) in m.redirect_uris is True on base and False on HEAD.
| ) -> 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)])} |
There was a problem hiding this comment.
🟡 nit (optional): the new tests type the validation payload as dict[str, object], which the repo's test-quality bar forbids when a real type exists. The values are a list/tuple of AnyHttpUrl plus a str client_id, so a concrete value type is available. Fix: give data its real value type (e.g. dict[str, Sequence[AnyHttpUrl] | str]) or build the dict inline per model so no widened annotation is needed, which covers the 2 sites listed. Same instruction at 2 sites (tests/shared/test_auth.py:229, tests/shared/test_auth.py:247).
Why this was flagged
AGENTS.md:68 directs test code to conform to .claude/skills/test-quality/SKILL.md, whose Hygiene section says "In tests, narrow types with assert isinstance; never Any/object when a real type exists." tests/shared/test_auth.py:229 and tests/shared/test_auth.py:247 declare data: dict[str, object] and then insert a container of AnyHttpUrl and a str client_id, all of which have real static types. Nothing fails at runtime; the base branch has no such annotation because these tests are new. The instruction guards against widened annotations hiding type mistakes in tests that pyright strict would otherwise catch.
Verification: nit. SKILL.md:101-102 reads "never Any/object when a real type exists." The diff adds two annotations that use object where a concrete type is available: tests/shared/test_auth.py:229 and tests/shared/test_auth.py:247. A real type such as dict[str, Sequence[AnyHttpUrl] | str] exists. Nothing fails at runtime: model_validate accepts Any.
| # Pydantic retains URL subclasses, whose equality differs from AnyUrl. | ||
| redirect_uris: list[Annotated[AnyUrl, AfterValidator(AnyUrl)]] | None = Field(..., min_length=1) |
There was a problem hiding this comment.
🟡 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 redirect_uris on the public OAuthClientMetadata and OAuthClientInformationFull models (visible in model_fields/pyright) and changes runtime behaviour: an AnyHttpUrl (or other AnyUrl subclass) element is now coerced to plain AnyUrl instead of being retained. Fix: confirm this is the intended, maintainer-sanctioned 2.x behaviour change (it fixes #2687) and record it as such; otherwise keep the field type and normalize only inside validate_redirect_uri when comparing. …
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 AnyHttpUrl instances in redirect_uris and later relied on isinstance(uri, AnyHttpUrl), on type(uri), or on the field's static type will see plain AnyUrl instead after this change; the serialized URI string is unchanged, and the PR author (a maintainer) states the change is intentional and links Fixes #2687 for the mismatch in validate_redirect_uri. Consequence is small and arguably a bug fix, but the instruction asks that such a behaviour change be an explicit maintainer decision rather than incidental.
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."
This branch has not been deployed
1 participant
Summary
AnyUrlin both OAuth registration models.Fixes #2687
Checks
./scripts/test(100% coverage; strict-no-cover passed)uv run --frozen pyrightuv run --frozen ruff check src/mcp/shared/auth.py tests/shared/test_auth.pyAI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.