Fix OAuth redirect URI validation for DCR compatibility (#1661)

This commit is contained in:
Jeremiah Lowin 2025-08-28 15:19:56 -04:00 committed by GitHub
commit 6d9088704e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 92 additions and 49 deletions

View file

@ -164,8 +164,8 @@ The `OAuthProxy` class provides the complete proxy implementation:
<ParamField body="allowed_client_redirect_uris" type="list[str] | None">
List of allowed redirect URI patterns for MCP clients. Patterns support wildcards (e.g., `"http://localhost:*"`, `"https://*.example.com/*"`).
- `None` (default): Only localhost redirect URIs allowed (`http://localhost:*`, `http://127.0.0.1:*`)
- Empty list `[]`: All redirect URIs allowed (not recommended for production)
- `None` (default): All redirect URIs allowed (for MCP/DCR compatibility)
- Empty list `[]`: No redirect URIs allowed
- Custom list: Only matching patterns allowed
These patterns apply to MCP client loopback redirects, NOT the upstream OAuth app redirect URI.
@ -230,29 +230,45 @@ The proxy automatically:
## Client Redirect URI Security
<Warning>
By default, OAuth Proxy only accepts localhost redirect URIs from MCP clients for security. You can customize this with the `allowed_client_redirect_uris` parameter:
<Note>
OAuth Proxy accepts all redirect URIs by default to maintain compatibility with MCP's Dynamic Client Registration (DCR) pattern, where clients register with unpredictable redirect URIs.
If you know which clients will connect, you can restrict redirect URIs using the `allowed_client_redirect_uris` parameter:
```python
# Default: localhost only (secure)
# Default: allow all (for DCR compatibility)
auth = OAuthProxy(...)
# Restrict to localhost only
auth = OAuthProxy(
...,
allowed_client_redirect_uris=[
"http://localhost:*",
"http://127.0.0.1:*"
]
)
# Allow specific known clients (e.g., Claude.ai)
auth = OAuthProxy(
...,
allowed_client_redirect_uris=[
"http://localhost:*",
"https://claude.ai/api/mcp/auth_callback"
]
)
# Custom patterns with wildcards
auth = OAuthProxy(
...,
allowed_client_redirect_uris=[
"http://localhost:*",
"https://app.example.com/auth/*"
"https://*.example.com/auth/*"
]
)
# Allow all (NOT recommended for production)
auth = OAuthProxy(
...,
allowed_client_redirect_uris=[]
)
```
</Warning>
**Tip:** Check your server logs for debug messages that say "Client registered with redirect_uri" messages to see what redirect URIs your clients are using.
</Note>
## Client Compatibility

View file

@ -422,6 +422,15 @@ class OAuthProxy(OAuthProvider):
# Store the ProxyDCRClient using the upstream ID
self._clients[upstream_id] = proxy_client
# Log redirect URIs to help users discover what patterns they might need
if client_info.redirect_uris:
for uri in client_info.redirect_uris:
logger.debug(
"Client registered with redirect_uri: %s - if restricting redirect URIs, "
"ensure this pattern is allowed in allowed_client_redirect_uris",
uri,
)
logger.debug(
"Registered client %s with %d redirect URIs",
upstream_id,

View file

@ -33,8 +33,9 @@ def validate_redirect_uri(
Args:
redirect_uri: The redirect URI to validate
allowed_patterns: List of allowed patterns. If None, defaults to localhost.
If empty list, all URIs are allowed.
allowed_patterns: List of allowed patterns. If None, all URIs are allowed (for DCR compatibility).
If empty list, no URIs are allowed.
To restrict to localhost only, explicitly pass DEFAULT_LOCALHOST_PATTERNS.
Returns:
True if the redirect URI is allowed
@ -44,15 +45,9 @@ def validate_redirect_uri(
uri_str = str(redirect_uri)
# If no patterns specified, default to localhost only
# If no patterns specified, allow all for DCR compatibility
# (clients need to dynamically register with their own redirect URIs)
if allowed_patterns is None:
allowed_patterns = [
"http://localhost:*",
"http://127.0.0.1:*",
]
# Empty list means allow all
if len(allowed_patterns) == 0:
return True
# Check if URI matches any allowed pattern

View file

@ -21,15 +21,15 @@ class MockTokenVerifier(TokenVerifier):
class TestProxyDCRClient:
"""Test ProxyDCRClient redirect URI validation."""
def test_default_localhost_only(self):
"""Test that default configuration only allows localhost."""
def test_default_allows_all(self):
"""Test that default configuration allows all URIs for DCR compatibility."""
client = ProxyDCRClient(
client_id="test",
client_secret="secret",
redirect_uris=[AnyUrl("http://localhost:3000")],
)
# Localhost should be allowed
# All URIs should be allowed by default for DCR compatibility
assert client.validate_redirect_uri(AnyUrl("http://localhost:3000")) == AnyUrl(
"http://localhost:3000"
)
@ -39,11 +39,12 @@ class TestProxyDCRClient:
assert client.validate_redirect_uri(AnyUrl("http://127.0.0.1:3000")) == AnyUrl(
"http://127.0.0.1:3000"
)
# Non-localhost should fallback to base validation
# This will check against registered redirect_uris
with pytest.raises(InvalidRedirectUriError):
client.validate_redirect_uri(AnyUrl("http://example.com"))
assert client.validate_redirect_uri(AnyUrl("http://example.com")) == AnyUrl(
"http://example.com"
)
assert client.validate_redirect_uri(
AnyUrl("https://claude.ai/api/mcp/auth_callback")
) == AnyUrl("https://claude.ai/api/mcp/auth_callback")
def test_custom_patterns(self):
"""Test custom redirect URI patterns."""
@ -65,8 +66,8 @@ class TestProxyDCRClient:
with pytest.raises(InvalidRedirectUriError):
client.validate_redirect_uri(AnyUrl("http://127.0.0.1:3000"))
def test_empty_list_allows_all(self):
"""Test that empty pattern list allows all URIs."""
def test_empty_list_allows_none(self):
"""Test that empty pattern list allows no URIs."""
client = ProxyDCRClient(
client_id="test",
client_secret="secret",
@ -74,10 +75,15 @@ class TestProxyDCRClient:
allowed_redirect_uri_patterns=[],
)
# Everything should be allowed
# Nothing should be allowed (except the pre-registered one via fallback)
# Pre-registered URI should work via fallback to base validation
assert client.validate_redirect_uri(AnyUrl("http://localhost:3000"))
assert client.validate_redirect_uri(AnyUrl("http://example.com"))
assert client.validate_redirect_uri(AnyUrl("https://anywhere.com:9999/path"))
# Non-registered URIs should be rejected
with pytest.raises(InvalidRedirectUriError):
client.validate_redirect_uri(AnyUrl("http://example.com"))
with pytest.raises(InvalidRedirectUriError):
client.validate_redirect_uri(AnyUrl("https://anywhere.com:9999/path"))
def test_none_redirect_uri(self):
"""Test that None redirect URI uses default behavior."""
@ -95,8 +101,8 @@ class TestProxyDCRClient:
class TestOAuthProxyRedirectValidation:
"""Test OAuth proxy with redirect URI validation."""
def test_proxy_default_localhost_validation(self):
"""Test that OAuth proxy defaults to localhost-only validation."""
def test_proxy_default_allows_all(self):
"""Test that OAuth proxy defaults to allowing all URIs for DCR compatibility."""
proxy = OAuthProxy(
upstream_authorization_endpoint="https://auth.example.com/authorize",
upstream_token_endpoint="https://auth.example.com/token",
@ -106,7 +112,7 @@ class TestOAuthProxyRedirectValidation:
base_url="http://localhost:8000",
)
# The proxy should store None for default localhost patterns
# The proxy should store None for default (allow all)
assert proxy._allowed_client_redirect_uris is None
def test_proxy_custom_patterns(self):
@ -126,7 +132,7 @@ class TestOAuthProxyRedirectValidation:
assert proxy._allowed_client_redirect_uris == custom_patterns
def test_proxy_empty_list_validation(self):
"""Test OAuth proxy with empty list (allow all)."""
"""Test OAuth proxy with empty list (allow none)."""
proxy = OAuthProxy(
upstream_authorization_endpoint="https://auth.example.com/authorize",
upstream_token_endpoint="https://auth.example.com/token",

View file

@ -66,21 +66,20 @@ class TestValidateRedirectUri:
assert validate_redirect_uri(None, [])
assert validate_redirect_uri(None, ["http://localhost:*"])
def test_default_localhost_patterns(self):
"""Test default localhost-only patterns when None is provided."""
# Localhost patterns should be allowed by default
def test_default_allows_all(self):
"""Test that None (default) allows all URIs for DCR compatibility."""
# All URIs should be allowed when None is provided (DCR compatibility)
assert validate_redirect_uri("http://localhost:3000", None)
assert validate_redirect_uri("http://127.0.0.1:8080", None)
assert validate_redirect_uri("http://example.com", None)
assert validate_redirect_uri("https://app.example.com", None)
assert validate_redirect_uri("https://claude.ai/api/mcp/auth_callback", None)
# Non-localhost should be rejected by default
assert not validate_redirect_uri("http://example.com", None)
assert not validate_redirect_uri("https://app.example.com", None)
def test_empty_list_allows_all(self):
"""Test that empty list allows all redirect URIs."""
assert validate_redirect_uri("http://localhost:3000", [])
assert validate_redirect_uri("http://example.com", [])
assert validate_redirect_uri("https://anywhere.com:9999/path", [])
def test_empty_list_allows_none(self):
"""Test that empty list allows no redirect URIs."""
assert not validate_redirect_uri("http://localhost:3000", [])
assert not validate_redirect_uri("http://example.com", [])
assert not validate_redirect_uri("https://anywhere.com:9999/path", [])
def test_custom_patterns(self):
"""Test validation with custom pattern list."""
@ -122,3 +121,21 @@ class TestDefaultPatterns:
"""Test that default patterns include localhost variations."""
assert "http://localhost:*" in DEFAULT_LOCALHOST_PATTERNS
assert "http://127.0.0.1:*" in DEFAULT_LOCALHOST_PATTERNS
def test_explicit_localhost_patterns(self):
"""Test that explicitly passing DEFAULT_LOCALHOST_PATTERNS restricts to localhost."""
# Localhost patterns should be allowed
assert validate_redirect_uri(
"http://localhost:3000", DEFAULT_LOCALHOST_PATTERNS
)
assert validate_redirect_uri(
"http://127.0.0.1:8080", DEFAULT_LOCALHOST_PATTERNS
)
# Non-localhost should be rejected
assert not validate_redirect_uri(
"http://example.com", DEFAULT_LOCALHOST_PATTERNS
)
assert not validate_redirect_uri(
"https://claude.ai/api/mcp/auth_callback", DEFAULT_LOCALHOST_PATTERNS
)