oauth: reject redirects for token endpoint requests#738
Conversation
WalkthroughOAuth token requests now use a cloned HTTP client that returns redirect responses instead of following them. Tests cover 307 and 308 redirects for code exchange and token refresh, confirming redirect targets receive no requests. ChangesOAuth redirect protection
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PostToken
participant tokenClient
participant tokenEndpoint
participant redirectTarget
PostToken->>tokenClient: Execute token request
tokenClient->>tokenEndpoint: POST token request
tokenEndpoint-->>tokenClient: 307 or 308 redirect
tokenClient-->>PostToken: Return redirect response
Note over tokenClient,redirectTarget: redirectTarget is not contacted
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/oauth/flow_test.go (1)
191-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unnecessary test state variables.
The
attackerHitboolean is unnecessary and could theoretically cause a data race if the test ever encounters a timeout, as the main test thread and the HTTP handler could execute concurrently under failure conditions.
testing.T.Errorfis fully thread-safe and reliably marks the test as failed. Furthermore,defer attacker.Close()blocks the main test goroutine until all active requests (and their handlers) have completely finished. You can safely remove the boolean state and rely entirely on the implicit synchronization provided by the test server closure.
internal/oauth/flow_test.go#L191-L227: RemoveattackerHitinitialization, its assignment in the handler, and the final check block inTestExchangeCodeRejects307Redirect.internal/oauth/flow_test.go#L229-L263: RemoveattackerHitinitialization, its assignment in the handler, and the final check block inTestRefreshRejects308Redirect.♻️ Proposed refactor for both tests
For
TestExchangeCodeRejects307Redirect:- var attackerHit bool - attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - attackerHit = true t.Errorf("redirect target received unexpected request") })) defer attacker.Close()if err == nil { t.Fatal("expected redirect error") } - - if attackerHit { - t.Fatal("redirect target received credential-bearing request") - } }For
TestRefreshRejects308Redirect:- var attackerHit bool - attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - attackerHit = true t.Errorf("redirect target received unexpected request") })) defer attacker.Close()if err == nil { t.Fatal("expected redirect error") } - - if attackerHit { - t.Fatal("redirect target received credential-bearing request") - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/flow_test.go` around lines 191 - 227, Remove the attackerHit state and its assignment and final assertion from TestExchangeCodeRejects307Redirect in internal/oauth/flow_test.go lines 191-227, relying on the handler’s testing.T.Errorf. Apply the same removal in TestRefreshRejects308Redirect at lines 229-263, leaving the redirect rejection assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/oauth/flow_test.go`:
- Around line 191-227: Remove the attackerHit state and its assignment and final
assertion from TestExchangeCodeRejects307Redirect in internal/oauth/flow_test.go
lines 191-227, relying on the handler’s testing.T.Errorf. Apply the same removal
in TestRefreshRejects308Redirect at lines 229-263, leaving the redirect
rejection assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f73ed8c-0c69-4e50-8918-94702282dd06
📒 Files selected for processing (2)
internal/oauth/flow.gointernal/oauth/flow_test.go
|
Thanks for tackling this, @shriza1991 - the redirect-replay risk on OAuth token requests was a real and worthwhile thing to fix. Since you opened this, #741 landed (merged as 974fc03) and closed the same issue #729, covering the token exchange/refresh path plus the device-authorization and poll flows in one pass, so it's now fully handled on main (and this branch has gone into conflict). Closing as already-resolved, but the instinct and the regression-test approach were spot on. Please keep the contributions coming - the surrounding endpoint-validation work is a good next area. Really appreciate you taking the time on a security fix. |
Summary
This PR hardens OAuth token requests by rejecting HTTP redirects.
Changes
CheckRedirectpolicy to prevent following redirects.Tests
Summary by CodeRabbit