Skip to content

fix: normalize OAuth redirect URI URL subtypes#2744

Open
he-yufeng wants to merge 1 commit into
modelcontextprotocol:mainfrom
he-yufeng:fix/oauth-redirect-uri-url-subtypes-v2
Open

fix: normalize OAuth redirect URI URL subtypes#2744
he-yufeng wants to merge 1 commit into
modelcontextprotocol:mainfrom
he-yufeng:fix/oauth-redirect-uri-url-subtypes-v2

Conversation

@he-yufeng
Copy link
Copy Markdown

Summary

  • normalize redirect_uris values at the OAuth client metadata boundary
  • keep raw string, AnyUrl, and URL subtype inputs serializing the same way
  • add a regression test for AnyHttpUrl registration followed by AnyUrl redirect validation

To verify

  • .\.venv\Scripts\python.exe -m pytest tests\shared\test_auth.py -q
  • .\.venv\Scripts\python.exe -m ruff check src\mcp\shared\auth.py tests\shared\test_auth.py
  • .\.venv\Scripts\python.exe -m ruff format --check src\mcp\shared\auth.py tests\shared\test_auth.py
  • git diff --check

Refs #2687

@he-yufeng he-yufeng force-pushed the fix/oauth-redirect-uri-url-subtypes-v2 branch from 062b5b4 to ffea4ae Compare May 31, 2026 23:59
Copy link
Copy Markdown

@StantonMatt StantonMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this still misses one input shape Pydantic accepts for this field. redirect_uris is typed as list[AnyUrl], but Pydantic also accepts tuples/sets and coerces them to a list. Because the before validator only normalizes list, a tuple of URL subtypes still stores the element as AnyHttpUrl, so the existing comparison against an incoming AnyUrl can fail:

info = OAuthClientInformationFull(
    client_id="abc123",
    redirect_uris=(AnyHttpUrl("https://example.com/callback"),),
)
info.validate_redirect_uri(AnyUrl("https://example.com/callback"))
# InvalidRedirectUriError: Redirect URI 'https://example.com/callback' not registered for client

I verified the list cases are fixed on ffea4ae; raw-string lists, AnyHttpUrl lists, mixed lists, and single-URI default selection all behave as expected. Local checks also pass:

  • uv run --frozen pytest tests/shared/test_auth.py -q
  • uv run --frozen ruff check src/mcp/shared/auth.py tests/shared/test_auth.py
  • uv run --frozen ruff format --check src/mcp/shared/auth.py tests/shared/test_auth.py
  • uv run --frozen pyright src/mcp/shared/auth.py tests/shared/test_auth.py

Could the normalizer cover the other collection inputs Pydantic accepts here too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants