From cbbeed423b741d7eb64e51cdd79be82d5f5f862c Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 07:56:52 +0100 Subject: [PATCH 01/14] Cache RPM packages under their full path RPM packages were cached under their bare filename. With upstream.rpm pointing at a mirror root that serves several distributions, Rocky and Alma both publish bash-5.1.8-9.el9.x86_64.rpm with different bytes, and whichever was fetched first was served for both, so dnf failed the checksum or signature check. Use the path under the upstream as the cache filename, as the APK and Arch handlers do. Packages cached before this change are no longer found and are fetched again once; retention still matches them by package name. --- docs/configuration.md | 2 +- internal/handler/rpm.go | 9 +++-- internal/handler/rpm_test.go | 50 +++++++++++++++++++++++++++ internal/server/retention_rpm_test.go | 4 +-- 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 584896bc..9046e289 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1820,7 +1820,7 @@ curl -X DELETE http://localhost:8080/api/cache/maven/com.google.guava:guava/32.1 curl -X DELETE 'http://localhost:8080/api/cache/maven/com.google.guava:guava/32.1.3-jre?filename=guava-32.1.3-jre.jar' ``` -The name is the package name the UI shows, for example `group:artifact` for Maven. The response lists the deleted files. The next request for a deleted file fetches it from the upstream again. The stored bytes are removed after the same delay as a replaced artifact, so downloads already in progress can finish. +The name is the package name the UI shows, for example `group:artifact` for Maven. An RPM package's `filename` is its path under `upstream.rpm` (`updates/40/Everything/x86_64/Packages/c/curl-8.6.0-10.fc40.x86_64.rpm`), since repositories under one upstream can ship different files under one name. The response lists the deleted files. The next request for a deleted file fetches it from the upstream again. The stored bytes are removed after the same delay as a replaced artifact, so downloads already in progress can finish. Pushed archives are not touched: a whole-version delete skips them, and naming one answers 409. Delete those through the [hosted API](#hosted-packages). diff --git a/internal/handler/rpm.go b/internal/handler/rpm.go index 95163b96..49773a73 100644 --- a/internal/handler/rpm.go +++ b/internal/handler/rpm.go @@ -93,6 +93,10 @@ func (h *RPMHandler) Routes() http.Handler { // Path format varies by repo structure, e.g.: // - releases/39/Everything/x86_64/os/Packages/n/nginx-1.24.0-1.fc39.x86_64.rpm // - updates/39/Everything/x86_64/Packages/n/nginx-1.24.0-2.fc39.x86_64.rpm +// +// The cache key is the full path, since repositories under one upstream +// (Rocky and Alma on one mirror, say) can ship different bytes under one +// filename. func (h *RPMHandler) handlePackageDownload(w http.ResponseWriter, r *http.Request, path string) { // Parse the path to extract package info name, version, arch := h.parseRPMPath(path) @@ -102,14 +106,13 @@ func (h *RPMHandler) handlePackageDownload(w http.ResponseWriter, r *http.Reques return } - filename := path[strings.LastIndex(path, "/")+1:] downloadURL := fmt.Sprintf("%s/%s", h.upstreamURL, path) h.proxy.Logger.Info("rpm package download", - "name", name, "version", version, "arch", arch, "filename", filename) + "name", name, "version", version, "arch", arch, "path", path) result, err := h.proxy.GetOrFetchArtifactFromURL( - r.Context(), "rpm", name, version, filename, downloadURL) + r.Context(), "rpm", name, version, path, downloadURL) if err != nil { h.proxy.serveArtifactError(w, err, "failed to fetch package") return diff --git a/internal/handler/rpm_test.go b/internal/handler/rpm_test.go index 3526dbd5..87275b21 100644 --- a/internal/handler/rpm_test.go +++ b/internal/handler/rpm_test.go @@ -1,9 +1,14 @@ package handler import ( + "io" + "net/http" + "net/http/httptest" + "sync/atomic" "testing" "github.com/git-pkgs/purl" + "github.com/git-pkgs/registries/fetch" ) func TestRPMHandler_parseRPMPath(t *testing.T) { @@ -52,3 +57,48 @@ func TestRPMRetentionKey(t *testing.T) { t.Error("rpmRetentionKey(nil) accepted") } } + +// TestRPMHandler_PackageDownloadCachesPerRepository checks that two +// repositories under one upstream that ship the same filename with different +// bytes are cached apart, as on a mirror root serving several distributions. +func TestRPMHandler_PackageDownloadCachesPerRepository(t *testing.T) { + packages := map[string]string{ + "/rocky/9/BaseOS/x86_64/os/Packages/b/bash-5.1.8-9.el9.x86_64.rpm": "rocky bash", + "/almalinux/9/BaseOS/x86_64/os/Packages/b/bash-5.1.8-9.el9.x86_64.rpm": "alma bash", + } + + var available atomic.Bool + available.Store(true) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + data, ok := packages[r.URL.Path] + if !available.Load() || !ok { + http.NotFound(w, r) + return + } + _, _ = w.Write([]byte(data)) + })) + defer upstream.Close() + + proxy, _, _, _ := setupTestProxy(t) + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + srv := httptest.NewServer(NewRPMHandlerWithUpstream(proxy, testProxyURL, upstream.URL).Routes()) + defer srv.Close() + + for round := range 2 { + for path, want := range packages { + resp, err := http.Get(srv.URL + path) + if err != nil { + t.Fatal(err) + } + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK || string(body) != want { + t.Errorf("round %d %s: %d %q, want 200 %q", round, path, resp.StatusCode, body, want) + } + } + // The second round must come from the cache. + available.Store(false) + } +} diff --git a/internal/server/retention_rpm_test.go b/internal/server/retention_rpm_test.go index 8c0536b6..bc556145 100644 --- a/internal/server/retention_rpm_test.go +++ b/internal/server/retention_rpm_test.go @@ -20,8 +20,8 @@ import ( const ( rpmCurlPath = "/updates/40/Everything/x86_64/Packages/c/curl-8.6.0-10.fc40.x86_64.rpm" rpmNMPath = "/updates/40/Everything/x86_64/Packages/n/NetworkManager-1.46.2-1.fc40.x86_64.rpm" - rpmCurlFile = "curl-8.6.0-10.fc40.x86_64.rpm" - rpmNMFile = "NetworkManager-1.46.2-1.fc40.x86_64.rpm" + rpmCurlFile = "updates/40/Everything/x86_64/Packages/c/curl-8.6.0-10.fc40.x86_64.rpm" + rpmNMFile = "updates/40/Everything/x86_64/Packages/n/NetworkManager-1.46.2-1.fc40.x86_64.rpm" ) // rpmRetentionUpstream serves package files the way a Fedora mirror does, From 3860a6cae5a6831641bc02b36ecd5efce70418a4 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 08:00:50 +0100 Subject: [PATCH 02/14] Don't cache Maven paths with dotted group directories parsePath joins the group directories with dots, and the cache filename is the file's base name, so org.foo/bar/1.0/bar-1.0.jar and org/foo/bar/1.0/bar-1.0.jar shared one cache entry. Only upstreams that serve content at such directories, such as raw or hosted repositories, are affected. The Maven layout splits a group at its dots, so a group directory never contains a dot or is empty. Treat such paths as outside the layout and proxy them uncached, which keeps every existing cache entry valid. --- docs/configuration.md | 4 ++++ internal/handler/maven.go | 9 +++++++++ internal/handler/maven_repositories_test.go | 19 +++++++++++++++++++ internal/handler/maven_test.go | 5 +++++ 4 files changed, 37 insertions(+) diff --git a/docs/configuration.md b/docs/configuration.md index 9046e289..59a33b00 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1004,6 +1004,10 @@ one doesn't fall back to the Gradle Plugin Portal. Files are cached per repository, so the same coordinates in two repositories never share a cache entry. +In every repository, a path whose group directories contain a dot or are empty +(`org.foo/bar/1.0/bar-1.0.jar`) is not in the Maven layout and is proxied +without caching, since its group would read the same as `org/foo`'s. + `upstream.maven_repositories` is additive. `/maven/…` keeps serving `upstream.maven` with unchanged cache identities. A repository name is matched before the default repository, so it hides every group ID whose first segment diff --git a/internal/handler/maven.go b/internal/handler/maven.go index 6c6a6478..ca81b743 100644 --- a/internal/handler/maven.go +++ b/internal/handler/maven.go @@ -266,6 +266,7 @@ func (h *MavenHandler) handleDownload(w http.ResponseWriter, r *http.Request, so // parsePath extracts Maven coordinates from a URL path. // e.g., "com/google/guava/guava/32.1.3-jre/guava-32.1.3-jre.jar" // -> ("com.google.guava", "guava", "32.1.3-jre", "guava-32.1.3-jre.jar") +// A path that isn't in the Maven layout returns empty strings. func (h *MavenHandler) parsePath(urlPath string) (group, artifact, version, filename string) { parts := strings.Split(urlPath, "/") if len(parts) < minMavenParts { @@ -276,6 +277,14 @@ func (h *MavenHandler) parsePath(urlPath string) (group, artifact, version, file version = parts[len(parts)-2] artifact = parts[len(parts)-3] groupParts := parts[:len(parts)-3] + // The Maven layout splits a group at its dots, so a group directory + // never holds a dot or is empty. Such a path would spell the same group + // as another and share its cache entries, so it is not parsed. + for _, part := range groupParts { + if part == "" || strings.Contains(part, ".") { + return "", "", "", "" + } + } group = strings.Join(groupParts, ".") return group, artifact, version, filename diff --git a/internal/handler/maven_repositories_test.go b/internal/handler/maven_repositories_test.go index 8272f376..15db066b 100644 --- a/internal/handler/maven_repositories_test.go +++ b/internal/handler/maven_repositories_test.go @@ -137,3 +137,22 @@ func TestMavenRepositoryRepointedServesTheNewUpstream(t *testing.T) { } } } + +// TestMavenDottedGroupDirectoryNotCachedAsGroup checks that a path whose +// group directories contain dots is not cached under the group the dots +// spell, where it would share an entry with the standard layout's path. +func TestMavenDottedGroupDirectoryNotCachedAsGroup(t *testing.T) { + const standard = "/org/foo/bar/1.0/bar-1.0.jar" + const dotted = "/org.foo/bar/1.0/bar-1.0.jar" + central, _ := mavenRepoUpstream(t, "central", standard, dotted) + portal, _ := mavenRepoUpstream(t, "portal") + grails, _ := mavenRepoUpstream(t, "grails") + h := newMavenRepositoriesHandler(t, central, portal, grails) + + for _, path := range []string{standard, dotted, standard} { + w := getMaven(h, path) + if want := "central " + path; w.Code != http.StatusOK || w.Body.String() != want { + t.Errorf("GET %s = %d %q, want 200 %q", path, w.Code, w.Body.String(), want) + } + } +} diff --git a/internal/handler/maven_test.go b/internal/handler/maven_test.go index 9d34b09f..3d70a329 100644 --- a/internal/handler/maven_test.go +++ b/internal/handler/maven_test.go @@ -31,6 +31,11 @@ func TestMavenParsePath(t *testing.T) { "short/path", "", "", "", "", }, + // Group directories never hold dots or are empty in the Maven + // layout; these would spell the same group as org/foo. + {"org.foo/bar/1.0/bar-1.0.jar", "", "", "", ""}, + {"org/.foo/bar/1.0/bar-1.0.jar", "", "", "", ""}, + {"org//foo/bar/1.0/bar-1.0.jar", "", "", "", ""}, } for _, tt := range tests { From 81c6ea26c2350bab1c6c610e419e866e76733022 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 08:00:50 +0100 Subject: [PATCH 03/14] Treat git keys differing only in case as one repository On a case-insensitive file system, as macOS and Windows use by default, acme/WIDGETS and acme/widgets name the same cache directory but were tracked as two repositories. Users of one spelling didn't keep the other from being evicted, so eviction could move the directory away while it was being read, and each entry had its own write lock, so two refreshes could fetch into the directory at once. Check at startup whether the cache directory ignores case and, if so, track repositories under their lowercased key. The directory and the upstream URL keep the spelling requested. Case-sensitive file systems, and so existing caches on Linux, are unchanged. --- docs/configuration.md | 6 ++++++ internal/gitcache/cache.go | 35 +++++++++++++++++++++++++++++---- internal/gitcache/cache_test.go | 27 +++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 59a33b00..30c9a0c0 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -946,6 +946,12 @@ those yourself. The proxy locks `cache_dir` while it runs, through a `.gitcache-lock` file in it, so a second process given the same directory, such as another replica on a shared volume, fails to start; give each its own. +Each repository's directory is named by its path as requested. Where +`cache_dir` ignores case in file names, as on macOS and Windows by default, +paths that differ only in case (`acme/widgets` and `acme/WIDGETS`) share one +directory and are treated as one repository. Use a case-sensitive file system +for an upstream whose repositories differ only in case. + Git LFS objects are cached in the artifact cache, so they use the configured storage backend and retention rather than `cache_dir`. git-lfs finds the LFS API under the rewritten remote, at `/git/{name}/{repo}.git/info/lfs`, with no diff --git a/internal/gitcache/cache.go b/internal/gitcache/cache.go index eb1578f7..6bc3c200 100644 --- a/internal/gitcache/cache.go +++ b/internal/gitcache/cache.go @@ -95,6 +95,9 @@ type Cache struct { refCheckTimeout time.Duration lowSpeedTime int maxRequestBody int64 + // foldCase is set where the cache directory ignores case, so keys that + // differ only in case, which name one directory, are one repository. + foldCase bool ctx context.Context cancel context.CancelFunc @@ -214,6 +217,7 @@ func New(opts Options) (*Cache, error) { tooLarge: make(map[string]time.Time), failedWants: make(map[string]time.Time), freed: make(chan struct{}), + foldCase: caseInsensitive(dir), } if opts.refCheckTimeout > 0 { c.refCheckTimeout = opts.refCheckTimeout @@ -284,7 +288,7 @@ func (c *Cache) load() error { return err } rel, _ := filepath.Rel(c.dir, path) - rp := newRepo(filepath.ToSlash(rel), path) + rp := newRepo(c.repoKey(filepath.ToSlash(rel)), path) rp.size.Store(dirSize(path)) rp.lastChecked.Store(stampTime(path, checkedStamp)) rp.lastUsed.Store(stampTime(path, usedStamp)) @@ -352,15 +356,24 @@ func (c *Cache) removeRepointed() { func (c *Cache) acquire(key string) *repo { c.mu.Lock() defer c.mu.Unlock() - rp := c.repos[key] + rp := c.repos[c.repoKey(key)] if rp == nil { - rp = newRepo(key, filepath.Join(c.dir, filepath.FromSlash(key))) - c.repos[key] = rp + rp = newRepo(c.repoKey(key), filepath.Join(c.dir, filepath.FromSlash(key))) + c.repos[rp.key] = rp } rp.users++ return rp } +// repoKey is the key a repository is tracked under: key itself, or key +// lowercased where the cache directory ignores case. +func (c *Cache) repoKey(key string) string { + if c.foldCase { + return strings.ToLower(key) + } + return key +} + func (c *Cache) release(rp *repo) { now := time.Now() rp.lastUsed.Store(now.UnixNano()) @@ -511,3 +524,17 @@ func touch(dir, name string, t time.Time) { } } } + +// caseInsensitive reports whether the file system holding dir ignores case +// in names, as macOS and Windows do by default. +func caseInsensitive(dir string) bool { + f, err := os.CreateTemp(dir, ".gitcache-case-") + if err != nil { + return false + } + name := f.Name() + _ = f.Close() + defer func() { _ = os.Remove(name) }() + _, err = os.Stat(filepath.Join(dir, strings.ToUpper(filepath.Base(name)))) + return err == nil +} diff --git a/internal/gitcache/cache_test.go b/internal/gitcache/cache_test.go index 4d43c5d5..690b11ed 100644 --- a/internal/gitcache/cache_test.go +++ b/internal/gitcache/cache_test.go @@ -786,3 +786,30 @@ func TestCloseTwice(t *testing.T) { t.Errorf("closing twice: %v", err) } } + +// TestCaseOnlyDifferentKeysShareOneRepository checks that, where the file +// system ignores case, keys that differ only in case are one repository, so +// a user of one spelling keeps the directory from being evicted under it. +func TestCaseOnlyDifferentKeysShareOneRepository(t *testing.T) { + if !caseInsensitive(t.TempDir()) { + t.Skip("file system is case-sensitive") + } + upstream := newFakeUpstream(t) + upstream.newRepo("repo") + tc := newTestCache(t, upstream.server.URL, nil) + tc.clone(t, "repo") + waitIdle(t, tc.Cache) + + rp := tc.acquire("up/REPO.git") + defer tc.release(rp) + if other := tc.repo("up/repo.git"); other != nil && other != rp { + t.Error("up/REPO.git and up/repo.git are separate repositories") + } + tc.mu.Lock() + tc.opts.MaxSize = 1 + tc.mu.Unlock() + tc.evict() + if !rp.exists() { + t.Error("repository in use as up/REPO.git was evicted") + } +} From fc0e94b080fe4b6f2bfec42800f7b0eb9995f159 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 08:56:20 +0100 Subject: [PATCH 04/14] Remove empty directories below a fetch directory on delete APK and Arch packages, and now RPM ones, have filenames with directories in them, which put the object below its fetch directory. Delete only removed the object's own directory when it was the fetch directory, so those directories stayed behind. It now removes them up to and including the fetch directory while they are empty. --- internal/storage/blob.go | 23 +++++++++++++++++++--- internal/storage/blob_test.go | 36 +++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 81af78f2..a999ff58 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -291,13 +291,30 @@ func (b *Blob) Delete(ctx context.Context, path string) error { return fmt.Errorf("deleting object: %w", err) } if p := b.localPath(path); p != "" { - if dir := filepath.Dir(p); dir != b.fileRoot && isFetchDir(filepath.Base(dir)) { - _ = os.Remove(dir) // fails, harmlessly, while the directory holds anything - } + b.removeEmptyFetchDirs(filepath.Dir(p)) } return nil } +// removeEmptyFetchDirs removes dir and its parents while they are empty, up +// to and including the fetch directory they are in, for an object whose +// filename has directories of its own. Outside a fetch directory, as in the +// layout before them, nothing is removed. +func (b *Blob) removeEmptyFetchDirs(dir string) { + var dirs []string + for d := dir; d != b.fileRoot && d != filepath.Dir(d); d = filepath.Dir(d) { + dirs = append(dirs, d) + if isFetchDir(filepath.Base(d)) { + for _, d := range dirs { + if os.Remove(d) != nil { + return // fails, harmlessly, while the directory holds anything + } + } + return + } + } +} + func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) (string, error) { url, err := b.bucket.SignedURL(ctx, path, &blob.SignedURLOptions{ Method: http.MethodGet, diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index 1a764de7..faaace8d 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -180,6 +180,42 @@ func TestBlobDeleteRemovesEmptyFetchDirectory(t *testing.T) { assertReadsBack(t, b, kept, "content") } +// A filename with directories in it, as APK, Arch and RPM packages have, puts +// the object below its fetch directory. Delete removes those directories +// too once empty, up to and including the fetch directory. +func TestBlobDeleteRemovesNestedFetchDirectories(t *testing.T) { + dir := t.TempDir() + b := openFileBlob(t, dir) + ctx := context.Background() + const ( + emptied = "rpm/bash/5.1/0123456789abcdef/rocky/9/BaseOS/bash.rpm" + shared = "rpm/bash/5.1/fedcba9876543210/alma/9/BaseOS/bash.rpm" + kept = "rpm/bash/5.1/fedcba9876543210/alma/9/AppStream/bash.rpm" + ) + for _, key := range []string{emptied, shared, kept} { + if _, _, err := b.Store(ctx, key, strings.NewReader("content")); err != nil { + t.Fatalf("Store(%q): %v", key, err) + } + } + for _, key := range []string{emptied, shared} { + if err := b.Delete(ctx, key); err != nil { + t.Fatalf("Delete(%q): %v", key, err) + } + } + + version := filepath.Join(dir, "rpm", "bash", "5.1") + if _, err := os.Stat(filepath.Join(version, "0123456789abcdef")); !os.IsNotExist(err) { + t.Errorf("emptied fetch directory left behind (stat err %v)", err) + } + if _, err := os.Stat(filepath.Join(version, "fedcba9876543210", "alma", "9", "BaseOS")); !os.IsNotExist(err) { + t.Errorf("emptied directory below the fetch directory left behind (stat err %v)", err) + } + if _, err := os.Stat(version); err != nil { + t.Errorf("version directory removed: %v", err) + } + assertReadsBack(t, b, kept, "content") +} + func TestBlobDeleteNotFound(t *testing.T) { b := createTestBlob(t) ctx := context.Background() From 3b4ddb43d8a0c162d74673495e37041f70e49849 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 10:04:58 +0100 Subject: [PATCH 05/14] Revert "Remove empty directories below a fetch directory on delete" This reverts commit fc0e94b. The cleanup stopped at the first segment that looked like a fetch id, but named Maven repositories put a hex upstream hash in the filename, so it stopped early. For objects in the old layout, with no fetch directory, the walk could remove version or package directories under a concurrent fetch. Leftover empty directories will be handled separately. --- internal/storage/blob.go | 23 +++------------------- internal/storage/blob_test.go | 36 ----------------------------------- 2 files changed, 3 insertions(+), 56 deletions(-) diff --git a/internal/storage/blob.go b/internal/storage/blob.go index a999ff58..81af78f2 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -291,28 +291,11 @@ func (b *Blob) Delete(ctx context.Context, path string) error { return fmt.Errorf("deleting object: %w", err) } if p := b.localPath(path); p != "" { - b.removeEmptyFetchDirs(filepath.Dir(p)) - } - return nil -} - -// removeEmptyFetchDirs removes dir and its parents while they are empty, up -// to and including the fetch directory they are in, for an object whose -// filename has directories of its own. Outside a fetch directory, as in the -// layout before them, nothing is removed. -func (b *Blob) removeEmptyFetchDirs(dir string) { - var dirs []string - for d := dir; d != b.fileRoot && d != filepath.Dir(d); d = filepath.Dir(d) { - dirs = append(dirs, d) - if isFetchDir(filepath.Base(d)) { - for _, d := range dirs { - if os.Remove(d) != nil { - return // fails, harmlessly, while the directory holds anything - } - } - return + if dir := filepath.Dir(p); dir != b.fileRoot && isFetchDir(filepath.Base(dir)) { + _ = os.Remove(dir) // fails, harmlessly, while the directory holds anything } } + return nil } func (b *Blob) SignedURL(ctx context.Context, path string, expiry time.Duration) (string, error) { diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index faaace8d..1a764de7 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -180,42 +180,6 @@ func TestBlobDeleteRemovesEmptyFetchDirectory(t *testing.T) { assertReadsBack(t, b, kept, "content") } -// A filename with directories in it, as APK, Arch and RPM packages have, puts -// the object below its fetch directory. Delete removes those directories -// too once empty, up to and including the fetch directory. -func TestBlobDeleteRemovesNestedFetchDirectories(t *testing.T) { - dir := t.TempDir() - b := openFileBlob(t, dir) - ctx := context.Background() - const ( - emptied = "rpm/bash/5.1/0123456789abcdef/rocky/9/BaseOS/bash.rpm" - shared = "rpm/bash/5.1/fedcba9876543210/alma/9/BaseOS/bash.rpm" - kept = "rpm/bash/5.1/fedcba9876543210/alma/9/AppStream/bash.rpm" - ) - for _, key := range []string{emptied, shared, kept} { - if _, _, err := b.Store(ctx, key, strings.NewReader("content")); err != nil { - t.Fatalf("Store(%q): %v", key, err) - } - } - for _, key := range []string{emptied, shared} { - if err := b.Delete(ctx, key); err != nil { - t.Fatalf("Delete(%q): %v", key, err) - } - } - - version := filepath.Join(dir, "rpm", "bash", "5.1") - if _, err := os.Stat(filepath.Join(version, "0123456789abcdef")); !os.IsNotExist(err) { - t.Errorf("emptied fetch directory left behind (stat err %v)", err) - } - if _, err := os.Stat(filepath.Join(version, "fedcba9876543210", "alma", "9", "BaseOS")); !os.IsNotExist(err) { - t.Errorf("emptied directory below the fetch directory left behind (stat err %v)", err) - } - if _, err := os.Stat(version); err != nil { - t.Errorf("version directory removed: %v", err) - } - assertReadsBack(t, b, kept, "content") -} - func TestBlobDeleteNotFound(t *testing.T) { b := createTestBlob(t) ctx := context.Background() From de280a1694f04b7527da5da1f35a9738a7070366 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 10:05:23 +0100 Subject: [PATCH 06/14] Don't cache Maven paths with a colon in the group or artifact The cache keys Maven artifacts as group:artifact, so org:foo/bar and org/foo:bar both came out as org:foo:bar and shared one entry. Such paths are now proxied without caching, like dotted group directories. --- internal/handler/maven.go | 8 ++++++-- internal/handler/maven_test.go | 4 ++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/handler/maven.go b/internal/handler/maven.go index ca81b743..0fd81ec3 100644 --- a/internal/handler/maven.go +++ b/internal/handler/maven.go @@ -279,12 +279,16 @@ func (h *MavenHandler) parsePath(urlPath string) (group, artifact, version, file groupParts := parts[:len(parts)-3] // The Maven layout splits a group at its dots, so a group directory // never holds a dot or is empty. Such a path would spell the same group - // as another and share its cache entries, so it is not parsed. + // as another and share its cache entries, so it is not parsed. Nor is a + // colon in the group or artifact, which are cached as group:artifact. for _, part := range groupParts { - if part == "" || strings.Contains(part, ".") { + if part == "" || strings.ContainsAny(part, ".:") { return "", "", "", "" } } + if strings.Contains(artifact, ":") { + return "", "", "", "" + } group = strings.Join(groupParts, ".") return group, artifact, version, filename diff --git a/internal/handler/maven_test.go b/internal/handler/maven_test.go index 3d70a329..14cb49f5 100644 --- a/internal/handler/maven_test.go +++ b/internal/handler/maven_test.go @@ -36,6 +36,10 @@ func TestMavenParsePath(t *testing.T) { {"org.foo/bar/1.0/bar-1.0.jar", "", "", "", ""}, {"org/.foo/bar/1.0/bar-1.0.jar", "", "", "", ""}, {"org//foo/bar/1.0/bar-1.0.jar", "", "", "", ""}, + // Cache identity is group:artifact, so a colon in either would + // let org:foo/bar and org/foo:bar share one entry. + {"org:foo/bar/1.0/bar-1.0.jar", "", "", "", ""}, + {"org/foo:bar/1.0/bar-1.0.jar", "", "", "", ""}, } for _, tt := range tests { From 8233ce33399f042bf24d4a87ef1d95c5ce69b3f9 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 10:05:58 +0100 Subject: [PATCH 07/14] Clean RPM package paths before caching The cache key for an RPM package is its request path, so a path with an empty or "." segment made a second entry for the same upstream file. The path is now cleaned before it is used as the key and the upstream URL. --- internal/handler/rpm.go | 9 +++++++- internal/handler/rpm_test.go | 42 ++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/internal/handler/rpm.go b/internal/handler/rpm.go index 49773a73..62b2798c 100644 --- a/internal/handler/rpm.go +++ b/internal/handler/rpm.go @@ -3,6 +3,7 @@ package handler import ( "fmt" "net/http" + pathpkg "path" "regexp" "strings" @@ -78,7 +79,7 @@ func (h *RPMHandler) Routes() http.Handler { switch { case strings.HasSuffix(path, ".rpm"): // Package downloads - cache these - h.handlePackageDownload(w, r, path) + h.handlePackageDownload(w, r, cleanRPMPath(path)) case strings.Contains(path, "/repodata/"): // Repository metadata h.handleMetadata(w, r, path) @@ -89,6 +90,12 @@ func (h *RPMHandler) Routes() http.Handler { })) } +// cleanRPMPath drops empty and "." segments from a relative path, so each +// package has one cache key. ".." segments are rejected before this. +func cleanRPMPath(p string) string { + return strings.TrimPrefix(pathpkg.Clean("/"+p), "/") +} + // handlePackageDownload fetches and caches .rpm packages. // Path format varies by repo structure, e.g.: // - releases/39/Everything/x86_64/os/Packages/n/nginx-1.24.0-1.fc39.x86_64.rpm diff --git a/internal/handler/rpm_test.go b/internal/handler/rpm_test.go index 87275b21..d9561e31 100644 --- a/internal/handler/rpm_test.go +++ b/internal/handler/rpm_test.go @@ -102,3 +102,45 @@ func TestRPMHandler_PackageDownloadCachesPerRepository(t *testing.T) { available.Store(false) } } + +// TestRPMHandler_PackageDownloadCleansPath checks that spellings of one +// path with empty or "." segments share one cache entry. +func TestRPMHandler_PackageDownloadCleansPath(t *testing.T) { + const want = "bash" + var hits atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/rocky/9/BaseOS/x86_64/os/Packages/b/bash-5.1.8-9.el9.x86_64.rpm" { + http.NotFound(w, r) + return + } + hits.Add(1) + _, _ = w.Write([]byte(want)) + })) + defer upstream.Close() + + proxy, _, _, _ := setupTestProxy(t) + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + srv := httptest.NewServer(NewRPMHandlerWithUpstream(proxy, testProxyURL, upstream.URL).Routes()) + defer srv.Close() + + for _, path := range []string{ + "/rocky/9/BaseOS/x86_64/os/Packages/b/bash-5.1.8-9.el9.x86_64.rpm", + "/rocky//9/BaseOS/x86_64/os/Packages/b/bash-5.1.8-9.el9.x86_64.rpm", + "/rocky/./9/BaseOS/x86_64/os/Packages/b/bash-5.1.8-9.el9.x86_64.rpm", + } { + resp, err := http.Get(srv.URL + path) + if err != nil { + t.Fatal(err) + } + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK || string(body) != want { + t.Errorf("%s: %d %q, want 200 %q", path, resp.StatusCode, body, want) + } + } + if n := hits.Load(); n != 1 { + t.Errorf("upstream fetched %d times, want 1", n) + } +} From 6381daa4edaf8dc5968be076a76ac0a84a441ae8 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 10:07:53 +0100 Subject: [PATCH 08/14] Test git key case folding on every file system The existing test only runs where the file system ignores case, so it was skipped on Linux, and it passed if the second spelling had no entry at all. Add a test that sets foldCase directly and checks both spellings share one entry, require the entry in the existing test, and work out the repository key once in acquire. --- internal/gitcache/cache.go | 7 ++++--- internal/gitcache/cache_test.go | 23 ++++++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/internal/gitcache/cache.go b/internal/gitcache/cache.go index 6bc3c200..66800571 100644 --- a/internal/gitcache/cache.go +++ b/internal/gitcache/cache.go @@ -356,10 +356,11 @@ func (c *Cache) removeRepointed() { func (c *Cache) acquire(key string) *repo { c.mu.Lock() defer c.mu.Unlock() - rp := c.repos[c.repoKey(key)] + rk := c.repoKey(key) + rp := c.repos[rk] if rp == nil { - rp = newRepo(c.repoKey(key), filepath.Join(c.dir, filepath.FromSlash(key))) - c.repos[rp.key] = rp + rp = newRepo(rk, filepath.Join(c.dir, filepath.FromSlash(key))) + c.repos[rk] = rp } rp.users++ return rp diff --git a/internal/gitcache/cache_test.go b/internal/gitcache/cache_test.go index 690b11ed..6bef422d 100644 --- a/internal/gitcache/cache_test.go +++ b/internal/gitcache/cache_test.go @@ -802,7 +802,7 @@ func TestCaseOnlyDifferentKeysShareOneRepository(t *testing.T) { rp := tc.acquire("up/REPO.git") defer tc.release(rp) - if other := tc.repo("up/repo.git"); other != nil && other != rp { + if other := tc.repo("up/repo.git"); other != rp { t.Error("up/REPO.git and up/repo.git are separate repositories") } tc.mu.Lock() @@ -813,3 +813,24 @@ func TestCaseOnlyDifferentKeysShareOneRepository(t *testing.T) { t.Error("repository in use as up/REPO.git was evicted") } } + +// TestFoldCaseKeysShareOneEntry checks key folding on any file system by +// setting foldCase directly. +func TestFoldCaseKeysShareOneEntry(t *testing.T) { + tc := newTestCache(t, newFakeUpstream(t).server.URL, nil) + tc.foldCase = true + + upper := tc.acquire("up/REPO.git") + defer tc.release(upper) + lower := tc.acquire("up/repo.git") + defer tc.release(lower) + if upper != lower { + t.Error("up/REPO.git and up/repo.git are separate repositories") + } + tc.mu.Lock() + n := len(tc.repos) + tc.mu.Unlock() + if n != 1 { + t.Errorf("%d repository entries, want 1", n) + } +} From 0ff118b6420ee207c9bc8f8ce18090332635c649 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 10:57:38 +0100 Subject: [PATCH 09/14] Try the Plugin Portal for Maven paths proxied uncached Paths the proxy doesn't cache, such as ones with a dotted group directory, went to the primary upstream only, so a file available only on the Gradle Plugin Portal returned 404. They now try the Plugin Portal on a 404, as cached files do. --- internal/handler/handler.go | 9 +++++++++ internal/handler/maven.go | 22 ++++++++++++++++++++- internal/handler/maven_repositories_test.go | 15 ++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/internal/handler/handler.go b/internal/handler/handler.go index d316aabc..dc54269a 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1481,6 +1481,15 @@ func copyArtifactBody(w http.ResponseWriter, reader io.Reader, expectedSize int6 // It copies the request, forwards specified headers, and streams the response back. // forwardHeaders controls the request headers sent upstream. End-to-end response // headers and trailers are relayed independently of that list. +// upstreamRequest sends r's method to upstreamURL with no request headers. +func (p *Proxy) upstreamRequest(r *http.Request, upstreamURL string) (*http.Response, error) { + req, err := http.NewRequestWithContext(r.Context(), r.Method, upstreamURL, nil) + if err != nil { + return nil, err + } + return p.HTTPClient.Do(req) +} + func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamURL string, forwardHeaders []string) { p.Logger.Debug("proxying to upstream", "url", upstreamURL) diff --git a/internal/handler/maven.go b/internal/handler/maven.go index 0fd81ec3..ed417d78 100644 --- a/internal/handler/maven.go +++ b/internal/handler/maven.go @@ -343,5 +343,25 @@ func (h *MavenHandler) isChecksumFile(filename string) bool { // proxyUpstream forwards a request to the source's upstream without caching. func (h *MavenHandler) proxyUpstream(w http.ResponseWriter, r *http.Request, source mavenSource, urlPath string) { - h.proxy.ProxyUpstream(w, r, source.upstreamURL+"/"+urlPath, nil) + if source.fallbackURL == "" { + h.proxy.ProxyUpstream(w, r, source.upstreamURL+"/"+urlPath, nil) + return + } + // As for cached files, a file the primary upstream lacks is looked for + // on the Gradle Plugin Portal. + resp, err := h.proxy.upstreamRequest(r, source.upstreamURL+"/"+urlPath) + if err == nil && resp.StatusCode == http.StatusNotFound { + _ = resp.Body.Close() + resp, err = h.proxy.upstreamRequest(r, source.fallbackURL+"/"+urlPath) + } + if err != nil { + if writeUpstreamHeld(w, err) { + return + } + h.proxy.Logger.Error("upstream request failed", "error", err) + http.Error(w, "upstream request failed", http.StatusBadGateway) + return + } + defer func() { _ = resp.Body.Close() }() + h.proxy.relayResponse(w, r, resp, nil) } diff --git a/internal/handler/maven_repositories_test.go b/internal/handler/maven_repositories_test.go index 15db066b..8fb6f27d 100644 --- a/internal/handler/maven_repositories_test.go +++ b/internal/handler/maven_repositories_test.go @@ -156,3 +156,18 @@ func TestMavenDottedGroupDirectoryNotCachedAsGroup(t *testing.T) { } } } + +// TestMavenUncachedPathFallsBackToPluginPortal checks that a path proxied +// without caching, such as one with a dotted group directory, still tries +// the Gradle Plugin Portal when the primary upstream lacks it. +func TestMavenUncachedPathFallsBackToPluginPortal(t *testing.T) { + const dotted = "/org.foo/bar/1.0/bar-1.0.jar" + central, _ := mavenRepoUpstream(t, "central") + portal, _ := mavenRepoUpstream(t, "portal", dotted) + grails, _ := mavenRepoUpstream(t, "grails") + h := newMavenRepositoriesHandler(t, central, portal, grails) + + if w := getMaven(h, dotted); w.Code != http.StatusOK || w.Body.String() != "portal "+dotted { + t.Errorf("GET %s = %d %q, want the Plugin Portal's copy", dotted, w.Code, w.Body.String()) + } +} From da090443eceb2f99e50f85b33a9a660b1d7eb662 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 12:19:19 +0100 Subject: [PATCH 10/14] Forward Range to upstream for Maven paths proxied uncached Paths proxied without caching sent no request headers, so a resumed download got the whole file back instead of the requested bytes. --- internal/handler/handler.go | 33 ++++++++------------- internal/handler/maven.go | 9 ++++-- internal/handler/maven_repositories_test.go | 25 ++++++++++++++++ 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/internal/handler/handler.go b/internal/handler/handler.go index dc54269a..6c3fef8f 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1477,36 +1477,29 @@ func copyArtifactBody(w http.ResponseWriter, reader io.Reader, expectedSize int6 } } -// ProxyUpstream forwards a request to an upstream URL without caching. -// It copies the request, forwards specified headers, and streams the response back. -// forwardHeaders controls the request headers sent upstream. End-to-end response -// headers and trailers are relayed independently of that list. -// upstreamRequest sends r's method to upstreamURL with no request headers. -func (p *Proxy) upstreamRequest(r *http.Request, upstreamURL string) (*http.Response, error) { +// upstreamRequest sends r's method to upstreamURL, copying only the +// forwardHeaders that r sets. +func (p *Proxy) upstreamRequest(r *http.Request, upstreamURL string, forwardHeaders []string) (*http.Response, error) { req, err := http.NewRequestWithContext(r.Context(), r.Method, upstreamURL, nil) if err != nil { return nil, err } - return p.HTTPClient.Do(req) -} - -func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamURL string, forwardHeaders []string) { - p.Logger.Debug("proxying to upstream", "url", upstreamURL) - - req, err := http.NewRequestWithContext(r.Context(), r.Method, upstreamURL, nil) - if err != nil { - http.Error(w, "failed to create request", http.StatusInternalServerError) - return - } - - // Copy request headers that affect content negotiation / caching for _, header := range forwardHeaders { if v := r.Header.Get(header); v != "" { req.Header.Set(header, v) } } + return p.HTTPClient.Do(req) +} - resp, err := p.HTTPClient.Do(req) +// ProxyUpstream forwards a request to an upstream URL without caching. +// It copies the request, forwards specified headers, and streams the response back. +// forwardHeaders controls the request headers sent upstream. End-to-end response +// headers and trailers are relayed independently of that list. +func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamURL string, forwardHeaders []string) { + p.Logger.Debug("proxying to upstream", "url", upstreamURL) + + resp, err := p.upstreamRequest(r, upstreamURL, forwardHeaders) if err != nil { if writeUpstreamHeld(w, err) { return diff --git a/internal/handler/maven.go b/internal/handler/maven.go index ed417d78..4b170be8 100644 --- a/internal/handler/maven.go +++ b/internal/handler/maven.go @@ -341,18 +341,21 @@ func (h *MavenHandler) isChecksumFile(filename string) bool { return false } +// mavenForwardHeaders lets clients resume artifacts proxied without caching. +var mavenForwardHeaders = []string{"Range", "If-Range"} + // proxyUpstream forwards a request to the source's upstream without caching. func (h *MavenHandler) proxyUpstream(w http.ResponseWriter, r *http.Request, source mavenSource, urlPath string) { if source.fallbackURL == "" { - h.proxy.ProxyUpstream(w, r, source.upstreamURL+"/"+urlPath, nil) + h.proxy.ProxyUpstream(w, r, source.upstreamURL+"/"+urlPath, mavenForwardHeaders) return } // As for cached files, a file the primary upstream lacks is looked for // on the Gradle Plugin Portal. - resp, err := h.proxy.upstreamRequest(r, source.upstreamURL+"/"+urlPath) + resp, err := h.proxy.upstreamRequest(r, source.upstreamURL+"/"+urlPath, mavenForwardHeaders) if err == nil && resp.StatusCode == http.StatusNotFound { _ = resp.Body.Close() - resp, err = h.proxy.upstreamRequest(r, source.fallbackURL+"/"+urlPath) + resp, err = h.proxy.upstreamRequest(r, source.fallbackURL+"/"+urlPath, mavenForwardHeaders) } if err != nil { if writeUpstreamHeld(w, err) { diff --git a/internal/handler/maven_repositories_test.go b/internal/handler/maven_repositories_test.go index 8fb6f27d..260d3a8f 100644 --- a/internal/handler/maven_repositories_test.go +++ b/internal/handler/maven_repositories_test.go @@ -7,6 +7,7 @@ import ( "strings" "sync/atomic" "testing" + "time" "github.com/git-pkgs/registries/fetch" ) @@ -171,3 +172,27 @@ func TestMavenUncachedPathFallsBackToPluginPortal(t *testing.T) { t.Errorf("GET %s = %d %q, want the Plugin Portal's copy", dotted, w.Code, w.Body.String()) } } + +func TestMavenUncachedPathKeepsByteRanges(t *testing.T) { + const dotted = "/org.foo/bar/1.0/bar-1.0.jar" + ranged := func() *httptest.Server { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.ServeContent(w, r, "bar.jar", time.Time{}, strings.NewReader("0123456789")) + })) + t.Cleanup(srv.Close) + return srv + } + central, grails := ranged(), ranged() + portal, _ := mavenRepoUpstream(t, "portal") + h := newMavenRepositoriesHandler(t, central, portal, grails) + + for _, p := range []string{dotted, "/grails" + dotted} { + req := httptest.NewRequest(http.MethodGet, p, nil) + req.Header.Set("Range", "bytes=2-4") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusPartialContent || w.Body.String() != "234" { + t.Errorf("GET %s with Range = %d %q, want 206 %q", p, w.Code, w.Body.String(), "234") + } + } +} From b61e35205c47238421c0c0b1b0c5a649d621e750 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 21:49:48 +0100 Subject: [PATCH 11/14] Answer 404 for Maven files outside the layout A file whose group directories hold a dot or colon was proxied without caching, which skipped the denylist, scan and cooldown. Such paths aren't in the Maven layout, so they now get 404. Directory listings and catalogs still go through proxyUpstream, which shares its error handling with Proxy.ProxyUpstream in relayUpstream. --- internal/handler/handler.go | 6 +++ internal/handler/maven.go | 18 ++++---- internal/handler/maven_repositories_test.go | 51 +++++++++++++-------- 3 files changed, 46 insertions(+), 29 deletions(-) diff --git a/internal/handler/handler.go b/internal/handler/handler.go index b3ef666b..9612e510 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1500,6 +1500,12 @@ func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamUR p.Logger.Debug("proxying to upstream", "url", upstreamURL) resp, err := p.upstreamRequest(r, upstreamURL, forwardHeaders) + p.relayUpstream(w, r, resp, err) +} + +// relayUpstream writes the result of an upstream request: the response, or +// a 502 when the request failed. +func (p *Proxy) relayUpstream(w http.ResponseWriter, r *http.Request, resp *http.Response, err error) { if err != nil { if writeUpstreamHeld(w, err) { return diff --git a/internal/handler/maven.go b/internal/handler/maven.go index 4b170be8..5008e00f 100644 --- a/internal/handler/maven.go +++ b/internal/handler/maven.go @@ -221,6 +221,13 @@ func (h *MavenHandler) handleDownload(w http.ResponseWriter, r *http.Request, so // e.g., com/google/guava/guava/32.1.3-jre/guava-32.1.3-jre.jar group, artifact, version, filename := h.parsePath(urlPath) if artifact == "" { + if strings.Count(urlPath, "/") >= minMavenParts-1 { + // A dot or colon in a group directory, or a colon in the + // artifact, which the Maven layout never has. Proxied, the + // file would skip the denylist, scan and cooldown. + http.NotFound(w, r) + return + } h.proxyUpstream(w, r, source, urlPath) return } @@ -357,14 +364,5 @@ func (h *MavenHandler) proxyUpstream(w http.ResponseWriter, r *http.Request, sou _ = resp.Body.Close() resp, err = h.proxy.upstreamRequest(r, source.fallbackURL+"/"+urlPath, mavenForwardHeaders) } - if err != nil { - if writeUpstreamHeld(w, err) { - return - } - h.proxy.Logger.Error("upstream request failed", "error", err) - http.Error(w, "upstream request failed", http.StatusBadGateway) - return - } - defer func() { _ = resp.Body.Close() }() - h.proxy.relayResponse(w, r, resp, nil) + h.proxy.relayUpstream(w, r, resp, err) } diff --git a/internal/handler/maven_repositories_test.go b/internal/handler/maven_repositories_test.go index 260d3a8f..923af928 100644 --- a/internal/handler/maven_repositories_test.go +++ b/internal/handler/maven_repositories_test.go @@ -139,45 +139,58 @@ func TestMavenRepositoryRepointedServesTheNewUpstream(t *testing.T) { } } -// TestMavenDottedGroupDirectoryNotCachedAsGroup checks that a path whose -// group directories contain dots is not cached under the group the dots -// spell, where it would share an entry with the standard layout's path. +// TestMavenDottedGroupDirectoryNotCachedAsGroup checks that a file whose +// group directories contain dots or colons, which the Maven layout never +// has, is not served: it would either share the cache entry of the group +// the dots spell or be proxied past the denylist, scan and cooldown. func TestMavenDottedGroupDirectoryNotCachedAsGroup(t *testing.T) { const standard = "/org/foo/bar/1.0/bar-1.0.jar" - const dotted = "/org.foo/bar/1.0/bar-1.0.jar" - central, _ := mavenRepoUpstream(t, "central", standard, dotted) + odd := []string{"/org.foo/bar/1.0/bar-1.0.jar", "/org/foo:x/bar/1.0/bar-1.0.jar", "/org/foo/b:ar/1.0/b:ar-1.0.jar"} + central, requests := mavenRepoUpstream(t, "central", append([]string{standard}, odd...)...) portal, _ := mavenRepoUpstream(t, "portal") - grails, _ := mavenRepoUpstream(t, "grails") + var restricted []string + for _, p := range odd { + restricted = append(restricted, "/restricted"+p) + } + grails, grailsRequests := mavenRepoUpstream(t, "grails", restricted...) h := newMavenRepositoriesHandler(t, central, portal, grails) - for _, path := range []string{standard, dotted, standard} { - w := getMaven(h, path) - if want := "central " + path; w.Code != http.StatusOK || w.Body.String() != want { - t.Errorf("GET %s = %d %q, want 200 %q", path, w.Code, w.Body.String(), want) + if w := getMaven(h, standard); w.Code != http.StatusOK || w.Body.String() != "central "+standard { + t.Errorf("GET %s = %d %q", standard, w.Code, w.Body.String()) + } + before := requests.Load() + grailsRequests.Load() + for _, p := range odd { + for _, path := range []string{p, "/grails" + p} { + if w := getMaven(h, path); w.Code != http.StatusNotFound { + t.Errorf("GET %s = %d %q, want 404", path, w.Code, w.Body.String()) + } } } + if n := requests.Load() + grailsRequests.Load() - before; n != 0 { + t.Errorf("%d upstream requests for paths outside the Maven layout", n) + } } // TestMavenUncachedPathFallsBackToPluginPortal checks that a path proxied -// without caching, such as one with a dotted group directory, still tries -// the Gradle Plugin Portal when the primary upstream lacks it. +// without caching, such as a catalog, still tries the Gradle Plugin Portal +// when the primary upstream lacks it. func TestMavenUncachedPathFallsBackToPluginPortal(t *testing.T) { - const dotted = "/org.foo/bar/1.0/bar-1.0.jar" + const uncached = "/org/foo/archetype-catalog.xml" central, _ := mavenRepoUpstream(t, "central") - portal, _ := mavenRepoUpstream(t, "portal", dotted) + portal, _ := mavenRepoUpstream(t, "portal", uncached) grails, _ := mavenRepoUpstream(t, "grails") h := newMavenRepositoriesHandler(t, central, portal, grails) - if w := getMaven(h, dotted); w.Code != http.StatusOK || w.Body.String() != "portal "+dotted { - t.Errorf("GET %s = %d %q, want the Plugin Portal's copy", dotted, w.Code, w.Body.String()) + if w := getMaven(h, uncached); w.Code != http.StatusOK || w.Body.String() != "portal "+uncached { + t.Errorf("GET %s = %d %q, want the Plugin Portal's copy", uncached, w.Code, w.Body.String()) } } func TestMavenUncachedPathKeepsByteRanges(t *testing.T) { - const dotted = "/org.foo/bar/1.0/bar-1.0.jar" + const uncached = "/org/foo/archetype-catalog.xml" ranged := func() *httptest.Server { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.ServeContent(w, r, "bar.jar", time.Time{}, strings.NewReader("0123456789")) + http.ServeContent(w, r, "archetype-catalog.xml", time.Time{}, strings.NewReader("0123456789")) })) t.Cleanup(srv.Close) return srv @@ -186,7 +199,7 @@ func TestMavenUncachedPathKeepsByteRanges(t *testing.T) { portal, _ := mavenRepoUpstream(t, "portal") h := newMavenRepositoriesHandler(t, central, portal, grails) - for _, p := range []string{dotted, "/grails" + dotted} { + for _, p := range []string{uncached, "/grails" + uncached} { req := httptest.NewRequest(http.MethodGet, p, nil) req.Header.Set("Range", "bytes=2-4") w := httptest.NewRecorder() From 3776366ba22582ef2c5940686bf9072bf826491b Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 21:58:47 +0100 Subject: [PATCH 12/14] Document the 404 for Maven files outside the layout --- docs/configuration.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index e61c7e9e..4272a846 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1018,9 +1018,10 @@ one doesn't fall back to the Gradle Plugin Portal. Files are cached per repository, so the same coordinates in two repositories never share a cache entry. -In every repository, a path whose group directories contain a dot or are empty -(`org.foo/bar/1.0/bar-1.0.jar`) is not in the Maven layout and is proxied -without caching, since its group would read the same as `org/foo`'s. +In every repository, a file whose group directories contain a dot or colon or +are empty (`org.foo/bar/1.0/bar-1.0.jar`), or whose artifact contains a colon, +is not in the Maven layout and gets 404 without an upstream request. Its group +would read the same as `org/foo`'s. `upstream.maven_repositories` is additive. `/maven/…` keeps serving `upstream.maven` with unchanged cache identities. A repository name is matched From 4ae55605b14c52749aa780c673824d74e0140b7f Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 22:18:14 +0100 Subject: [PATCH 13/14] Send Maven coordinate files under odd groups to the 404 A file named {artifact}-{version} under a dotted group directory, such as org.foo/bar/1.0/bar-1.0-linux.exe, was not recognised as a coordinate file and so was proxied past the checks. isCoordinateFile no longer checks the group, so handleDownload answers 404 for it. The docs say which files get the 404, and the Range test covers a stale If-Range. --- docs/configuration.md | 10 ++++++---- internal/handler/maven.go | 9 ++++++++- internal/handler/maven_repositories_test.go | 16 +++++++++++++++- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 4272a846..9aa7d803 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1018,10 +1018,12 @@ one doesn't fall back to the Gradle Plugin Portal. Files are cached per repository, so the same coordinates in two repositories never share a cache entry. -In every repository, a file whose group directories contain a dot or colon or -are empty (`org.foo/bar/1.0/bar-1.0.jar`), or whose artifact contains a colon, -is not in the Maven layout and gets 404 without an upstream request. Its group -would read the same as `org/foo`'s. +In every repository, an artifact file (one with an artifact extension or named +`{artifact}-{version}…`) whose group directories contain a dot or colon or are +empty (`org.foo/bar/1.0/bar-1.0.jar`), or whose artifact contains a colon, is +not in the Maven layout and gets 404 without an upstream request. Its group +would read the same as `org/foo`'s. Other paths there, such as +`maven-metadata.xml` or a directory listing, are handled as anywhere else. `upstream.maven_repositories` is additive. `/maven/…` keeps serving `upstream.maven` with unchanged cache identities. A repository name is matched diff --git a/internal/handler/maven.go b/internal/handler/maven.go index 5008e00f..e841d083 100644 --- a/internal/handler/maven.go +++ b/internal/handler/maven.go @@ -321,8 +321,15 @@ func (h *MavenHandler) isArtifactFile(filename string) bool { // artifact and version of its directory, {artifact}-{version}..., as Maven // names every file it publishes, whatever the extension. A SNAPSHOT's // files carry a timestamp in place of "SNAPSHOT". +// +// The group isn't checked, so a file under a group directory outside the +// Maven layout still goes to handleDownload, which answers 404. func (h *MavenHandler) isCoordinateFile(urlPath string) bool { - _, artifact, version, filename := h.parsePath(urlPath) + parts := strings.Split(urlPath, "/") + if len(parts) < minMavenParts { + return false + } + filename, version, artifact := parts[len(parts)-1], parts[len(parts)-2], parts[len(parts)-3] if artifact == "" || version == "" { return false } diff --git a/internal/handler/maven_repositories_test.go b/internal/handler/maven_repositories_test.go index 923af928..e9b2fcf4 100644 --- a/internal/handler/maven_repositories_test.go +++ b/internal/handler/maven_repositories_test.go @@ -145,7 +145,10 @@ func TestMavenRepositoryRepointedServesTheNewUpstream(t *testing.T) { // the dots spell or be proxied past the denylist, scan and cooldown. func TestMavenDottedGroupDirectoryNotCachedAsGroup(t *testing.T) { const standard = "/org/foo/bar/1.0/bar-1.0.jar" - odd := []string{"/org.foo/bar/1.0/bar-1.0.jar", "/org/foo:x/bar/1.0/bar-1.0.jar", "/org/foo/b:ar/1.0/b:ar-1.0.jar"} + odd := []string{ + "/org.foo/bar/1.0/bar-1.0.jar", "/org.foo/bar/1.0/bar-1.0-linux.exe", + "/org/foo:x/bar/1.0/bar-1.0.jar", "/org/foo/b:ar/1.0/b:ar-1.0.jar", + } central, requests := mavenRepoUpstream(t, "central", append([]string{standard}, odd...)...) portal, _ := mavenRepoUpstream(t, "portal") var restricted []string @@ -190,6 +193,7 @@ func TestMavenUncachedPathKeepsByteRanges(t *testing.T) { const uncached = "/org/foo/archetype-catalog.xml" ranged := func() *httptest.Server { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", `"v2"`) http.ServeContent(w, r, "archetype-catalog.xml", time.Time{}, strings.NewReader("0123456789")) })) t.Cleanup(srv.Close) @@ -207,5 +211,15 @@ func TestMavenUncachedPathKeepsByteRanges(t *testing.T) { if w.Code != http.StatusPartialContent || w.Body.String() != "234" { t.Errorf("GET %s with Range = %d %q, want 206 %q", p, w.Code, w.Body.String(), "234") } + + // A Range for an older copy gets the whole current file. + req = httptest.NewRequest(http.MethodGet, p, nil) + req.Header.Set("Range", "bytes=2-4") + req.Header.Set("If-Range", `"v1"`) + w = httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusOK || w.Body.String() != "0123456789" { + t.Errorf("GET %s with stale If-Range = %d %q, want 200 with the whole file", p, w.Code, w.Body.String()) + } } } From d7399b10435925d80815b6f726ae2e5dab8a2a62 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 8 Nov 2026 22:51:25 +0100 Subject: [PATCH 14/14] Say which Maven files get the 404 without listing name rules --- docs/configuration.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 9aa7d803..d887509a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1018,12 +1018,12 @@ one doesn't fall back to the Gradle Plugin Portal. Files are cached per repository, so the same coordinates in two repositories never share a cache entry. -In every repository, an artifact file (one with an artifact extension or named -`{artifact}-{version}…`) whose group directories contain a dot or colon or are -empty (`org.foo/bar/1.0/bar-1.0.jar`), or whose artifact contains a colon, is -not in the Maven layout and gets 404 without an upstream request. Its group -would read the same as `org/foo`'s. Other paths there, such as -`maven-metadata.xml` or a directory listing, are handled as anywhere else. +In every repository, a file the proxy would cache (an artifact, checksum or +signature) whose group directories contain a dot or colon +(`org.foo/bar/1.0/bar-1.0.jar`), or whose artifact contains a colon, is not in +the Maven layout and gets 404 without an upstream request. Its group would read +the same as `org/foo`'s. Other paths there, such as `maven-metadata.xml` or a +directory listing, are handled as anywhere else. `upstream.maven_repositories` is additive. `/maven/…` keeps serving `upstream.maven` with unchanged cache identities. A repository name is matched