Default to client_secret_basic when server omits token_endpoint_auth_methods_supported#1594
Open
bhosmer-ant wants to merge 3 commits intomainfrom
Open
Default to client_secret_basic when server omits token_endpoint_auth_methods_supported#1594bhosmer-ant wants to merge 3 commits intomainfrom
bhosmer-ant wants to merge 3 commits intomainfrom
Conversation
…methods_supported Per RFC 8414 §2, when an authorization server's metadata omits token_endpoint_auth_methods_supported, the default is client_secret_basic, not client_secret_post. RFC 6749 §2.3.1 also requires servers to support HTTP Basic authentication for clients with a secret, making it the safest default. Also: honor the DCR-returned token_endpoint_auth_method even when supportedMethods is empty — previously the early return swallowed the DCR hint in this scenario. Related to #951 (PR #1022 fixed the DCR-preference case when metadata IS present, but left this RFC-default case untouched).
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
The mock auth servers in sse.test.ts validated client credentials by checking body params (client_secret_post). With the default changed to client_secret_basic, credentials now arrive in the Authorization header instead. Updated the three affected mock servers to parse and validate HTTP Basic auth headers.
Contributor
|
Sounds good. I think @pcarlton and I noticed this a while ago so good to be fixed. Will approve in a bit.
|
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When an authorization server's metadata omits
token_endpoint_auth_methods_supported, the SDK currently defaults toclient_secret_post. Per RFC 8414 §2, the default in this case isclient_secret_basic:RFC 6749 §2.3.1 also requires compliant servers to support HTTP Basic for clients with a secret, making it the safest default.
This PR:
client_secret_posttoclient_secret_basicwhensupportedMethodsis emptytoken_endpoint_auth_methodwhensupportedMethodsis empty (previously the early return swallowed the DCR hint in this scenario)supportedMethodsscenariosRelated to #951 — PR #1022 fixed the DCR-preference case when server metadata is present, but left the omitted-metadata default untouched. This PR addresses the remaining case.
Motivation and Context
A server that doesn't publish
token_endpoint_auth_methods_supportedbut follows RFC 6749 is guaranteed to accept HTTP Basic auth for confidential clients. A server that only acceptsclient_secret_postand doesn't advertise that fact is non-compliant — our default shouldn't cater to that case.How Has This Been Tested?
selectClientAuthMethodwith emptysupportedMethods(confidential client, public client, DCR-hint-present)exchangeAuthorization/refreshAuthorizationintegration tests to verify HTTP Basic auth header is sent when no metadata is providedBreaking Changes
Behavioral change for clients talking to servers that omit
token_endpoint_auth_methods_supportedfrom their metadata: the SDK will now send credentials via HTTP Basic auth (Authorization header) instead of in the request body. This aligns with RFC 8414/6749 and should work against any compliant server. Servers that were only accepting body-param auth without advertising it will need to either advertiseclient_secret_postin their metadata or accept Basic auth.Types of changes
Checklist
Additional context
Follow-up identified:
applyBasicAuthat line ~345 does rawbtoa(\${clientId}:${clientSecret}`)without percent-encoding the components first, as required by RFC 6749 §2.3.1. This is a pre-existing bug that this PR surfaces more frequently by makingclient_secret_basicthe default. The fix isencodeURIComponent()on both components beforebtoa()`. Tracked separately to keep this PR focused.