fix: URL-encode client credentials in Basic Auth per RFC 6749 §2.3.1#873
Conversation
RFC 6749 Section 2.3.1 requires that client credentials be encoded with the application/x-www-form-urlencoded encoding algorithm before being sent in HTTP Basic Authentication. Four Auth() methods call req.SetBasicAuth() with raw credentials, which only base64-encodes them without URL-encoding first. When client secrets contain characters like %, authorization servers that URL-decode per the RFC (e.g., Keycloak) crash with "URLDecoder: Incomplete trailing escape (%) pattern". The existing AuthorizeBasic() helper in pkg/http/http.go already applies url.QueryEscape correctly. This commit 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)
|
@muhlemmer Would appreciate a review when you get a chance. This is a small fix (one-line change per method) that's causing production issues for us — Keycloak crashes with |
There was a problem hiding this comment.
Pull request overview
This PR fixes OAuth2 client authentication interoperability by URL-encoding client_id and client_secret before using HTTP Basic Auth, aligning behavior with RFC 6749 §2.3.1 and the existing http.AuthorizeBasic() helper.
Changes:
- URL-encode client credentials before calling
req.SetBasicAuth()in fourAuth()implementations. - Add the needed
net/urlimport where required.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/oidc/token_request.go | URL-encodes client credentials in ClientCredentialsRequest.Auth before Basic Auth. |
| pkg/client/rp/relying_party.go | URL-encodes client credentials in RefreshTokenRequest.Auth before Basic Auth. |
| pkg/client/client.go | URL-encodes client credentials in RevokeRequest.Auth and DeviceAccessTokenRequest.Auth before Basic Auth. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wim07101993
left a comment
wim07101993
left a comment
There was a problem hiding this comment.
Hi, thank you for the contribution. Looks good, will merge.
github-project-automation
Bot
moved this from 👀 In review
to ✅ Done
in Product Management
|
🎉 This PR is included in version 3.47.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Four
Auth()methods callreq.SetBasicAuth()with raw client credentials. Per RFC 6749 Section 2.3.1, client credentials MUST be encoded using theapplication/x-www-form-urlencodedencoding algorithm before being sent via HTTP Basic Authentication.net/http.SetBasicAuthonly 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 likeURLDecoder: Incomplete trailing escape (%) pattern.The existing
AuthorizeBasic()helper inpkg/http/http.goalready correctly appliesurl.QueryEscape. This PR applies the same encoding to the fourAuth()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 theClientSecretBasicAuthRequestinterface, 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().