Conversation
WalkthroughThe OAuth2 round tripper implementation has been refactored to handle form-encoded request bodies instead of always appending parameters to URL query strings. The Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/commonhttp/client_oauth2.go`:
- Around line 349-350: When Body is nil the code copies newReq.URL.Query() into
q and then serializes q into the form body, but leaves newReq.URL.RawQuery
intact so parameters are sent twice and sensitive params remain in the URL;
after seeding q from newReq.URL.Query() (the branch where Body == nil and
variable q is assigned from newReq.URL.Query()), clear the URL query (set
newReq.URL.RawQuery to an empty string) so the parameters exist only in the
request body; apply the same change to the other code path that seeds q and
serializes it (the block around the code that handles form encoding—references
to newReq, q, Body, and RawQuery) so no params are duplicated or left in the
URL.
- Around line 334-335: The code allows client_auth_method values
"client_secret_post", "client_secret_jwt", and "private_key_jwt" to fall through
and send a partially-authenticated request when isFormBody is false; instead,
detect when client authentication requires an application/x-www-form-urlencoded
body (using isFormBody which checks newReq.Method and
newReq.Header.Get("Content-Type")) and fail fast by returning an error before
forwarding the request if !isFormBody and client_auth_method is one of those
three values. Update the branch that currently appends only client_id (and the
similar code paths) to validate client_auth_method against isFormBody and return
an explicit error rather than forwarding a malformed request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19b19617-398e-4c4a-bc8f-fd32a3d00a07
📒 Files selected for processing (2)
pkg/commonhttp/client_oauth2.gopkg/commonhttp/client_oauth2_test.go
| isFormBody := newReq.Method == http.MethodPost && | ||
| strings.HasPrefix(newReq.Header.Get("Content-Type"), "application/x-www-form-urlencoded") |
There was a problem hiding this comment.
Fail fast instead of sending a partially-authenticated request.
For client_secret_post, client_secret_jwt, and private_key_jwt, the secret/assertion is only added when isFormBody is true. Lines 397-401 still forward the non-form path, so a malformed request can go out with only client_id appended and no actual client authentication.
🐛 Proposed fix
isFormBody := newReq.Method == http.MethodPost &&
strings.HasPrefix(newReq.Header.Get("Content-Type"), "application/x-www-form-urlencoded")
+
+ requiresFormAuth :=
+ (t.ClientSecretPost != nil && *t.ClientSecretPost != "") ||
+ (t.ClientSecretJWT != nil && *t.ClientSecretJWT != "") ||
+ (t.ClientAssertion != nil && t.ClientAssertionType != nil)
+ if requiresFormAuth && !isFormBody {
+ return nil, errors.New("oauth2 client authentication requires POST application/x-www-form-urlencoded")
+ }Also applies to: 359-387, 397-401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/commonhttp/client_oauth2.go` around lines 334 - 335, The code allows
client_auth_method values "client_secret_post", "client_secret_jwt", and
"private_key_jwt" to fall through and send a partially-authenticated request
when isFormBody is false; instead, detect when client authentication requires an
application/x-www-form-urlencoded body (using isFormBody which checks
newReq.Method and newReq.Header.Get("Content-Type")) and fail fast by returning
an error before forwarding the request if !isFormBody and client_auth_method is
one of those three values. Update the branch that currently appends only
client_id (and the similar code paths) to validate client_auth_method against
isFormBody and return an explicit error rather than forwarding a malformed
request.
| } else { | ||
| q = newReq.URL.Query() |
There was a problem hiding this comment.
Clear the URL query after reusing it as the form source.
If Body is nil, Line 350 seeds q from newReq.URL.Query(), and Lines 390-396 then serialize the same values into the new form body. Because RawQuery is left intact, those params are sent twice, and any caller-supplied credentials/assertions remain exposed in the URL.
🐛 Proposed fix
if isFormBody && newReq.Body != nil {
bodyBytes, err := io.ReadAll(newReq.Body)
if err != nil {
return nil, fmt.Errorf("reading request body: %w", err)
}
newReq.Body.Close()
q, err = url.ParseQuery(string(bodyBytes))
if err != nil {
return nil, fmt.Errorf("parsing form body: %w", err)
}
} else {
q = newReq.URL.Query()
+ if isFormBody {
+ newReq.URL.RawQuery = ""
+ }
}Also applies to: 390-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/commonhttp/client_oauth2.go` around lines 349 - 350, When Body is nil the
code copies newReq.URL.Query() into q and then serializes q into the form body,
but leaves newReq.URL.RawQuery intact so parameters are sent twice and sensitive
params remain in the URL; after seeding q from newReq.URL.Query() (the branch
where Body == nil and variable q is assigned from newReq.URL.Query()), clear the
URL query (set newReq.URL.RawQuery to an empty string) so the parameters exist
only in the request body; apply the same change to the other code path that
seeds q and serializes it (the block around the code that handles form
encoding—references to newReq, q, Body, and RawQuery) so no params are
duplicated or left in the URL.
1 participant
Summary by CodeRabbit
Bug Fixes