diff --git a/src/mcp/client/auth/oauth2.py b/src/mcp/client/auth/oauth2.py index 7dc62b52b9..079f108c76 100644 --- a/src/mcp/client/auth/oauth2.py +++ b/src/mcp/client/auth/oauth2.py @@ -262,8 +262,11 @@ def prepare_token_auth( credentials = f"{encoded_id}:{encoded_secret}" encoded_credentials = base64.b64encode(credentials.encode()).decode() headers["Authorization"] = f"Basic {encoded_credentials}" - # Don't include client_secret in body for basic auth - data = {k: v for k, v in data.items() if k != "client_secret"} + # RFC 6749 section 2.3: with HTTP Basic auth the client credentials + # must not also appear in the body. Some strict token endpoints + # reject a request that carries both the Authorization header and + # client_id in the body as two auth methods at once. + data = {k: v for k, v in data.items() if k not in ("client_secret", "client_id")} elif auth_method == "client_secret_post" and self.client_info.client_secret: # Include client_id and client_secret in request body (RFC 6749 ยง2.3.1) data["client_id"] = self.client_info.client_id diff --git a/src/mcp/server/auth/handlers/token.py b/src/mcp/server/auth/handlers/token.py index 0e644c378a..6d09eb6576 100644 --- a/src/mcp/server/auth/handlers/token.py +++ b/src/mcp/server/auth/handlers/token.py @@ -24,7 +24,10 @@ class AuthorizationCodeRequest(BaseModel): grant_type: Literal["authorization_code"] code: str = Field(..., description="The authorization code") redirect_uri: AnyUrl | None = Field(None, description="Must be the same as redirect URI provided in /authorize") - client_id: str + # Optional: a client_secret_basic client authenticates via the Authorization header and + # isn't required to repeat client_id in the body (RFC 6749 section 2.3). handle() backfills + # this from the already-authenticated client when the body omits it. + client_id: str | None = None # we use the client_secret param, per https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 client_secret: str | None = None # See https://datatracker.ietf.org/doc/html/rfc7636#section-4.5 @@ -38,7 +41,8 @@ class RefreshTokenRequest(BaseModel): grant_type: Literal["refresh_token"] refresh_token: str = Field(..., description="The refresh token") scope: str | None = Field(None, description="Optional scope parameter") - client_id: str + # Optional, see AuthorizationCodeRequest.client_id above. + client_id: str | None = None # we use the client_secret param, per https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 client_secret: str | None = None # RFC 8707 resource indicator @@ -52,7 +56,8 @@ class JwtBearerRequest(BaseModel): # See https://datatracker.ietf.org/doc/html/rfc7523#section-2.1 assertion: str = Field(..., description="The ID-JAG (a signed JWT) being presented as the grant") scope: str | None = Field(None, description="Optional scope parameter") - client_id: str + # Optional, see AuthorizationCodeRequest.client_id above. + client_id: str | None = None # we use the client_secret param, per https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 client_secret: str | None = None # RFC 8707 resource indicator @@ -121,6 +126,12 @@ async def handle(self, request: Request): 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)) + # A client_secret_basic client authenticated via the Authorization header and may + # have omitted client_id from the body; client_info is already the verified identity + # (authenticate_request cross-checks any body-supplied client_id against it), so + # backfill from there rather than requiring the body to repeat it. + if token_request.client_id is None: + token_request.client_id = client_info.client_id except ValidationError as validation_error: return self.response( TokenErrorResponse( diff --git a/src/mcp/server/auth/middleware/client_auth.py b/src/mcp/server/auth/middleware/client_auth.py index 3d5067d611..dd324eab9e 100644 --- a/src/mcp/server/auth/middleware/client_auth.py +++ b/src/mcp/server/auth/middleware/client_auth.py @@ -52,37 +52,45 @@ async def authenticate_request(self, request: Request) -> OAuthClientInformation AuthenticationError: If authentication fails """ form_data = await request.form() - client_id = form_data.get("client_id") - if not client_id: - raise AuthenticationError("Missing client_id") - - client = await self.provider.get_client(str(client_id)) - if not client: - raise AuthenticationError("Invalid client_id") # pragma: no cover - - request_client_secret: str | None = None auth_header = request.headers.get("Authorization", "") - if client.token_endpoint_auth_method == "client_secret_basic": - if not auth_header.startswith("Basic "): - raise AuthenticationError("Missing or invalid Basic authentication in Authorization header") - + # A Basic header carries client_id itself, so a client using client_secret_basic + # is not required to also repeat it in the body (RFC 6749 section 2.3 treats the + # header and the body as alternatives, and strict token endpoints reject both at + # once). Decode it up front so the lookup below can fall back to it. + basic_client_id: str | None = None + basic_client_secret: str | None = None + if auth_header.startswith("Basic "): try: encoded_credentials = auth_header[6:] # Remove "Basic " prefix decoded = base64.b64decode(encoded_credentials).decode("utf-8") if ":" not in decoded: raise ValueError("Invalid Basic auth format") - basic_client_id, request_client_secret = decoded.split(":", 1) - + raw_client_id, raw_client_secret = decoded.split(":", 1) # URL-decode both parts per RFC 6749 Section 2.3.1 - basic_client_id = unquote(basic_client_id) - request_client_secret = unquote(request_client_secret) - - if basic_client_id != client_id: - raise AuthenticationError("Client ID mismatch in Basic auth") + basic_client_id = unquote(raw_client_id) + basic_client_secret = unquote(raw_client_secret) except (ValueError, UnicodeDecodeError, binascii.Error): raise AuthenticationError("Invalid Basic authentication header") + form_client_id = form_data.get("client_id") + client_id = str(form_client_id) if form_client_id else basic_client_id + if not client_id: + raise AuthenticationError("Missing client_id") + + client = await self.provider.get_client(client_id) + if not client: + raise AuthenticationError("Invalid client_id") # pragma: no cover + + request_client_secret: str | None = None + + if client.token_endpoint_auth_method == "client_secret_basic": + if basic_client_id is None: + raise AuthenticationError("Missing or invalid Basic authentication in Authorization header") + if form_client_id and str(form_client_id) != basic_client_id: + raise AuthenticationError("Client ID mismatch in Basic auth") + request_client_secret = basic_client_secret + elif client.token_endpoint_auth_method == "client_secret_post": raw_form_data = form_data.get("client_secret") # form_data.get() can return an UploadFile or None, so we need to check if it's a string diff --git a/tests/client/test_auth.py b/tests/client/test_auth.py index be96cc8eec..9b46f927bc 100644 --- a/tests/client/test_auth.py +++ b/tests/client/test_auth.py @@ -676,10 +676,10 @@ async def test_basic_auth_token_exchange(self, oauth_provider: OAuthClientProvid assert unquote(client_id) == client_id_raw assert unquote(client_secret) == client_secret_raw - # client_secret should NOT be in body for basic auth + # Neither credential belongs in the body for basic auth (RFC 6749 2.3) content = request.content.decode() assert "client_secret=" not in content - assert "client_id=test%40client" in content # client_id still in body + assert "client_id=" not in content @pytest.mark.anyio async def test_basic_auth_refresh_token(self, oauth_provider: OAuthClientProvider, valid_tokens: OAuthToken): @@ -712,9 +712,10 @@ async def test_basic_auth_refresh_token(self, oauth_provider: OAuthClientProvide decoded = base64.b64decode(encoded_creds).decode() assert decoded == f"{client_id}:{client_secret}" - # client_secret should NOT be in body + # Neither credential belongs in the body for basic auth (RFC 6749 2.3) content = request.content.decode() assert "client_secret=" not in content + assert "client_id=" not in content @pytest.mark.anyio async def test_none_auth_method(self, oauth_provider: OAuthClientProvider):