rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat: reject cross-host redirects and HTTPS→HTTP downgrades (#95) #111

Merged
aweiker merged 2 commits from review-bot-issue-95 into main 2026-05-13 16:58:49 +00:00
Conversation 12 Commits 2 Files Changed 4
+18 -14
4 changed files with 18 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 7de6fdd9ec - Show all commits
gitea/client.go
+8 -6
View File
2
 @@ -79,27 +79,29 @@ type Client struct {
}
// defaultCheckRedirect is the redirect policy used by NewClient.
// It rejects HTTPS→HTTP protocol downgrades (to prevent plaintext leakage)
// NOTE: This function is intentionally duplicated in github/client.go (and vice versa)
// because the packages are separate. Changes here must be mirrored there.
sonnet-review-bot commented 2026-05-13 16:29:39 +00:00
Review
Copy Link
Copy Source

[NIT] The duplication comment says Changes here must be mirrored there but 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 shared internal/ package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but the internal/ pattern exists precisely for this use case.

**[NIT]** The duplication comment says `Changes here must be mirrored there` but 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 shared `internal/` package would eliminate the drift risk entirely. The PR description acknowledges this is intentional due to package separation, but the `internal/` pattern exists precisely for this use case.
// It rejects HTTPS->HTTP protocol downgrades (to prevent plaintext leakage)
// and cross-host redirects (to prevent following responses from untrusted
// endpoints). Same-host, same-or-upgraded-scheme redirects are allowed.
func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
if len(via) >= 10 {
sonnet-review-bot commented 2026-05-13 15:52:37 +00:00
Review
Copy Link
Copy Source

[MINOR] The defaultCheckRedirect function is duplicated verbatim between gitea/client.go and github/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, an internal/httppolicy package could house the shared function — though that adds structural complexity for a single function.

**[MINOR]** The `defaultCheckRedirect` function is duplicated verbatim between `gitea/client.go` and `github/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, an `internal/httppolicy` package could house the shared function — though that adds structural complexity for a single function.
return fmt.Errorf("stopped after 10 redirects")
}
// Guard: net/http guarantees len(via) >= 1 but defend against
// zero-length to avoid panic on index out of range.
// Guard for direct invocation in tests and any future callers;
// net/http guarantees len(via) >= 1 during actual redirects.
if len(via) == 0 {
sonnet-review-bot commented 2026-05-13 15:52:37 +00:00
Review
Copy Link
Copy Source

[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) >= 1 when CheckRedirect is called during an actual redirect, but the test TestDefaultCheckRedirect_EmptyViaAllowed calls the function directly with nil. 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'.

**[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) >= 1` when `CheckRedirect` is called during an actual redirect, but the test `TestDefaultCheckRedirect_EmptyViaAllowed` calls the function directly with `nil`. 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'.
security-review-bot commented 2026-05-13 15:54:00 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
return nil
}
prev := via[len(via)-1]
sonnet-review-bot commented 2026-05-13 16:29:39 +00:00
Review
Copy Link
Copy Source

[NIT] The defaultCheckRedirect function uses fmt.Errorf for all error paths, but none of these errors wrap an underlying cause (no %w). Per the error-handling patterns, fmt.Errorf without %w is fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting that errors.New could be used instead for the static messages (e.g. stopped after 10 redirects) since there's no format interpolation needed there, though fmt.Errorf is not wrong.

**[NIT]** The `defaultCheckRedirect` function uses `fmt.Errorf` for all error paths, but none of these errors wrap an underlying cause (no `%w`). Per the error-handling patterns, `fmt.Errorf` without `%w` is fine here since these are newly constructed errors with no upstream cause to preserve. This is correct — just noting that `errors.New` could be used instead for the static messages (e.g. `stopped after 10 redirects`) since there's no format interpolation needed there, though `fmt.Errorf` is not wrong.
// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.
// Reject protocol downgrade: HTTPS->HTTP leaks request metadata over plaintext.
if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
gpt-review-bot commented 2026-05-13 15:53:32 +00:00
Review
Copy Link
Copy Source

[NIT] Error messages include a Unicode arrow ("→"). While harmless, consider using ASCII ("->") for environments that may not render Unicode consistently in logs.

**[NIT]** Error messages include a Unicode arrow ("→"). While harmless, consider using ASCII ("->") for environments that may not render Unicode consistently in logs.
return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s → %s)", prev.URL.Host, req.URL.Host)
return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s -> %s)", prev.URL.Host, req.URL.Host)
}
// Reject cross-host redirect entirely to avoid consuming responses
// from untrusted endpoints.
if req.URL.Host != prev.URL.Host {
return fmt.Errorf("refusing redirect: cross-host (%s → %s)", prev.URL.Host, req.URL.Host)
return fmt.Errorf("refusing redirect: cross-host (%s -> %s)", prev.URL.Host, req.URL.Host)
}
return nil
}
gitea/client_test.go
+1 -1
View File
@@ -1176,7 +1176,7 @@ func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
}
err := defaultCheckRedirect(req, []*http.Request{prev})
if err == nil {
t.Fatal("expected error on HTTPS→HTTP redirect")
t.Fatal("expected error on HTTPS->HTTP redirect")
}
if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") {
t.Errorf("unexpected error message: %v", err)
github/client.go
+8 -6
View File
2
 @@ -108,27 +108,29 @@ type Client struct {
}
// defaultCheckRedirect is the redirect policy used by NewClient.
// It rejects HTTPS→HTTP protocol downgrades (to prevent plaintext leakage)
// NOTE: This function is intentionally duplicated in gitea/client.go (and vice versa)
// because the packages are separate. Changes here must be mirrored there.
// It rejects HTTPS->HTTP protocol downgrades (to prevent plaintext leakage)
// and cross-host redirects (to prevent following responses from untrusted
// endpoints). Same-host, same-or-upgraded-scheme redirects are allowed.
func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
if len(via) >= 10 {
return fmt.Errorf("stopped after 10 redirects")
}
// Guard: net/http guarantees len(via) >= 1 but defend against
// zero-length to avoid panic on index out of range.
// Guard for direct invocation in tests and any future callers;
// net/http guarantees len(via) >= 1 during actual redirects.
if len(via) == 0 {
security-review-bot marked this conversation as resolved
security-review-bot commented 2026-05-13 15:54:00 +00:00
Review
Copy Link
Copy Source

[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.

**[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.
return nil
gpt-review-bot commented 2026-05-13 15:53:32 +00:00
Review
Copy Link
Copy Source

[NIT] Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers.

**[NIT]** Error messages use a Unicode arrow ("→"). Consider ASCII arrows ("->") to maximize compatibility with plain-text log consumers.
}
prev := via[len(via)-1]
// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.
// 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)
return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s -> %s)", prev.URL.Host, req.URL.Host)
}
// Reject cross-host redirect entirely to avoid consuming responses
// from untrusted endpoints.
if req.URL.Host != prev.URL.Host {
return fmt.Errorf("refusing redirect: cross-host (%s → %s)", prev.URL.Host, req.URL.Host)
return fmt.Errorf("refusing redirect: cross-host (%s -> %s)", prev.URL.Host, req.URL.Host)
}
return nil
}
github/client_test.go
+1 -1
View File
@@ -425,7 +425,7 @@ func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
}
err := defaultCheckRedirect(req, []*http.Request{prev})
if err == nil {
t.Fatal("expected error on HTTPS→HTTP redirect")
t.Fatal("expected error on HTTPS->HTTP redirect")
}
if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") {
t.Errorf("unexpected error message: %v", err)