Skip to content

Commit 4d9c87b

Browse files
authored
Merge pull request #678 from simonchrz/fix/cache-key-collisions
Fix cache key collisions in RPM, Maven and the git cache
2 parents 1c9509d + d7399b1 commit 4d9c87b

10 files changed

Lines changed: 360 additions & 25 deletions

File tree

‎docs/configuration.md‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -954,6 +954,12 @@ those yourself. The proxy locks `cache_dir` while it runs, through a
954954
`.gitcache-lock` file in it, so a second process given the same directory,
955955
such as another replica on a shared volume, fails to start; give each its own.
956956

957+
Each repository's directory is named by its path as requested. Where
958+
`cache_dir` ignores case in file names, as on macOS and Windows by default,
959+
paths that differ only in case (`acme/widgets` and `acme/WIDGETS`) share one
960+
directory and are treated as one repository. Use a case-sensitive file system
961+
for an upstream whose repositories differ only in case.
962+
957963
Git LFS objects are cached in the artifact cache, so they use the configured
958964
storage backend and retention rather than `cache_dir`. git-lfs finds the LFS
959965
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
10121018
repository, so the same coordinates in two repositories never share a cache
10131019
entry.
10141020

1021+
In every repository, a file the proxy would cache (an artifact, checksum or
1022+
signature) whose group directories contain a dot or colon
1023+
(`org.foo/bar/1.0/bar-1.0.jar`), or whose artifact contains a colon, is not in
1024+
the Maven layout and gets 404 without an upstream request. Its group would read
1025+
the same as `org/foo`'s. Other paths there, such as `maven-metadata.xml` or a
1026+
directory listing, are handled as anywhere else.
1027+
10151028
`upstream.maven_repositories` is additive. `/maven/…` keeps serving
10161029
`upstream.maven` with unchanged cache identities. A repository name is matched
10171030
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
18381851
curl -X DELETE 'http://localhost:8080/api/cache/maven/com.google.guava:guava/32.1.3-jre?filename=guava-32.1.3-jre.jar'
18391852
```
18401853

1841-
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.
1854+
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.
18421855

18431856
Pushed archives are not touched: a whole-version delete skips them, and naming one answers 409. Delete those through the [hosted API](#hosted-packages).
18441857

‎internal/gitcache/cache.go‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,9 @@ type Cache struct {
9595
refCheckTimeout time.Duration
9696
lowSpeedTime int
9797
maxRequestBody int64
98+
// foldCase is set where the cache directory ignores case, so keys that
99+
// differ only in case, which name one directory, are one repository.
100+
foldCase bool
98101

99102
ctx context.Context
100103
cancel context.CancelFunc
@@ -217,6 +220,7 @@ func New(opts Options) (*Cache, error) {
217220
tooLarge: make(map[string]time.Time),
218221
failedWants: make(map[string]time.Time),
219222
freed: make(chan struct{}),
223+
foldCase: caseInsensitive(dir),
220224
}
221225
if opts.refCheckTimeout > 0 {
222226
c.refCheckTimeout = opts.refCheckTimeout
@@ -287,7 +291,7 @@ func (c *Cache) load() error {
287291
return err
288292
}
289293
rel, _ := filepath.Rel(c.dir, path)
290-
rp := newRepo(filepath.ToSlash(rel), path)
294+
rp := newRepo(c.repoKey(filepath.ToSlash(rel)), path)
291295
rp.size.Store(dirSize(path))
292296
rp.lastChecked.Store(stampTime(path, checkedStamp))
293297
rp.lastUsed.Store(stampTime(path, usedStamp))
@@ -355,15 +359,25 @@ func (c *Cache) removeRepointed() {
355359
func (c *Cache) acquire(key string) *repo {
356360
c.mu.Lock()
357361
defer c.mu.Unlock()
358-
rp := c.repos[key]
362+
rk := c.repoKey(key)
363+
rp := c.repos[rk]
359364
if rp == nil {
360-
rp = newRepo(key, filepath.Join(c.dir, filepath.FromSlash(key)))
361-
c.repos[key] = rp
365+
rp = newRepo(rk, filepath.Join(c.dir, filepath.FromSlash(key)))
366+
c.repos[rk] = rp
362367
}
363368
rp.users++
364369
return rp
365370
}
366371

372+
// repoKey is the key a repository is tracked under: key itself, or key
373+
// lowercased where the cache directory ignores case.
374+
func (c *Cache) repoKey(key string) string {
375+
if c.foldCase {
376+
return strings.ToLower(key)
377+
}
378+
return key
379+
}
380+
367381
func (c *Cache) release(rp *repo) {
368382
now := time.Now()
369383
rp.lastUsed.Store(now.UnixNano())
@@ -514,3 +528,17 @@ func touch(dir, name string, t time.Time) {
514528
}
515529
}
516530
}
531+
532+
// caseInsensitive reports whether the file system holding dir ignores case
533+
// in names, as macOS and Windows do by default.
534+
func caseInsensitive(dir string) bool {
535+
f, err := os.CreateTemp(dir, ".gitcache-case-")
536+
if err != nil {
537+
return false
538+
}
539+
name := f.Name()
540+
_ = f.Close()
541+
defer func() { _ = os.Remove(name) }()
542+
_, err = os.Stat(filepath.Join(dir, strings.ToUpper(filepath.Base(name))))
543+
return err == nil
544+
}

‎internal/gitcache/cache_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -984,3 +984,51 @@ func TestFetchRedirectRefusesRepository(t *testing.T) {
984984
t.Errorf("ls-refs served after the fetch was redirected")
985985
}
986986
}
987+
988+
// TestCaseOnlyDifferentKeysShareOneRepository checks that, where the file
989+
// system ignores case, keys that differ only in case are one repository, so
990+
// a user of one spelling keeps the directory from being evicted under it.
991+
func TestCaseOnlyDifferentKeysShareOneRepository(t *testing.T) {
992+
if !caseInsensitive(t.TempDir()) {
993+
t.Skip("file system is case-sensitive")
994+
}
995+
upstream := newFakeUpstream(t)
996+
upstream.newRepo("repo")
997+
tc := newTestCache(t, upstream.server.URL, nil)
998+
tc.clone(t, "repo")
999+
waitIdle(t, tc.Cache)
1000+
1001+
rp := tc.acquire("up/REPO.git")
1002+
defer tc.release(rp)
1003+
if other := tc.repo("up/repo.git"); other != rp {
1004+
t.Error("up/REPO.git and up/repo.git are separate repositories")
1005+
}
1006+
tc.mu.Lock()
1007+
tc.opts.MaxSize = 1
1008+
tc.mu.Unlock()
1009+
tc.evict()
1010+
if !rp.exists() {
1011+
t.Error("repository in use as up/REPO.git was evicted")
1012+
}
1013+
}
1014+
1015+
// TestFoldCaseKeysShareOneEntry checks key folding on any file system by
1016+
// setting foldCase directly.
1017+
func TestFoldCaseKeysShareOneEntry(t *testing.T) {
1018+
tc := newTestCache(t, newFakeUpstream(t).server.URL, nil)
1019+
tc.foldCase = true
1020+
1021+
upper := tc.acquire("up/REPO.git")
1022+
defer tc.release(upper)
1023+
lower := tc.acquire("up/repo.git")
1024+
defer tc.release(lower)
1025+
if upper != lower {
1026+
t.Error("up/REPO.git and up/repo.git are separate repositories")
1027+
}
1028+
tc.mu.Lock()
1029+
n := len(tc.repos)
1030+
tc.mu.Unlock()
1031+
if n != 1 {
1032+
t.Errorf("%d repository entries, want 1", n)
1033+
}
1034+
}

‎internal/handler/handler.go‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1477,27 +1477,35 @@ func copyArtifactBody(w http.ResponseWriter, reader io.Reader, expectedSize int6
14771477
}
14781478
}
14791479

1480-
// ProxyUpstream forwards a request to an upstream URL without caching.
1481-
// It copies the request, forwards specified headers, and streams the response back.
1482-
// forwardHeaders controls the request headers sent upstream. End-to-end response
1483-
// headers and trailers are relayed independently of that list.
1484-
func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamURL string, forwardHeaders []string) {
1485-
p.Logger.Debug("proxying to upstream", "url", upstreamURL)
1486-
1480+
// upstreamRequest sends r's method to upstreamURL, copying only the
1481+
// forwardHeaders that r sets.
1482+
func (p *Proxy) upstreamRequest(r *http.Request, upstreamURL string, forwardHeaders []string) (*http.Response, error) {
14871483
req, err := http.NewRequestWithContext(r.Context(), r.Method, upstreamURL, nil)
14881484
if err != nil {
1489-
http.Error(w, "failed to create request", http.StatusInternalServerError)
1490-
return
1485+
return nil, err
14911486
}
1492-
1493-
// Copy request headers that affect content negotiation / caching
14941487
for _, header := range forwardHeaders {
14951488
if v := r.Header.Get(header); v != "" {
14961489
req.Header.Set(header, v)
14971490
}
14981491
}
1492+
return p.HTTPClient.Do(req)
1493+
}
14991494

1500-
resp, err := p.HTTPClient.Do(req)
1495+
// ProxyUpstream forwards a request to an upstream URL without caching.
1496+
// It copies the request, forwards specified headers, and streams the response back.
1497+
// forwardHeaders controls the request headers sent upstream. End-to-end response
1498+
// headers and trailers are relayed independently of that list.
1499+
func (p *Proxy) ProxyUpstream(w http.ResponseWriter, r *http.Request, upstreamURL string, forwardHeaders []string) {
1500+
p.Logger.Debug("proxying to upstream", "url", upstreamURL)
1501+
1502+
resp, err := p.upstreamRequest(r, upstreamURL, forwardHeaders)
1503+
p.relayUpstream(w, r, resp, err)
1504+
}
1505+
1506+
// relayUpstream writes the result of an upstream request: the response, or
1507+
// a 502 when the request failed.
1508+
func (p *Proxy) relayUpstream(w http.ResponseWriter, r *http.Request, resp *http.Response, err error) {
15011509
if err != nil {
15021510
if writeUpstreamHeld(w, err) {
15031511
return

‎internal/handler/maven.go‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,13 @@ func (h *MavenHandler) handleDownload(w http.ResponseWriter, r *http.Request, so
221221
// e.g., com/google/guava/guava/32.1.3-jre/guava-32.1.3-jre.jar
222222
group, artifact, version, filename := h.parsePath(urlPath)
223223
if artifact == "" {
224+
if strings.Count(urlPath, "/") >= minMavenParts-1 {
225+
// A dot or colon in a group directory, or a colon in the
226+
// artifact, which the Maven layout never has. Proxied, the
227+
// file would skip the denylist, scan and cooldown.
228+
http.NotFound(w, r)
229+
return
230+
}
224231
h.proxyUpstream(w, r, source, urlPath)
225232
return
226233
}
@@ -266,6 +273,7 @@ func (h *MavenHandler) handleDownload(w http.ResponseWriter, r *http.Request, so
266273
// parsePath extracts Maven coordinates from a URL path.
267274
// e.g., "com/google/guava/guava/32.1.3-jre/guava-32.1.3-jre.jar"
268275
// -> ("com.google.guava", "guava", "32.1.3-jre", "guava-32.1.3-jre.jar")
276+
// A path that isn't in the Maven layout returns empty strings.
269277
func (h *MavenHandler) parsePath(urlPath string) (group, artifact, version, filename string) {
270278
parts := strings.Split(urlPath, "/")
271279
if len(parts) < minMavenParts {
@@ -276,6 +284,18 @@ func (h *MavenHandler) parsePath(urlPath string) (group, artifact, version, file
276284
version = parts[len(parts)-2]
277285
artifact = parts[len(parts)-3]
278286
groupParts := parts[:len(parts)-3]
287+
// The Maven layout splits a group at its dots, so a group directory
288+
// never holds a dot or is empty. Such a path would spell the same group
289+
// as another and share its cache entries, so it is not parsed. Nor is a
290+
// colon in the group or artifact, which are cached as group:artifact.
291+
for _, part := range groupParts {
292+
if part == "" || strings.ContainsAny(part, ".:") {
293+
return "", "", "", ""
294+
}
295+
}
296+
if strings.Contains(artifact, ":") {
297+
return "", "", "", ""
298+
}
279299
group = strings.Join(groupParts, ".")
280300

281301
return group, artifact, version, filename
@@ -301,8 +321,15 @@ func (h *MavenHandler) isArtifactFile(filename string) bool {
301321
// artifact and version of its directory, {artifact}-{version}..., as Maven
302322
// names every file it publishes, whatever the extension. A SNAPSHOT's
303323
// files carry a timestamp in place of "SNAPSHOT".
324+
//
325+
// The group isn't checked, so a file under a group directory outside the
326+
// Maven layout still goes to handleDownload, which answers 404.
304327
func (h *MavenHandler) isCoordinateFile(urlPath string) bool {
305-
_, artifact, version, filename := h.parsePath(urlPath)
328+
parts := strings.Split(urlPath, "/")
329+
if len(parts) < minMavenParts {
330+
return false
331+
}
332+
filename, version, artifact := parts[len(parts)-1], parts[len(parts)-2], parts[len(parts)-3]
306333
if artifact == "" || version == "" {
307334
return false
308335
}
@@ -328,7 +355,21 @@ func (h *MavenHandler) isChecksumFile(filename string) bool {
328355
return false
329356
}
330357

358+
// mavenForwardHeaders lets clients resume artifacts proxied without caching.
359+
var mavenForwardHeaders = []string{"Range", "If-Range"}
360+
331361
// proxyUpstream forwards a request to the source's upstream without caching.
332362
func (h *MavenHandler) proxyUpstream(w http.ResponseWriter, r *http.Request, source mavenSource, urlPath string) {
333-
h.proxy.ProxyUpstream(w, r, source.upstreamURL+"/"+urlPath, nil)
363+
if source.fallbackURL == "" {
364+
h.proxy.ProxyUpstream(w, r, source.upstreamURL+"/"+urlPath, mavenForwardHeaders)
365+
return
366+
}
367+
// As for cached files, a file the primary upstream lacks is looked for
368+
// on the Gradle Plugin Portal.
369+
resp, err := h.proxy.upstreamRequest(r, source.upstreamURL+"/"+urlPath, mavenForwardHeaders)
370+
if err == nil && resp.StatusCode == http.StatusNotFound {
371+
_ = resp.Body.Close()
372+
resp, err = h.proxy.upstreamRequest(r, source.fallbackURL+"/"+urlPath, mavenForwardHeaders)
373+
}
374+
h.proxy.relayUpstream(w, r, resp, err)
334375
}

‎internal/handler/maven_repositories_test.go‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"strings"
88
"sync/atomic"
99
"testing"
10+
"time"
1011

1112
"github.com/git-pkgs/registries/fetch"
1213
)
@@ -137,3 +138,88 @@ func TestMavenRepositoryRepointedServesTheNewUpstream(t *testing.T) {
137138
}
138139
}
139140
}
141+
142+
// TestMavenDottedGroupDirectoryNotCachedAsGroup checks that a file whose
143+
// group directories contain dots or colons, which the Maven layout never
144+
// has, is not served: it would either share the cache entry of the group
145+
// the dots spell or be proxied past the denylist, scan and cooldown.
146+
func TestMavenDottedGroupDirectoryNotCachedAsGroup(t *testing.T) {
147+
const standard = "/org/foo/bar/1.0/bar-1.0.jar"
148+
odd := []string{
149+
"/org.foo/bar/1.0/bar-1.0.jar", "/org.foo/bar/1.0/bar-1.0-linux.exe",
150+
"/org/foo:x/bar/1.0/bar-1.0.jar", "/org/foo/b:ar/1.0/b:ar-1.0.jar",
151+
}
152+
central, requests := mavenRepoUpstream(t, "central", append([]string{standard}, odd...)...)
153+
portal, _ := mavenRepoUpstream(t, "portal")
154+
var restricted []string
155+
for _, p := range odd {
156+
restricted = append(restricted, "/restricted"+p)
157+
}
158+
grails, grailsRequests := mavenRepoUpstream(t, "grails", restricted...)
159+
h := newMavenRepositoriesHandler(t, central, portal, grails)
160+
161+
if w := getMaven(h, standard); w.Code != http.StatusOK || w.Body.String() != "central "+standard {
162+
t.Errorf("GET %s = %d %q", standard, w.Code, w.Body.String())
163+
}
164+
before := requests.Load() + grailsRequests.Load()
165+
for _, p := range odd {
166+
for _, path := range []string{p, "/grails" + p} {
167+
if w := getMaven(h, path); w.Code != http.StatusNotFound {
168+
t.Errorf("GET %s = %d %q, want 404", path, w.Code, w.Body.String())
169+
}
170+
}
171+
}
172+
if n := requests.Load() + grailsRequests.Load() - before; n != 0 {
173+
t.Errorf("%d upstream requests for paths outside the Maven layout", n)
174+
}
175+
}
176+
177+
// TestMavenUncachedPathFallsBackToPluginPortal checks that a path proxied
178+
// without caching, such as a catalog, still tries the Gradle Plugin Portal
179+
// when the primary upstream lacks it.
180+
func TestMavenUncachedPathFallsBackToPluginPortal(t *testing.T) {
181+
const uncached = "/org/foo/archetype-catalog.xml"
182+
central, _ := mavenRepoUpstream(t, "central")
183+
portal, _ := mavenRepoUpstream(t, "portal", uncached)
184+
grails, _ := mavenRepoUpstream(t, "grails")
185+
h := newMavenRepositoriesHandler(t, central, portal, grails)
186+
187+
if w := getMaven(h, uncached); w.Code != http.StatusOK || w.Body.String() != "portal "+uncached {
188+
t.Errorf("GET %s = %d %q, want the Plugin Portal's copy", uncached, w.Code, w.Body.String())
189+
}
190+
}
191+
192+
func TestMavenUncachedPathKeepsByteRanges(t *testing.T) {
193+
const uncached = "/org/foo/archetype-catalog.xml"
194+
ranged := func() *httptest.Server {
195+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
196+
w.Header().Set("ETag", `"v2"`)
197+
http.ServeContent(w, r, "archetype-catalog.xml", time.Time{}, strings.NewReader("0123456789"))
198+
}))
199+
t.Cleanup(srv.Close)
200+
return srv
201+
}
202+
central, grails := ranged(), ranged()
203+
portal, _ := mavenRepoUpstream(t, "portal")
204+
h := newMavenRepositoriesHandler(t, central, portal, grails)
205+
206+
for _, p := range []string{uncached, "/grails" + uncached} {
207+
req := httptest.NewRequest(http.MethodGet, p, nil)
208+
req.Header.Set("Range", "bytes=2-4")
209+
w := httptest.NewRecorder()
210+
h.ServeHTTP(w, req)
211+
if w.Code != http.StatusPartialContent || w.Body.String() != "234" {
212+
t.Errorf("GET %s with Range = %d %q, want 206 %q", p, w.Code, w.Body.String(), "234")
213+
}
214+
215+
// A Range for an older copy gets the whole current file.
216+
req = httptest.NewRequest(http.MethodGet, p, nil)
217+
req.Header.Set("Range", "bytes=2-4")
218+
req.Header.Set("If-Range", `"v1"`)
219+
w = httptest.NewRecorder()
220+
h.ServeHTTP(w, req)
221+
if w.Code != http.StatusOK || w.Body.String() != "0123456789" {
222+
t.Errorf("GET %s with stale If-Range = %d %q, want 200 with the whole file", p, w.Code, w.Body.String())
223+
}
224+
}
225+
}

0 commit comments

Comments
 (0)