From aa0b543ae8043154abffed0c071d6d212b6590b1 Mon Sep 17 00:00:00 2001 From: simon chrzanowski Date: Wed, 14 Oct 2026 08:38:51 +0200 Subject: [PATCH] Stop forwarding custom auth headers across redirects Handlers set the configured upstream auth header on the request itself before handing it to the client. When upstream redirected to another host, Go's client copied that header to the next hop: it strips only Authorization and Cookie, so a "header" type credential such as X-JFrog-Art-Api reached the redirect target. The auth transport already applies credentials per hop, scoped to each hop's URL. - Remove Proxy.applyUpstreamAuth and Proxy.AuthForURL; auth comes only from the transport - Add a server test that redirects a header-authenticated upstream to another host and asserts the header stays with the upstream - Wire handler tests' auth through the transport, as the server does --- internal/handler/apk_test.go | 4 +- internal/handler/arch_test.go | 2 +- internal/handler/cargo_test.go | 4 +- internal/handler/handler.go | 16 --- internal/handler/npm.go | 1 - internal/handler/npm_test.go | 12 +- internal/handler/swift.go | 3 - internal/handler/upstream_auth_helper_test.go | 21 +++ internal/server/server.go | 1 - .../server/upstream_auth_redirect_test.go | 125 ++++++++++++++++++ 10 files changed, 152 insertions(+), 37 deletions(-) create mode 100644 internal/handler/upstream_auth_helper_test.go create mode 100644 internal/server/upstream_auth_redirect_test.go diff --git a/internal/handler/apk_test.go b/internal/handler/apk_test.go index 61c874ee..fc906814 100644 --- a/internal/handler/apk_test.go +++ b/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}) diff --git a/internal/handler/arch_test.go b/internal/handler/arch_test.go index 2650f022..3affad5c 100644 --- a/internal/handler/arch_test.go +++ b/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. diff --git a/internal/handler/cargo_test.go b/internal/handler/cargo_test.go index 895ff7b6..de3b2f3a 100644 --- a/internal/handler/cargo_test.go +++ b/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", diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 776fa8cb..b84e827c 100644 --- a/internal/handler/handler.go +++ b/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) { diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 96ab10ea..92629d24 100644 --- a/internal/handler/npm.go +++ b/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 { diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index 5f45feb8..a1ad53d2 100644 --- a/internal/handler/npm_test.go +++ b/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) diff --git a/internal/handler/swift.go b/internal/handler/swift.go index bd46fab2..15825e02 100644 --- a/internal/handler/swift.go +++ b/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 { diff --git a/internal/handler/upstream_auth_helper_test.go b/internal/handler/upstream_auth_helper_test.go new file mode 100644 index 00000000..261636d2 --- /dev/null +++ b/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 +} diff --git a/internal/server/server.go b/internal/server/server.go index 8ee1c623..6473e76a 100644 --- a/internal/server/server.go +++ b/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) diff --git a/internal/server/upstream_auth_redirect_test.go b/internal/server/upstream_auth_redirect_test.go new file mode 100644 index 00000000..7c2f0c44 --- /dev/null +++ b/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) + } +}