Stop forwarding custom auth headers across redirects

internal/handler/apk_test.go

@@ -144,9 +144,7 @@ func TestAPKHandler_IndexesServedUnchanged(t *testing.T) {
 	proxy.CacheMetadata = true
 	proxy.MetadataTTL = time.Hour
 	proxy.HTTPClient = upstream.Client()
-	proxy.AuthForURL = func(string) (string, string) {
-		return "Authorization", "Bearer apk-token"
-	}
+	authorizeUpstream(proxy, "Bearer apk-token")
 
 	h := NewAPKHandler(proxy, "http://proxy.example", map[string]string{"alpine": upstream.URL})
 

internal/handler/arch_test.go

@@ -202,7 +202,7 @@ func TestArchHandler_DatabasesServedUnchanged(t *testing.T) {
 	}
 	upstream := newArchUpstream(t, files)
 	proxy := newArchTestProxy(t, upstream)
-	proxy.AuthForURL = func(string) (string, string) { return "Authorization", "Bearer arch-token" }
+	authorizeUpstream(proxy, "Bearer arch-token")
 	h := NewArchHandler(proxy, "http://proxy.example", map[string]string{"archlinux": upstream.URL})
 
 	// The database goes first, as pacman requests it.

internal/handler/cargo_test.go

@@ -89,9 +89,7 @@ func TestCargoHandlerUsesConfiguredUpstreams(t *testing.T) {
 
 		proxy, _, _, _ := setupTestProxy(t)
 		proxy.HTTPClient = upstream.Client()
-		proxy.AuthForURL = func(string) (string, string) {
-			return "Authorization", "Bearer cargo-token"
-		}
+		authorizeUpstream(proxy, "Bearer cargo-token")
 		h := NewCargoHandler(
 			proxy,
 			"http://proxy.test",

internal/handler/handler.go

@@ -173,7 +173,6 @@ type Proxy struct {
 	// storage at an internal one.
 	DirectServeBaseURL string
 	HTTPClient         *http.Client
-	AuthForURL         func(string) (headerName, headerValue string)
 
 	// StreamArtifacts streams artifacts from upstream without storing them.
 	// Each request fetches its own copy: there is no cache to check and
@@ -1059,7 +1058,6 @@ func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamUR
 			req.Header.Set(header, v)
 		}
 	}
-	p.applyUpstreamAuth(req)
 
 	resp, err := p.HTTPClient.Do(req)
 	if err != nil {
@@ -1079,7 +1077,6 @@ func (p *Proxy) ProxyFile(w http.ResponseWriter, r *http.Request, upstreamURL st
 		http.Error(w, "failed to create request", http.StatusInternalServerError)
 		return
 	}
-	p.applyUpstreamAuth(req)
 
 	resp, err := p.HTTPClient.Do(req)
 	if err != nil {
@@ -1422,7 +1419,6 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e
 	if acceptEncoding != "" {
 		req.Header.Set(headerAcceptEncoding, acceptEncoding)
 	}
-	p.applyUpstreamAuth(req)
 
 	if entry != nil && entry.ETag.Valid {
 		req.Header.Set("If-None-Match", entry.ETag.String)
@@ -1678,7 +1674,6 @@ func (p *Proxy) proxyMetadataStream(w http.ResponseWriter, r *http.Request, upst
 	if acceptEncoding != "" {
 		req.Header.Set(headerAcceptEncoding, acceptEncoding)
 	}
-	p.applyUpstreamAuth(req)
 
 	for _, header := range []string{"If-Modified-Since", "If-None-Match"} {
 		if v := r.Header.Get(header); v != "" {
@@ -1702,17 +1697,6 @@ func (p *Proxy) proxyMetadataStream(w http.ResponseWriter, r *http.Request, upst
 	})
 }
 
-func (p *Proxy) applyUpstreamAuth(req *http.Request) {
-	if p.AuthForURL == nil {
-		return
-	}
-
-	headerName, headerValue := p.AuthForURL(req.URL.String())
-	if headerName != "" && headerValue != "" {
-		req.Header.Set(headerName, headerValue)
-	}
-}
-
 // GetOrFetchArtifactFromURL retrieves an artifact from cache or fetches from a specific URL.
 // This is useful for registries where download URLs are determined from metadata.
 func (p *Proxy) GetOrFetchArtifactFromURL(ctx context.Context, ecosystem, name, version, filename, downloadURL string) (*CacheResult, error) {

internal/handler/npm.go

@@ -160,7 +160,6 @@ func (h *NPMHandler) handleSecurity(w http.ResponseWriter, r *http.Request) {
 	if req.Header.Get(headerContentType) == "" {
 		req.Header.Set(headerContentType, contentTypeJSON)
 	}
-	h.proxy.applyUpstreamAuth(req)
 
 	resp, err := h.proxy.HTTPClient.Do(req)
 	if err != nil {

internal/handler/npm_test.go

@@ -72,9 +72,7 @@ func TestNPMHandlerUsesConfiguredUpstream(t *testing.T) {
 
 		proxy, _, _, _ := setupTestProxy(t)
 		proxy.HTTPClient = upstream.Client()
-		proxy.AuthForURL = func(string) (string, string) {
-			return "Authorization", "Bearer npm-token"
-		}
+		authorizeUpstream(proxy, "Bearer npm-token")
 		h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL+"/root/")
 
 		req := httptest.NewRequest(http.MethodGet, "/testpkg", nil)
@@ -845,9 +843,7 @@ func TestNPMAuditCoversAllSecurityEndpoints(t *testing.T) {
 
 func TestNPMAuditAppliesUpstreamAuth(t *testing.T) {
 	h, got := newNPMAuditUpstream(t, npmAuditJSON(`{}`))
-	h.proxy.AuthForURL = func(string) (string, string) {
-		return "Authorization", "Bearer npm-token"
-	}
+	authorizeUpstream(h.proxy, "Bearer npm-token")
 
 	serveNPM(t, h, http.MethodPost, "/-/npm/v1/security/audits", strings.NewReader(`{}`), nil)
 
@@ -1011,9 +1007,7 @@ func TestNPMKeysProxiesUpstream(t *testing.T) {
 
 	proxy, _, _, _ := setupTestProxy(t)
 	proxy.HTTPClient = upstream.Client()
-	proxy.AuthForURL = func(string) (string, string) {
-		return "Authorization", "Bearer npm-token"
-	}
+	authorizeUpstream(proxy, "Bearer npm-token")
 	h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL)
 
 	w := serveNPM(t, h, http.MethodGet, npmKeysPath, nil, nil)

internal/handler/swift.go

@@ -269,7 +269,6 @@ func (h *SwiftHandler) probeSourceArchive(ctx context.Context, upstreamURL, acce
 	}
 	req.Header.Set("Accept", accept)
 	req.Header.Set("Range", "bytes=0-0")
-	h.proxy.applyUpstreamAuth(req)
 
 	resp, err := h.proxy.HTTPClient.Do(req)
 	if err != nil {
@@ -313,7 +312,6 @@ func (h *SwiftHandler) fetchMetadataWithHeaders(
 		return nil, "", nil, fmt.Errorf("creating upstream metadata request: %w", err)
 	}
 	req.Header.Set("Accept", accept)
-	h.proxy.applyUpstreamAuth(req)
 
 	resp, err := h.proxy.HTTPClient.Do(req)
 	if err != nil {
@@ -438,7 +436,6 @@ func (h *SwiftHandler) proxySwiftResource(w http.ResponseWriter, r *http.Request
 			req.Header.Set(name, value)
 		}
 	}
-	h.proxy.applyUpstreamAuth(req)
 
 	resp, err := h.proxy.HTTPClient.Do(req)
 	if err != nil {

internal/handler/upstream_auth_helper_test.go

@@ -0,0 +1,21 @@
+package handler
+
+import (
+	"net/http"
+
+	upstreamhttp "github.com/git-pkgs/proxy/internal/httpclient"
+)
+
+// authorizeUpstream sends authorization on every upstream request the way the
+// server does: through the auth transport, which scopes it to each hop.
+func authorizeUpstream(proxy *Proxy, authorization string) {
+	base := proxy.HTTPClient.Transport
+	if base == nil {
+		base = http.DefaultTransport
+	}
+	client := *proxy.HTTPClient
+	client.Transport = upstreamhttp.NewTransport(base, func(string) (string, string) {
+		return "Authorization", authorization
+	})
+	proxy.HTTPClient = &client
+}

internal/server/server.go

@@ -247,7 +247,6 @@ func (s *Server) serve(listener net.Listener) error {
 	}
 	proxy := handler.NewProxy(s.db, s.storage, fetcher, resolver, s.logger)
 	proxy.HTTPClient = &metadataClient
-	proxy.AuthForURL = s.authForURL
 	cooldownPolicy, err := cooldownpolicy.New(cd, s.cfg.Cooldown.PackagePatterns)
 	if err != nil {
 		return fmt.Errorf("configuring cooldown policy: %w", err)

internal/server/upstream_auth_redirect_test.go

@@ -0,0 +1,125 @@
+package server
+
+import (
+	"context"
+	"errors"
+	"io"
+	"log/slog"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"path/filepath"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/git-pkgs/proxy/internal/config"
+)
+
+const redirectAuthHeader = "X-Api-Key"
+
+// headerRecorder remembers the auth header value each request carried.
+type headerRecorder struct {
+	mu     sync.Mutex
+	values []string
+}
+
+func (h *headerRecorder) record(r *http.Request) {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	h.values = append(h.values, r.Header.Get(redirectAuthHeader))
+}
+
+func (h *headerRecorder) seen() []string {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	return append([]string(nil), h.values...)
+}
+
+func TestHeaderAuthIsNotForwardedAcrossRedirects(t *testing.T) {
+	var atOther headerRecorder
+	other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		atOther.record(r)
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = io.WriteString(w, `{"name":"leftpad","versions":{}}`)
+	}))
+	defer other.Close()
+
+	var atRegistry headerRecorder
+	registry := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		atRegistry.record(r)
+		http.Redirect(w, r, other.URL+r.URL.Path, http.StatusFound)
+	}))
+	defer registry.Close()
+
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("reserving proxy address: %v", err)
+	}
+	t.Cleanup(func() { _ = listener.Close() })
+
+	tempDir := t.TempDir()
+	cfg := config.Default()
+	cfg.Listen = listener.Addr().String()
+	cfg.BaseURL = "http://" + cfg.Listen
+	cfg.Database.Path = filepath.Join(tempDir, "proxy.db")
+	cfg.Storage.URL = "file://" + filepath.Join(tempDir, "artifacts")
+	cfg.Upstream.AllowLoopback = true
+	cfg.Upstream.NPM = registry.URL
+	cfg.Upstream.Auth = map[string]config.AuthConfig{
+		registry.URL: {Type: "header", HeaderName: redirectAuthHeader, HeaderValue: "registry-secret"},
+	}
+	if err := cfg.Validate(); err != nil {
+		t.Fatalf("validating config: %v", err)
+	}
+
+	proxyServer, err := New(cfg, slog.New(slog.NewTextHandler(io.Discard, nil)), BuildInfo{Version: "test", Commit: "test"})
+	if err != nil {
+		t.Fatalf("creating server: %v", err)
+	}
+	startErr := make(chan error, 1)
+	go func() { startErr <- proxyServer.Start(listener) }()
+	defer func() {
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+		if err := proxyServer.Shutdown(ctx); err != nil {
+			t.Errorf("shutting down server: %v", err)
+		}
+		if err := <-startErr; !errors.Is(err, http.ErrServerClosed) {
+			t.Errorf("Start() error = %v, want %v", err, http.ErrServerClosed)
+		}
+	}()
+
+	resp := getWhenListening(t, cfg.BaseURL+"/npm/leftpad")
+	_ = resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusOK)
+	}
+
+	registrySeen := atRegistry.seen()
+	if len(registrySeen) == 0 || registrySeen[0] != "registry-secret" {
+		t.Errorf("registry received %s = %q, want the configured value", redirectAuthHeader, registrySeen)
+	}
+	for _, value := range atOther.seen() {
+		if value != "" {
+			t.Errorf("redirect target received %s = %q, want none", redirectAuthHeader, value)
+		}
+	}
+}
+
+// getWhenListening retries a GET until the proxy accepts connections.
+func getWhenListening(t *testing.T, url string) *http.Response {
+	t.Helper()
+	client := &http.Client{Timeout: 5 * time.Second}
+	deadline := time.Now().Add(5 * time.Second)
+	for {
+		resp, err := client.Get(url)
+		if err == nil {
+			return resp
+		}
+		if time.Now().After(deadline) {
+			t.Fatalf("proxy did not start: %v", err)
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+}