fix: add RFC 8707 resource validation to OAuth client (#2069)
This commit is contained in:
parent
78ddff022b
commit
67458948ae
5 changed files with 102 additions and 18 deletions
|
|
@ -1,9 +1,4 @@
|
||||||
# Known conformance test failures for v1.x
|
# Known conformance test failures for v1.x
|
||||||
# These are tracked and should be removed as they're fixed.
|
# These are tracked and should be removed as they're fixed.
|
||||||
#
|
server: []
|
||||||
# auth/resource-mismatch: Client must validate that the Protected Resource
|
client: []
|
||||||
# Metadata (PRM) resource field matches the server URL before proceeding
|
|
||||||
# with authorization (RFC 8707). Implemented on main (PR #2010), needs
|
|
||||||
# backport to v1.x.
|
|
||||||
client:
|
|
||||||
- auth/resource-mismatch
|
|
||||||
|
|
|
||||||
|
|
@ -267,6 +267,15 @@ class OAuthClientProvider(httpx.Auth):
|
||||||
)
|
)
|
||||||
self._initialized = False
|
self._initialized = False
|
||||||
|
|
||||||
|
async def _validate_resource_match(self, prm: ProtectedResourceMetadata) -> None:
|
||||||
|
"""Validate that PRM resource matches the server URL per RFC 8707."""
|
||||||
|
prm_resource = str(prm.resource) if prm.resource else None
|
||||||
|
if not prm_resource:
|
||||||
|
return # pragma: no cover
|
||||||
|
default_resource = resource_url_from_server_url(self.context.server_url)
|
||||||
|
if not check_resource_allowed(requested_resource=default_resource, configured_resource=prm_resource):
|
||||||
|
raise OAuthFlowError(f"Protected resource {prm_resource} does not match expected {default_resource}")
|
||||||
|
|
||||||
async def _handle_protected_resource_response(self, response: httpx.Response) -> bool:
|
async def _handle_protected_resource_response(self, response: httpx.Response) -> bool:
|
||||||
"""
|
"""
|
||||||
Handle protected resource metadata discovery response.
|
Handle protected resource metadata discovery response.
|
||||||
|
|
@ -520,6 +529,8 @@ class OAuthClientProvider(httpx.Auth):
|
||||||
|
|
||||||
prm = await handle_protected_resource_response(discovery_response)
|
prm = await handle_protected_resource_response(discovery_response)
|
||||||
if prm:
|
if prm:
|
||||||
|
# Validate PRM resource matches server URL (RFC 8707)
|
||||||
|
await self._validate_resource_match(prm)
|
||||||
self.context.protected_resource_metadata = prm
|
self.context.protected_resource_metadata = prm
|
||||||
|
|
||||||
# todo: try all authorization_servers to find the OASM
|
# todo: try all authorization_servers to find the OASM
|
||||||
|
|
|
||||||
|
|
@ -51,22 +51,17 @@ def check_resource_allowed(requested_resource: str, configured_resource: str) ->
|
||||||
if requested.scheme.lower() != configured.scheme.lower() or requested.netloc.lower() != configured.netloc.lower():
|
if requested.scheme.lower() != configured.scheme.lower() or requested.netloc.lower() != configured.netloc.lower():
|
||||||
return False
|
return False
|
||||||
|
|
||||||
# Handle cases like requested=/foo and configured=/foo/
|
# Normalize trailing slashes before comparison so that
|
||||||
|
# "/foo" and "/foo/" are treated as equivalent.
|
||||||
requested_path = requested.path
|
requested_path = requested.path
|
||||||
configured_path = configured.path
|
configured_path = configured.path
|
||||||
|
|
||||||
# If requested path is shorter, it cannot be a child
|
|
||||||
if len(requested_path) < len(configured_path):
|
|
||||||
return False
|
|
||||||
|
|
||||||
# Check if the requested path starts with the configured path
|
|
||||||
# Ensure both paths end with / for proper comparison
|
|
||||||
# This ensures that paths like "/api123" don't incorrectly match "/api"
|
|
||||||
if not requested_path.endswith("/"):
|
if not requested_path.endswith("/"):
|
||||||
requested_path += "/"
|
requested_path += "/"
|
||||||
if not configured_path.endswith("/"):
|
if not configured_path.endswith("/"):
|
||||||
configured_path += "/"
|
configured_path += "/"
|
||||||
|
|
||||||
|
# Check hierarchical match: requested must start with configured path.
|
||||||
|
# The trailing-slash normalization ensures "/api123/" won't match "/api/".
|
||||||
return requested_path.startswith(configured_path)
|
return requested_path.startswith(configured_path)
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -13,6 +13,7 @@ from inline_snapshot import Is, snapshot
|
||||||
from pydantic import AnyHttpUrl, AnyUrl
|
from pydantic import AnyHttpUrl, AnyUrl
|
||||||
|
|
||||||
from mcp.client.auth import OAuthClientProvider, PKCEParameters
|
from mcp.client.auth import OAuthClientProvider, PKCEParameters
|
||||||
|
from mcp.client.auth.exceptions import OAuthFlowError
|
||||||
from mcp.client.auth.utils import (
|
from mcp.client.auth.utils import (
|
||||||
build_oauth_authorization_server_metadata_discovery_urls,
|
build_oauth_authorization_server_metadata_discovery_urls,
|
||||||
build_protected_resource_metadata_discovery_urls,
|
build_protected_resource_metadata_discovery_urls,
|
||||||
|
|
@ -965,7 +966,7 @@ class TestAuthFlow:
|
||||||
# Send a successful discovery response with minimal protected resource metadata
|
# Send a successful discovery response with minimal protected resource metadata
|
||||||
discovery_response = httpx.Response(
|
discovery_response = httpx.Response(
|
||||||
200,
|
200,
|
||||||
content=b'{"resource": "https://api.example.com/mcp", "authorization_servers": ["https://auth.example.com"]}',
|
content=b'{"resource": "https://api.example.com/v1/mcp", "authorization_servers": ["https://auth.example.com"]}',
|
||||||
request=discovery_request,
|
request=discovery_request,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -2030,3 +2031,85 @@ class TestCIMD:
|
||||||
await auth_flow.asend(final_response)
|
await auth_flow.asend(final_response)
|
||||||
except StopAsyncIteration:
|
except StopAsyncIteration:
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_validate_resource_rejects_mismatched_resource(
|
||||||
|
client_metadata: OAuthClientMetadata, mock_storage: MockTokenStorage
|
||||||
|
) -> None:
|
||||||
|
"""Client must reject PRM resource that doesn't match server URL."""
|
||||||
|
provider = OAuthClientProvider(
|
||||||
|
server_url="https://api.example.com/v1/mcp",
|
||||||
|
client_metadata=client_metadata,
|
||||||
|
storage=mock_storage,
|
||||||
|
)
|
||||||
|
provider._initialized = True
|
||||||
|
|
||||||
|
prm = ProtectedResourceMetadata(
|
||||||
|
resource=AnyHttpUrl("https://evil.example.com/mcp"),
|
||||||
|
authorization_servers=[AnyHttpUrl("https://auth.example.com")],
|
||||||
|
)
|
||||||
|
with pytest.raises(OAuthFlowError, match="does not match expected"):
|
||||||
|
await provider._validate_resource_match(prm)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_validate_resource_accepts_matching_resource(
|
||||||
|
client_metadata: OAuthClientMetadata, mock_storage: MockTokenStorage
|
||||||
|
) -> None:
|
||||||
|
"""Client must accept PRM resource that matches server URL."""
|
||||||
|
provider = OAuthClientProvider(
|
||||||
|
server_url="https://api.example.com/v1/mcp",
|
||||||
|
client_metadata=client_metadata,
|
||||||
|
storage=mock_storage,
|
||||||
|
)
|
||||||
|
provider._initialized = True
|
||||||
|
|
||||||
|
prm = ProtectedResourceMetadata(
|
||||||
|
resource=AnyHttpUrl("https://api.example.com/v1/mcp"),
|
||||||
|
authorization_servers=[AnyHttpUrl("https://auth.example.com")],
|
||||||
|
)
|
||||||
|
# Should not raise
|
||||||
|
await provider._validate_resource_match(prm)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_validate_resource_accepts_root_url_with_trailing_slash(
|
||||||
|
client_metadata: OAuthClientMetadata, mock_storage: MockTokenStorage
|
||||||
|
) -> None:
|
||||||
|
"""Root URLs with trailing slash normalization should match."""
|
||||||
|
provider = OAuthClientProvider(
|
||||||
|
server_url="https://api.example.com/",
|
||||||
|
client_metadata=client_metadata,
|
||||||
|
storage=mock_storage,
|
||||||
|
)
|
||||||
|
provider._initialized = True
|
||||||
|
|
||||||
|
prm = ProtectedResourceMetadata(
|
||||||
|
resource=AnyHttpUrl("https://api.example.com/"),
|
||||||
|
authorization_servers=[AnyHttpUrl("https://auth.example.com")],
|
||||||
|
)
|
||||||
|
# Should not raise - both already have trailing slashes
|
||||||
|
await provider._validate_resource_match(prm)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.anyio
|
||||||
|
async def test_get_resource_url_falls_back_when_prm_mismatches(
|
||||||
|
client_metadata: OAuthClientMetadata, mock_storage: MockTokenStorage
|
||||||
|
) -> None:
|
||||||
|
"""get_resource_url returns canonical URL when PRM resource doesn't match."""
|
||||||
|
provider = OAuthClientProvider(
|
||||||
|
server_url="https://api.example.com/v1/mcp",
|
||||||
|
client_metadata=client_metadata,
|
||||||
|
storage=mock_storage,
|
||||||
|
)
|
||||||
|
provider._initialized = True
|
||||||
|
|
||||||
|
# Set PRM with a resource that is NOT a parent of the server URL
|
||||||
|
provider.context.protected_resource_metadata = ProtectedResourceMetadata(
|
||||||
|
resource=AnyHttpUrl("https://other.example.com/mcp"),
|
||||||
|
authorization_servers=[AnyHttpUrl("https://auth.example.com")],
|
||||||
|
)
|
||||||
|
|
||||||
|
# get_resource_url should return the canonical server URL, not the PRM resource
|
||||||
|
assert provider.context.get_resource_url() == "https://api.example.com/v1/mcp"
|
||||||
|
|
|
||||||
|
|
@ -95,7 +95,7 @@ class TestCheckResourceAllowed:
|
||||||
"""Trailing slashes should be handled correctly."""
|
"""Trailing slashes should be handled correctly."""
|
||||||
# With and without trailing slashes
|
# With and without trailing slashes
|
||||||
assert check_resource_allowed("https://example.com/api/", "https://example.com/api") is True
|
assert check_resource_allowed("https://example.com/api/", "https://example.com/api") is True
|
||||||
assert check_resource_allowed("https://example.com/api", "https://example.com/api/") is False
|
assert check_resource_allowed("https://example.com/api", "https://example.com/api/") is True
|
||||||
assert check_resource_allowed("https://example.com/api/v1", "https://example.com/api") is True
|
assert check_resource_allowed("https://example.com/api/v1", "https://example.com/api") is True
|
||||||
assert check_resource_allowed("https://example.com/api/v1", "https://example.com/api/") is True
|
assert check_resource_allowed("https://example.com/api/v1", "https://example.com/api/") is True
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue