From 1e0959b077bd0b06807ea984ba17dd137b8023f9 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 08:51:36 -0700 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20reject=20cross-host=20redirects=20a?= =?UTF-8?q?nd=20HTTPS=E2=86=92HTTP=20downgrades=20(#95)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add defaultCheckRedirect to both GitHub and Gitea clients that rejects: - HTTPS→HTTP protocol downgrades (prevents plaintext leakage) - Cross-host redirects entirely (prevents consuming untrusted responses) Same-host, same-or-upgraded-scheme redirects remain allowed. Both NewClient constructors wire the policy, and SetHTTPClient(nil) restores it. Callers providing a non-nil client are responsible for configuring their own safe redirect policy. Closes #95 --- gitea/client.go | 47 ++++++++++++++++++- gitea/client_test.go | 102 +++++++++++++++++++++++++++++++++++++++++ github/client.go | 49 ++++++++++++++++++-- github/client_test.go | 104 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 297 insertions(+), 5 deletions(-) diff --git a/gitea/client.go b/gitea/client.go index 32c3cae..34e3fc0 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -78,18 +78,61 @@ type Client struct { MaxDiffSize int64 } +// defaultCheckRedirect is the redirect policy used by NewClient. +// 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. + if len(via) == 0 { + return nil + } + prev := via[len(via)-1] + // 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) + } + // 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 nil +} + // NewClient creates a new Gitea API client. func NewClient(baseURL, token string) *Client { return &Client{ baseURL: strings.TrimRight(baseURL, "/"), token: token, - http: &http.Client{Timeout: 30 * time.Second}, + http: &http.Client{ + Timeout: 30 * time.Second, + CheckRedirect: defaultCheckRedirect, + }, } } // SetHTTPClient sets the underlying HTTP client used for requests. -// This is intended for testing to inject mock transports. +// This is intended for test setup only to inject mock transports; it must be +// called before any goroutines issue requests. +// +// Passing nil restores the default client (30s timeout + redirect-rejecting +// CheckRedirect policy matching NewClient). +// +// Callers providing a non-nil client are responsible for configuring a safe +// CheckRedirect policy. Without one, the default net/http behavior will follow +// redirects and may forward the Authorization header to untrusted hosts. func (c *Client) SetHTTPClient(hc *http.Client) { + if hc == nil { + hc = &http.Client{ + Timeout: 30 * time.Second, + CheckRedirect: defaultCheckRedirect, + } + } c.http = hc } diff --git a/gitea/client_test.go b/gitea/client_test.go index eec0b7d..86a04d1 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -9,6 +9,7 @@ import ( "net" "net/http" "net/http/httptest" + "net/url" "strings" "sync/atomic" "syscall" @@ -1159,3 +1160,104 @@ func TestSanitizeErrorForLog(t *testing.T) { }) } } + +func TestNewClient_HasCheckRedirect(t *testing.T) { + c := NewClient("https://gitea.example.com", "token") + if c.http.CheckRedirect == nil { + t.Fatal("expected CheckRedirect to be set") + } +} + +func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "http", Host: "gitea.example.com", Path: "/foo"}, + Header: http.Header{"Authorization": []string{"token abc"}}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err == nil { + t.Fatal("expected error on HTTPS→HTTP redirect") + } + if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDefaultCheckRedirect_RejectsCrossHost(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "https", Host: "cdn.example.com", Path: "/bar"}, + Header: http.Header{"Authorization": []string{"token abc"}}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err == nil { + t.Fatal("expected error on cross-host redirect") + } + if !strings.Contains(err.Error(), "cross-host") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDefaultCheckRedirect_AllowsSameHost(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/bar"}, + Header: http.Header{"Authorization": []string{"token abc"}}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if auth := req.Header.Get("Authorization"); auth != "token abc" { + t.Errorf("expected Authorization to be preserved, got %q", auth) + } +} + +func TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "http", Host: "localhost:3000", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "http", Host: "localhost:3000", Path: "/bar"}, + Header: http.Header{}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestDefaultCheckRedirect_RejectsTooManyRedirects(t *testing.T) { + via := make([]*http.Request, 10) + for i := range via { + via[i] = &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/"}} + } + req := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/final"}} + err := defaultCheckRedirect(req, via) + if err == nil { + t.Fatal("expected error after 10 redirects") + } + if !strings.Contains(err.Error(), "10 redirects") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDefaultCheckRedirect_EmptyViaAllowed(t *testing.T) { + req := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}} + err := defaultCheckRedirect(req, nil) + if err != nil { + t.Fatalf("unexpected error with empty via: %v", err) + } +} + +func TestSetHTTPClient_NilRestoresDefault(t *testing.T) { + c := NewClient("https://gitea.example.com", "token") + c.SetHTTPClient(nil) + if c.http == nil { + t.Fatal("expected non-nil http client after SetHTTPClient(nil)") + } + if c.http.Timeout != 30*time.Second { + t.Errorf("expected 30s timeout, got %v", c.http.Timeout) + } + if c.http.CheckRedirect == nil { + t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)") + } +} diff --git a/github/client.go b/github/client.go index 2dc27ca..b625268 100644 --- a/github/client.go +++ b/github/client.go @@ -107,6 +107,32 @@ type Client struct { now func() time.Time } +// defaultCheckRedirect is the redirect policy used by NewClient. +// 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. + if len(via) == 0 { + return nil + } + prev := via[len(via)-1] + // 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) + } + // 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 nil +} + // NewClient creates a new GitHub API client. // If baseURL is empty, it defaults to https://api.github.com. // For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3). @@ -117,14 +143,31 @@ func NewClient(token, baseURL string) *Client { return &Client{ baseURL: strings.TrimRight(baseURL, "/"), token: token, - httpClient: &http.Client{Timeout: 30 * time.Second}, - now: time.Now, + httpClient: &http.Client{ + Timeout: 30 * time.Second, + CheckRedirect: defaultCheckRedirect, + }, + now: time.Now, } } // SetHTTPClient sets the underlying HTTP client used for requests. -// This is intended for testing to inject mock transports. +// This is intended for test setup only to inject mock transports; it must be +// called before any goroutines issue requests. +// +// Passing nil restores the default client (30s timeout + redirect-rejecting +// CheckRedirect policy matching NewClient). +// +// Callers providing a non-nil client are responsible for configuring a safe +// CheckRedirect policy. Without one, the default net/http behavior will follow +// redirects and may forward the Authorization header to untrusted hosts. func (c *Client) SetHTTPClient(hc *http.Client) { + if hc == nil { + hc = &http.Client{ + Timeout: 30 * time.Second, + CheckRedirect: defaultCheckRedirect, + } + } c.httpClient = hc } diff --git a/github/client_test.go b/github/client_test.go index 8a5e4a3..3e3d3d6 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -5,6 +5,8 @@ import ( "errors" "net/http" "net/http/httptest" + "net/url" + "strings" "testing" "time" ) @@ -407,3 +409,105 @@ func TestAPIError_Error_NewlineSanitized(t *testing.T) { } } } + +func TestNewClient_HasCheckRedirect(t *testing.T) { + c := NewClient("secret-token", "https://api.github.com") + if c.httpClient.CheckRedirect == nil { + t.Fatal("expected CheckRedirect to be set") + } +} + +func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "http", Host: "api.github.com", Path: "/foo"}, + Header: http.Header{"Authorization": []string{"Bearer token"}}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err == nil { + t.Fatal("expected error on HTTPS→HTTP redirect") + } + if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDefaultCheckRedirect_RejectsCrossHost(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "https", Host: "objects.githubusercontent.com", Path: "/bar"}, + Header: http.Header{"Authorization": []string{"Bearer token"}}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err == nil { + t.Fatal("expected error on cross-host redirect") + } + if !strings.Contains(err.Error(), "cross-host") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDefaultCheckRedirect_AllowsSameHost(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/bar"}, + Header: http.Header{"Authorization": []string{"Bearer token"}}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Auth should be preserved on same-host redirect + if auth := req.Header.Get("Authorization"); auth != "Bearer token" { + t.Errorf("expected Authorization to be preserved, got %q", auth) + } +} + +func TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP(t *testing.T) { + prev := &http.Request{URL: &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/foo"}} + req := &http.Request{ + URL: &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/bar"}, + Header: http.Header{}, + } + err := defaultCheckRedirect(req, []*http.Request{prev}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestDefaultCheckRedirect_RejectsTooManyRedirects(t *testing.T) { + via := make([]*http.Request, 10) + for i := range via { + via[i] = &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/"}} + } + req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/final"}} + err := defaultCheckRedirect(req, via) + if err == nil { + t.Fatal("expected error after 10 redirects") + } + if !strings.Contains(err.Error(), "10 redirects") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDefaultCheckRedirect_EmptyViaAllowed(t *testing.T) { + req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}} + err := defaultCheckRedirect(req, nil) + if err != nil { + t.Fatalf("unexpected error with empty via: %v", err) + } +} + +func TestSetHTTPClient_NilRestoresDefault(t *testing.T) { + c := NewClient("token", "https://api.github.com") + c.SetHTTPClient(nil) + if c.httpClient == nil { + t.Fatal("expected non-nil httpClient after SetHTTPClient(nil)") + } + if c.httpClient.Timeout != 30*time.Second { + t.Errorf("expected 30s timeout, got %v", c.httpClient.Timeout) + } + if c.httpClient.CheckRedirect == nil { + t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)") + } +} From 7de6fdd9ec7db7441bc1bbd58dafabc6a4ad4aad Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 09:28:46 -0700 Subject: [PATCH 2/2] 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)