fix: only allow one authenticate method#858
Open
suqin-haha wants to merge 7 commits intozitadel:mainfrom
Open
Conversation
2 tasks
muir
reviewed
Mar 18, 2026
muir
reviewed
Mar 26, 2026
Collaborator
|
@wim07101993 can you please review this PR? |
wim07101993
reviewed
Apr 2, 2026
Comment on lines
-819
to
-823
| func (r RefreshTokenRequest) Auth(req *http.Request) { | ||
| if r.ClientSecret != "" { | ||
| req.SetBasicAuth(r.ClientID, r.ClientSecret) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Removing this method creates a breaking change. Both in changing the public api and removing the implementation of the ClientSecretBasicAuthRequest interface. Wouldn't there be another solution without breaking changes?
Author
There was a problem hiding this comment.
it's a wrong behavior we should mark the release as broken or bug ASAP.
I don't have any idea to fix it without breaking change. Does any one have a good migration idea?
Author
There was a problem hiding this comment.
any update on that issue?
feel free to bring up migration idea.
Or we might need to make it as a break change release?
Member
|
@suqin-haha thank you for the contribution. Could you have a look at my comments? |
wim07101993
pushed a commit
that referenced
this pull request
Apr 17, 2026
…873) ## Summary Four `Auth()` methods call `req.SetBasicAuth()` with raw client credentials. Per [RFC 6749 Section 2.3.1](https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1), client credentials MUST be encoded using the `application/x-www-form-urlencoded` encoding algorithm before being sent via HTTP Basic Authentication. `net/http.SetBasicAuth` only base64-encodes the credentials — it does not URL-encode them first. When a client secret contains characters like `%`, authorization servers that URL-decode per the RFC (e.g., Keycloak) fail with errors like `URLDecoder: Incomplete trailing escape (%) pattern`. The existing `AuthorizeBasic()` helper in `pkg/http/http.go` already correctly applies `url.QueryEscape`. This PR applies the same encoding to the four `Auth()` methods that were missing it: - `RefreshTokenRequest.Auth` (`pkg/client/rp/relying_party.go`) - `RevokeRequest.Auth` (`pkg/client/client.go`) - `DeviceAccessTokenRequest.Auth` (`pkg/client/client.go`) - `ClientCredentialsRequest.Auth` (`pkg/oidc/token_request.go`) ## Related This is distinct from #857 / #858 (duplicate credentials issue), though both stem from the same `Auth()` methods. This PR fixes the encoding bug without removing the `ClientSecretBasicAuthRequest` interface, so it is not a breaking change. ## Testing All existing tests pass. The fix is a one-line change per method, consistent with the pattern already used by `AuthorizeBasic()`.
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
5 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.
according to the RFC 6749 §2.3
The client MUST NOT use more than one authentication method in each request.this PR resolve issue #857
NOTE: it's a breaking change since it removes
ClientSecretBasicAuthRequestinterface andfunc Auth(code.test:
added united test
Definition of Ready
- [ ] PR is linked to the corresponding user story- [ ] All open todos and follow ups are defined in a new ticket and justified- [ ] Deviations from the acceptance criteria and design are agreed with the PO and documented.- [ ] Where possible E2E tests are implemented- [ ] Documentation/examples are up-to-date- [ ] Functionality of the acceptance criteria is checked manually on the dev system.