fix: only allow one authenticate method#858
Conversation
|
@wim07101993 can you please review this PR? |
| func (r RefreshTokenRequest) Auth(req *http.Request) { | ||
| if r.ClientSecret != "" { | ||
| req.SetBasicAuth(r.ClientID, r.ClientSecret) | ||
| } | ||
| } |
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?
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?
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?
There was a problem hiding this comment.
@wim07101993 @muhlemmer
Just want to follow up on this one, since it's been a while.
Could I know what is our plan on this PR?
Do we confirm the issue?
If so, do we consider to merge it into next breaking change version?
it would be great to see the problem solved. Thanks.
|
@suqin-haha thank you for the contribution. Could you have a look at my comments? |
…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()`.
5 participants
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.