Fix cache key collisions in RPM, Maven and the git cache

docs/configuration.md

@@ -954,6 +954,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
@@ -1012,6 +1018,13 @@ 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 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
 before the default repository, so it hides every group ID whose first segment
@@ -1838,7 +1851,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).
 

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
@@ -217,6 +220,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
@@ -287,7 +291,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))
@@ -355,15 +359,25 @@ func (c *Cache) removeRepointed() {
 func (c *Cache) acquire(key string) *repo {
 	c.mu.Lock()
 	defer c.mu.Unlock()
-	rp := c.repos[key]
+	rk := c.repoKey(key)
+	rp := c.repos[rk]
 	if rp == nil {
-		rp = newRepo(key, filepath.Join(c.dir, filepath.FromSlash(key)))
-		c.repos[key] = rp
+		rp = newRepo(rk, filepath.Join(c.dir, filepath.FromSlash(key)))
+		c.repos[rk] = 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())
@@ -514,3 +528,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
+}

internal/gitcache/cache_test.go

@@ -984,3 +984,51 @@ func TestFetchRedirectRefusesRepository(t *testing.T) {
 		t.Errorf("ls-refs served after the fetch was redirected")
 	}
 }
+
+// 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 != 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")
+	}
+}
+
+// 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)
+	}
+}

internal/handler/handler.go

@@ -1477,27 +1477,35 @@ 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.
-func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamURL string, forwardHeaders []string) {
-	p.Logger.Debug("proxying to upstream", "url", upstreamURL)
-
+// 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 {
-		http.Error(w, "failed to create request", http.StatusInternalServerError)
-		return
+		return nil, err
 	}
-
-	// 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)
+	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

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
 	}
@@ -266,6 +273,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 +284,18 @@ 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. Nor is a
+	// colon in the group or artifact, which are cached as group:artifact.
+	for _, part := range groupParts {
+		if part == "" || strings.ContainsAny(part, ".:") {
+			return "", "", "", ""
+		}
+	}
+	if strings.Contains(artifact, ":") {
+		return "", "", "", ""
+	}
 	group = strings.Join(groupParts, ".")
 
 	return group, artifact, version, filename
@@ -301,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
 	}
@@ -328,7 +355,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) {
-	h.proxy.ProxyUpstream(w, r, source.upstreamURL+"/"+urlPath, nil)
+	if source.fallbackURL == "" {
+		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, mavenForwardHeaders)
+	if err == nil && resp.StatusCode == http.StatusNotFound {
+		_ = resp.Body.Close()
+		resp, err = h.proxy.upstreamRequest(r, source.fallbackURL+"/"+urlPath, mavenForwardHeaders)
+	}
+	h.proxy.relayUpstream(w, r, resp, err)
 }

internal/handler/maven_repositories_test.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/git-pkgs/registries/fetch"
 )
@@ -137,3 +138,88 @@ func TestMavenRepositoryRepointedServesTheNewUpstream(t *testing.T) {
 		}
 	}
 }
+
+// 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"
+	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
+	for _, p := range odd {
+		restricted = append(restricted, "/restricted"+p)
+	}
+	grails, grailsRequests := mavenRepoUpstream(t, "grails", restricted...)
+	h := newMavenRepositoriesHandler(t, central, portal, grails)
+
+	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 a catalog, still tries the Gradle Plugin Portal
+// when the primary upstream lacks it.
+func TestMavenUncachedPathFallsBackToPluginPortal(t *testing.T) {
+	const uncached = "/org/foo/archetype-catalog.xml"
+	central, _ := mavenRepoUpstream(t, "central")
+	portal, _ := mavenRepoUpstream(t, "portal", uncached)
+	grails, _ := mavenRepoUpstream(t, "grails")
+	h := newMavenRepositoriesHandler(t, central, portal, grails)
+
+	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 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)
+		return srv
+	}
+	central, grails := ranged(), ranged()
+	portal, _ := mavenRepoUpstream(t, "portal")
+	h := newMavenRepositoriesHandler(t, central, portal, grails)
+
+	for _, p := range []string{uncached, "/grails" + uncached} {
+		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")
+		}
+
+		// 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())
+		}
+	}
+}