Withhold Composer archives still in cooldown

internal/handler/composer.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"io"
 	"net/http"
 	"net/url"
 	"path"
@@ -576,37 +575,44 @@ 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)
 		http.Error(w, "version not found", http.StatusNotFound)
 		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.
 	requested := path.Base(r.URL.EscapedPath())
 	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,66 +702,72 @@ 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() }()
 
 	h.proxy.Logger.Debug("upstream metadata response",
 		"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,18 +785,19 @@ 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
 	})
 	if errors.Is(err, errStopScan) {
 		err = nil
 	}
-	return url, distType, err
+	return dist, err
 }
 
 // proxyUpstream forwards a request to packagist.org without caching.

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)
 		}
 	}
 }

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")
 	}
 }

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)
+			}
+		})
+	}
+}