diff --git a/gitea/client.go b/gitea/client.go index 32c3cae..572ee18 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -78,18 +78,63 @@ type Client struct { MaxDiffSize int64 } +// 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. +// 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 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. + 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..66b1bf7 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..041a49c 100644 --- a/github/client.go +++ b/github/client.go @@ -107,6 +107,34 @@ type Client struct { now func() time.Time } +// defaultCheckRedirect is the redirect policy used by NewClient. +// 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 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. + 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 +145,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..6477f6f 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)") + } +}