From 6013e105d633561a802510a03df2dd69d84587d0 Mon Sep 17 00:00:00 2001 From: Christian Heim Date: Sun, 11 Oct 2026 01:01:47 +0200 Subject: [PATCH 1/4] Fetch the pinned commit for Composer branch downloads The download handler resolved a version's dist URL from the current metadata and cached the archive under whatever file name the client asked for. A lock file pinning an older commit of a branch like dev-master got the branch head, stored and served as that older commit. Downloads now fetch the commit the file name stands for when the dist URL ends in a commit hash (GitHub zipballs, Bitbucket archives), and return 404 for other names the metadata no longer lists. --- internal/handler/composer.go | 106 ++++++++---- .../handler/composer_rewrite_bench_test.go | 2 +- internal/handler/composer_rewrite_test.go | 4 +- internal/handler/composer_test.go | 153 +++++++++++++++++- 4 files changed, 231 insertions(+), 34 deletions(-) diff --git a/internal/handler/composer.go b/internal/handler/composer.go index da7a9577..8c1947b2 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "path" + "regexp" "strings" "time" ) @@ -329,7 +330,17 @@ func (h *ComposerHandler) proxyDistURL(packageName, version, upstreamURL, distTy if upstreamURL == "" { return "", false } + parts := strings.SplitN(packageName, "/", vendorPackageParts) + if len(parts) != vendorPackageParts { + return "", false + } + return fmt.Sprintf("%s/composer/files/%s/%s/%s/%s", + h.proxyURL, parts[0], parts[1], version, distFilename(upstreamURL, distType)), true +} +// distFilename returns the file name this proxy serves an archive under: the +// last segment of its upstream URL. +func distFilename(upstreamURL, distType string) string { filename := "package.zip" if idx := strings.LastIndex(upstreamURL, "/"); idx >= 0 { filename = upstreamURL[idx+1:] @@ -340,13 +351,31 @@ func (h *ComposerHandler) proxyDistURL(packageName, version, upstreamURL, distTy if path.Ext(filename) == "" && distType == "zip" { filename += ".zip" } + return filename +} - parts := strings.SplitN(packageName, "/", vendorPackageParts) - if len(parts) != vendorPackageParts { +// composerCommitPattern matches a full git commit hash, SHA-1 or SHA-256. +var composerCommitPattern = regexp.MustCompile(`^([0-9a-f]{40}|[0-9a-f]{64})$`) + +// upstreamDistURL returns the upstream URL of the archive served as filename, +// given the dist URL the version's metadata lists now. A branch version only +// lists its current head while lock files pin older commits, so a dist URL +// ending in a commit hash is rebuilt for the requested commit. Any other name +// gives false: fetching the listed archive would cache it under a name it +// does not have. +func upstreamDistURL(listedURL, distType, filename string) (string, bool) { + listed := distFilename(listedURL, distType) + if filename == listed { + return listedURL, true + } + ext := path.Ext(listed) + commit := strings.TrimSuffix(listed, ext) + requested, ok := strings.CutSuffix(filename, ext) + if !ok || !composerCommitPattern.MatchString(commit) || !composerCommitPattern.MatchString(requested) { return "", false } - return fmt.Sprintf("%s/composer/files/%s/%s/%s/%s", - h.proxyURL, parts[0], parts[1], version, filename), true + idx := strings.LastIndex(listedURL, commit) + return listedURL[:idx] + requested + listedURL[idx+len(commit):], true } // composerFields holds one version's fields as Composer sees them after @@ -449,16 +478,16 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) "package", packageName, "version", version, "metadata_urls", metaURLs) - var downloadURL string + var downloadURL, distType string for _, metaURL := range metaURLs { - url, err := h.findDownloadURLFromMetadata(r.Context(), metaURL, packageName, version) + url, typ, 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 = url + downloadURL, distType = url, typ break } } @@ -471,6 +500,22 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) return } + // The archive is cached under the requested name, so fetch the one that + // name stands for. Compare it as sent, the way rewriteDist handed it out. + requested := path.Base(r.URL.EscapedPath()) + if r.URL.RawQuery != "" { + requested += "?" + r.URL.RawQuery + } + listedURL := downloadURL + downloadURL, ok := upstreamDistURL(listedURL, distType, requested) + if !ok { + h.proxy.Logger.Info("composer file not in upstream metadata", + "package", packageName, "version", version, + "filename", requested, "listed_url", listedURL) + http.Error(w, "file not found", http.StatusNotFound) + return + } + h.proxy.Logger.Debug("resolved download URL", "package", packageName, "version", version, "download_url", downloadURL) @@ -507,21 +552,22 @@ func (h *ComposerHandler) metadataURLsForVersion(vendor, pkg, version string) [] } // findDownloadURLFromMetadata fetches a metadata document and returns the dist -// URL for the given version, or an empty string 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 string so the caller can fall back. -func (h *ComposerHandler) findDownloadURLFromMetadata(ctx context.Context, metaURL, packageName, version string) (string, error) { +// 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) { 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 "", "", err } resp, err := h.proxy.HTTPClient.Do(req) if err != nil { - return "", err + return "", "", err } defer func() { _ = resp.Body.Close() }() @@ -529,42 +575,42 @@ func (h *ComposerHandler) findDownloadURLFromMetadata(ctx context.Context, metaU "url", metaURL, "status", resp.StatusCode) if resp.StatusCode != http.StatusOK { - return "", nil + return "", "", nil } body, err := io.ReadAll(resp.Body) if err != nil { - return "", err + return "", "", err } - url, err := composerDistURL(body, packageName, version) + url, distType, err := composerDistURL(body, packageName, version) if err != nil { - return "", err + return "", "", err } h.proxy.Logger.Debug("download URL lookup result", "url", metaURL, "package", packageName, "version", version, "download_url", url) - return url, nil + return url, distType, nil } -// composerDistURL returns the upstream dist URL 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, error) { +// 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) { format, _, err := lookupJSONString(body, "minified") if err != nil { - return "", err + return "", "", err } versions, err := lookupJSON(body, "packages", packageName) if err != nil || len(versions) == 0 || versions[0] != '[' { - return "", err + return "", "", err } minified := format == composerMinified fields := newComposerFields() - var url string + var url, distType string err = forEachJSONElement(versions, func(entry []byte) error { if minified && jsonStringIs(entry, composerDevReset) { fields.reset() @@ -582,8 +628,10 @@ func composerDistURL(body []byte, packageName, version string) (string, error) { if !jsonStringIs(fields.get("version"), version) { return nil } - url, _, _ = lookupJSONString(fields.get("dist"), "url") + dist := fields.get("dist") + url, _, _ = lookupJSONString(dist, "url") if url != "" { + distType, _, _ = lookupJSONString(dist, "type") return errStopScan } return nil @@ -591,7 +639,7 @@ func composerDistURL(body []byte, packageName, version string) (string, error) { if errors.Is(err, errStopScan) { err = nil } - return url, err + return url, distType, 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 dd1ba577..9f0ab7bb 100644 --- a/internal/handler/composer_rewrite_bench_test.go +++ b/internal/handler/composer_rewrite_bench_test.go @@ -73,7 +73,7 @@ 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") + 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) } diff --git a/internal/handler/composer_rewrite_test.go b/internal/handler/composer_rewrite_test.go index 9d2bb512..c484e1aa 100644 --- a/internal/handler/composer_rewrite_test.go +++ b/internal/handler/composer_rewrite_test.go @@ -240,7 +240,7 @@ 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) } @@ -252,7 +252,7 @@ func TestComposerDistURL(t *testing.T) { } 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 f6f077c9..872de663 100644 --- a/internal/handler/composer_test.go +++ b/internal/handler/composer_test.go @@ -3,6 +3,8 @@ package handler import ( "context" "encoding/json" + "fmt" + "io" "log/slog" "net/http" "net/http/httptest" @@ -11,6 +13,7 @@ import ( "time" "github.com/git-pkgs/cooldown" + "github.com/git-pkgs/registries/fetch" ) func TestComposerRewriteMetadata(t *testing.T) { @@ -489,7 +492,7 @@ 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) + got, _, err := h.findDownloadURLFromMetadata(ctx, stableURL, pkg, version) if err != nil { t.Fatalf("unexpected error fetching regular metadata: %v", err) } @@ -507,7 +510,7 @@ func TestComposerDownloadDevVersionUsesDevMetadata(t *testing.T) { var resolved string for _, u := range urls { - resolved, err = h.findDownloadURLFromMetadata(ctx, u, pkg, version) + resolved, _, err = h.findDownloadURLFromMetadata(ctx, u, pkg, version) if err != nil { t.Fatalf("unexpected error fetching metadata %q: %v", u, err) } @@ -521,6 +524,152 @@ func TestComposerDownloadDevVersionUsesDevMetadata(t *testing.T) { } } +const ( + composerCommitA = "1111111111111111111111111111111111111111" + composerCommitB = "2222222222222222222222222222222222222222" +) + +// setupComposerBranch returns a caching Composer handler whose upstream lists +// distPath as the dist of v/p's dev-master and serves archives by request URI. +func setupComposerBranch(t *testing.T, distPath string, archives map[string]string) (http.Handler, *Proxy) { + t.Helper() + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/p2/v/p~dev.json" { + _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":"dev-master","dist":{"url":%q,"type":"zip"}}]}}`, + srv.URL+distPath) + return + } + body, ok := archives[r.URL.RequestURI()] + if !ok { + http.NotFound(w, r) + return + } + _, _ = io.WriteString(w, body) + })) + t.Cleanup(srv.Close) + + proxy, _, _, _ := setupTestProxy(t) + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(srv.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + return NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL).Routes(), proxy +} + +// cachedComposerFile returns the cached bytes of v/p dev-master's filename, +// or "" when nothing is cached under it. +func cachedComposerFile(t *testing.T, proxy *Proxy, filename string) string { + t.Helper() + cached, err := proxy.GetCachedArtifact(context.Background(), "composer", "v/p", "dev-master", filename) + if err != nil { + t.Fatalf("reading cache for %s: %v", filename, err) + } + if cached == nil { + return "" + } + defer func() { _ = cached.Reader.Close() }() + data, err := io.ReadAll(cached.Reader) + if err != nil { + t.Fatalf("reading cached %s: %v", filename, err) + } + return string(data) +} + +// TestComposerDownloadPinnedBranchCommit checks that a lock file pinning an +// older commit of a branch gets that commit rather than the head the branch's +// metadata lists now, and that the head is not cached under the older name. +func TestComposerDownloadPinnedBranchCommit(t *testing.T) { + tests := []struct { + name, distPath, filename string + }{ + {"github zipball", "/repos/v/p/zipball/%s", "%s.zip"}, + {"bitbucket archive", "/v/p/get/%s.zip", "%s.zip"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h, proxy := setupComposerBranch(t, fmt.Sprintf(tt.distPath, composerCommitB), map[string]string{ + fmt.Sprintf(tt.distPath, composerCommitA): "commit A", + fmt.Sprintf(tt.distPath, composerCommitB): "commit B", + }) + for _, c := range []struct{ commit, want string }{ + {composerCommitA, "commit A"}, + {composerCommitB, "commit B"}, + } { + filename, want := fmt.Sprintf(tt.filename, c.commit), c.want + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+filename, nil)) + if w.Code != http.StatusOK || w.Body.String() != want { + t.Errorf("%s: status %d, body %q, want 200 %q", filename, w.Code, w.Body.String(), want) + } + if got := cachedComposerFile(t, proxy, filename); got != want { + t.Errorf("%s cached as %q, want %q", filename, got, want) + } + } + }) + } +} + +// TestComposerDownloadUnlistedFile checks that a file the metadata no longer +// lists, and that names no commit to fetch instead, is a 404 rather than the +// listed archive cached under the requested name. +func TestComposerDownloadUnlistedFile(t *testing.T) { + tests := []struct { + name, distPath, listed, stale string + }{ + {"plain name", "/dist/main.zip", "main.zip", "old.zip"}, + {"escaped name", "/dist/p%2Bdev-main.zip", "p%2Bdev-main.zip", "old.zip"}, + {"gitlab archive", "/api/v4/projects/v%2Fp/repository/archive.zip?sha=" + composerCommitB, + "archive.zip?sha=" + composerCommitB, "archive.zip?sha=" + composerCommitA}, + {"branch name for a commit", "/repos/v/p/zipball/" + composerCommitB, composerCommitB + ".zip", "master.zip"}, + {"commit without zip extension", "/repos/v/p/zipball/" + composerCommitB, composerCommitB + ".zip", composerCommitA}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h, proxy := setupComposerBranch(t, tt.distPath, map[string]string{tt.distPath: "head"}) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+tt.stale, nil)) + if w.Code != http.StatusNotFound { + t.Errorf("%s: status %d, body %q, want 404", tt.stale, w.Code, w.Body.String()) + } + staleName, _, _ := strings.Cut(tt.stale, "?") + if got := cachedComposerFile(t, proxy, staleName); got != "" { + t.Errorf("%s cached as %q, want nothing", staleName, got) + } + + w = httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+tt.listed, nil)) + if w.Code != http.StatusOK || w.Body.String() != "head" { + t.Errorf("%s: status %d, body %q, want 200 %q", tt.listed, w.Code, w.Body.String(), "head") + } + }) + } +} + +func TestComposerUpstreamDistURL(t *testing.T) { + const zipball = "https://api.github.com/repos/v/p/zipball/" + sha256A := strings.Repeat("a", 64) + tests := []struct { + name, listed, distType, filename, want string + }{ + {"listed file", zipball + composerCommitB, "zip", composerCommitB + ".zip", zipball + composerCommitB}, + {"older commit", zipball + composerCommitB, "zip", composerCommitA + ".zip", zipball + composerCommitA}, + {"sha256 commit", zipball + composerCommitB, "zip", sha256A + ".zip", zipball + sha256A}, + {"tar dist keeps bare name", zipball + composerCommitB, "tar", composerCommitA, zipball + composerCommitA}, + {"short hash", zipball + composerCommitB, "zip", "1111111.zip", ""}, + {"upper-case hash", zipball + composerCommitB, "zip", strings.ToUpper(sha256A) + ".zip", ""}, + {"other name", "https://example.com/dist/main.zip", "zip", "old.zip", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := upstreamDistURL(tt.listed, tt.distType, tt.filename) + if got != tt.want || ok != (tt.want != "") { + t.Errorf("upstreamDistURL(%q, %q) = %q, %v; want %q", tt.listed, tt.filename, got, ok, tt.want) + } + }) + } +} + func TestComposerRewriteMetadataCooldown(t *testing.T) { now := time.Now() old := now.Add(-10 * 24 * time.Hour).Format(time.RFC3339) From d124ed38747326abb1924c7619a7ca26de4d31cd Mon Sep 17 00:00:00 2001 From: Christian Heim Date: Sun, 11 Oct 2026 01:17:51 +0200 Subject: [PATCH 2/4] Accept bare commit names from older Composer lock files Proxy versions before the .zip suffix handed out GitHub zipball URLs ending in the bare commit hash. Lock files written back then still ask for those names, and the new file name check turned them into 404s. --- internal/handler/composer.go | 5 +++++ internal/handler/composer_test.go | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 8c1947b2..a9500d35 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -365,6 +365,11 @@ var composerCommitPattern = regexp.MustCompile(`^([0-9a-f]{40}|[0-9a-f]{64})$`) // does not have. func upstreamDistURL(listedURL, distType, filename string) (string, bool) { listed := distFilename(listedURL, distType) + // Proxy versions before the .zip suffix handed out bare commit names, + // and lock files written back then still ask for them. + if bare := distFilename(listedURL, ""); listed != bare && path.Ext(filename) == "" { + filename += listed[len(bare):] + } if filename == listed { return listedURL, true } diff --git a/internal/handler/composer_test.go b/internal/handler/composer_test.go index 872de663..3fa26c4b 100644 --- a/internal/handler/composer_test.go +++ b/internal/handler/composer_test.go @@ -583,6 +583,7 @@ func TestComposerDownloadPinnedBranchCommit(t *testing.T) { name, distPath, filename string }{ {"github zipball", "/repos/v/p/zipball/%s", "%s.zip"}, + {"github zipball without .zip", "/repos/v/p/zipball/%s", "%s"}, {"bitbucket archive", "/v/p/get/%s.zip", "%s.zip"}, } for _, tt := range tests { @@ -621,7 +622,6 @@ func TestComposerDownloadUnlistedFile(t *testing.T) { {"gitlab archive", "/api/v4/projects/v%2Fp/repository/archive.zip?sha=" + composerCommitB, "archive.zip?sha=" + composerCommitB, "archive.zip?sha=" + composerCommitA}, {"branch name for a commit", "/repos/v/p/zipball/" + composerCommitB, composerCommitB + ".zip", "master.zip"}, - {"commit without zip extension", "/repos/v/p/zipball/" + composerCommitB, composerCommitB + ".zip", composerCommitA}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -655,6 +655,8 @@ func TestComposerUpstreamDistURL(t *testing.T) { {"listed file", zipball + composerCommitB, "zip", composerCommitB + ".zip", zipball + composerCommitB}, {"older commit", zipball + composerCommitB, "zip", composerCommitA + ".zip", zipball + composerCommitA}, {"sha256 commit", zipball + composerCommitB, "zip", sha256A + ".zip", zipball + sha256A}, + {"bare listed commit", zipball + composerCommitB, "zip", composerCommitB, zipball + composerCommitB}, + {"bare older commit", zipball + composerCommitB, "zip", composerCommitA, zipball + composerCommitA}, {"tar dist keeps bare name", zipball + composerCommitB, "tar", composerCommitA, zipball + composerCommitA}, {"short hash", zipball + composerCommitB, "zip", "1111111.zip", ""}, {"upper-case hash", zipball + composerCommitB, "zip", strings.ToUpper(sha256A) + ".zip", ""}, From 12f7711c741847e335a4d80a8879f0b35c1b0317 Mon Sep 17 00:00:00 2001 From: Christian Heim Date: Sun, 11 Oct 2026 01:32:32 +0200 Subject: [PATCH 3/4] Rebuild Composer commit URLs only for branch versions A tagged release lists one commit and lock files ask for that one. Rebuilding the URL for any requested commit let a client store another commit, even one from a fork, as the release, and the browse view could then show it. --- internal/handler/composer.go | 14 +++++------ internal/handler/composer_test.go | 41 +++++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/internal/handler/composer.go b/internal/handler/composer.go index a9500d35..32549f2f 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -359,11 +359,11 @@ var composerCommitPattern = regexp.MustCompile(`^([0-9a-f]{40}|[0-9a-f]{64})$`) // upstreamDistURL returns the upstream URL of the archive served as filename, // given the dist URL the version's metadata lists now. A branch version only -// lists its current head while lock files pin older commits, so a dist URL -// ending in a commit hash is rebuilt for the requested commit. Any other name -// gives false: fetching the listed archive would cache it under a name it -// does not have. -func upstreamDistURL(listedURL, distType, filename string) (string, bool) { +// lists its current head while lock files pin older commits, so for a branch +// a dist URL ending in a commit hash is rebuilt for the requested commit. A +// tagged release has to ask for the listed name. Any other name gives false: +// fetching the listed archive would cache it under a name it does not have. +func upstreamDistURL(listedURL, distType, filename string, branch bool) (string, bool) { listed := distFilename(listedURL, distType) // Proxy versions before the .zip suffix handed out bare commit names, // and lock files written back then still ask for them. @@ -376,7 +376,7 @@ func upstreamDistURL(listedURL, distType, filename string) (string, bool) { ext := path.Ext(listed) commit := strings.TrimSuffix(listed, ext) requested, ok := strings.CutSuffix(filename, ext) - if !ok || !composerCommitPattern.MatchString(commit) || !composerCommitPattern.MatchString(requested) { + if !branch || !ok || !composerCommitPattern.MatchString(commit) || !composerCommitPattern.MatchString(requested) { return "", false } idx := strings.LastIndex(listedURL, commit) @@ -512,7 +512,7 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) requested += "?" + r.URL.RawQuery } listedURL := downloadURL - downloadURL, ok := upstreamDistURL(listedURL, distType, requested) + downloadURL, ok := upstreamDistURL(listedURL, distType, requested, isDevVersion(version)) if !ok { h.proxy.Logger.Info("composer file not in upstream metadata", "package", packageName, "version", version, diff --git a/internal/handler/composer_test.go b/internal/handler/composer_test.go index 3fa26c4b..f47550e3 100644 --- a/internal/handler/composer_test.go +++ b/internal/handler/composer_test.go @@ -530,14 +530,15 @@ const ( ) // setupComposerBranch returns a caching Composer handler whose upstream lists -// distPath as the dist of v/p's dev-master and serves archives by request URI. +// distPath as the dist of v/p's dev-master and 1.0.0 and serves archives by +// request URI. func setupComposerBranch(t *testing.T, distPath string, archives map[string]string) (http.Handler, *Proxy) { t.Helper() var srv *httptest.Server srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/p2/v/p~dev.json" { - _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":"dev-master","dist":{"url":%q,"type":"zip"}}]}}`, - srv.URL+distPath) + if r.URL.Path == "/p2/v/p.json" || r.URL.Path == "/p2/v/p~dev.json" { + dist := fmt.Sprintf(`"dist":{"url":%q,"type":"zip"}`, srv.URL+distPath) + _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":"dev-master",%s},{"version":"1.0.0",%s}]}}`, dist, dist) return } body, ok := archives[r.URL.RequestURI()] @@ -646,6 +647,33 @@ func TestComposerDownloadUnlistedFile(t *testing.T) { } } +// TestComposerDownloadTaggedReleaseCommit checks that a tagged release is only +// served as the commit its metadata lists, so a client cannot have another +// commit stored as that release. +func TestComposerDownloadTaggedReleaseCommit(t *testing.T) { + zipball := "/repos/v/p/zipball/" + h, proxy := setupComposerBranch(t, zipball+composerCommitB, map[string]string{ + zipball + composerCommitA: "commit A", + zipball + composerCommitB: "commit B", + }) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/1.0.0/"+composerCommitA+".zip", nil)) + if w.Code != http.StatusNotFound { + t.Errorf("other commit: status %d, body %q, want 404", w.Code, w.Body.String()) + } + cached, err := proxy.GetCachedArtifact(context.Background(), "composer", "v/p", "1.0.0", composerCommitA+".zip") + if err != nil || cached != nil { + t.Errorf("other commit cached as 1.0.0: %v, %v", cached, err) + } + + w = httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/1.0.0/"+composerCommitB+".zip", nil)) + if w.Code != http.StatusOK || w.Body.String() != "commit B" { + t.Errorf("listed commit: status %d, body %q, want 200 %q", w.Code, w.Body.String(), "commit B") + } +} + func TestComposerUpstreamDistURL(t *testing.T) { const zipball = "https://api.github.com/repos/v/p/zipball/" sha256A := strings.Repeat("a", 64) @@ -664,12 +692,15 @@ func TestComposerUpstreamDistURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, ok := upstreamDistURL(tt.listed, tt.distType, tt.filename) + got, ok := upstreamDistURL(tt.listed, tt.distType, tt.filename, true) if got != tt.want || ok != (tt.want != "") { t.Errorf("upstreamDistURL(%q, %q) = %q, %v; want %q", tt.listed, tt.filename, got, ok, tt.want) } }) } + if got, ok := upstreamDistURL(zipball+composerCommitB, "zip", composerCommitA+".zip", false); ok { + t.Errorf("tagged release rebuilt to %q, want only the listed archive", got) + } } func TestComposerRewriteMetadataCooldown(t *testing.T) { From 5b3d265b326736cd82d6fe8eacd01c37d2bcfde6 Mon Sep 17 00:00:00 2001 From: Christian Heim Date: Sun, 11 Oct 2026 01:32:44 +0200 Subject: [PATCH 4/4] Note that the Composer cache key leaves out the query --- internal/handler/composer.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 32549f2f..5135915b 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -505,8 +505,10 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) return } - // The archive is cached under the requested name, so fetch the one that - // name stands for. Compare it as sent, the way rewriteDist handed it out. + // 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. A query such as GitLab's ?sha= is compared but is not part of + // the cache key. requested := path.Base(r.URL.EscapedPath()) if r.URL.RawQuery != "" { requested += "?" + r.URL.RawQuery