feat: reject cross-host redirects and HTTPS→HTTP downgrades (#95) #111
Reference in New Issue
Block a user
Delete Branch "review-bot-issue-95"
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?
Summary
Reject cross-host redirects and HTTPS→HTTP downgrades entirely in both GitHub and Gitea HTTP clients, rather than just stripping auth headers.
Changes
github/client.godefaultCheckRedirectfunction that rejects:defaultCheckRedirectintoNewClientSetHTTPClient(nil)to restore the policygitea/client.godefaultCheckRedirectfunctionNewClientSetHTTPClient(nil)to restore the policyTests (
github/client_test.go,gitea/client_test.go)TestNewClient_HasCheckRedirect— verify policy is wiredTestDefaultCheckRedirect_RejectsHTTPSToHTTP— downgrade rejectedTestDefaultCheckRedirect_RejectsCrossHost— cross-host rejectedTestDefaultCheckRedirect_AllowsSameHost— same-host allowed, auth preservedTestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP— no false positive on HTTP→HTTP same-hostTestDefaultCheckRedirect_RejectsTooManyRedirects— 10+ redirects rejectedTestDefaultCheckRedirect_EmptyViaAllowed— defensive nil/empty guardTestSetHTTPClient_NilRestoresDefault— nil restore includes policySecurity Rationale
Previously, cross-host redirects only stripped the Authorization header but still followed the redirect. This meant the client could consume responses from untrusted endpoints, which could be exploited for response manipulation. Now the client fails fast with a clear error message.
Behavioral Change
This is intentionally more restrictive. The review-bot uses JSON-based API endpoints (contents API, PR API, review API) that should never redirect cross-host in normal operation. If a legitimate redirect flow is needed in the future, callers can provide a custom
http.ClientviaSetHTTPClient.Closes #95
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1e0959b0)Sonnet Review
Summary
The PR correctly implements a restrictive redirect policy for both GitHub and Gitea HTTP clients, rejecting HTTPS→HTTP downgrades and cross-host redirects. The implementation is correct, well-documented, and thoroughly tested. CI passes.
Findings
gitea/client.godefaultCheckRedirectfunction is duplicated verbatim betweengitea/client.goandgithub/client.go. Since these are separate packages this is unavoidable without introducing a shared internal package, but it means any future change must be made in both places. A comment noting the duplication and pointing to the sibling package would help future maintainers. Alternatively, aninternal/httppolicypackage could house the shared function — though that adds structural complexity for a single function.gitea/client.golen(via) >= 1whenCheckRedirectis called during an actual redirect, but the testTestDefaultCheckRedirect_EmptyViaAllowedcalls the function directly withnil. The guard is appropriate for testability and defensive coding; the comment could be tightened to say 'Guard for direct invocation in tests and any future callers'.Recommendation
APPROVE — Approve. The security improvement is correct and intentional. The
defaultCheckRedirectfunction logic is sound: the 10-redirect limit check is evaluated before thelen(via) == 0guard (so a 10-element via slice correctly returns an error before the zero-length check runs), the HTTPS→HTTP downgrade check correctly usesprevnotreq, and the cross-host check usesreq.URL.Host != prev.URL.Hostwhich correctly handles ports as part of the host string. Tests are comprehensive, covering all code paths including the defensive nil/empty via case. The duplicate implementation across packages is a minor maintenance concern but acceptable given the project's package structure.Review by sonnet
Evaluated against
1e0959b0@@ -81,0 +85,4 @@func defaultCheckRedirect(req *http.Request, via []*http.Request) error {if len(via) >= 10 {return fmt.Errorf("stopped after 10 redirects")}[MINOR] The
defaultCheckRedirectfunction is duplicated verbatim betweengitea/client.goandgithub/client.go. Since these are separate packages this is unavoidable without introducing a shared internal package, but it means any future change must be made in both places. A comment noting the duplication and pointing to the sibling package would help future maintainers. Alternatively, aninternal/httppolicypackage could house the shared function — though that adds structural complexity for a single function.@@ -81,0 +90,4 @@// zero-length to avoid panic on index out of range.if len(via) == 0 {return nil}[NIT] The comment 'Guard: net/http guarantees len(via) >= 1 but defend against zero-length to avoid panic on index out of range' is accurate but slightly misleading: net/http does guarantee
len(via) >= 1whenCheckRedirectis called during an actual redirect, but the testTestDefaultCheckRedirect_EmptyViaAllowedcalls the function directly withnil. The guard is appropriate for testability and defensive coding; the comment could be tightened to say 'Guard for direct invocation in tests and any future callers'.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1e0959b0)Gpt Review
Summary
The changes correctly add a strict redirect policy to both GitHub and Gitea clients, wire it into constructors and SetHTTPClient(nil), and include comprehensive tests. The implementation follows idiomatic Go patterns and repository conventions, and CI passed.
Findings
gitea/client.gogitea/client.gogithub/client.gogithub/client.goRecommendation
APPROVE — Overall, this is a solid, security-improving change that aligns with Go patterns and repository conventions. The redirect policy is clearly defined, properly wired into NewClient and SetHTTPClient(nil), and thoroughly tested, including downgrade, cross-host, same-host allowances, maximum redirect limits, and default restoration. CI has passed. As a small improvement, consider moving the identical defaultCheckRedirect implementation to a shared internal package to avoid duplication and keep behavior consistent across clients. Also consider using ASCII arrows in error messages for broader log compatibility. With these minor suggestions, the PR is good to merge.
Review by gpt
Evaluated against
1e0959b0@@ -78,18 +78,61 @@ type Client struct {MaxDiffSize int64[MINOR] defaultCheckRedirect duplicates identical logic in github/client.go. Consider consolidating shared redirect policy in an internal package to avoid duplication and keep behavior consistent (see internal/ packages pattern).
@@ -81,0 +95,4 @@// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.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)}[NIT] Error messages include a Unicode arrow ("→"). While harmless, consider using ASCII ("->") for environments that may not render Unicode consistently in logs.
@@ -107,6 +107,32 @@ type Client struct {now func() time.Time[MINOR] defaultCheckRedirect duplicates identical logic in gitea/client.go. A shared helper under internal/ could reduce duplication and ensure consistent policy across clients.
@@ -110,0 +120,4 @@if len(via) == 0 {return nil}prev := via[len(via)-1][NIT] Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
1e0959b0)Security Review
Summary
Security posture is improved by rejecting cross-host redirects and HTTPS→HTTP downgrades, and by ensuring the default policy is restored when resetting the HTTP client. Tests robustly cover the new behavior. One minor hardening opportunity exists around case-insensitive scheme comparison.
Findings
gitea/client.gogithub/client.goRecommendation
APPROVE — Approve this change. It meaningfully strengthens redirect handling by rejecting cross-host redirects and HTTPS→HTTP downgrades, reducing the risk of credential leakage and untrusted response consumption. For additional hardening, normalize or compare URL schemes case-insensitively (e.g., strings.EqualFold or strings.ToLower) in the downgrade check to avoid edge cases with mixed/uppercase schemes in Location headers. Optionally consider normalizing hosts before comparison for consistency, though the current strictness is safe from a security perspective.
Review by security
Evaluated against
1e0959b0@@ -81,0 +90,4 @@// zero-length to avoid panic on index out of range.if len(via) == 0 {return nil}[MINOR] Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). If a server returns a Location with an uppercase or mixed-case scheme (e.g., "HTTP"), this may bypass the downgrade check depending on parsing behavior. Use strings.EqualFold or normalize schemes to lowercase before comparison.
@@ -110,0 +119,4 @@// zero-length to avoid panic on index out of range.if len(via) == 0 {return nil}[MINOR] Scheme comparison is case-sensitive (prev.URL.Scheme == "https" && req.URL.Scheme == "http"). A mixed/uppercase scheme in a redirect could bypass the downgrade check. Use strings.EqualFold or normalize schemes to lowercase before comparison.
Self-Review: PR #111
Self-review against
1e0959b077Phase 1: Independent Findings
None — diff looks clean. The implementation is correct, well-structured, and thoroughly tested. The
defaultCheckRedirectfunction logic is sound (max redirects → empty-via guard → downgrade check → cross-host check), and theSetHTTPClient(nil)restoration path properly reinstates the secure defaults. Tests cover all branches.Phase 2: Prior Review Verification
defaultCheckRedirectacross packagesinternal/package, reasonable for a single functionlen(via) >= 1guaranteenet/httperrors are rarely piped to ASCII-only consumers in this codebasenet/httpnormalizes URL schemes to lowercase during parsing (url.Parsealways lowercases scheme), so this is safe in practice. Hardening withstrings.EqualFoldwould be belt-and-suspenders.Assessment: ✅ Clean
All three reviewers (sonnet, gpt, security) approved the PR. The outstanding findings are all acknowledged NITs or minor hardening suggestions that don't affect correctness or security in practice. The core security improvement (rejecting cross-host redirects and HTTPS→HTTP downgrades) is properly implemented and tested. Ready to proceed.
Fix Plan against
1e0959b077:Findings & Actions
defaultCheckRedirectduplicated across packageslen(via) >= 1guardurl.Parsenormalizes schemes to lowercase (tested with Go 1.26.2 —"HTTP://..."parses to scheme"http"). The comparison is safe.→with->in both packagesImplementation Details
Fix #2 (comment wording): In both
gitea/client.goandgithub/client.go, change:to:
Fix #4 (Unicode arrows): In both packages, replace
→with->in error format strings and comments.Push-back #1 (duplication): Reply explaining that for a single ~25-line function, an
internal/package adds structural complexity not warranted here. Add a comment noting the sibling implementation.Push-back #3 (scheme case): Reply with verification that Go's
url.Parsenormalizes schemes to lowercase, makingstrings.EqualFoldunnecessary.Acknowledged — I've added cross-reference comments pointing to the sibling implementation in both files. An
internal/package would add structural overhead for a single function (~25 lines), so I'm keeping the duplication with clear comments noting it must be kept in sync. If this function grows or we add more shared HTTP policy, we can extract then.Verified: Go's
url.Parsenormalizes schemes to lowercase. Tested with Go 1.26.2:Since
net/http's redirect handling parses the Location header throughurl.Parse, the scheme will always be lowercase by the timeCheckRedirectis called. The case-sensitive comparison is safe.Adding
strings.EqualFoldhere would be misleading — it would suggest the normalization doesn't happen, when it does.Sonnet Review
Summary
This PR correctly implements a secure redirect policy for both the GitHub and Gitea HTTP clients. The logic is sound, the duplication is explicitly documented and justified, and the test coverage is thorough. CI passes.
Findings
gitea/client.godefaultCheckRedirectfunction usesfmt.Errorffor all error paths, but none of these errors wrap an underlying cause (no%w). Per the error-handling patterns,fmt.Errorfwithout%wis fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting thaterrors.Newcould be used instead for the static messages (e.g.stopped after 10 redirects) since there's no format interpolation needed there, thoughfmt.Errorfis not wrong.gitea/client.goChanges here must be mirrored therebut there is no enforcement mechanism (no shared test, no lint rule, no CI check). This is a maintenance risk. A follow-up to extract this into a sharedinternal/package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but theinternal/pattern exists precisely for this use case.Recommendation
APPROVE — Approve. The security fix is correct and necessary — cross-host redirects were previously only stripping auth headers rather than refusing outright, which is the right hardening. The
defaultCheckRedirectlogic correctly handles all edge cases: HTTPS→HTTP downgrade, cross-host, too many redirects, and the empty-via guard. Tests are comprehensive and idiomatic. The duplication between packages is a mild design concern but is explicitly documented and the packages are intentionally separate. A future refactor to extract this into aninternal/redirectpolicypackage would be cleaner, but is not required for this PR to be safe and correct.Review by sonnet
Evaluated against
7de6fdd9@@ -80,1 +80,4 @@// defaultCheckRedirect is the redirect policy used by NewClient.// NOTE: This function is intentionally duplicated in github/client.go (and vice versa)// because the packages are separate. Changes here must be mirrored there.[NIT] The duplication comment says
Changes here must be mirrored therebut there is no enforcement mechanism (no shared test, no lint rule, no CI check). This is a maintenance risk. A follow-up to extract this into a sharedinternal/package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but theinternal/pattern exists precisely for this use case.@@ -81,0 +93,4 @@if len(via) == 0 {return nil}prev := via[len(via)-1][NIT] The
defaultCheckRedirectfunction usesfmt.Errorffor all error paths, but none of these errors wrap an underlying cause (no%w). Per the error-handling patterns,fmt.Errorfwithout%wis fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting thaterrors.Newcould be used instead for the static messages (e.g.stopped after 10 redirects) since there's no format interpolation needed there, thoughfmt.Errorfis not wrong.Security Review
Summary
Security posture is improved by enforcing a strict redirect policy (rejecting cross-host redirects and HTTPS→HTTP downgrades) in both GitHub and Gitea clients. The implementation is correct, follows secure defaults, and includes thorough tests.
Recommendation
APPROVE — Approve as-is. The new CheckRedirect policy prevents credential leakage and SSRF-like redirect abuse by failing fast on cross-host and HTTPS→HTTP redirects, while retaining safe same-host behavior and a redirect cap. Tests cover key scenarios, and CI has passed.
Review by security
Evaluated against
7de6fdd9Gpt Review
Summary
The changes introduce a strict, security-minded redirect policy for both GitHub and Gitea clients and wire it into constructors and SetHTTPClient(nil). The implementation is correct, idiomatic, covered by thorough tests, and aligns with the stated security rationale. CI passed.
Findings
gitea/client.gogithub/client.goRecommendation
APPROVE — Overall, this PR is well-implemented and improves security by rejecting cross-host redirects and HTTPS→HTTP downgrades. The redirect policy is correctly applied in both clients and restored in SetHTTPClient(nil), and tests comprehensively verify behaviors. As a minor robustness improvement, consider using case-insensitive comparisons (strings.EqualFold) for URL scheme and host in defaultCheckRedirect to handle atypical casing in Location headers. Otherwise, this is ready to merge.
Review by gpt
Evaluated against
7de6fdd9@@ -78,18 +78,63 @@ type Client struct {MaxDiffSize int64[NIT] defaultCheckRedirect compares URL schemes and hosts with case-sensitive equality. While practical, schemes and hosts are case-insensitive by spec; using strings.EqualFold for scheme/host comparisons would make this more robust in edge cases.
@@ -107,6 +107,34 @@ type Client struct {now func() time.Time[NIT] defaultCheckRedirect uses case-sensitive comparisons for scheme and host. Consider strings.EqualFold to be resilient to unusual case in redirect URLs.
Self-Review: PR #111
Self-review against
7de6fdd9ecPhase 1: Independent Findings
None — diff looks clean. The
defaultCheckRedirectimplementation is correct and well-structured:prev.URL.Schemevsreq.URL.Schemereq.URL.Host != prev.URL.Hostwhich handles ports correctlySetHTTPClient(nil)properly restores secure defaults with both timeout and CheckRedirectPhase 2: Prior Review Verification
defaultCheckRedirectacross packageslen(via) >= 1guard->errors.Newfor static messagesfmt.Errorffor consistency across all error paths is fine; no action neededAssessment: ✅ Clean
All reviewers approved on current HEAD (
7de6fdd9). All actionable findings from the first round have been addressed (comment wording fix, Unicode→ASCII arrows, cross-reference comments). Remaining findings from the second round are NITs that don't affect correctness or security. The PR is ready.