Refuse HTTPS to HTTP redirects in the shared HTTP client

internal/resolve/resolve.go

@@ -310,8 +310,12 @@ func newClientForBaseURL(domain, baseURL string) *forges.Client {
 		opts = append(opts, forges.WithToken(domain, token))
 	}
 
-	// Register default forges first, so config-based registrations override them.
+	// Share one client for detection and every backend, so forges found by
+	// probing get the same User-Agent and redirect policy as the defaults.
 	hc := HTTPClient()
+	opts = append(opts, forges.WithHTTPClient(hc))
+
+	// Register default forges first, so config-based registrations override them.
 	defaults := map[string]forges.Forge{
 		"github.com":    ghforge.New(TokenForDomain("github.com"), hc),
 		"gitlab.com":    glforge.New("https://gitlab.com", TokenForDomain("gitlab.com"), hc),

internal/resolve/transport.go

@@ -1,6 +1,13 @@
 package resolve
 
-import "net/http"
+import (
+	"errors"
+	"fmt"
+	"net/http"
+)
+
+// maxRedirects matches the limit net/http applies when CheckRedirect is nil.
+const maxRedirects = 10
 
 var userAgent = "forge/dev"
 
@@ -12,9 +19,31 @@ func SetUserAgent(ua string) {
 
 // HTTPClient returns an http.Client whose transport sets the User-Agent
 // header on outbound requests. Used for all forge API traffic so requests
-// are identifiable in server logs.
+// are identifiable in server logs. It refuses redirects from HTTPS to HTTP,
+// since net/http would otherwise resend headers such as Authorization in
+// cleartext when the redirect stays on the same host.
 func HTTPClient() *http.Client {
-	return &http.Client{Transport: &userAgentTransport{base: http.DefaultTransport}}
+	return &http.Client{
+		Transport:     &userAgentTransport{base: http.DefaultTransport},
+		CheckRedirect: checkRedirect,
+	}
+}
+
+// errInsecureRedirect is returned when a redirect would downgrade from
+// HTTPS to HTTP. The message names no URL, because net/http already wraps
+// it in a *url.Error that carries the redirect target.
+var errInsecureRedirect = errors.New("refusing redirect from https to insecure http")
+
+// checkRedirect keeps the default redirect limit and rejects any hop that
+// downgrades from HTTPS to HTTP.
+func checkRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= maxRedirects {
+		return fmt.Errorf("stopped after %d redirects", maxRedirects)
+	}
+	if len(via) > 0 && via[len(via)-1].URL.Scheme == "https" && req.URL.Scheme == "http" {
+		return errInsecureRedirect
+	}
+	return nil
 }
 
 type userAgentTransport struct {

internal/resolve/transport_test.go

@@ -1,9 +1,16 @@
 package resolve
 
 import (
+	"context"
+	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
+	"sync"
 	"testing"
+
+	"github.com/git-pkgs/forge/internal/config"
 )
 
 func TestHTTPClientSetsUserAgent(t *testing.T) {
@@ -92,3 +99,195 @@ func TestUserAgentTransportDoesNotMutateRequest(t *testing.T) {
 		t.Errorf("transport mutated the original request: User-Agent=%q", req.Header.Get("User-Agent"))
 	}
 }
+
+// tlsHTTPClient returns HTTPClient with its transport trusting srv's
+// self-signed certificate, keeping the redirect policy under test.
+func tlsHTTPClient(srv *httptest.Server) *http.Client {
+	c := HTTPClient()
+	c.Transport = &userAgentTransport{base: srv.Client().Transport}
+	return c
+}
+
+func TestHTTPClientRejectsHTTPSToHTTPRedirect(t *testing.T) {
+	var plainHits int
+	plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		plainHits++
+		if r.Header.Get("Authorization") != "" {
+			t.Errorf("Authorization header sent over plain HTTP")
+		}
+	}))
+	defer plain.Close()
+
+	secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, plain.URL+"/target", http.StatusFound)
+	}))
+	defer secure.Close()
+
+	req, _ := http.NewRequest(http.MethodGet, secure.URL, nil)
+	req.Header.Set("Authorization", "Bearer secret")
+	resp, err := tlsHTTPClient(secure).Do(req)
+	if err == nil {
+		_ = resp.Body.Close()
+		t.Fatal("expected HTTPS to HTTP redirect to be refused")
+	}
+	if !errors.Is(err, errInsecureRedirect) {
+		t.Errorf("error = %v, want errInsecureRedirect", err)
+	}
+	if plainHits != 0 {
+		t.Errorf("plain HTTP server was contacted %d times", plainHits)
+	}
+}
+
+func TestHTTPClientFollowsSafeRedirects(t *testing.T) {
+	t.Run("https to https", func(t *testing.T) {
+		var srv *httptest.Server
+		srv = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path == "/start" {
+				http.Redirect(w, r, srv.URL+"/end", http.StatusFound)
+				return
+			}
+			_, _ = w.Write([]byte("ok"))
+		}))
+		defer srv.Close()
+
+		resp, err := tlsHTTPClient(srv).Get(srv.URL + "/start")
+		if err != nil {
+			t.Fatalf("redirect within HTTPS failed: %v", err)
+		}
+		_ = resp.Body.Close()
+		if resp.Request.URL.Path != "/end" {
+			t.Errorf("final path = %q, want /end", resp.Request.URL.Path)
+		}
+	})
+
+	t.Run("http to https", func(t *testing.T) {
+		secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
+		defer secure.Close()
+		plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			http.Redirect(w, r, secure.URL+"/end", http.StatusFound)
+		}))
+		defer plain.Close()
+
+		resp, err := tlsHTTPClient(secure).Get(plain.URL)
+		if err != nil {
+			t.Fatalf("upgrade redirect failed: %v", err)
+		}
+		_ = resp.Body.Close()
+		if resp.Request.URL.Scheme != "https" {
+			t.Errorf("final scheme = %q, want https", resp.Request.URL.Scheme)
+		}
+	})
+
+	t.Run("http to http", func(t *testing.T) {
+		var srv *httptest.Server
+		srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path == "/start" {
+				http.Redirect(w, r, srv.URL+"/end", http.StatusFound)
+			}
+		}))
+		defer srv.Close()
+
+		resp, err := HTTPClient().Get(srv.URL + "/start")
+		if err != nil {
+			t.Fatalf("redirect within HTTP failed: %v", err)
+		}
+		_ = resp.Body.Close()
+		if resp.Request.URL.Path != "/end" {
+			t.Errorf("final path = %q, want /end", resp.Request.URL.Path)
+		}
+	})
+}
+
+func TestHTTPClientKeepsRedirectLimit(t *testing.T) {
+	var hits int
+	var srv *httptest.Server
+	srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		hits++
+		http.Redirect(w, r, srv.URL+"/loop", http.StatusFound)
+	}))
+	defer srv.Close()
+
+	resp, err := HTTPClient().Get(srv.URL)
+	if err == nil {
+		_ = resp.Body.Close()
+		t.Fatal("expected redirect loop to stop")
+	}
+	if !strings.Contains(err.Error(), fmt.Sprintf("stopped after %d redirects", maxRedirects)) {
+		t.Errorf("unexpected error: %v", err)
+	}
+	if hits != maxRedirects {
+		t.Errorf("server hit %d times, want %d", hits, maxRedirects)
+	}
+}
+
+// TestDetectedForgeUsesSharedHTTPClient covers a self-hosted forge with no
+// type in config, so the client is built by RegisterDomain after probing.
+// Both the probes and the backend must use HTTPClient, which means they get
+// the User-Agent and refuse the HTTPS to HTTP redirect.
+func TestDetectedForgeUsesSharedHTTPClient(t *testing.T) {
+	config.ResetCache()
+	t.Cleanup(config.ResetCache)
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	t.Setenv("FORGE_HOST", "")
+	t.Setenv("FORGE_TOKEN", "detected-secret")
+	t.Chdir(t.TempDir())
+
+	oldH, oldS, oldT := hostOverride, schemeOverride, forgeTypeOverride
+	t.Cleanup(func() { hostOverride, schemeOverride, forgeTypeOverride = oldH, oldS, oldT })
+	hostOverride, schemeOverride, forgeTypeOverride = "", "", ""
+
+	var plainHits int
+	plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		plainHits++
+		if r.Header.Get("Authorization") != "" {
+			t.Errorf("token sent over plain HTTP")
+		}
+	}))
+	defer plain.Close()
+
+	var mu sync.Mutex
+	var agents []string
+	secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		mu.Lock()
+		agents = append(agents, r.URL.Path+" "+r.Header.Get("User-Agent"))
+		mu.Unlock()
+		if r.URL.Path == "/" {
+			w.Header().Set("X-Gitea-Version", "1.22.0")
+			return
+		}
+		http.Redirect(w, r, plain.URL+r.URL.Path, http.StatusFound)
+	}))
+	defer secure.Close()
+
+	// HTTPClient wraps http.DefaultTransport, so point that at a transport
+	// trusting the test server's certificate. This also lets a nil client
+	// (http.DefaultClient) reach the server, which is the regression.
+	oldTransport := http.DefaultTransport
+	http.DefaultTransport = secure.Client().Transport
+	t.Cleanup(func() { http.DefaultTransport = oldTransport })
+
+	domain := strings.TrimPrefix(secure.URL, "https://")
+	f, err := ForgeForDomain(domain)
+	if err != nil {
+		t.Fatalf("ForgeForDomain: %v", err)
+	}
+
+	_, err = f.Repos().Get(context.Background(), "owner", "repo")
+	if !errors.Is(err, errInsecureRedirect) {
+		t.Errorf("Repos().Get error = %v, want errInsecureRedirect", err)
+	}
+	if plainHits != 0 {
+		t.Errorf("plain HTTP server was contacted %d times", plainHits)
+	}
+
+	mu.Lock()
+	defer mu.Unlock()
+	if len(agents) == 0 {
+		t.Fatal("no requests reached the forge")
+	}
+	for _, a := range agents {
+		if !strings.HasSuffix(a, " "+userAgent) {
+			t.Errorf("request %q did not use the shared User-Agent %q", a, userAgent)
+		}
+	}
+}