diff --git a/docs/migration.md b/docs/migration.md index 91379bd0f6..42f2ae3118 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -2618,7 +2618,7 @@ v2 hardens client authentication on SDK-hosted authorization servers (`create_au `unauthorized_client` is now reserved for a client that authenticated successfully but is not permitted the requested grant. Update any client code, integration tests, or alerting that string-matches the old error code, or accept both while clients and servers migrate at different times. -**Secret-based clients without a stored secret are rejected.** In v1, `ClientAuthenticator` only validated a secret when one was stored, so a hand-provisioned client record with a secret-based auth method but no secret authenticated with no credentials at all. v2 rejects such clients before any grant processing: `/token` returns 401 `invalid_client` and `/revoke` returns 401 `unauthorized_client`, both with the description "Client is registered for secret-based authentication but has no stored secret". Only records that explicitly set `client_secret_post` or `client_secret_basic` with no secret are affected: records left at the default `token_endpoint_auth_method=None` fail in both versions, and DCR-registered clients always receive a generated secret. +**Secret-based clients without a stored secret are rejected.** In v1, `ClientAuthenticator` only validated a secret when one was stored, so a hand-provisioned client record with a secret-based auth method but no secret authenticated with no credentials at all. v2 rejects such clients before any grant processing: `/token` and `/revoke` both return 401 `invalid_client`, with the description "Client is registered for secret-based authentication but has no stored secret". Only records that explicitly set `client_secret_post` or `client_secret_basic` with no secret are affected: records left at the default `token_endpoint_auth_method=None` fail in both versions, and DCR-registered clients always receive a generated secret. **Before (v1):** diff --git a/src/mcp/server/auth/handlers/revoke.py b/src/mcp/server/auth/handlers/revoke.py index 4efd154001..a0aeff3b3c 100644 --- a/src/mcp/server/auth/handlers/revoke.py +++ b/src/mcp/server/auth/handlers/revoke.py @@ -10,7 +10,11 @@ stringify_pydantic_error, ) from mcp.server.auth.json_response import PydanticJSONResponse -from mcp.server.auth.middleware.client_auth import AuthenticationError, ClientAuthenticator +from mcp.server.auth.middleware.client_auth import ( + AuthenticationError, + ClientAuthenticator, + www_authenticate_headers, +) from mcp.server.auth.provider import AccessToken, OAuthAuthorizationServerProvider, RefreshToken @@ -24,7 +28,8 @@ class RevocationRequest(BaseModel): class RevocationErrorResponse(BaseModel): - error: Literal["invalid_request", "unauthorized_client"] + # RFC 7009 §2.2.1: the error presentation conforms to RFC 6749 §5.2. + error: Literal["invalid_request", "invalid_client", "unauthorized_client"] error_description: str | None = None @@ -37,13 +42,15 @@ async def handle(self, request: Request) -> Response: """Handler for the OAuth 2.0 Token Revocation endpoint.""" try: client = await self.client_authenticator.authenticate_request(request) - except AuthenticationError as e: # pragma: no cover + except AuthenticationError as e: + # RFC 6749 §5.2 / RFC 7009 §2.2.1: failed client authentication is invalid_client. return PydanticJSONResponse( status_code=401, content=RevocationErrorResponse( - error="unauthorized_client", + error="invalid_client", error_description=e.message, ), + headers=www_authenticate_headers(request), ) try: diff --git a/src/mcp/server/auth/handlers/token.py b/src/mcp/server/auth/handlers/token.py index 00df115573..a6eef25ea4 100644 --- a/src/mcp/server/auth/handlers/token.py +++ b/src/mcp/server/auth/handlers/token.py @@ -9,14 +9,22 @@ from mcp.server.auth.errors import stringify_pydantic_error from mcp.server.auth.json_response import PydanticJSONResponse -from mcp.server.auth.middleware.client_auth import AuthenticationError, ClientAuthenticator +from mcp.server.auth.middleware.client_auth import AuthenticationError, ClientAuthenticator, www_authenticate_headers from mcp.server.auth.provider import ( IdentityAssertionParams, OAuthAuthorizationServerProvider, TokenError, TokenErrorCode, ) -from mcp.shared.auth import OAuthToken +from mcp.shared.auth import JWT_BEARER_GRANT_TYPE, OAuthToken + + +def supported_grant_types(*, identity_assertion_enabled: bool) -> list[str]: + """Grant types this authorization server implements and advertises.""" + grant_types = ["authorization_code", "refresh_token"] + if identity_assertion_enabled: + grant_types.append(JWT_BEARER_GRANT_TYPE) + return grant_types class AuthorizationCodeRequest(BaseModel): @@ -104,7 +112,7 @@ async def handle(self, request: Request): try: client_info = await self.client_authenticator.authenticate_request(request) except AuthenticationError as e: - # Authentication failures should return 401 + # RFC 6749 §5.2: failed client authentication is invalid_client. return PydanticJSONResponse( content=TokenErrorResponse( error="invalid_client", @@ -114,13 +122,28 @@ async def handle(self, request: Request): headers={ "Cache-Control": "no-store", "Pragma": "no-cache", + **www_authenticate_headers(request), }, ) + form_data = await request.form() + # TODO(Marcelo): Can someone check if this `dict()` wrapper is necessary? + form = dict(form_data) + grant_type = form.get("grant_type") + # A grant this server does not implement is unsupported_grant_type, before the + # request union is validated, so the body is not a pydantic error (RFC 6749 §5.2). + if isinstance(grant_type, str) and grant_type not in supported_grant_types( + identity_assertion_enabled=self.identity_assertion_enabled + ): + return self.response( + TokenErrorResponse( + error="unsupported_grant_type", + error_description="The authorization grant type is not supported by the authorization server", + ) + ) + try: - form_data = await request.form() - # TODO(Marcelo): Can someone check if this `dict()` wrapper is necessary? - token_request = token_request_adapter.validate_python(dict(form_data)) + token_request = token_request_adapter.validate_python(form) except ValidationError as validation_error: return self.response( TokenErrorResponse( @@ -129,11 +152,12 @@ async def handle(self, request: Request): ) ) + # The server implements this grant, but this client is not registered for it. if token_request.grant_type not in client_info.grant_types: return self.response( TokenErrorResponse( - error="unsupported_grant_type", - error_description=(f"Unsupported grant type (supported grant types are {client_info.grant_types})"), + error="unauthorized_client", + error_description="The authenticated client is not authorized to use this authorization grant type", ) ) @@ -240,14 +264,6 @@ async def handle(self, request: Request): return self.response(TokenErrorResponse(error=e.error, error_description=e.error_description)) case JwtBearerRequest(): # pragma: no branch - if not self.identity_assertion_enabled: - return self.response( - TokenErrorResponse( - error="unsupported_grant_type", - error_description="The JWT bearer grant is not supported by this authorization server", - ) - ) - # SDK policy, adopting the SHOULD in draft-ietf-oauth-identity-assertion-authz-grant-04 # §9.1: only confidential clients may present an ID-JAG. ClientAuthenticator already # rejects a secret-based method with no stored secret; this additionally rejects the diff --git a/src/mcp/server/auth/middleware/client_auth.py b/src/mcp/server/auth/middleware/client_auth.py index 3d5067d611..6d4ab76c36 100644 --- a/src/mcp/server/auth/middleware/client_auth.py +++ b/src/mcp/server/auth/middleware/client_auth.py @@ -16,6 +16,20 @@ def __init__(self, message: str): self.message = message +def www_authenticate_headers(request: Request) -> dict[str, str]: + """RFC 6749 §5.2 challenge when the client attempted HTTP Basic authentication. + + A failed attempt via the Authorization header must be a 401 whose + WWW-Authenticate value matches that scheme. Basic challenges include a + realm (RFC 7617 §2); "OAuth" names this server's client-authentication + protection space. Other schemes are not rewritten into a Basic challenge. + """ + authorization = request.headers.get("Authorization", "") + if authorization.split(" ", 1)[0].lower() != "basic": + return {} + return {"WWW-Authenticate": 'Basic realm="OAuth"'} + + class ClientAuthenticator: """ClientAuthenticator is a callable which validates requests from a client application, used to verify /token calls. diff --git a/src/mcp/server/auth/routes.py b/src/mcp/server/auth/routes.py index 848604dc98..9ead720165 100644 --- a/src/mcp/server/auth/routes.py +++ b/src/mcp/server/auth/routes.py @@ -13,12 +13,12 @@ from mcp.server.auth.handlers.metadata import MetadataHandler, ProtectedResourceMetadataHandler from mcp.server.auth.handlers.register import RegistrationHandler from mcp.server.auth.handlers.revoke import RevocationHandler -from mcp.server.auth.handlers.token import TokenHandler +from mcp.server.auth.handlers.token import TokenHandler, supported_grant_types from mcp.server.auth.middleware.client_auth import ClientAuthenticator from mcp.server.auth.provider import OAuthAuthorizationServerProvider from mcp.server.auth.settings import ClientRegistrationOptions, RevocationOptions from mcp.server.transport_security import DEFAULT_MAX_REQUEST_BODY_SIZE, RequestBodyLimitMiddleware -from mcp.shared.auth import JWT_BEARER_GRANT_TYPE, OAuthMetadata, ProtectedResourceMetadata +from mcp.shared.auth import OAuthMetadata, ProtectedResourceMetadata from mcp.shared.inbound import MCP_PROTOCOL_VERSION_HEADER @@ -158,12 +158,11 @@ def build_metadata( authorization_url = AnyHttpUrl(str(issuer_url).rstrip("/") + AUTHORIZATION_PATH) token_url = AnyHttpUrl(str(issuer_url).rstrip("/") + TOKEN_PATH) - grant_types_supported = ["authorization_code", "refresh_token"] + grant_types_supported = supported_grant_types(identity_assertion_enabled=supports_identity_assertion) # SEP-990 / ext-auth §6: support for the ID-JAG flow is advertised as a grant PROFILE, not as # the jwt-bearer grant type (which an AS might support for other purposes). authorization_grant_profiles_supported: list[str] | None = None if supports_identity_assertion: - grant_types_supported.append(JWT_BEARER_GRANT_TYPE) authorization_grant_profiles_supported = [ID_JAG_GRANT_PROFILE] # Create metadata diff --git a/tests/server/auth/test_error_handling.py b/tests/server/auth/test_error_handling.py index f13f23ea33..6552d54c2b 100644 --- a/tests/server/auth/test_error_handling.py +++ b/tests/server/auth/test_error_handling.py @@ -10,6 +10,7 @@ import httpx2 import pytest from httpx2 import ASGITransport +from inline_snapshot import snapshot from pydantic import AnyHttpUrl from starlette.applications import Starlette @@ -347,3 +348,172 @@ async def test_oversized_cross_origin_request_gets_413_with_cors_headers(client: ) assert response.status_code == 413 assert response.headers["access-control-allow-origin"] == "*" + + +async def _register( + client: httpx2.AsyncClient, + *, + grant_types: list[str], + token_endpoint_auth_method: str, +) -> dict[str, Any]: + response = await client.post( + "/register", + json={ + "redirect_uris": ["https://client.example.com/callback"], + "token_endpoint_auth_method": token_endpoint_auth_method, + "grant_types": grant_types, + "response_types": ["code"], + "client_name": "Test Client", + }, + ) + assert response.status_code == 201 + body = response.json() + return body + + +@pytest.mark.anyio +async def test_refresh_for_a_client_not_registered_for_it_is_unauthorized_client(client: httpx2.AsyncClient): + """A grant this server implements, but this client is not registered for, is unauthorized_client. + + RFC 6749 §5.2 reserves unsupported_grant_type for a grant the authorization server + itself does not implement. + """ + registered = await _register( + client, grant_types=["authorization_code"], token_endpoint_auth_method="client_secret_post" + ) + client_id = registered["client_id"] + client_secret = registered["client_secret"] + assert isinstance(client_id, str) + assert isinstance(client_secret, str) + + response = await client.post( + "/token", + data={ + "grant_type": "refresh_token", + "refresh_token": "not-a-real-token", + "client_id": client_id, + "client_secret": client_secret, + }, + ) + + assert response.status_code == 400 + assert response.json() == snapshot( + { + "error": "unauthorized_client", + "error_description": "The authenticated client is not authorized to use this authorization grant type", + } + ) + + +@pytest.mark.anyio +@pytest.mark.parametrize("grant_type", ["client_credentials", "password", "not_a_grant"]) +async def test_grant_the_server_does_not_implement_is_unsupported_grant_type( + client: httpx2.AsyncClient, grant_type: str +): + """A grant outside the set this server advertises is unsupported_grant_type, with no validation text. + + RFC 6749 §5.2. The code is chosen from grant_type before the request union is validated, + so client_credentials and password do not surface as invalid_request. + """ + registered = await _register( + client, grant_types=["authorization_code"], token_endpoint_auth_method="client_secret_post" + ) + client_id = registered["client_id"] + client_secret = registered["client_secret"] + assert isinstance(client_id, str) + assert isinstance(client_secret, str) + + response = await client.post( + "/token", + data={"grant_type": grant_type, "client_id": client_id, "client_secret": client_secret}, + ) + + assert response.status_code == 400 + assert response.json() == snapshot( + { + "error": "unsupported_grant_type", + "error_description": "The authorization grant type is not supported by the authorization server", + } + ) + + +@pytest.mark.anyio +async def test_authenticated_token_request_without_grant_type_is_invalid_request(client: httpx2.AsyncClient): + """Omitting grant_type is a malformed request, so the code stays invalid_request. + + RFC 6749 §5.2: a missing required parameter is invalid_request. The description is + pydantic's validation text, so only its presence is pinned. + """ + registered = await _register( + client, + grant_types=["authorization_code", "refresh_token"], + token_endpoint_auth_method="client_secret_post", + ) + client_id = registered["client_id"] + client_secret = registered["client_secret"] + assert isinstance(client_id, str) + assert isinstance(client_secret, str) + + response = await client.post("/token", data={"client_id": client_id, "client_secret": client_secret}) + + assert response.status_code == 400 + body = response.json() + assert body["error"] == "invalid_request" + assert body["error_description"] + + +@pytest.mark.anyio +@pytest.mark.parametrize("path", ["/token", "/revoke"]) +async def test_failed_basic_authentication_includes_www_authenticate(client: httpx2.AsyncClient, path: str): + """Failed HTTP Basic client authentication is 401 invalid_client with a Basic challenge. + + RFC 6749 §5.2 requires WWW-Authenticate matching the scheme the client used, and + RFC 7617 §2 requires a realm on a Basic challenge. RFC 7009 §2.2.1 uses that same + presentation for /revoke, so a bad secret there is invalid_client. + """ + registered = await _register( + client, grant_types=["authorization_code"], token_endpoint_auth_method="client_secret_basic" + ) + client_id = registered["client_id"] + assert isinstance(client_id, str) + credentials = base64.b64encode(f"{client_id}:wrong".encode()).decode() + + response = await client.post( + path, + headers={"Authorization": f"Basic {credentials}"}, + data={"client_id": client_id, "grant_type": "refresh_token", "refresh_token": "rt", "token": "rt"}, + ) + + assert response.status_code == 401 + assert response.json() == snapshot({"error": "invalid_client", "error_description": "Invalid client_secret"}) + assert response.headers["www-authenticate"] == 'Basic realm="OAuth"' + + +@pytest.mark.anyio +@pytest.mark.parametrize("path", ["/token", "/revoke"]) +async def test_failed_client_secret_post_authentication_omits_www_authenticate(client: httpx2.AsyncClient, path: str): + """A client_secret_post failure is invalid_client and carries no WWW-Authenticate header. + + RFC 6749 §5.2 requires that header only when the client used the Authorization field. + /revoke uses the same code (RFC 7009 §2.2.1). + """ + registered = await _register( + client, grant_types=["authorization_code"], token_endpoint_auth_method="client_secret_post" + ) + client_id = registered["client_id"] + assert isinstance(client_id, str) + + response = await client.post( + path, + data={ + "client_id": client_id, + "client_secret": "wrong", + "grant_type": "refresh_token", + "refresh_token": "rt", + "token": "rt", + }, + ) + + assert response.status_code == 401 + assert response.json() == snapshot({"error": "invalid_client", "error_description": "Invalid client_secret"}) + assert "www-authenticate" not in response.headers diff --git a/tests/server/auth/test_identity_assertion.py b/tests/server/auth/test_identity_assertion.py index 9ce12cb8fb..714a776ec2 100644 --- a/tests/server/auth/test_identity_assertion.py +++ b/tests/server/auth/test_identity_assertion.py @@ -288,7 +288,10 @@ async def test_malformed_request_missing_assertion_is_invalid_request(client: ht async def test_client_without_the_grant_registered_is_rejected( client: httpx2.AsyncClient, provider: IdentityAssertionProvider ): - """A confidential client whose registration omits the jwt-bearer grant is refused the grant.""" + """A confidential client whose registration omits the jwt-bearer grant is refused the grant. + + RFC 6749 §5.2: the server implements this grant, so the code is unauthorized_client. + """ provider.clients["no-grant-client"] = OAuthClientInformationFull( client_id="no-grant-client", client_secret="s", @@ -309,7 +312,7 @@ async def test_client_without_the_grant_registered_is_rejected( ) assert response.status_code == 400 - assert response.json()["error"] == "unsupported_grant_type" + assert response.json()["error"] == "unauthorized_client" assert provider.last_params is None