Reject cross-host redirects and HTTPS→HTTP downgrades entirely #95
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
From security review on PR #93 (review #2870, finding #1):
Currently the client strips the Authorization header on cross-host redirects or protocol downgrades (HTTPS→HTTP), but still follows the redirect. This can lead to consuming responses from untrusted or downgraded endpoints.
Proposed: Reject cross-host redirects and HTTPS→HTTP downgrades entirely (return an error from CheckRedirect) rather than just stripping auth. This provides stronger integrity/confidentiality guarantees.
Consideration: This is a behavioral change that could break legitimate redirect flows (e.g., GitHub sometimes redirects to CDN hosts for raw content). Needs careful evaluation of which redirect patterns are expected vs. dangerous.
Ref: PR #93 review #2870
Plan: Reject cross-host redirects and HTTPS→HTTP downgrades entirely### ProblemThe GitHub client's
defaultCheckRedirectcurrently:1. ✅ Rejects HTTPS→HTTP downgrades (returns error)2. ⚠️ Strips Authorization on cross-host redirects but still follows themFollowing cross-host redirects (even without auth) can lead to consuming responses from untrusted endpoints, which could be exploited for response manipulation or information disclosure.Additionally, the Gitea client has noCheckRedirectpolicy at all, meaning it uses Go's default behavior (follows up to 10 redirects, forwarding all headers including Authorization).### Constraints- Must not break normal API usage (contents API, diff API, PR API, review submission)- Both GitHub and Gitea clients use JSON-based API endpoints that should never redirect cross-host in normal operation- The review-bot never fetches raw blob downloads that might legitimately redirect to CDN hosts- Change must be opt-in or at least clearly documented for anyone extending the client### Proposed ApproachChangedefaultCheckRedirectin the GitHub client to reject (return error) on cross-host redirects instead of stripping auth and following:gofunc defaultCheckRedirect(req *http.Request, via []*http.Request) error { if len(via) >= 10 { return fmt.Errorf("stopped after 10 redirects") } if len(via) == 0 { return nil } prev := via[len(via)-1] // Reject HTTPS→HTTP downgrade if prev.URL.Scheme == "https" && req.URL.Scheme == "http" { return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s → %s)", prev.URL.Host, req.URL.Host) } // Reject cross-host redirect entirely if req.URL.Host != prev.URL.Host { return fmt.Errorf("refusing redirect: cross-host (%s → %s)", prev.URL.Host, req.URL.Host) } return nil}AdddefaultCheckRedirectto the Gitea client with the same policy, applied inNewClientandSetHTTPClient(nil).### Files to Change1.github/client.go— UpdatedefaultCheckRedirectto return error on cross-host redirect instead of stripping auth2.github/client_test.go— UpdateTestDefaultCheckRedirect_StripsAuthOnCrossHost→ rename toTestDefaultCheckRedirect_RejectsCrossHostand assert error returned3.gitea/client.go— AdddefaultCheckRedirectfunction, wire intoNewClientandSetHTTPClient4.gitea/client_test.go— Add tests for the new redirect policy### Error Cases- Cross-host redirect → clear error message identifying source and target hosts- HTTPS→HTTP downgrade → clear error message (already implemented for GitHub, new for Gitea)- Same-host same-scheme redirect → allowed (normal behavior for path normalization, trailing slash, etc.)### Edge Cases-len(via) == 0— defensive guard, allow redirect (matches current behavior)- Port differences on same hostname (e.g.api.github.comvsapi.github.com:443) — Go normalizes these, butHostfield includes port when non-default; this meanshost:443≠hosteven though they're the same. Decision: compare hosts as-is (strict). This is more secure — if a redirect goes to an explicit port variant, it's unusual and worth rejecting.- Redirect chain: first to same-host, then to cross-host — the function checks againstvia[len(via)-1](the immediately previous request), so this is correctly caught at the second hop.### Testing Strategy1. Unit tests fordefaultCheckRedirectin both packages: - Same-host same-scheme → allowed - Cross-host → error - HTTPS→HTTP → error - Same-host HTTP→HTTP → allowed (no downgrade) - 10+ redirects → error2. Existing integration-style tests should continue passing (they use mock servers on same host)### Open Questions- Port normalization: Go'sRequest.URL.Hostincludes port for non-standard ports. Shouldexample.com==example.com:443? Current decision is strict comparison (no normalization) — more secure, simpler. If a legitimate same-origin redirect adds an explicit port, it would be rejected. This seems acceptable for an API client that only talks to known endpoints.### Completion Checklist1. [ ]defaultCheckRedirectingithub/client.goreturns error on cross-host redirect2. [ ] Error message clearly identifies source and target hosts3. [ ] Same-host redirects still work (no regression)4. [ ] HTTPS→HTTP rejection preserved with updated message format5. [ ] Gitea client getsdefaultCheckRedirectwith identical logic6. [ ] GiteaNewClientwires CheckRedirect7. [ ] GiteaSetHTTPClient(nil)restores CheckRedirect8. [ ] All existing tests pass (no regressions)9. [ ] New tests cover cross-host rejection for both clients10. [ ]go test ./...passes clean