Uh oh!
There was an error while loading. Please reload this page.
fix(client): don't send client_id in token body under client_secret_b… - #3175
Draft
manjunathbhaskar wants to merge 2 commits into
Draft
fix(client): don't send client_id in token body under client_secret_b…#3175manjunathbhaskar wants to merge 2 commits into
manjunathbhaskar wants to merge 2 commits into
Conversation
…et_basic RFC 6749 section 2.3 says client credentials should not be in the request body when they are already in the Authorization header. The basic auth branch stripped client_secret but left client_id, so strict token endpoints (Keycloak, Okta in strict mode) reject the request as presenting two auth methods at once. Drop client_id too, and flip the two basic auth tests plus the refresh test that asserted the old behavior. Fixesmodelcontextprotocol#3138
manjunathbhaskarforce-pushed
the
fix/basic-auth-client-id-body
branch
from
August 8, 2026 23:38
6a5cbe6 to
461546bCompareauthenticate_request() required client_id in the form body unconditionally, even for client_secret_basic clients that already present it via the Authorization header. Stripping client_id from the body for Basic auth (the actual fix here) is correct per RFC 6749 section 2.3, but broke our own server: it had nothing to look the client up by. Decode the Basic header up front and fall back to it when the body doesn't have client_id. The three token request models (authorization_code, refresh_token, jwt-bearer) had the same body-required assumption baked in; client_id is now optional there too, backfilled from the already- authenticated client before the code/token ownership checks that rely on it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RFC 6749 section 2.3 requires that with HTTP Basic auth, client credentials must not also appear in the request body.
prepare_token_authstrippedclient_secretfrom the body for theclient_secret_basicbranch but leftclient_idin, so strict token endpoints (Keycloak, Okta in strict mode, and the RFC 6749 compliance test suite) reject the request as presenting two authentication methods at once.Fixes#3138
Two existing tests asserted the old behavior explicitly, updated both to assert the corrected one, and added the same check to the refresh token test for symmetry.
Server-side companion fix
The client-only change above breaks this SDK's own reference auth server:
ClientAuthenticator.authenticate_request()requiredclient_idin the form body unconditionally, and the three token request models (AuthorizationCodeRequest,RefreshTokenRequest,JwtBearerRequest) had the same assumption baked in as a required field. A client that stops sendingclient_idin the body got rejected by our own server withinvalid_client: Missing client_id— a regression, not a fix, without this half.Fixed both:
authenticate_request()decodes the Basic header up front and falls back to theclient_idembedded there when the body omits it (still cross-checks against a body-supplied one if present).client_idis now optional on the three token request models;handle()backfills it from the already-authenticated client before the code/token ownership checks that rely on it.How Has This Been Tested?
tests/interaction/auth/test_authorize_token.py::test_a_client_with_a_secret_authenticates_the_token_request_with_http_basic— the integration test that exercises client and server together, fails without the server-side fix, passes with it.uv run pytest— 5580 passed, 0 failed, 10 skipped, 1 xfailed.ruff check,ruff format --check,mypyclean on touched files.Breaking Changes
None for callers.
client_idis optional on the token request models now but always resolved (body or Basic header) before use.Types of changes
Checklist
Additional context
Two other independent attempts at the client-only half (#3159, #3160) hit this same server-side incompatibility in CI and were closed without a fix — this PR includes the missing half.