From 03c8e680cc8fc002fc18f045af764f0edfa5de47 Mon Sep 17 00:00:00 2001 From: simon chrzanowski Date: Wed, 14 Oct 2026 08:37:39 +0200 Subject: [PATCH] Withhold Composer archives still in cooldown Cooldown only filtered the metadata. A composer.lock names the proxy's /files URL directly, so `composer install` fetched versions still inside the cooldown window without ever seeing the filtered metadata. npm already checks the publish time on tarball downloads. - Return the version's publish time with its dist URL, following the minified format's inheritance - Answer 404 for a version in cooldown before fetching its archive - Read the download lookup's metadata through ReadMetadata, so it has the same size limit as other metadata reads --- internal/handler/composer.go | 85 +++++++++++-------- .../handler/composer_rewrite_bench_test.go | 6 +- internal/handler/composer_rewrite_test.go | 8 +- internal/handler/composer_test.go | 76 ++++++++++++++++- 4 files changed, 128 insertions(+), 47 deletions(-) diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 53d07728..5b9ab965 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/http" "net/url" "path" @@ -576,21 +575,21 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) "package", packageName, "version", version, "metadata_urls", metaURLs) - var downloadURL, distType string + var dist composerDist for _, metaURL := range metaURLs { - url, typ, err := h.findDownloadURLFromMetadata(r.Context(), metaURL, packageName, version) + found, err := h.findDownloadURLFromMetadata(r.Context(), metaURL, packageName, version) if err != nil { h.proxy.Logger.Error("failed to fetch metadata", "error", err, "url", metaURL) http.Error(w, "failed to fetch metadata", http.StatusBadGateway) return } - if url != "" { - downloadURL, distType = url, typ + if found.url != "" { + dist = found break } } - if downloadURL == "" { + if dist.url == "" { h.proxy.Logger.Debug("version not found in any metadata source", "package", packageName, "version", version, "tried_urls", metaURLs) @@ -598,6 +597,13 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) return } + // Lock files name archive URLs directly, so a version filtered out of the + // metadata still has to be withheld here. + if h.shouldFilterVersion(canonicalPackagePURL("composer", packageName), packageName, version, dist.published) { + http.Error(w, "version not found", http.StatusNotFound) + return + } + // The archive is cached under the file name from the path, so fetch the // one that name stands for. Compare it as sent, the way rewriteDist handed // it out, query included. @@ -605,8 +611,8 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) if r.URL.RawQuery != "" { requested += "?" + r.URL.RawQuery } - listedURL := downloadURL - downloadURL, ok := upstreamDistURL(listedURL, distType, requested, isDevVersion(version)) + listedURL := dist.url + downloadURL, ok := upstreamDistURL(listedURL, dist.distType, requested, isDevVersion(version)) if !ok { h.proxy.Logger.Info("composer file not in upstream metadata", "package", packageName, "version", version, @@ -696,23 +702,29 @@ func (h *ComposerHandler) metadataURLsForVersion(vendor, pkg, version string) [] return []string{stable, dev} } +// composerDist is what a version's metadata says about its archive: the dist +// URL and type, and the raw publish time the cooldown is checked against. +type composerDist struct { + url, distType string + published []byte +} + // findDownloadURLFromMetadata fetches a metadata document and returns the dist -// URL and type for the given version, or an empty URL if the version is not -// present. An error is returned only on transport failure; a missing document -// (non-200) or a missing version both yield an empty URL so the caller can -// fall back. -func (h *ComposerHandler) findDownloadURLFromMetadata(ctx context.Context, metaURL, packageName, version string) (string, string, error) { +// of the given version, with an empty URL if the version is not present. An +// error is returned only on transport failure; a missing document (non-200) +// or a missing version both yield an empty URL so the caller can fall back. +func (h *ComposerHandler) findDownloadURLFromMetadata(ctx context.Context, metaURL, packageName, version string) (composerDist, error) { h.proxy.Logger.Debug("fetching upstream metadata for download lookup", "url", metaURL, "package", packageName, "version", version) req, err := http.NewRequestWithContext(ctx, http.MethodGet, metaURL, nil) if err != nil { - return "", "", err + return composerDist{}, err } resp, err := h.proxy.HTTPClient.Do(req) if err != nil { - return "", "", err + return composerDist{}, err } defer func() { _ = resp.Body.Close() }() @@ -720,42 +732,42 @@ func (h *ComposerHandler) findDownloadURLFromMetadata(ctx context.Context, metaU "url", metaURL, "status", resp.StatusCode) if resp.StatusCode != http.StatusOK { - return "", "", nil + return composerDist{}, nil } - body, err := io.ReadAll(resp.Body) + body, err := h.proxy.ReadMetadata(resp.Body) if err != nil { - return "", "", err + return composerDist{}, err } - url, distType, err := composerDistURL(body, packageName, version) + dist, err := composerDistURL(body, packageName, version) if err != nil { - return "", "", err + return composerDist{}, err } h.proxy.Logger.Debug("download URL lookup result", "url", metaURL, "package", packageName, "version", version, - "download_url", url) - return url, distType, nil + "download_url", dist.url) + return dist, nil } -// composerDistURL returns the upstream dist URL and type of version from -// Composer metadata, or "" when the package has no such version or it has no -// dist URL. It walks the version list in place, expanding only as much of the -// minified format as it needs, since it runs on each archive the proxy has not -// cached yet. -func composerDistURL(body []byte, packageName, version string) (string, string, error) { +// composerDistURL returns the upstream dist of version from Composer +// metadata, with an empty URL when the package has no such version or it has +// no dist URL. It walks the version list in place, expanding only as much of +// the minified format as it needs, since it runs on each archive the proxy +// has not cached yet. +func composerDistURL(body []byte, packageName, version string) (composerDist, error) { format, _, err := lookupJSONString(body, "minified") if err != nil { - return "", "", err + return composerDist{}, err } versions, err := lookupJSON(body, "packages", packageName) if err != nil || len(versions) == 0 || versions[0] != '[' { - return "", "", err + return composerDist{}, err } minified := format == composerMinified fields := newComposerFields() - var url, distType string + var dist composerDist err = forEachJSONElement(versions, func(entry []byte) error { if minified && jsonStringIs(entry, composerDevReset) { fields.reset() @@ -773,10 +785,11 @@ func composerDistURL(body []byte, packageName, version string) (string, string, if !jsonStringIs(fields.get("version"), version) { return nil } - dist := fields.get("dist") - url, _, _ = lookupJSONString(dist, "url") - if url != "" { - distType, _, _ = lookupJSONString(dist, "type") + distRaw := fields.get("dist") + dist.url, _, _ = lookupJSONString(distRaw, "url") + if dist.url != "" { + dist.distType, _, _ = lookupJSONString(distRaw, "type") + dist.published = fields.get("time") return errStopScan } return nil @@ -784,7 +797,7 @@ func composerDistURL(body []byte, packageName, version string) (string, string, if errors.Is(err, errStopScan) { err = nil } - return url, distType, err + return dist, err } // proxyUpstream forwards a request to packagist.org without caching. diff --git a/internal/handler/composer_rewrite_bench_test.go b/internal/handler/composer_rewrite_bench_test.go index 9f0ab7bb..1fc8c5a7 100644 --- a/internal/handler/composer_rewrite_bench_test.go +++ b/internal/handler/composer_rewrite_bench_test.go @@ -73,9 +73,9 @@ func BenchmarkComposerFindDownloadURL(b *testing.B) { b.SetBytes(int64(len(body))) b.ReportAllocs() for b.Loop() { - url, _, err := h.findDownloadURLFromMetadata(context.Background(), srv.URL, "big/sdk", "3.2500.0") - if err != nil || url == "" { - b.Fatalf("download URL not found: %q, %v", url, err) + dist, err := h.findDownloadURLFromMetadata(context.Background(), srv.URL, "big/sdk", "3.2500.0") + if err != nil || dist.url == "" { + b.Fatalf("download URL not found: %q, %v", dist.url, err) } } } diff --git a/internal/handler/composer_rewrite_test.go b/internal/handler/composer_rewrite_test.go index c484e1aa..1eb6f7f7 100644 --- a/internal/handler/composer_rewrite_test.go +++ b/internal/handler/composer_rewrite_test.go @@ -240,19 +240,19 @@ func TestComposerDistURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, _, err := composerDistURL([]byte(tt.body), "v/p", tt.version) + got, err := composerDistURL([]byte(tt.body), "v/p", tt.version) if err != nil { t.Fatal(err) } - if got != tt.want { - t.Errorf("composerDistURL(%s) = %q, want %q", tt.version, got, tt.want) + if got.url != tt.want { + t.Errorf("composerDistURL(%s) = %q, want %q", tt.version, got.url, tt.want) } }) } } func TestComposerDistURLMalformed(t *testing.T) { - if _, _, err := composerDistURL([]byte(`{"packages":{"v/p":[{"version":`), "v/p", "1.0.0"); err == nil { + if _, err := composerDistURL([]byte(`{"packages":{"v/p":[{"version":`), "v/p", "1.0.0"); err == nil { t.Error("expected an error for truncated JSON") } } diff --git a/internal/handler/composer_test.go b/internal/handler/composer_test.go index be672cca..e6a99626 100644 --- a/internal/handler/composer_test.go +++ b/internal/handler/composer_test.go @@ -494,13 +494,13 @@ func TestComposerDownloadDevVersionUsesDevMetadata(t *testing.T) { // OLD behavior: fetching only the regular file fails to resolve the dev // version, which is what produced the 404 before the fix. stableURL := srv.URL + "/p2/phpmd/phpmd.json" - got, _, err := h.findDownloadURLFromMetadata(ctx, stableURL, pkg, version) + found, err := h.findDownloadURLFromMetadata(ctx, stableURL, pkg, version) if err != nil { t.Fatalf("unexpected error fetching regular metadata: %v", err) } - if got != "" { + if found.url != "" { t.Fatalf("regular metadata unexpectedly contained dev version %q (got %q); "+ - "the test no longer reproduces the original bug", version, got) + "the test no longer reproduces the original bug", version, found.url) } // FIXED behavior: the handler consults the ~dev file (it is first in the @@ -512,10 +512,11 @@ func TestComposerDownloadDevVersionUsesDevMetadata(t *testing.T) { var resolved string for _, u := range urls { - resolved, _, err = h.findDownloadURLFromMetadata(ctx, u, pkg, version) + found, err := h.findDownloadURLFromMetadata(ctx, u, pkg, version) if err != nil { t.Fatalf("unexpected error fetching metadata %q: %v", u, err) } + resolved = found.url if resolved != "" { break } @@ -1007,3 +1008,70 @@ func TestComposerRetentionKey(t *testing.T) { t.Error("composerRetentionKey(nil) accepted") } } + +// TestComposerDownloadCooldown checks that an archive still in cooldown is +// withheld when a lock file requests it directly, without the metadata that +// would have filtered it out. In minified metadata a version inherits the +// time of the entry before it. +func TestComposerDownloadCooldown(t *testing.T) { + now := time.Now() + old := now.Add(-10 * 24 * time.Hour).Format(time.RFC3339) + recent := now.Add(-1 * time.Hour).Format(time.RFC3339) + + var archiveRequests atomic.Int32 + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + dist := func(version string) string { + return fmt.Sprintf(`"dist":{"url":%q,"type":"zip"}`, srv.URL+"/archives/"+version+".zip") + } + switch r.URL.Path { + case "/p2/v/p.json": + _, _ = fmt.Fprintf(w, `{"minified":"composer/2.0","packages":{"v/p":[`+ + `{"version":"2.0.0","time":%q,%s},`+ + `{"version":"1.9.0",%s},`+ + `{"version":"1.0.0","time":%q,%s}]}}`, + recent, dist("2.0.0"), dist("1.9.0"), old, dist("1.0.0")) + case "/p2/v/p~dev.json": + http.NotFound(w, r) + default: + archiveRequests.Add(1) + _, _ = io.WriteString(w, "archive "+r.URL.Path) + } + })) + t.Cleanup(srv.Close) + + proxy, _, _, _ := setupTestProxy(t) + proxy.Cooldown = &cooldown.Config{Default: "3d"} + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(srv.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + h := NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL).Routes() + + tests := []struct { + version string + wantStatus int + }{ + {"2.0.0", http.StatusNotFound}, + {"1.9.0", http.StatusNotFound}, + {"1.0.0", http.StatusOK}, + } + for _, tt := range tests { + t.Run(tt.version, func(t *testing.T) { + archiveRequests.Store(0) + req := httptest.NewRequest(http.MethodGet, "/files/v/p/"+tt.version+"/"+tt.version+".zip", nil) + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + + if w.Code != tt.wantStatus { + t.Fatalf("status = %d, want %d (body %q)", w.Code, tt.wantStatus, w.Body.String()) + } + wantRequests := int32(0) + if tt.wantStatus == http.StatusOK { + wantRequests = 1 + } + if got := archiveRequests.Load(); got != wantRequests { + t.Errorf("upstream archive requests = %d, want %d", got, wantRequests) + } + }) + } +}