Hash path-based metadata cache keys

internal/handler/apk.go

@@ -74,7 +74,7 @@ func NewAPKHandler(proxy *Proxy, proxyURL string, repositories map[string]string
 // Routes returns the HTTP handler for APK requests.
 // Mount this at /apk on your router.
 func (h *APKHandler) Routes() http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return rejectQueryChars(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != http.MethodGet && r.Method != http.MethodHead {
 			http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
 			return
@@ -107,7 +107,7 @@ func (h *APKHandler) Routes() http.Handler {
 			// Other files - proxy directly.
 			h.proxyFile(w, r, upstreamURL, rest)
 		}
-	})
+	}))
 }
 
 // isAPKIndex reports whether the path names a repository index:

internal/handler/cache_key.go

@@ -0,0 +1,21 @@
+package handler
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"strconv"
+)
+
+// hashedCacheKey derives a metadata cache key from parts such as the
+// upstream URL, the request path and the query. Each part is length-prefixed
+// before hashing, so no two different part lists share a key, whatever bytes
+// the parts contain.
+func hashedCacheKey(parts ...string) string {
+	h := sha256.New()
+	for _, part := range parts {
+		_, _ = h.Write([]byte(strconv.Itoa(len(part))))
+		_, _ = h.Write([]byte{':'})
+		_, _ = h.Write([]byte(part))
+	}
+	return hex.EncodeToString(h.Sum(nil))
+}

internal/handler/cache_key_test.go

@@ -0,0 +1,74 @@
+package handler
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+)
+
+// TestConanMetadataKeysKeepReferencesApart checks that Conan references whose
+// paths only differ in where '/' and '_' fall, or in a query, are cached
+// apart. Names and versions may contain '_'.
+func TestConanMetadataKeysKeepReferencesApart(t *testing.T) {
+	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, _ = io.WriteString(w, r.URL.Path+"?"+r.URL.RawQuery)
+	}))
+	defer upstream.Close()
+
+	proxy, _, _, _ := setupTestProxy(t)
+	proxy.CacheMetadata = true
+	proxy.MetadataTTL = time.Hour
+	proxy.HTTPClient = upstream.Client()
+	srv := httptest.NewServer(NewConanHandlerWithUpstream(proxy, testProxyURL, upstream.URL).Routes())
+	defer srv.Close()
+
+	for _, pair := range [][2]string{
+		{"/v2/conans/foo/bar_1.0/_/_/latest", "/v2/conans/foo_bar/1.0/_/_/latest"},
+		{"/v2/conans/foo/bar_1.0/_/_/revisions/r1/files/conanfile.py", "/v2/conans/foo_bar/1.0/_/_/revisions/r1/files/conanfile.py"},
+		{"/v2/conans/search?q=zlib", "/v2/conans/search_q=zlib"},
+	} {
+		for _, target := range pair {
+			resp, err := http.Get(srv.URL + target)
+			if err != nil {
+				t.Fatal(err)
+			}
+			body, _ := io.ReadAll(resp.Body)
+			_ = resp.Body.Close()
+			want := target
+			if !strings.Contains(want, "?") {
+				want += "?"
+			}
+			if string(body) != want {
+				t.Errorf("GET %s served %q, want %q", target, body, want)
+			}
+		}
+	}
+}
+
+// TestMetadataCacheKeysUnambiguous checks that the path-based metadata cache
+// keys of the other ecosystems keep paths apart that differ only in where
+// '/' and '_' fall.
+func TestMetadataCacheKeysUnambiguous(t *testing.T) {
+	const a, b = "x/foo/bar_1.0/file", "x/foo_bar/1.0/file"
+
+	maven := mavenSource{upstreamURL: "https://repo.test"}
+	debian := NewDebianHandler(nil, "http://proxy.test", "https://deb.test", nil)
+	rpm := NewRPMHandlerWithUpstream(nil, "http://proxy.test", "https://rpm.test")
+	cran := NewCRANHandlerWithUpstream(nil, "http://proxy.test", "https://cran.test")
+	conda := NewCondaHandlerWithUpstream(nil, "http://proxy.test", "https://conda.test")
+
+	for name, key := range map[string]func(string) string{
+		"maven":  maven.metadataCacheKey,
+		"debian": func(p string) string { return debian.metadataCacheKeyFor("", debian.upstreamURL, p) },
+		"rpm":    func(p string) string { return rpm.metadataCacheKey(p) },
+		"cran":   func(p string) string { return cran.metadataCacheKey("/" + p) },
+		"conda":  func(p string) string { return conda.metadataCacheKey("/" + p) },
+	} {
+		if key(a) == key(b) {
+			t.Errorf("%s: %s and %s share metadata cache key %q", name, a, b, key(a))
+		}
+	}
+}

internal/handler/conan.go

@@ -162,11 +162,7 @@ func (h *ConanHandler) shouldCacheFile(filename string) bool {
 
 // proxyCached forwards a request with metadata caching.
 func (h *ConanHandler) proxyCached(w http.ResponseWriter, r *http.Request) {
-	cacheKey := strings.TrimPrefix(r.URL.Path, "/")
-	cacheKey = strings.ReplaceAll(cacheKey, "/", "_")
-	if r.URL.RawQuery != "" {
-		cacheKey += "_" + r.URL.RawQuery
-	}
+	cacheKey := hashedCacheKey(h.upstreamURL, r.URL.Path, r.URL.RawQuery)
 	upstreamURL := h.upstreamURL + r.URL.Path
 	if r.URL.RawQuery != "" {
 		upstreamURL += "?" + r.URL.RawQuery

internal/handler/conda.go

@@ -320,9 +320,14 @@ func (h *CondaHandler) recordInCooldown(filename, name, version string, timestam
 	return true
 }
 
+// metadataCacheKey is the metadata cache key of a request path.
+func (h *CondaHandler) metadataCacheKey(urlPath string) string {
+	return hashedCacheKey(h.upstreamURL, urlPath)
+}
+
 // proxyCached forwards a metadata request with caching.
 func (h *CondaHandler) proxyCached(w http.ResponseWriter, r *http.Request) {
-	cacheKey := condaCacheKey(r.URL.Path)
+	cacheKey := h.metadataCacheKey(r.URL.Path)
 	// Large JSON indexes can exceed the metadata limit uncompressed. Conda
 	// clients decode Content-Encoding, so cache and relay gzip verbatim. The
 	// already-compressed repodata.json.bz2 route must stay on identity.

internal/handler/conda_encoding_test.go

@@ -40,7 +40,8 @@ func TestCondaRepodataGzip(t *testing.T) {
 			if tt.mode == "cached" {
 				proxy.MetadataTTL = time.Hour
 			}
-			handler := NewCondaHandlerWithUpstream(proxy, "http://proxy.local", upstream.URL).Routes()
+			h := NewCondaHandlerWithUpstream(proxy, "http://proxy.local", upstream.URL)
+			handler := h.Routes()
 			serve := func() *httptest.ResponseRecorder {
 				w := httptest.NewRecorder()
 				handler.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/conda-forge/linux-64/"+tt.filename, nil))
@@ -53,7 +54,7 @@ func TestCondaRepodataGzip(t *testing.T) {
 			if tt.mode == "stream" {
 				return
 			}
-			entry, err := db.GetMetadataCache("conda", "conda-forge_linux-64_"+tt.filename)
+			entry, err := db.GetMetadataCache("conda", h.metadataCacheKey("/conda-forge/linux-64/"+tt.filename))
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -122,7 +123,8 @@ func TestCondaRepodataCacheSizeLimit(t *testing.T) {
 			proxy.HTTPClient = upstream.Client()
 			proxy.CacheMetadata = true
 			proxy.MetadataMaxSize = tt.limit
-			handler := NewCondaHandlerWithUpstream(proxy, "http://proxy.local", upstream.URL).Routes()
+			h := NewCondaHandlerWithUpstream(proxy, "http://proxy.local", upstream.URL)
+			handler := h.Routes()
 			w := httptest.NewRecorder()
 			handler.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/conda-forge/linux-64/repodata.json", nil))
 			if w.Code != tt.status {
@@ -132,7 +134,7 @@ func TestCondaRepodataCacheSizeLimit(t *testing.T) {
 				if !bytes.Equal(w.Body.Bytes(), plain) || w.Header().Get(headerContentEncoding) != "" {
 					t.Fatal("identity response was changed or mislabeled as gzip")
 				}
-			} else if entry, _ := db.GetMetadataCache("conda", "conda-forge_linux-64_repodata.json"); entry != nil {
+			} else if entry, _ := db.GetMetadataCache("conda", h.metadataCacheKey("/conda-forge/linux-64/repodata.json")); entry != nil {
 				t.Fatal("oversized response was cached")
 			}
 		})

internal/handler/conda_shards.go

@@ -56,7 +56,7 @@ func (h *CondaHandler) cooldownEnabled() bool {
 func (h *CondaHandler) handleShardIndex(w http.ResponseWriter, r *http.Request) {
 	channel, arch := r.PathValue("channel"), r.PathValue("arch")
 	upstreamURL := h.upstreamURL + r.URL.Path
-	cacheKey := condaCacheKey(r.URL.Path)
+	cacheKey := h.metadataCacheKey(r.URL.Path)
 
 	body, contentType, contentEncoding, err := h.proxy.fetchOrCacheMetadata(r.Context(), "conda", cacheKey, upstreamURL, "identity", nil, "*/*")
 	if err != nil {
@@ -121,7 +121,7 @@ func (h *CondaHandler) handleCooldownShard(w http.ResponseWriter, r *http.Reques
 	}
 	upstreamPath := "/" + channel + "/" + arch + "/" + dir + hex.EncodeToString(masked) + condaShardSuffix
 
-	body, _, _, err := h.proxy.fetchOrCacheMetadata(r.Context(), "conda", condaCacheKey(upstreamPath), h.upstreamURL+upstreamPath, "identity", nil, "*/*")
+	body, _, _, err := h.proxy.fetchOrCacheMetadata(r.Context(), "conda", h.metadataCacheKey(upstreamPath), h.upstreamURL+upstreamPath, "identity", nil, "*/*")
 	if err != nil {
 		h.proxy.writeMetadataFetchError(w, err)
 		return
@@ -454,8 +454,3 @@ func msgpackNumber(v any) float64 {
 		return 0
 	}
 }
-
-// condaCacheKey is the metadata cache key for a request path.
-func condaCacheKey(urlPath string) string {
-	return strings.ReplaceAll(strings.TrimPrefix(urlPath, "/"), "/", "_")
-}

internal/handler/cran.go

@@ -63,7 +63,7 @@ func (h *CRANHandler) Routes() http.Handler {
 	// Binary package downloads
 	mux.HandleFunc("GET /bin/{platform}/contrib/{rversion}/{filename}", h.handleBinaryDownload)
 
-	return mux
+	return rejectQueryChars(mux)
 }
 
 // handleSourceDownload serves a source package, fetching and caching from upstream.
@@ -165,11 +165,14 @@ func (h *CRANHandler) isBinaryPackage(filename string) bool {
 	return strings.HasSuffix(filename, ".zip") || strings.HasSuffix(filename, ".tgz")
 }
 
+// metadataCacheKey is the metadata cache key of a request path.
+func (h *CRANHandler) metadataCacheKey(urlPath string) string {
+	return hashedCacheKey(h.upstreamURL, urlPath)
+}
+
 // proxyCached forwards a metadata request with caching.
 func (h *CRANHandler) proxyCached(w http.ResponseWriter, r *http.Request) {
-	cacheKey := strings.TrimPrefix(r.URL.Path, "/")
-	cacheKey = strings.ReplaceAll(cacheKey, "/", "_")
-	h.proxy.ProxyCached(w, r, h.upstreamURL+r.URL.Path, "cran", cacheKey, "*/*")
+	h.proxy.ProxyCached(w, r, h.upstreamURL+r.URL.Path, "cran", h.metadataCacheKey(r.URL.Path), "*/*")
 }
 
 // proxyUpstream forwards a request to CRAN without caching.

internal/handler/debian.go

@@ -75,7 +75,7 @@ func NewDebianHandler(
 // Routes returns the HTTP handler for Debian requests.
 // Mount this at /debian on your router.
 func (h *DebianHandler) Routes() http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return rejectQueryChars(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != http.MethodGet && r.Method != http.MethodHead {
 			http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
 			return
@@ -120,7 +120,7 @@ func (h *DebianHandler) Routes() http.Handler {
 			// Other files (like README, etc.) - proxy directly
 			h.proxyFile(w, r, upstreamURL, rest)
 		}
-	})
+	}))
 }
 
 // repositoryNotFound answers a request addressed to a repository that is not
@@ -203,11 +203,11 @@ func (h *DebianHandler) artifactCacheFilename(repository, filename string) strin
 }
 
 // metadataCacheKeyFor returns the metadata cache key for a request. The main
-// archive keeps its separator-based key so existing cache entries stay valid;
-// a named repository hashes its identity as APKHandler.metadataCacheKey does.
+// archive hashes its upstream URL and the path; a named repository hashes its
+// identity as APKHandler.metadataCacheKey does.
 func (h *DebianHandler) metadataCacheKeyFor(repository, upstreamURL, path string) string {
 	if repository == "" {
-		return strings.ReplaceAll(path, "/", "_")
+		return hashedCacheKey(upstreamURL, path)
 	}
 	identity := repository + "\x00" + upstreamURL + "\x00" + path
 	digest := sha256.Sum256([]byte(identity))

internal/handler/debian_test.go

@@ -35,9 +35,11 @@ func TestDebianHandler_Routes(t *testing.T) {
 	assertRoutesBasics(t, h.Routes(), "/dists/stable/Release", "/pool/../../../etc/passwd")
 }
 
-// TestDebianHandler_LegacyCacheKeysUnchanged pins the cache identities the
-// main archive used before named repositories existed. Deployments carry warm
-// caches across upgrades, so a changed key here silently discards them.
+// TestDebianHandler_LegacyCacheKeysUnchanged pins the artifact cache identity
+// the main archive used before named repositories existed. Deployments carry
+// warm caches across upgrades, so a changed key here silently discards them.
+// Metadata is cheap to refetch; its keys are covered by
+// TestMetadataCacheKeysUnambiguous.
 func TestDebianHandler_LegacyCacheKeysUnchanged(t *testing.T) {
 	const (
 		poolPath = "pool/main/h/hello/hello_2.10-3_amd64.deb"
@@ -54,17 +56,12 @@ func TestDebianHandler_LegacyCacheKeysUnchanged(t *testing.T) {
 		t.Errorf("legacy artifact cache filename = %q, want %q", got, "hello_2.10-3_amd64.deb")
 	}
 
-	// Metadata cache: path separators replaced with underscores.
-	if got := h.metadataCacheKeyFor("", h.upstreamURL, distPath); got != "dists_trixie_InRelease" {
-		t.Errorf("legacy metadata cache key = %q, want %q", got, "dists_trixie_InRelease")
-	}
-
 	// A named repository must not reuse either identity.
 	if got := h.artifactCacheFilename("security", filename); got == "hello_2.10-3_amd64.deb" {
 		t.Error("named repository reused the legacy artifact cache filename")
 	}
-	if got := h.metadataCacheKeyFor("security", "https://security.test", distPath); got == "dists_trixie_InRelease" {
-		t.Error("named repository reused the legacy metadata cache key")
+	if h.metadataCacheKeyFor("security", "https://security.test", distPath) == h.metadataCacheKeyFor("", h.upstreamURL, distPath) {
+		t.Error("named repository reused the main archive's metadata cache key")
 	}
 }
 

internal/handler/handler.go

@@ -44,6 +44,18 @@ func containsPathTraversal(path string) bool {
 	return false
 }
 
+// rejectQueryChars refuses a decoded path containing '?' or '#', which would cut
+// the upstream URL short and cache one file under another's name.
+func rejectQueryChars(next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.ContainsAny(r.URL.Path, "?#") {
+			http.Error(w, "invalid path", http.StatusBadRequest)
+			return
+		}
+		next.ServeHTTP(w, r)
+	})
+}
+
 func hasDotDotSegment(path string) bool {
 	path = strings.ReplaceAll(path, "\\", "/")
 	for segment := range strings.SplitSeq(path, "/") {

internal/handler/maven.go

@@ -103,7 +103,7 @@ func (h *MavenHandler) Routes() http.Handler {
 	// e.g., /com/google/guava/guava/32.1.3-jre/guava-32.1.3-jre.jar
 	mux.HandleFunc("GET /", h.handleRequest)
 
-	return mux
+	return rejectQueryChars(mux)
 }
 
 // handleRequest routes Maven requests based on the path.
@@ -146,12 +146,12 @@ func (h *MavenHandler) handleRequest(w http.ResponseWriter, r *http.Request) {
 }
 
 // metadataCacheKey is the metadata cache key of urlPath. The default
-// repository keeps its original keys. A named repository's keys are hashed
-// from its name, upstream URL and the path, so repositories never share
-// entries and repointing one drops what it cached.
+// repository's keys are hashed from its upstream URLs and the path. A named
+// repository's keys are hashed from its name, upstream URL and the path, so
+// repositories never share entries and repointing one drops what it cached.
 func (s mavenSource) metadataCacheKey(urlPath string) string {
 	if s.name == "" {
-		return strings.ReplaceAll(urlPath, "/", "_")
+		return hashedCacheKey(s.upstreamURL, s.fallbackURL, urlPath)
 	}
 	digest := sha256.Sum256([]byte(s.name + "\x00" + s.upstreamURL + "\x00" + urlPath))
 	return "repo_" + hex.EncodeToString(digest[:])

internal/handler/pypi.go

@@ -84,7 +84,7 @@ func (h *PyPIHandler) Routes() http.Handler {
 	// Package downloads (cache these)
 	mux.HandleFunc("GET /packages/{path...}", h.handleDownload)
 
-	return mux
+	return rejectQueryChars(mux)
 }
 
 // handleSimpleIndex serves the simple API index.