Skip to content

fix(oauth): conditionally set client_id based on auth method #2968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
imdominicreed wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(oauth): conditionally set client_id based on auth method #2968

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sdk/auth/oauth/oauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ func getAccessTokenRequest(tokenEndpoint, dpopNonce string, scopes []string, cli

formData := url.Values{}
formData.Set("grant_type", "client_credentials")
formData.Set("client_id", clientCredentials.ClientID)
if len(scopes) > 0 {
formData.Set("scope", strings.Join(scopes, " "))
}
Expand All @@ -95,6 +94,7 @@ func setClientAuth(cc ClientCredentials, formData *url.Values, req *http.Request
if err != nil {
return fmt.Errorf("error building signed auth token to authenticate with IDP: %w", err)
}
formData.Set("client_id", cc.ClientID)
formData.Set("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer")
formData.Set("client_assertion", string(signedToken))
default:
Expand Down Expand Up @@ -333,7 +333,7 @@ func DoCertExchange(ctx context.Context, tokenEndpoint string, exchangeInfo Cert
}

func getCertExchangeRequest(ctx context.Context, tokenEndpoint string, clientCredentials ClientCredentials, exchangeInfo CertExchangeInfo, key jwk.Key) (*http.Request, error) {
data := url.Values{"grant_type": {"password"}, "client_id": {clientCredentials.ClientID}, "username": {""}, "password": {""}}
data := url.Values{"grant_type": {"password"}, "username": {""}, "password": {""}}
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

While removing the unconditional client_id here is correct, it reveals a critical logic bug in this function's order of operations.

The request body is created from data on line 342, but setClientAuth—which should conditionally add client_id for client assertion authentication—is called on line 355, after the body has been created. This means any modifications setClientAuth makes to the data map will not be included in the request body, breaking client assertion authentication.

To fix this, this function should be refactored to follow the pattern in getAccessTokenRequest:

  1. Create the http.Request with a nil body.
  2. Call setClientAuth to populate headers and/or the url.Values map.
  3. Encode the url.Values and set the req.Body.

A similar bug also exists in getTokenExchangeRequest and should be addressed.


for _, a := range exchangeInfo.Audience {
data.Add("audience", a)
Expand Down
Loading