From be50be27e20309d3840f6fb231ae4c31b732b8f1 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Fri, 23 Oct 2026 13:22:56 +0200 Subject: [PATCH] Serve cached Composer releases without a metadata lookup Every Composer archive download resolved its dist from the package metadata before checking the artifact cache. With cache_metadata off, the default, that meant an uncoalesced upstream GET of the p2 document for every download of an archive the proxy already had: three downloads of a cached archive made three metadata requests. With the cache on, each download after the TTL revalidated and rescanned the document. The lookup is only needed for the publish time cooldown checks downloads against, since lock files name archive URLs directly. Like the npm handler, record that time in the versions table when the dist is first resolved, and on later downloads of a tagged release read it from there and serve the cached archive straight away. Without a stored time, as for archives cached by an older proxy, the download takes the metadata path, which then records it. The time is stored even with cooldown off, so enabling cooldown later keeps the fast path. The cache still enforces the denylist and verifies the stored digest, and an archive is only ever cached under a name that was checked against the metadata, so those guarantees hold. Branches, whose archive and time change with every push, routed packages, whose versions row would not say which repository the time came from, and requests with a query string such as ?sha= keep resolving from the metadata. --- docs/configuration.md | 4 +- internal/handler/composer.go | 94 +++++++- .../handler/composer_download_lookup_test.go | 203 +++++++++++++++++- 3 files changed, 283 insertions(+), 18 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 4da6f634..660421f0 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -349,7 +349,7 @@ upstream: - An npm route URL is the registry base that serves `/{package}`. Tarball URLs in its packuments must be on the same host and below the same path. - A routed package is not looked up anywhere else. If the private registry does not have it, the client gets a 404. If the registry fails or rejects the credentials, the client gets a 502. A package with the same name on the public registry is never served in its place (dependency confusion). - Metadata and artifacts of routed packages are cached under keys that include the route's URL. Entries cached from the public registry before a route was added are never served for a routed package. -- With cooldown enabled, the publish time of a routed npm version always comes from the route's metadata. Times recorded for the public package of the same name, or for a route's previous URL, are not used, and routed downloads do not record times of their own. +- With cooldown enabled, the publish time of a routed npm or Composer version always comes from the route's metadata. Times recorded for the public package of the same name, or for a route's previous URL, are not used, and routed downloads do not record times of their own. - Composer metadata of routed packages carries an empty `notification-url`, so Composer does not report their installs to the default upstream's `notify-batch` endpoint. - Use `upstream.auth` for the registry's credentials. The proxy has no authentication of its own, so every client that can reach it can download routed packages. - Search (`/composer/search.json`) and the package list (`/composer/packages/list.json`) still come from the default upstream only. @@ -1209,7 +1209,7 @@ cache_metadata: true Or via environment variable: `PROXY_CACHE_METADATA=true`. -Composer archive downloads look up their upstream URL in the same cached metadata, so a cached archive is served without asking Packagist again and stays installable while it is down. Only a version or file the cached metadata does not list yet is looked up upstream. +Composer archive downloads look up their upstream URL in the same cached metadata, so an archive is resolved without asking Packagist again and stays installable while it is down. Only a version or file the cached metadata does not list yet is looked up upstream. With or without `cache_metadata`, a cached archive of a tagged release from the default repository is served without any metadata lookup; with cooldown enabled it is checked against the publish time recorded when the archive was first resolved. Branch (dev) versions, routed packages and archives picked by a query string such as `?sha=` are always resolved from the metadata. The `proxy mirror` command always enables metadata caching regardless of this setting. diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 643b287f..768b89ae 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -383,13 +383,8 @@ func (h *ComposerHandler) shouldFilterVersion(packagePURL, packageName, version return false } - var timeStr string - if err := json.Unmarshal(published, &timeStr); err != nil { - return false - } - - publishedAt, err := time.Parse(time.RFC3339, timeStr) - if err != nil { + publishedAt, ok := composerPublishTime(published) + if !ok { return false } @@ -402,6 +397,17 @@ func (h *ComposerHandler) shouldFilterVersion(packagePURL, packageName, version return false } +// composerPublishTime parses the raw "time" field of a version, reporting +// false when it is missing or not an RFC 3339 string. +func composerPublishTime(published []byte) (time.Time, bool) { + var timeStr string + if err := json.Unmarshal(published, &timeStr); err != nil { + return time.Time{}, false + } + publishedAt, err := time.Parse(time.RFC3339, timeStr) + return publishedAt, err == nil +} + // disableDefaultNotification stops Composer from reporting installs of a routed // package to the default repository's notify-batch URL, which would disclose // the package name to it. Composer only falls back to notify-batch when a @@ -633,6 +639,8 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) requested := path.Base(r.URL.EscapedPath()) if r.URL.RawQuery != "" { requested += "?" + r.URL.RawQuery + } else if h.serveCachedRelease(w, r, packageName, version, filename) { + return } files := h.metadataFilesForVersion(vendor, pkg, version) dist, downloadURL, ok := h.cachedDist(r.Context(), files, packageName, version, requested) @@ -641,6 +649,7 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) return } } + h.recordPublishTime(packageName, version, dist.published) // Lock files name archive URLs directly, so a version filtered out of the // metadata still has to be withheld here. @@ -682,6 +691,77 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) ServeArtifactRequest(w, r, result) } +// releaseTimeStored reports whether the publish time of version is kept in +// the versions table, so a download can be checked against cooldown without +// the metadata. That holds for tagged releases of packages from the default +// repository. A branch is left out: its archive and time change with every +// push. So are routed packages, as for npm: the versions row is keyed by the +// plain PURL, which does not say which repository the version came from, so +// a time read from the default repository would be trusted for a private +// version of the same name, and the other way round. +func (h *ComposerHandler) releaseTimeStored(packageName, version string) bool { + if h.proxy.DB == nil || isDevVersion(version) { + return false + } + _, routed := h.routes.match(packageName) + return !routed +} + +// serveCachedRelease serves a tagged release's archive from the cache +// without looking up its dist in the metadata, and reports whether it wrote a +// response. The archive was only cached under its name after that name was +// checked against the metadata, and the cache verifies it against the digest +// recorded then. Cooldown is checked against the publish time recorded when +// the dist was first resolved; without one, as for an archive cached by an +// older proxy, it returns false and the caller takes the metadata path, which +// records it. Requests with a query string, which pick a commit, always take +// that path. +func (h *ComposerHandler) serveCachedRelease(w http.ResponseWriter, r *http.Request, packageName, version, filename string) bool { + if !h.releaseTimeStored(packageName, version) { + return false + } + if h.proxy.Cooldown != nil && h.proxy.Cooldown.Enabled() { + ver, err := h.proxy.DB.GetVersionByPURL(canonicalVersionPURL("composer", packageName, version)) + if err != nil || ver == nil || !ver.PublishedAt.Valid { + return false + } + if !h.proxy.Cooldown.IsAllowed("composer", canonicalPackagePURL("composer", packageName), ver.PublishedAt.Time) { + h.proxy.Logger.Info("cooldown: withholding composer archive", + "package", packageName, "version", version) + http.Error(w, "version not found", http.StatusNotFound) + return true + } + } + cached, err := h.proxy.GetCachedArtifact(r.Context(), "composer", packageName, version, filename) + if err != nil { + h.proxy.serveArtifactError(w, err, "failed to read cached package") + return true + } + if cached == nil { + return false + } + ServeArtifactRequest(w, r, cached) + return true +} + +// recordPublishTime stores the publish time a release's metadata lists, for +// serveCachedRelease. It is stored whether or not cooldown is enabled, so +// enabling it later does not send cached archives back to the metadata. +func (h *ComposerHandler) recordPublishTime(packageName, version string, published []byte) { + if !h.releaseTimeStored(packageName, version) { + return + } + publishedAt, ok := composerPublishTime(published) + if !ok { + return + } + if err := h.proxy.DB.SetVersionPublishedAt(canonicalVersionPURL("composer", packageName, version), + canonicalPackagePURL("composer", packageName), publishedAt); err != nil { + h.proxy.Logger.Warn("failed to store composer publish time", + "package", packageName, "version", version, "error", err) + } +} + // discardOtherCommit clears a cached branch archive named for commit that was // fetched from another commit's URL. Proxy versions before the pinned-commit // fix cached the branch head under whatever commit a lock file asked for. The diff --git a/internal/handler/composer_download_lookup_test.go b/internal/handler/composer_download_lookup_test.go index 5ac4c75c..56350004 100644 --- a/internal/handler/composer_download_lookup_test.go +++ b/internal/handler/composer_download_lookup_test.go @@ -10,23 +10,29 @@ import ( "testing" "time" + "github.com/git-pkgs/cooldown" "github.com/git-pkgs/registries/fetch" ) // composerLookupUpstream serves v/p's metadata listing versions and the // archives they point at, counts metadata and archive requests, and fails -// every request while down is set. +// every request while down is set. A version listed in published carries that +// publish time. type composerLookupUpstream struct { - h http.Handler - versions atomic.Value // []string - down atomic.Bool - metadata atomic.Int32 - archives atomic.Int32 + h http.Handler + handler *ComposerHandler + proxy *Proxy + versions atomic.Value // []string + published map[string]time.Time + down atomic.Bool + metadata atomic.Int32 + archives atomic.Int32 + routeURL string } func setupComposerLookup(t *testing.T, cacheMetadata bool, versions ...string) *composerLookupUpstream { t.Helper() - u := &composerLookupUpstream{} + u := &composerLookupUpstream{published: map[string]time.Time{}} u.versions.Store(versions) var srv *httptest.Server srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -39,7 +45,11 @@ func setupComposerLookup(t *testing.T, cacheMetadata bool, versions ...string) * u.metadata.Add(1) var entries []string for _, v := range u.versions.Load().([]string) { - entries = append(entries, fmt.Sprintf(`{"version":%q,"dist":{"url":%q,"type":"zip"}}`, v, srv.URL+"/archives/"+v+".zip")) + var published string + if at, ok := u.published[v]; ok { + published = fmt.Sprintf(`"time":%q,`, at.Format(time.RFC3339)) + } + entries = append(entries, fmt.Sprintf(`{"version":%q,%s"dist":{"url":%q,"type":"zip"}}`, v, published, srv.URL+"/archives/"+v+".zip")) } _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[%s]}}`, strings.Join(entries, ",")) case r.URL.Path == "/p2/v/p~dev.json": @@ -60,10 +70,19 @@ func setupComposerLookup(t *testing.T, cacheMetadata bool, versions ...string) * fetcher := fetch.NewFetcher(fetch.WithHTTPClient(srv.Client()), fetch.WithMaxRetries(0)) proxy.Fetcher = fetcher t.Cleanup(func() { _ = fetcher.Close() }) - u.h = NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL).Routes() + u.proxy = proxy + u.handler = NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL) + u.h = u.handler.Routes() + u.routeURL = srv.URL return u } +// route sends v/p to a custom repository, which is the same server under +// another name. +func (u *composerLookupUpstream) route() { + u.h = u.handler.WithPackageRoutes(map[string]string{"v/*": u.routeURL}).Routes() +} + func (u *composerLookupUpstream) get(t *testing.T, target string) *httptest.ResponseRecorder { t.Helper() w := httptest.NewRecorder() @@ -126,3 +145,169 @@ func TestComposerDownloadWithoutMetadataCache(t *testing.T) { t.Errorf("upstream metadata requests = %d, want 1", n) } } + +// storedPublishTime returns the publish time recorded for v/p at version. +func (u *composerLookupUpstream) storedPublishTime(t *testing.T, version string) (time.Time, bool) { + t.Helper() + ver, err := u.proxy.DB.GetVersionByPURL(canonicalVersionPURL("composer", "v/p", version)) + if err != nil { + t.Fatalf("reading versions row: %v", err) + } + if ver == nil || !ver.PublishedAt.Valid { + return time.Time{}, false + } + return ver.PublishedAt.Time, true +} + +func (u *composerLookupUpstream) wantMetadataRequests(t *testing.T, want int32) { + t.Helper() + if n := u.metadata.Load(); n != want { + t.Errorf("upstream metadata requests = %d, want %d", n, want) + } +} + +// TestComposerCachedReleaseSkipsMetadata checks that a tagged release whose +// archive is cached is served without asking upstream for the metadata +// again, with and without cache_metadata, and with cooldown, which takes the +// publish time recorded when the archive was first resolved. +func TestComposerCachedReleaseSkipsMetadata(t *testing.T) { + for _, tt := range []struct { + name string + cacheMetadata bool + cooldown bool + }{ + {"metadata not cached", false, false}, + {"metadata cached", true, false}, + {"cooldown", false, true}, + } { + t.Run(tt.name, func(t *testing.T) { + u := setupComposerLookup(t, tt.cacheMetadata, "1.0.0") + u.published["1.0.0"] = time.Now().Add(-30 * 24 * time.Hour) + if tt.cooldown { + u.proxy.Cooldown = &cooldown.Config{Default: "3d"} + } + u.download(t, "1.0.0") + // With cache_metadata the cached copy expires; the archive must + // not need it. + u.proxy.MetadataTTL = 0 + u.download(t, "1.0.0") + u.download(t, "1.0.0") + u.wantMetadataRequests(t, 1) + if n := u.archives.Load(); n != 1 { + t.Errorf("upstream archive requests = %d, want 1", n) + } + }) + } +} + +// TestComposerDownloadStoresPublishTime checks that resolving a tagged +// release records its publish time, so later downloads can check cooldown +// without the metadata. +func TestComposerDownloadStoresPublishTime(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + published := time.Date(2024, 5, 6, 7, 8, 9, 0, time.UTC) + u.published["1.0.0"] = published + u.download(t, "1.0.0") + got, ok := u.storedPublishTime(t, "1.0.0") + if !ok || !got.Equal(published) { + t.Errorf("stored publish time = %v, %v, want %v", got, ok, published) + } +} + +// TestComposerCachedReleaseCooldownFromStoredTime checks that a cached +// archive whose stored publish time is still inside the cooldown window is +// withheld without the metadata, including when cooldown was enabled or +// raised after the archive was cached. +func TestComposerCachedReleaseCooldownFromStoredTime(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0", "2.0.0") + u.published["1.0.0"] = time.Now().Add(-5 * 24 * time.Hour) + u.published["2.0.0"] = time.Now().Add(-1 * time.Hour) + u.download(t, "2.0.0") + u.proxy.Cooldown = &cooldown.Config{Default: "3d"} + u.download(t, "1.0.0") + + if w := u.get(t, "/files/v/p/2.0.0/2.0.0.zip"); w.Code != http.StatusNotFound { + t.Errorf("2.0.0 after enabling cooldown: status %d, want 404", w.Code) + } + u.proxy.Cooldown = &cooldown.Config{Default: "7d"} + if w := u.get(t, "/files/v/p/1.0.0/1.0.0.zip"); w.Code != http.StatusNotFound { + t.Errorf("1.0.0 after raising cooldown: status %d, want 404", w.Code) + } + u.wantMetadataRequests(t, 2) +} + +// TestComposerCachedReleaseWithoutStoredTime checks that with cooldown a +// cached archive the proxy recorded no publish time for, such as one cached +// by an older version, is checked against the metadata, which then records +// the time. +func TestComposerCachedReleaseWithoutStoredTime(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + u.published["1.0.0"] = time.Now().Add(-1 * time.Hour) + u.download(t, "1.0.0") + if _, err := u.proxy.DB.Exec(`UPDATE versions SET published_at = NULL`); err != nil { + t.Fatal(err) + } + + u.proxy.Cooldown = &cooldown.Config{Default: "3d"} + if w := u.get(t, "/files/v/p/1.0.0/1.0.0.zip"); w.Code != http.StatusNotFound { + t.Errorf("status %d, want 404", w.Code) + } + u.wantMetadataRequests(t, 2) + if _, ok := u.storedPublishTime(t, "1.0.0"); !ok { + t.Error("publish time not recorded from the metadata") + } +} + +// TestComposerCachedDevVersionUsesMetadata checks that a branch, whose +// archive and publish time change with every push, is still resolved from +// the metadata on each download and records no publish time. +func TestComposerCachedDevVersionUsesMetadata(t *testing.T) { + u := setupComposerLookup(t, false, "dev-main") + u.published["dev-main"] = time.Now().Add(-30 * 24 * time.Hour) + u.download(t, "dev-main") + before := u.metadata.Load() + u.download(t, "dev-main") + if u.metadata.Load() == before { + t.Error("cached branch archive served without consulting the metadata") + } + if _, ok := u.storedPublishTime(t, "dev-main"); ok { + t.Error("publish time recorded for a branch") + } +} + +// TestComposerCachedRoutedReleaseUsesMetadata checks that a routed package +// keeps resolving every download from its route's metadata: the versions +// table is keyed by a PURL that does not say which repository the version +// came from, so it neither stores nor trusts a publish time for it. +func TestComposerCachedRoutedReleaseUsesMetadata(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + u.published["1.0.0"] = time.Now().Add(-1 * time.Hour) + u.route() + u.download(t, "1.0.0") + u.download(t, "1.0.0") + u.wantMetadataRequests(t, 2) + if _, ok := u.storedPublishTime(t, "1.0.0"); ok { + t.Error("publish time recorded for a routed package") + } + + // A time stored for the public package of the same name is not used. + if err := u.proxy.DB.SetVersionPublishedAt(canonicalVersionPURL("composer", "v/p", "1.0.0"), + canonicalPackagePURL("composer", "v/p"), time.Now().Add(-30*24*time.Hour)); err != nil { + t.Fatal(err) + } + u.proxy.Cooldown = &cooldown.Config{Default: "3d"} + if w := u.get(t, "/files/v/p/1.0.0/1.0.0.zip"); w.Code != http.StatusNotFound { + t.Errorf("routed release in cooldown: status %d, want 404", w.Code) + } +} + +// TestComposerCachedReleaseDenied checks that the denylist still blocks a +// cached release served without the metadata. +func TestComposerCachedReleaseDenied(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + u.download(t, "1.0.0") + setTestDenylist(t, u.proxy, "pkg:composer/v/p@1.0.0") + if w := u.get(t, "/files/v/p/1.0.0/1.0.0.zip"); w.Code != http.StatusForbidden { + t.Errorf("status %d, want 403", w.Code) + } +}