Validate the iss authorization-response parameter (RFC 9207 / SEP-2468) (#2921)
This commit is contained in:
parent
b7a5bffed0
commit
48cf4950dc
5 changed files with 335 additions and 103 deletions
|
|
@ -3279,16 +3279,10 @@ REQUIREMENTS: dict[str, Requirement] = {
|
|||
source=f"{SPEC_BASE_URL}/basic/authorization#authorization-server-metadata-discovery",
|
||||
behavior=(
|
||||
"The client rejects authorization-server metadata whose issuer does not match the URL the "
|
||||
"metadata was retrieved from (RFC 8414 section 3.3)."
|
||||
"metadata was retrieved from (RFC 8414 section 3.3 / SEP-2468)."
|
||||
),
|
||||
transports=("streamable-http",),
|
||||
note="OAuth is HTTP-only.",
|
||||
divergence=Divergence(
|
||||
note=(
|
||||
"The SDK parses authorization-server metadata without comparing issuer to the discovery "
|
||||
"URL; a mismatched issuer is accepted and the flow proceeds."
|
||||
),
|
||||
),
|
||||
),
|
||||
"client-auth:authorize:error-surfaces": Requirement(
|
||||
source=f"{SPEC_BASE_URL}/basic/authorization#authorization-flow-steps",
|
||||
|
|
@ -3492,6 +3486,16 @@ REQUIREMENTS: dict[str, Requirement] = {
|
|||
transports=("streamable-http",),
|
||||
note="OAuth is HTTP-only.",
|
||||
),
|
||||
"client-auth:authorization-response:iss-verify": Requirement(
|
||||
source=f"{SPEC_BASE_URL}/basic/authorization#authorization-server-metadata-discovery",
|
||||
behavior=(
|
||||
"The client validates the RFC 9207 iss authorization-response parameter against the "
|
||||
"authorization server issuer (simple string comparison) and rejects a mismatch, or a "
|
||||
"missing iss when the server advertises support (SEP-2468)."
|
||||
),
|
||||
transports=("streamable-http",),
|
||||
note="OAuth is HTTP-only.",
|
||||
),
|
||||
"client-auth:token-endpoint-auth-method": Requirement(
|
||||
source="sdk",
|
||||
behavior="The client authenticates to the token endpoint using the auth method established at registration.",
|
||||
|
|
|
|||
|
|
@ -26,7 +26,7 @@ from mcp.client.streamable_http import streamable_http_client
|
|||
from mcp.server import Server
|
||||
from mcp.server.auth.provider import AccessToken, ProviderTokenVerifier
|
||||
from mcp.server.auth.settings import AuthSettings, ClientRegistrationOptions, RevocationOptions
|
||||
from mcp.shared.auth import OAuthClientInformationFull, OAuthClientMetadata, OAuthToken
|
||||
from mcp.shared.auth import AuthorizationCodeResult, OAuthClientInformationFull, OAuthClientMetadata, OAuthToken
|
||||
from tests.interaction._connect import BASE_URL, NO_DNS_REBINDING_PROTECTION
|
||||
from tests.interaction.auth._provider import InMemoryAuthorizationServerProvider
|
||||
from tests.interaction.transports._bridge import StreamingASGITransport
|
||||
|
|
@ -136,16 +136,21 @@ class HeadlessOAuth:
|
|||
|
||||
`state_override`: when set, `callback_handler` returns this value as the state instead of
|
||||
the one parsed from the redirect, so tests can drive the state-mismatch path.
|
||||
|
||||
`iss_override`: when set, `callback_handler` returns this value as the RFC 9207 issuer
|
||||
instead of the one parsed from the redirect, so tests can drive the iss-mismatch path.
|
||||
"""
|
||||
|
||||
def __init__(self, *, state_override: str | None = None) -> None:
|
||||
def __init__(self, *, state_override: str | None = None, iss_override: str | None = None) -> None:
|
||||
self.authorize_url: str | None = None
|
||||
self.authorize_urls: list[str] = []
|
||||
self.error: str | None = None
|
||||
self._state_override = state_override
|
||||
self._iss_override = iss_override
|
||||
self._http: httpx.AsyncClient | None = None
|
||||
self._code: str = ""
|
||||
self._state: str | None = None
|
||||
self._iss: str | None = None
|
||||
|
||||
def bind(self, http_client: httpx.AsyncClient) -> None:
|
||||
self._http = http_client
|
||||
|
|
@ -161,10 +166,15 @@ class HeadlessOAuth:
|
|||
params = parse_qs(urlsplit(response.headers["location"]).query)
|
||||
self._code = params.get("code", [""])[0]
|
||||
self._state = params.get("state", [None])[0]
|
||||
self._iss = params.get("iss", [None])[0]
|
||||
self.error = params.get("error", [None])[0]
|
||||
|
||||
async def callback_handler(self) -> tuple[str, str | None]:
|
||||
return self._code, self._state_override if self._state_override is not None else self._state
|
||||
async def callback_handler(self) -> AuthorizationCodeResult:
|
||||
return AuthorizationCodeResult(
|
||||
code=self._code,
|
||||
state=self._state_override if self._state_override is not None else self._state,
|
||||
iss=self._iss_override if self._iss_override is not None else self._iss,
|
||||
)
|
||||
|
||||
|
||||
def auth_settings(
|
||||
|
|
|
|||
|
|
@ -21,6 +21,7 @@ from mcp.server.auth.provider import (
|
|||
construct_redirect_uri,
|
||||
)
|
||||
from mcp.shared.auth import OAuthClientInformationFull, OAuthToken
|
||||
from tests.interaction._connect import BASE_URL
|
||||
|
||||
_TOKEN_LIFETIME_SECONDS = 3600
|
||||
|
||||
|
|
@ -53,9 +54,14 @@ class InMemoryAuthorizationServerProvider(
|
|||
issue_expired_first: bool = False,
|
||||
fail_next_refresh: bool = False,
|
||||
reject_all_tokens: bool = False,
|
||||
issuer: str | None = None,
|
||||
) -> None:
|
||||
self._default_scopes = list(default_scopes) if default_scopes is not None else ["mcp"]
|
||||
self._issuer = "http://127.0.0.1:8000"
|
||||
# The authorization-response iss must equal the AS metadata issuer the client recorded
|
||||
# (RFC 9207 simple string comparison). `real_asm` builds the issuer from an AnyHttpUrl
|
||||
# object, so it carries the trailing slash; the redirect iss matches it. Path-issuer
|
||||
# tests pass the recorded issuer explicitly.
|
||||
self._issuer = issuer if issuer is not None else f"{BASE_URL}/"
|
||||
self._deny_authorize = deny_authorize
|
||||
self._issue_expired_first = issue_expired_first
|
||||
self._fail_next_refresh = fail_next_refresh
|
||||
|
|
|
|||
|
|
@ -187,6 +187,24 @@ async def test_a_mismatched_state_on_the_callback_aborts_the_flow() -> None:
|
|||
await connect_with_oauth(server, provider=provider, headless=headless).__aenter__()
|
||||
|
||||
|
||||
@requirement("client-auth:authorization-response:iss-verify")
|
||||
async def test_a_mismatched_iss_on_the_callback_aborts_the_flow() -> None:
|
||||
"""A callback whose RFC 9207 iss does not match the authorization server issuer aborts the flow.
|
||||
|
||||
`iss_override` makes the headless callback return an issuer the AS never advertised; the SDK
|
||||
compares it to `oauth_metadata.issuer` and raises `OAuthFlowError` before the token exchange.
|
||||
"""
|
||||
provider = InMemoryAuthorizationServerProvider()
|
||||
server = Server("guarded", on_list_tools=list_tools)
|
||||
headless = HeadlessOAuth(iss_override="https://attacker.example.com")
|
||||
|
||||
with anyio.fail_after(5):
|
||||
with pytest.RaisesGroup(
|
||||
pytest.RaisesExc(OAuthFlowError, match="^Authorization response iss mismatch:"), flatten_subgroups=True
|
||||
):
|
||||
await connect_with_oauth(server, provider=provider, headless=headless).__aenter__()
|
||||
|
||||
|
||||
@requirement("client-auth:resource-parameter")
|
||||
async def test_the_authorization_code_token_request_carries_grant_type_code_redirect_and_resource(
|
||||
recorded_oauth_flow: RecordedFlow,
|
||||
|
|
|
|||
|
|
@ -236,7 +236,10 @@ async def test_as_metadata_discovery_falls_back_through_the_spec_endpoint_order(
|
|||
unrecognized field to prove the client's parser ignores unknown members (RFC 8414 §3.2).
|
||||
"""
|
||||
recorded, on_request = record_requests()
|
||||
provider = InMemoryAuthorizationServerProvider()
|
||||
asm = real_asm()
|
||||
asm.issuer = AnyHttpUrl(authorization_server)
|
||||
# The redirect iss must equal the issuer the client records from this metadata.
|
||||
provider = InMemoryAuthorizationServerProvider(issuer=str(asm.issuer))
|
||||
server = Server("guarded", on_list_tools=list_tools)
|
||||
|
||||
prm = ProtectedResourceMetadata(
|
||||
|
|
@ -246,7 +249,7 @@ async def test_as_metadata_discovery_falls_back_through_the_spec_endpoint_order(
|
|||
not_found=not_found,
|
||||
serve={
|
||||
PRM_PATH_SUFFIXED: metadata_body(prm),
|
||||
serve_at: metadata_body(real_asm(), x_unknown_extension="ignored"),
|
||||
serve_at: metadata_body(asm, x_unknown_extension="ignored"),
|
||||
},
|
||||
)
|
||||
|
||||
|
|
@ -311,23 +314,26 @@ async def test_as_metadata_advertises_authorize_token_registration_and_s256() ->
|
|||
|
||||
|
||||
@requirement("client-auth:as-metadata-discovery:issuer-validation")
|
||||
async def test_as_metadata_with_a_mismatched_issuer_is_accepted_and_the_flow_proceeds() -> None:
|
||||
"""Authorization-server metadata whose `issuer` does not match the discovery URL is accepted.
|
||||
async def test_as_metadata_with_a_mismatched_issuer_aborts_the_flow() -> None:
|
||||
"""Authorization-server metadata whose `issuer` does not match the discovery URL is rejected.
|
||||
|
||||
RFC 8414 §3.3 requires the client to reject the document; the SDK parses and uses it
|
||||
without comparing `issuer` to the URL it was fetched from. See the divergence on the
|
||||
requirement. The served body carries an unrecognized field as a fold-in proof of
|
||||
unknown-field tolerance.
|
||||
RFC 8414 §3.3 / SEP-2468 require the client to reject the document; the SDK compares `issuer`
|
||||
to the URL the metadata was fetched from and raises `OAuthFlowError` before any authorize or
|
||||
token request is made.
|
||||
"""
|
||||
recorded, on_request = record_requests()
|
||||
provider = InMemoryAuthorizationServerProvider()
|
||||
server = Server("guarded", on_list_tools=list_tools)
|
||||
|
||||
metadata = real_asm()
|
||||
metadata.issuer = AnyHttpUrl(f"{BASE_URL}/wrong-issuer")
|
||||
app_shim = shim(serve={ASM_ROOT: metadata_body(metadata, x_unknown_extension="ignored")})
|
||||
app_shim = shim(serve={ASM_ROOT: metadata_body(metadata)})
|
||||
|
||||
with anyio.fail_after(5):
|
||||
async with connect_with_oauth(server, provider=provider, app_shim=app_shim) as (client, _):
|
||||
result = await client.list_tools()
|
||||
with pytest.RaisesGroup(
|
||||
pytest.RaisesExc(OAuthFlowError, match="^Authorization server metadata issuer mismatch"),
|
||||
flatten_subgroups=True,
|
||||
):
|
||||
await connect_with_oauth(server, provider=provider, app_shim=app_shim, on_request=on_request).__aenter__()
|
||||
|
||||
assert result.tools[0].name == "probe"
|
||||
assert [r.path for r in recorded if r.path in ("/authorize", "/token")] == []
|
||||
|
|
|
|||
Loading…
Reference in a new issue