Merge pull request 'feat: reject cross-host redirects and HTTPS→HTTP downgrades (#95)' (#111) from review-bot-issue-95 into main

Reviewed-on: #111
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>

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
 }
 

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)")
+	}
+}

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
 }
 

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)")
+	}
+}