From 7de6fdd9ec7db7441bc1bbd58dafabc6a4ad4aad Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 09:28:46 -0700 Subject: [PATCH] fix: address review feedback on redirect policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace Unicode arrows (→) with ASCII (->) in error messages and comments for log compatibility (gpt-review NITs #19626, #19628) - Improve guard comment to clarify it exists for testability, not runtime safety (sonnet-review NIT #19619) - Add cross-reference comments noting intentional duplication between gitea/client.go and github/client.go (sonnet-review #19618, gpt-review #19625, #19627) Pushed back on: - internal/ package for dedup: structural overhead not warranted for a single ~25-line function - strings.EqualFold for scheme: Go's url.Parse normalizes schemes to lowercase, making case-insensitive comparison unnecessary --- gitea/client.go | 14 ++++++++------ gitea/client_test.go | 2 +- github/client.go | 14 ++++++++------ github/client_test.go | 2 +- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/gitea/client.go b/gitea/client.go index 34e3fc0..572ee18 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -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. +// 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 { return nil } 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 } diff --git a/gitea/client_test.go b/gitea/client_test.go index 86a04d1..66b1bf7 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -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) diff --git a/github/client.go b/github/client.go index b625268..041a49c 100644 --- a/github/client.go +++ b/github/client.go @@ -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 { return nil } 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 } diff --git a/github/client_test.go b/github/client_test.go index 3e3d3d6..6477f6f 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -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)