From 4c3c925cd1f6e32a5b7121ff54382962c7f40795 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Mon, 9 Nov 2026 17:44:19 +0100 Subject: [PATCH 1/2] Key Debian and Conan archives by their whole path A Debian package was cached under its repository and file name, but fetched from its full path. Docker's apt repository has a pool under each suite with the same file names, so a bookworm client could get the bullseye build that was fetched first, and fail its hash check. Packages are now cached under their path. Conan joined revision, package reference, package revision and file name with underscores, which revisions can contain, so a recipe file and a package file could share a cache entry. The parts are now joined with slashes. Entries cached under the old names are still served, but only for the URL they were fetched from, so warm caches keep working. --- internal/handler/conan.go | 32 ++++++++-- internal/handler/conan_test.go | 84 +++++++++++++++++++++++-- internal/handler/debian.go | 26 ++++++-- internal/handler/debian_test.go | 84 +++++++++++++++++++++++-- internal/handler/handler.go | 20 ++++++ internal/server/retention_conan_test.go | 8 +-- internal/server/retention_deb_test.go | 4 +- 7 files changed, 230 insertions(+), 28 deletions(-) diff --git a/internal/handler/conan.go b/internal/handler/conan.go index dddd79f4..9a07f24d 100644 --- a/internal/handler/conan.go +++ b/internal/handler/conan.go @@ -89,7 +89,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 { @@ -121,7 +127,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 +145,19 @@ 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 parts are path +// segments, which can't contain 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...), "/") +} + +// 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 } diff --git a/internal/handler/conan_test.go b/internal/handler/conan_test.go index cf8038e6..5be99fe3 100644 --- a/internal/handler/conan_test.go +++ b/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,72 @@ 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()) + } +} diff --git a/internal/handler/debian.go b/internal/handler/debian.go index 565f181a..dc6d6dcc 100644 --- a/internal/handler/debian.go +++ b/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 } diff --git a/internal/handler/debian_test.go b/internal/handler/debian_test.go index 54a72b61..f9c62847 100644 --- a/internal/handler/debian_test.go +++ b/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()) + } +} diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 9612e510..62a49e89 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -2396,6 +2396,26 @@ func (p *Proxy) GetOrFetchArtifactFromURL(ctx context.Context, ecosystem, name, return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, "", "") } +// cacheFilenameKeepingLegacy returns the filename to cache an artifact +// fetched from downloadURL under: legacy, an earlier cache filename that more +// than one URL could share, while an artifact recorded for downloadURL is +// cached under it, and filename otherwise. It keeps entries cached before a +// key change in use, without serving one to a URL it wasn't fetched from. +func (p *Proxy) cacheFilenameKeepingLegacy(ecosystem, name, version, filename, legacy, downloadURL string) (string, error) { + _, versionPURL, err := packagePURLStrings(ecosystem, name, version) + if err != nil { + return "", err + } + record, err := p.DB.GetArtifact(versionPURL, legacy) + if err != nil { + return "", fmt.Errorf("checking artifact cache: %w", err) + } + if record != nil && record.UpstreamURL == downloadURL { + return legacy, nil + } + return filename, nil +} + // GetOrFetchArtifactFromURLWithHeaders retrieves an artifact from cache or fetches from a URL // with additional request-specific HTTP headers. func (p *Proxy) GetOrFetchArtifactFromURLWithHeaders(ctx context.Context, ecosystem, name, version, filename, downloadURL string, headers http.Header) (*CacheResult, error) { diff --git a/internal/server/retention_conan_test.go b/internal/server/retention_conan_test.go index 17607bc7..e5023be0 100644 --- a/internal/server/retention_conan_test.go +++ b/internal/server/retention_conan_test.go @@ -29,15 +29,15 @@ type conanFile struct { var ( conanZlibSources = conanFile{ "/v2/conans/zlib/1.3.1/_/_/revisions/" + conanRecipeRevision + "/files/conan_sources.tgz", - conanRecipeRevision + "_conan_sources.tgz", + "_/_/" + conanRecipeRevision + "/conan_sources.tgz", } conanZlibPackage = conanFile{ "/v2/conans/zlib/1.3.1/_/_/revisions/" + conanRecipeRevision + "/packages/0123abc/revisions/4567def/files/conan_package.tgz", - conanRecipeRevision + "_0123abc_4567def_conan_package.tgz", + "_/_/" + conanRecipeRevision + "/packages/0123abc/4567def/conan_package.tgz", } conanZlibAcme = conanFile{ "/v2/conans/zlib/1.2.13/acme/stable/revisions/" + conanRecipeRevision + "/files/conan_export.tgz", - "acme@stable_" + conanRecipeRevision + "_conan_export.tgz", + "acme/stable/" + conanRecipeRevision + "/conan_export.tgz", } conanOpenSSLSources = conanFile{ "/v2/conans/openssl/3.3.2/_/_/revisions/" + conanRecipeRevision + "/files/conan_sources.tgz", @@ -209,7 +209,7 @@ func TestConanRetentionPackageRule(t *testing.T) { t.Errorf("%s evicted despite the never-evict rule for zlib", f.filename) } } - if conanArtifact(t, db, "pkg:conan/openssl", conanRecipeRevision+"_conan_sources.tgz").StoragePath.Valid { + if conanArtifact(t, db, "pkg:conan/openssl", "_/_/"+conanRecipeRevision+"/conan_sources.tgz").StoragePath.Valid { t.Error("openssl kept past the conan rule") } } diff --git a/internal/server/retention_deb_test.go b/internal/server/retention_deb_test.go index 8081224f..3f6f61d2 100644 --- a/internal/server/retention_deb_test.go +++ b/internal/server/retention_deb_test.go @@ -20,8 +20,8 @@ import ( const ( debCurlPath = "/pool/main/c/curl/curl_8.14.1-2_amd64.deb" debBusyboxPath = "/pool/main/b/busybox/busybox_1.37.0-6_amd64.deb" - debCurlFile = "curl_8.14.1-2_amd64.deb" - debBusyboxFile = "busybox_1.37.0-6_amd64.deb" + debCurlFile = "pool/main/c/curl/curl_8.14.1-2_amd64.deb" + debBusyboxFile = "pool/main/b/busybox/busybox_1.37.0-6_amd64.deb" ) // debRetentionUpstream serves package files the way a Debian archive does, From 3e50f45b0d4fb3ead542b1ada48c8639eba7c06d Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Tue, 10 Nov 2026 13:03:00 +0100 Subject: [PATCH 2/2] Refuse Conan file requests with an encoded slash A path value is decoded, so a revision of R%2Fpackages%2FP%2FQ gave a recipe file the cache key of a package file. The recipe and package file routes now answer 400 when user, channel, a revision, the package reference or the filename contains a slash. --- internal/handler/conan.go | 23 ++++++++++++++++++++--- internal/handler/conan_test.go | 30 ++++++++++++++++++++++++++++++ internal/handler/handler.go | 7 ++----- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/internal/handler/conan.go b/internal/handler/conan.go index 9a07f24d..90edcdb2 100644 --- a/internal/handler/conan.go +++ b/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) @@ -117,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) @@ -146,13 +157,19 @@ 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 user, channel and the -// revisions, to keep every recipe and binary apart. The parts are path -// segments, which can't contain a slash, so joining them with one keeps -// different references apart. +// 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 diff --git a/internal/handler/conan_test.go b/internal/handler/conan_test.go index 5be99fe3..200f8671 100644 --- a/internal/handler/conan_test.go +++ b/internal/handler/conan_test.go @@ -723,3 +723,33 @@ func TestConanLegacyFileEntryServedForItsURL(t *testing.T) { 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()) + } + } +} diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 62a49e89..9e8b5657 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -2396,11 +2396,8 @@ func (p *Proxy) GetOrFetchArtifactFromURL(ctx context.Context, ecosystem, name, return p.getOrFetchArtifactFromURL(ctx, ecosystem, name, version, filename, downloadURL, nil, "", "") } -// cacheFilenameKeepingLegacy returns the filename to cache an artifact -// fetched from downloadURL under: legacy, an earlier cache filename that more -// than one URL could share, while an artifact recorded for downloadURL is -// cached under it, and filename otherwise. It keeps entries cached before a -// key change in use, without serving one to a URL it wasn't fetched from. +// cacheFilenameKeepingLegacy returns legacy when an artifact recorded under +// it was fetched from downloadURL, and filename otherwise. func (p *Proxy) cacheFilenameKeepingLegacy(ecosystem, name, version, filename, legacy, downloadURL string) (string, error) { _, versionPURL, err := packagePURLStrings(ecosystem, name, version) if err != nil {