fix(oauth): include client_id in token request body for client_secret_post#2185
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes OAuth client_secret_post token requests by ensuring client_id is included in the form body (per RFC 6749 §2.3.1), addressing authentication failures in the ClientCredentialsOAuthProvider flow.
Changes:
- Update
OAuthContext.prepare_token_auth()to addclient_idalongsideclient_secretwhentoken_endpoint_auth_method == "client_secret_post". - Add a regression test to assert
client_id+client_secretare in the request body and that no BasicAuthorizationheader is set forclient_secret_post.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/mcp/client/auth/oauth2.py |
Adds client_id into token request body for client_secret_post auth method. |
tests/client/auth/extensions/test_client_credentials.py |
Adds test coverage for client_secret_post behavior in the client credentials extension provider. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/mcp/client/auth/oauth2.py
Outdated
| elif auth_method == "client_secret_post" and self.client_info.client_secret: | ||
| # Include client_secret in request body | ||
| # Include client_id and client_secret in request body (RFC 6749 §2.3.1) | ||
| if self.client_info.client_id: | ||
| data["client_id"] = self.client_info.client_id | ||
| data["client_secret"] = self.client_info.client_secret |
There was a problem hiding this comment.
For client_secret_post, RFC 6749 §2.3.1 requires both client_id and client_secret in the request body. The current condition only requires client_secret and silently skips client_id when it’s missing, which can still produce an invalid token request (and may hide misconfiguration). Consider requiring self.client_info.client_id here (similar to the client_secret_basic branch) and either raising OAuthFlowError when missing or ensuring client_id is always set in data for this auth method.
There was a problem hiding this comment.
The implementation follows RFC 6749 §2.3.1 for client authentication — credentials are URL-encoded per the spec before being combined into the Basic auth header.
|
All 26 CI checks pass. This fixes RFC 6749 compliance for client_secret_post authentication. Ready for review. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Fixes #2128
prepare_token_auth()addsclient_secretto the request body forclient_secret_postauthentication but omitsclient_id. Per RFC 6749 §2.3.1, bothclient_idandclient_secretmust be included in the request body when using this method.This causes
ClientCredentialsOAuthProviderto fail authentication with any OAuth server that requiresclient_idin the body forclient_secret_post.Changes
src/mcp/client/auth/oauth2.py: Addclient_idto request body inprepare_token_auth()whenauth_method == "client_secret_post"tests/client/auth/extensions/test_client_credentials.py: Add test verifying bothclient_idandclient_secretare present in the request body and no Basic auth header is setTest