diff --git a/internal/handler/composer.go b/internal/handler/composer.go index da7a957..5135915 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,36 @@ 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 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. + if bare := distFilename(listedURL, ""); listed != bare && path.Ext(filename) == "" { + filename += listed[len(bare):] + } + if filename == listed { + return listedURL, true + } + ext := path.Ext(listed) + commit := strings.TrimSuffix(listed, ext) + requested, ok := strings.CutSuffix(filename, ext) + if !branch || !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 +483,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 +505,24 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) 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. 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 + } + listedURL := downloadURL + downloadURL, ok := upstreamDistURL(listedURL, distType, requested, isDevVersion(version)) + 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 +559,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 +582,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 +635,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 +646,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 dd1ba57..9f0ab7b 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 9d2bb51..c484e1a 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 f6f077c..f47550e 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,185 @@ 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 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.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()] + 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"}, + {"github zipball without .zip", "/repos/v/p/zipball/%s", "%s"}, + {"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"}, + } + 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") + } + }) + } +} + +// 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) + 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}, + {"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", ""}, + {"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, 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) { now := time.Now() old := now.Add(-10 * 24 * time.Hour).Format(time.RFC3339)