Key Debian and Conan archives by their whole path

internal/handler/conan.go

@@ -2,6 +2,7 @@ package handler
 
 import (
 	"net/http"
+	"slices"
 	"strings"
 
 	"github.com/git-pkgs/proxy/internal/retention"
@@ -79,6 +80,11 @@ func (h *ConanHandler) handleRecipeFile(w http.ResponseWriter, r *http.Request)
 	revision := r.PathValue("revision")
 	filename := r.PathValue("filename")
 
+	if conanPartHasSlash(user, channel, revision, filename) {
+		http.Error(w, "invalid path", http.StatusBadRequest)
+		return
+	}
+
 	// Only the archives are artifacts; the rest goes through the metadata cache.
 	if !h.shouldCacheFile(filename) {
 		h.proxyCached(w, r)
@@ -89,7 +95,13 @@ func (h *ConanHandler) handleRecipeFile(w http.ResponseWriter, r *http.Request)
 		"name", name, "version", version, "user", user, "channel", channel, "filename", filename)
 
 	upstreamURL := h.upstreamURL + r.URL.Path
-	storageFilename := conanStorageFilename(user, channel, revision+"_"+filename)
+	storageFilename, err := h.proxy.cacheFilenameKeepingLegacy("conan", name, version,
+		conanStorageFilename(user, channel, revision, filename),
+		conanLegacyStorageFilename(user, channel, revision+"_"+filename), upstreamURL)
+	if err != nil {
+		h.proxy.serveArtifactError(w, err, "failed to fetch file")
+		return
+	}
 
 	result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "conan", name, version, storageFilename, upstreamURL)
 	if err != nil {
@@ -111,6 +123,11 @@ func (h *ConanHandler) handlePackageFile(w http.ResponseWriter, r *http.Request)
 	pkgrev := r.PathValue("pkgrev")
 	filename := r.PathValue("filename")
 
+	if conanPartHasSlash(user, channel, revision, pkgref, pkgrev, filename) {
+		http.Error(w, "invalid path", http.StatusBadRequest)
+		return
+	}
+
 	// Only the archives are artifacts; the rest goes through the metadata cache.
 	if !h.shouldCacheFile(filename) {
 		h.proxyCached(w, r)
@@ -121,7 +138,13 @@ func (h *ConanHandler) handlePackageFile(w http.ResponseWriter, r *http.Request)
 		"name", name, "version", version, "pkgref", pkgref, "filename", filename)
 
 	upstreamURL := h.upstreamURL + r.URL.Path
-	storageFilename := conanStorageFilename(user, channel, revision+"_"+pkgref+"_"+pkgrev+"_"+filename)
+	storageFilename, err := h.proxy.cacheFilenameKeepingLegacy("conan", name, version,
+		conanStorageFilename(user, channel, revision, "packages", pkgref, pkgrev, filename),
+		conanLegacyStorageFilename(user, channel, revision+"_"+pkgref+"_"+pkgrev+"_"+filename), upstreamURL)
+	if err != nil {
+		h.proxy.serveArtifactError(w, err, "failed to fetch file")
+		return
+	}
 
 	result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "conan", name, version, storageFilename, upstreamURL)
 	if err != nil {
@@ -133,9 +156,25 @@ func (h *ConanHandler) handlePackageFile(w http.ResponseWriter, r *http.Request)
 }
 
 // conanStorageFilename names a cached archive. Archives are stored under
-// pkg:conan/{name}@{version}, so the file name carries the revisions and, when
-// set, user and channel, to keep every recipe and binary apart.
-func conanStorageFilename(user, channel, file string) string {
+// pkg:conan/{name}@{version}, so the file name carries user, channel and the
+// revisions, to keep every recipe and binary apart. The handlers refuse a
+// part that contains a slash, so joining them with one keeps different
+// references apart.
+func conanStorageFilename(user, channel string, parts ...string) string {
+	return strings.Join(append([]string{user, channel}, parts...), "/")
+}
+
+// conanPartHasSlash reports whether a path value contains a slash, which it
+// can only get from an encoded one (%2F) in the request.
+func conanPartHasSlash(parts ...string) bool {
+	return slices.ContainsFunc(parts, func(part string) bool { return strings.Contains(part, "/") })
+}
+
+// conanLegacyStorageFilename is the name archives were cached under before
+// their parts were joined with slashes. Underscores, which revisions, user
+// and channel can contain, joined them, so it is only read for the URL an
+// entry was fetched from.
+func conanLegacyStorageFilename(user, channel, file string) string {
 	if user == "_" && channel == "_" {
 		return file
 	}

internal/handler/conan_test.go

@@ -1,6 +1,7 @@
 package handler
 
 import (
+	"context"
 	"io"
 	"log/slog"
 	"net/http"
@@ -9,6 +10,8 @@ import (
 	"sync/atomic"
 	"testing"
 	"time"
+
+	"github.com/git-pkgs/registries/fetch"
 )
 
 const testProxyURL = "http://localhost:8080"
@@ -302,13 +305,13 @@ func TestConanV2FileRoutesCacheArchives(t *testing.T) {
 		filename string
 	}{
 		{"recipe", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/files/conan_sources.tgz",
-			"rrev1_conan_sources.tgz"},
+			"_/_/rrev1/conan_sources.tgz"},
 		{"package", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/packages/pkgid1/revisions/prev1/files/conan_package.tgz",
-			"rrev1_pkgid1_prev1_conan_package.tgz"},
+			"_/_/rrev1/packages/pkgid1/prev1/conan_package.tgz"},
 		{"recipe xz", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/files/conan_export.txz",
-			"rrev1_conan_export.txz"},
+			"_/_/rrev1/conan_export.txz"},
 		{"package zstd", "/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/packages/pkgid1/revisions/prev1/files/conan_package.tzst",
-			"rrev1_pkgid1_prev1_conan_package.tzst"},
+			"_/_/rrev1/packages/pkgid1/prev1/conan_package.tzst"},
 	}
 
 	for _, tt := range tests {
@@ -362,9 +365,9 @@ func TestConanUserChannelKeptApart(t *testing.T) {
 
 	for _, ref := range []struct{ path, body, filename string }{
 		{"/v2/conans/zlib/1.3.1/_/_/revisions/rrev1/files/conan_sources.tgz",
-			"center sources", "rrev1_conan_sources.tgz"},
+			"center sources", "_/_/rrev1/conan_sources.tgz"},
 		{"/v2/conans/zlib/1.3.1/acme/stable/revisions/rrev1/files/conan_sources.tgz",
-			"acme sources", "acme@stable_rrev1_conan_sources.tgz"},
+			"acme sources", "acme/stable/rrev1/conan_sources.tgz"},
 	} {
 		fetcher.artifact = artifactBody(ref.body)
 		fetcher.fetchCalled = false
@@ -651,3 +654,102 @@ func TestConanMetadataPreservesStatusCodes(t *testing.T) {
 		upstream.Close()
 	}
 }
+
+const (
+	conanRecipeLookalike = "/v2/conans/zlib/1.3/_/_/revisions/R_P_Q/files/conan_package.tgz"
+	conanPackageFile     = "/v2/conans/zlib/1.3/_/_/revisions/R/packages/P/revisions/Q/files/conan_package.tgz"
+)
+
+// conanPathEchoServer answers every file with its own path, so each URL has
+// different bytes.
+func conanPathEchoServer(t *testing.T, available *atomic.Bool) *httptest.Server {
+	t.Helper()
+	upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !available.Load() {
+			http.Error(w, "unavailable", http.StatusServiceUnavailable)
+			return
+		}
+		_, _ = w.Write([]byte(r.URL.Path))
+	}))
+	t.Cleanup(upstream.Close)
+	return upstream
+}
+
+func serveConanRequest(h *ConanHandler, target string) *httptest.ResponseRecorder {
+	w := httptest.NewRecorder()
+	h.Routes().ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil))
+	return w
+}
+
+// A recipe revision with underscores must not share a cache entry with a
+// package whose revision, reference and package revision spell it.
+func TestConanFileKeysKeepPathFieldsApart(t *testing.T) {
+	var available atomic.Bool
+	available.Store(true)
+	upstream := conanPathEchoServer(t, &available)
+	proxy, _, _, _ := setupTestProxy(t)
+	proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0))
+	h := NewConanHandlerWithUpstream(proxy, testProxyURL, upstream.URL)
+
+	for _, target := range []string{conanRecipeLookalike, conanPackageFile} {
+		if w := serveConanRequest(h, target); w.Code != http.StatusOK || w.Body.String() != target {
+			t.Errorf("GET %s = %d %q, want its own file", target, w.Code, w.Body.String())
+		}
+	}
+}
+
+// A file cached under the name used before path fields were kept apart is
+// still served for the URL it was fetched from, and only for that URL.
+func TestConanLegacyFileEntryServedForItsURL(t *testing.T) {
+	var available atomic.Bool
+	available.Store(true)
+	upstream := conanPathEchoServer(t, &available)
+	proxy, _, _, _ := setupTestProxy(t)
+	proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0))
+	h := NewConanHandlerWithUpstream(proxy, testProxyURL, upstream.URL)
+
+	legacy, err := proxy.GetOrFetchArtifactFromURL(context.Background(), "conan", "zlib", "1.3",
+		"R_P_Q_conan_package.tgz", upstream.URL+conanRecipeLookalike)
+	if err != nil {
+		t.Fatalf("seed legacy entry: %v", err)
+	}
+	_ = legacy.Reader.Close()
+
+	if w := serveConanRequest(h, conanPackageFile); w.Body.String() != conanPackageFile {
+		t.Errorf("package file = %d %q, want its own file, not the legacy entry", w.Code, w.Body.String())
+	}
+	available.Store(false)
+	if w := serveConanRequest(h, conanRecipeLookalike); w.Code != http.StatusOK || w.Body.String() != conanRecipeLookalike {
+		t.Errorf("recipe file with the upstream down = %d %q, want the legacy entry", w.Code, w.Body.String())
+	}
+}
+
+// A path value is decoded, so one holding an encoded slash could spell the
+// cache key of another reference. Those requests are refused.
+func TestConanFileRoutesRefuseEncodedSlash(t *testing.T) {
+	var available atomic.Bool
+	available.Store(true)
+	upstream := conanPathEchoServer(t, &available)
+	proxy, _, _, _ := setupTestProxy(t)
+	proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0))
+	h := NewConanHandlerWithUpstream(proxy, testProxyURL, upstream.URL)
+
+	if w := serveConanRequest(h, conanPackageFile); w.Code != http.StatusOK || w.Body.String() != conanPackageFile {
+		t.Fatalf("GET %s = %d %q, want its own file", conanPackageFile, w.Code, w.Body.String())
+	}
+
+	const recipe, pkg = "/v2/conans/zlib/1.3/_/_/revisions/", "/v2/conans/zlib/1.3/_/_/revisions/R/packages/"
+	for field, target := range map[string]string{
+		"revision spelling a package file": recipe + "R%2Fpackages%2FP%2FQ/files/conan_package.tgz",
+		"user":                             "/v2/conans/zlib/1.3/a%2Fb/_/revisions/R/files/conan_package.tgz",
+		"channel":                          "/v2/conans/zlib/1.3/_/a%2Fb/revisions/R/files/conan_package.tgz",
+		"recipe filename":                  recipe + "R/files/a%2Fconan_package.tgz",
+		"package reference":                pkg + "P%2FX/revisions/Q/files/conan_package.tgz",
+		"package revision":                 pkg + "P/revisions/Q%2FX/files/conan_package.tgz",
+		"package filename":                 pkg + "P/revisions/Q/files/a%2Fconan_package.tgz",
+	} {
+		if w := serveConanRequest(h, target); w.Code != http.StatusBadRequest {
+			t.Errorf("%s: GET %s = %d %q, want 400", field, target, w.Code, w.Body.String())
+		}
+	}
+}

internal/handler/debian.go

@@ -168,7 +168,12 @@ func (h *DebianHandler) handlePackageDownload(
 	filename := path[strings.LastIndex(path, "/")+1:]
 	downloadURL := fmt.Sprintf("%s/%s", upstreamURL, path)
 
-	cacheFilename := h.artifactCacheFilename(repository, filename)
+	cacheFilename, err := h.proxy.cacheFilenameKeepingLegacy("deb", name, version,
+		h.artifactCacheFilename(repository, path), h.legacyArtifactCacheFilename(repository, filename), downloadURL)
+	if err != nil {
+		h.proxy.serveArtifactError(w, err, "failed to fetch package")
+		return
+	}
 
 	h.proxy.Logger.Info("debian package download",
 		"repository", repository,
@@ -196,10 +201,21 @@ func (h *DebianHandler) handleMetadata(
 	h.proxy.ProxyCached(w, r, fmt.Sprintf("%s/%s", upstreamURL, path), "debian", cacheKey, "*/*")
 }
 
-// artifactCacheFilename scopes a package by repository, since the same
-// filename can hold different bytes in different archives. The main archive
-// keeps the unscoped filename its existing cache entries were stored under.
-func (h *DebianHandler) artifactCacheFilename(repository, filename string) string {
+// artifactCacheFilename names a package in the cache by its repository and
+// its path in it, since the same file name can hold different bytes in
+// different archives, and in different directories of one archive: Docker's
+// repository has a pool under each suite.
+func (h *DebianHandler) artifactCacheFilename(repository, path string) string {
+	if repository == "" {
+		return path
+	}
+	return repository + "/" + path
+}
+
+// legacyArtifactCacheFilename is the name packages were cached under before
+// they were keyed by path: the file name, scoped by repository for a named
+// one. It is still read for the URL an entry was fetched from.
+func (h *DebianHandler) legacyArtifactCacheFilename(repository, filename string) string {
 	if repository == "" {
 		return filename
 	}

internal/handler/debian_test.go

@@ -1,10 +1,12 @@
 package handler
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -37,10 +39,11 @@ func TestDebianHandler_Routes(t *testing.T) {
 }
 
 // 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.
+// the main archive used before named repositories existed. Packages are now
+// keyed by path, but deployments carry warm caches across upgrades, so
+// entries under this name are still read for the URL they were fetched from
+// (TestDebianHandler_LegacyPackageEntryServedForItsPath). 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"
@@ -53,12 +56,12 @@ func TestDebianHandler_LegacyCacheKeysUnchanged(t *testing.T) {
 
 	// Artifact cache: the bare filename, with no repository prefix.
 	filename := poolPath[strings.LastIndex(poolPath, "/")+1:]
-	if got := h.artifactCacheFilename("", filename); got != "hello_2.10-3_amd64.deb" {
+	if got := h.legacyArtifactCacheFilename("", filename); got != "hello_2.10-3_amd64.deb" {
 		t.Errorf("legacy artifact cache filename = %q, want %q", got, "hello_2.10-3_amd64.deb")
 	}
 
 	// A named repository must not reuse either identity.
-	if got := h.artifactCacheFilename("security", filename); got == "hello_2.10-3_amd64.deb" {
+	if got := h.legacyArtifactCacheFilename("security", filename); got == "hello_2.10-3_amd64.deb" {
 		t.Error("named repository reused the legacy artifact cache filename")
 	}
 	if h.metadataCacheKeyFor("security", "https://security.test", distPath) == h.metadataCacheKeyFor("", h.upstreamURL, distPath) {
@@ -324,3 +327,72 @@ func TestDebRetentionKey(t *testing.T) {
 		t.Error("debRetentionKey(nil) accepted")
 	}
 }
+
+// debianSuiteArchive serves a different build of one package file name per
+// suite, as Docker's repository nests a pool under each suite.
+func debianSuiteArchive(t *testing.T, available *atomic.Bool) *httptest.Server {
+	t.Helper()
+	archive := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !available.Load() {
+			http.Error(w, "unavailable", http.StatusServiceUnavailable)
+			return
+		}
+		suite := strings.Split(strings.TrimPrefix(r.URL.Path, "/dists/"), "/")[0]
+		_, _ = fmt.Fprint(w, suite+" build")
+	}))
+	t.Cleanup(archive.Close)
+	return archive
+}
+
+const (
+	debianBullseyePackage = "/docker/dists/bullseye/pool/stable/amd64/containerd.io_1.7.25-1_amd64.deb"
+	debianBookwormPackage = "/docker/dists/bookworm/pool/stable/amd64/containerd.io_1.7.25-1_amd64.deb"
+)
+
+// TestDebianHandler_PackageCacheScopedPerPath checks that one file name in
+// two directories of a repository does not resolve to one cached artifact.
+func TestDebianHandler_PackageCacheScopedPerPath(t *testing.T) {
+	var available atomic.Bool
+	available.Store(true)
+	archive := debianSuiteArchive(t, &available)
+	proxy, _, _, _ := setupTestProxy(t)
+	proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(archive.Client()), fetch.WithMaxRetries(0))
+	h := NewDebianHandler(proxy, "http://proxy.example", "https://archive.test", map[string]string{"docker": archive.URL})
+
+	for _, tc := range []struct{ path, want string }{
+		{debianBullseyePackage, "bullseye build"},
+		{debianBookwormPackage, "bookworm build"},
+	} {
+		if w := serveDebianRequest(h, tc.path); w.Code != http.StatusOK || w.Body.String() != tc.want {
+			t.Errorf("GET %s = %d %q, want %q", tc.path, w.Code, w.Body.String(), tc.want)
+		}
+	}
+}
+
+// TestDebianHandler_LegacyPackageEntryServedForItsPath checks that a package
+// cached under the file name alone, as before packages were keyed by path, is
+// still served for the path it was fetched from, and only for that path.
+func TestDebianHandler_LegacyPackageEntryServedForItsPath(t *testing.T) {
+	var available atomic.Bool
+	available.Store(true)
+	archive := debianSuiteArchive(t, &available)
+	proxy, _, _, _ := setupTestProxy(t)
+	proxy.Fetcher = fetch.NewFetcher(fetch.WithHTTPClient(archive.Client()), fetch.WithMaxRetries(0))
+	h := NewDebianHandler(proxy, "http://proxy.example", "https://archive.test", map[string]string{"docker": archive.URL})
+
+	// The entry an earlier version cached for the bullseye path.
+	legacy, err := proxy.GetOrFetchArtifactFromURL(context.Background(), "deb", "containerd.io", "1.7.25-1",
+		"docker/containerd.io_1.7.25-1_amd64.deb", archive.URL+strings.TrimPrefix(debianBullseyePackage, "/docker"))
+	if err != nil {
+		t.Fatalf("seed legacy entry: %v", err)
+	}
+	_ = legacy.Reader.Close()
+
+	if w := serveDebianRequest(h, debianBookwormPackage); w.Body.String() != "bookworm build" {
+		t.Errorf("bookworm = %d %q, want its own build, not the legacy bullseye entry", w.Code, w.Body.String())
+	}
+	available.Store(false)
+	if w := serveDebianRequest(h, debianBullseyePackage); w.Code != http.StatusOK || w.Body.String() != "bullseye build" {
+		t.Errorf("bullseye with the archive down = %d %q, want the legacy entry", w.Code, w.Body.String())
+	}
+}