diff --git a/internal/database/queries.go b/internal/database/queries.go index 95030224..963ff30b 100644 --- a/internal/database/queries.go +++ b/internal/database/queries.go @@ -359,6 +359,7 @@ func (db *DB) upsertArtifactFrom(a *Artifact, previous sql.NullString) (bool, er created_at, updated_at) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) ON CONFLICT(version_purl, filename) DO UPDATE SET + upstream_url = EXCLUDED.upstream_url, storage_path = EXCLUDED.storage_path, content_hash = EXCLUDED.content_hash, size = EXCLUDED.size, @@ -374,6 +375,7 @@ func (db *DB) upsertArtifactFrom(a *Artifact, previous sql.NullString) (bool, er created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(version_purl, filename) DO UPDATE SET + upstream_url = excluded.upstream_url, storage_path = excluded.storage_path, content_hash = excluded.content_hash, size = excluded.size, diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 182fe7ab..f7b40ae8 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net/http" + "net/url" "path" "regexp" "strings" @@ -372,7 +373,7 @@ func (h *ComposerHandler) proxyDistURL(packageName, version, upstreamURL, distTy return "", false } return fmt.Sprintf("%s/composer/files/%s/%s/%s/%s", - h.proxyURL, parts[0], parts[1], version, distFilename(upstreamURL, distType)), true + h.proxyURL, parts[0], parts[1], url.PathEscape(version), distFilename(upstreamURL, distType)), true } // distFilename returns the file name this proxy serves an archive under: the @@ -410,6 +411,16 @@ func upstreamDistURL(listedURL, distType, filename string, branch bool) (string, if filename == listed { return listedURL, true } + if commit, ok := shaQueryCommit(listed); ok { + requested, ok := shaQueryCommit(filename) + listedSha, requestedSha := "sha="+commit, "sha="+requested + idx := strings.LastIndex(listed, listedSha) + if !branch || !ok || idx < 0 || listed[:idx]+requestedSha+listed[idx+len(listedSha):] != filename { + return "", false + } + idx = strings.LastIndex(listedURL, listedSha) + return listedURL[:idx] + requestedSha + listedURL[idx+len(listedSha):], true + } ext := path.Ext(listed) commit := strings.TrimSuffix(listed, ext) requested, ok := strings.CutSuffix(filename, ext) @@ -420,6 +431,22 @@ func upstreamDistURL(listedURL, distType, filename string, branch bool) (string, return listedURL[:idx] + requested + listedURL[idx+len(commit):], true } +// shaQueryCommit returns the commit an archive name passes in a sha query +// parameter. GitLab names the archive of every commit archive.zip and picks +// the commit that way. +func shaQueryCommit(filename string) (string, bool) { + _, query, ok := strings.Cut(filename, "?") + if !ok { + return "", false + } + values, err := url.ParseQuery(query) + if err != nil { + return "", false + } + commit := values.Get("sha") + return commit, composerCommitPattern.MatchString(commit) +} + // composerFields holds one version's fields as Composer sees them after // expanding the minified format: raw values under their decoded names, in the // order the names first appeared. A removed field keeps its slot with a nil @@ -544,8 +571,7 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) // 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. + // it out, query included. requested := path.Base(r.URL.EscapedPath()) if r.URL.RawQuery != "" { requested += "?" + r.URL.RawQuery @@ -564,16 +590,60 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) "package", packageName, "version", version, "download_url", downloadURL) + // Archives picked by ?sha= share one file name, so the commit goes into + // the name they are cached under. Proxy versions before that cached every + // commit under the shared name, which is dropped once a commit is fetched. + var sharedFilename string + if commit, ok := shaQueryCommit(requested); ok { + sharedFilename = h.routes.cacheFilename(packageName, filename) + ext := path.Ext(filename) + filename = strings.TrimSuffix(filename, ext) + "-" + commit + ext + } cacheFilename := h.routes.cacheFilename(packageName, filename) + if commit := strings.TrimSuffix(filename, path.Ext(filename)); isDevVersion(version) && composerCommitPattern.MatchString(commit) { + h.discardOtherCommit(packageName, version, cacheFilename, commit) + } result, err := h.proxy.GetOrFetchArtifactFromURL(r.Context(), "composer", packageName, version, cacheFilename, downloadURL) if err != nil { h.proxy.serveArtifactError(w, err, "failed to fetch package") return } + if sharedFilename != "" && !result.Cached { + if err := h.proxy.ClearCachedArtifact("composer", packageName, version, sharedFilename); err != nil { + h.proxy.Logger.Warn("failed to discard composer archive cached under a shared name", + "package", packageName, "version", version, "filename", sharedFilename, "error", err) + } + } ServeArtifactRequest(w, r, result) } +// 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 +// record is only cleared while it still points at the object checked here. +func (h *ComposerHandler) discardOtherCommit(packageName, version, cacheFilename, commit string) { + _, versionPURL, err := packagePURLStrings("composer", packageName, version) + if err != nil { + return + } + record, err := h.proxy.DB.GetArtifact(versionPURL, cacheFilename) + if err != nil { + h.proxy.Logger.Warn("failed to read composer cache record", + "purl", versionPURL, "filename", cacheFilename, "error", err) + return + } + if record == nil || !record.StoragePath.Valid || strings.Contains(record.UpstreamURL, commit) { + return + } + h.proxy.Logger.Warn("cached composer archive is from another commit, discarding", + "purl", versionPURL, "filename", cacheFilename, "upstream_url", record.UpstreamURL) + if err := h.proxy.DB.DiscardArtifact(versionPURL, cacheFilename, record.StoragePath.String); err != nil { + h.proxy.Logger.Warn("failed to discard composer cache record", + "purl", versionPURL, "filename", cacheFilename, "error", err) + } +} + // isDevVersion reports whether a Composer version string refers to a // development (unstable, branch) version rather than a tagged release. // Composer formats these as either "dev-" (e.g. "dev-master") or diff --git a/internal/handler/composer_test.go b/internal/handler/composer_test.go index f47550e3..ad3eb7ac 100644 --- a/internal/handler/composer_test.go +++ b/internal/handler/composer_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -579,13 +580,15 @@ func cachedComposerFile(t *testing.T, proxy *Proxy, filename string) string { // 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. +// The second round is served from the cache. func TestComposerDownloadPinnedBranchCommit(t *testing.T) { tests := []struct { - name, distPath, filename string + name, distPath, filename, cached 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"}, + {"github zipball", "/repos/v/p/zipball/%s", "%s.zip", "%s.zip"}, + {"github zipball without .zip", "/repos/v/p/zipball/%s", "%s", "%s"}, + {"bitbucket archive", "/v/p/get/%s.zip", "%s.zip", "%s.zip"}, + {"gitlab archive", "/api/v4/projects/v%%2Fp/repository/archive.zip?sha=%s", "archive.zip?sha=%s", "archive-%s.zip"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -593,24 +596,238 @@ func TestComposerDownloadPinnedBranchCommit(t *testing.T) { 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 + for range 2 { + 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) + } + cached := fmt.Sprintf(tt.cached, c.commit) + if got := cachedComposerFile(t, proxy, cached); got != want { + t.Errorf("%s cached as %q, want %q", cached, got, want) + } + } + } + }) + } +} + +// composerMovingBranch is an upstream whose dev-master head can move. It +// serves the archives of commits A and B under distPath and counts how often +// they are fetched. +type composerMovingBranch struct { + h http.Handler + proxy *Proxy + url string + head atomic.Value + fetches atomic.Int32 +} + +func setupComposerMovingBranch(t *testing.T, distPath string) *composerMovingBranch { + t.Helper() + b := &composerMovingBranch{} + b.head.Store(composerCommitA) + archives := map[string]string{ + fmt.Sprintf(distPath, composerCommitA): "commit A", + fmt.Sprintf(distPath, composerCommitB): "commit B", + } + 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 := b.url + fmt.Sprintf(distPath, b.head.Load()) + _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":"dev-master","dist":{"url":%q,"type":"zip"}}]}}`, dist) + return + } + body, ok := archives[r.URL.RequestURI()] + if !ok { + http.NotFound(w, r) + return + } + b.fetches.Add(1) + _, _ = io.WriteString(w, body) + })) + t.Cleanup(srv.Close) + b.url = srv.URL + + b.proxy, _, _, _ = setupTestProxy(t) + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(srv.Client()), fetch.WithMaxRetries(0)) + b.proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + b.h = NewComposerHandlerWithUpstreams(b.proxy, "http://proxy", srv.URL, srv.URL).Routes() + return b +} + +// TestComposerDownloadBranchMovesOn checks a lock file pinning A while the +// branch moves from A to B: A is cached while it is the head and still served +// from the cache afterwards, and B, the head a new lock file pins, comes from +// upstream. +func TestComposerDownloadBranchMovesOn(t *testing.T) { + tests := []struct { + name, distPath, filename, cached string + }{ + {"github zipball", "/repos/v/p/zipball/%s", "%s.zip", "%s.zip"}, + {"gitlab archive", "/api/v4/projects/v%%2Fp/repository/archive.zip?sha=%s", "archive.zip?sha=%s", "archive-%s.zip"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + b := setupComposerMovingBranch(t, tt.distPath) + get := func(commit, want string, wantFetches int32) { + t.Helper() + filename := fmt.Sprintf(tt.filename, commit) w := httptest.NewRecorder() - h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+filename, nil)) + b.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 { + if got := cachedComposerFile(t, b.proxy, fmt.Sprintf(tt.cached, commit)); got != want { t.Errorf("%s cached as %q, want %q", filename, got, want) } + if n := b.fetches.Load(); n != wantFetches { + t.Errorf("after %s: %d upstream fetches, want %d", filename, n, wantFetches) + } } + get(composerCommitA, "commit A", 1) + b.head.Store(composerCommitB) + get(composerCommitA, "commit A", 1) + get(composerCommitB, "commit B", 2) + get(composerCommitB, "commit B", 2) }) } } +// TestComposerDownloadGitLabSharedName checks that an archive an older proxy +// cached under GitLab's shared archive.zip name is dropped once a commit is +// fetched under its own name. +func TestComposerDownloadGitLabSharedName(t *testing.T) { + distPath := "/api/v4/projects/v%%2Fp/repository/archive.zip?sha=%s" + b := setupComposerMovingBranch(t, distPath) + shared, err := b.proxy.GetOrFetchArtifactFromURL(context.Background(), "composer", "v/p", "dev-master", + "archive.zip", b.url+fmt.Sprintf(distPath, composerCommitB)) + if err != nil { + t.Fatalf("seeding shared entry: %v", err) + } + _ = shared.Reader.Close() + + w := httptest.NewRecorder() + b.h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/archive.zip?sha="+composerCommitA, nil)) + if w.Code != http.StatusOK || w.Body.String() != "commit A" { + t.Errorf("status %d, body %q, want 200 %q", w.Code, w.Body.String(), "commit A") + } + if got := cachedComposerFile(t, b.proxy, "archive.zip"); got != "" { + t.Errorf("archive.zip still cached as %q", got) + } +} + +// TestComposerDownloadCachedFromOtherCommit checks that an archive an older +// proxy cached under a commit's name but fetched from another commit's URL is +// fetched again, while a correct entry is served from the cache. +func TestComposerDownloadCachedFromOtherCommit(t *testing.T) { + zipball := "/repos/v/p/zipball/" + var fetches atomic.Int32 + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/p2/v/p.json", "/p2/v/p~dev.json": + _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":"dev-master","dist":{"url":%q,"type":"zip"}}]}}`, srv.URL+zipball+composerCommitB) + case zipball + composerCommitA: + fetches.Add(1) + _, _ = io.WriteString(w, "commit A") + case zipball + composerCommitB: + fetches.Add(1) + _, _ = io.WriteString(w, "commit B") + default: + http.NotFound(w, r) + } + })) + 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() }) + h := NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL).Routes() + + // What proxy versions before the pinned-commit fix stored: the head + // under the name of the commit the lock file pins. + stale, err := proxy.GetOrFetchArtifactFromURL(context.Background(), "composer", "v/p", "dev-master", + composerCommitA+".zip", srv.URL+zipball+composerCommitB) + if err != nil { + t.Fatalf("seeding stale entry: %v", err) + } + _ = stale.Reader.Close() + fetches.Store(0) + + for range 2 { + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/dev-master/"+composerCommitA+".zip", nil)) + if w.Code != http.StatusOK || w.Body.String() != "commit A" { + t.Errorf("status %d, body %q, want 200 %q", w.Code, w.Body.String(), "commit A") + } + } + if got := cachedComposerFile(t, proxy, composerCommitA+".zip"); got != "commit A" { + t.Errorf("cached as %q, want %q", got, "commit A") + } + if n := fetches.Load(); n != 1 { + t.Errorf("upstream archive fetched %d times, want 1", n) + } +} + +// TestComposerDownloadGitLabTaggedRelease checks that a tagged release on +// GitLab is only served as the commit its metadata lists. +func TestComposerDownloadGitLabTaggedRelease(t *testing.T) { + archive := "/api/v4/projects/v%2Fp/repository/archive.zip?sha=" + h, _ := setupComposerBranch(t, archive+composerCommitB, map[string]string{ + archive + composerCommitA: "commit A", + archive + composerCommitB: "commit B", + }) + + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/1.0.0/archive.zip?sha="+composerCommitA, nil)) + if w.Code != http.StatusNotFound { + t.Errorf("other commit: status %d, body %q, want 404", w.Code, w.Body.String()) + } + + w = httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/files/v/p/1.0.0/archive.zip?sha="+composerCommitB, 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") + } +} + +// TestComposerDownloadBranchWithSlash checks that a branch whose name holds a +// slash gets a dist URL the download route matches. +func TestComposerDownloadBranchWithSlash(t *testing.T) { + const version = "dev-feature/x" + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/zip/x.zip" { + _, _ = io.WriteString(w, "feature") + return + } + _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[{"version":%q,"dist":{"url":%q,"type":"zip"}}]}}`, version, srv.URL+"/zip/x.zip") + })) + 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() }) + h := NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL) + + distURL, ok := h.proxyDistURL("v/p", version, srv.URL+"/zip/x.zip", "zip") + if want := "http://proxy/composer/files/v/p/dev-feature%2Fx/x.zip"; !ok || distURL != want { + t.Fatalf("dist URL = %q, want %q", distURL, want) + } + + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, httptest.NewRequest(http.MethodGet, strings.TrimPrefix(distURL, "http://proxy/composer"), nil)) + if w.Code != http.StatusOK || w.Body.String() != "feature" { + t.Errorf("status %d, body %q, want 200 %q", w.Code, w.Body.String(), "feature") + } +} + // 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. @@ -620,8 +837,8 @@ func TestComposerDownloadUnlistedFile(t *testing.T) { }{ {"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}, + {"gitlab archive with other query", "/api/v4/projects/v%2Fp/repository/archive.zip?sha=" + composerCommitB, + "archive.zip?sha=" + composerCommitB, "archive.zip?sha=" + composerCommitA + "&path=src"}, {"branch name for a commit", "/repos/v/p/zipball/" + composerCommitB, composerCommitB + ".zip", "master.zip"}, } for _, tt := range tests { @@ -676,6 +893,7 @@ func TestComposerDownloadTaggedReleaseCommit(t *testing.T) { func TestComposerUpstreamDistURL(t *testing.T) { const zipball = "https://api.github.com/repos/v/p/zipball/" + const gitlab = "https://gitlab.com/api/v4/projects/v%2Fp/repository/archive.zip?sha=" sha256A := strings.Repeat("a", 64) tests := []struct { name, listed, distType, filename, want string @@ -689,6 +907,9 @@ func TestComposerUpstreamDistURL(t *testing.T) { {"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", ""}, + {"gitlab older commit", gitlab + composerCommitB, "zip", "archive.zip?sha=" + composerCommitA, gitlab + composerCommitA}, + {"gitlab short hash", gitlab + composerCommitB, "zip", "archive.zip?sha=1111111", ""}, + {"gitlab other archive", gitlab + composerCommitB, "zip", "other.zip?sha=" + composerCommitA, ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -701,6 +922,9 @@ func TestComposerUpstreamDistURL(t *testing.T) { if got, ok := upstreamDistURL(zipball+composerCommitB, "zip", composerCommitA+".zip", false); ok { t.Errorf("tagged release rebuilt to %q, want only the listed archive", got) } + if got, ok := upstreamDistURL(gitlab+composerCommitB, "zip", "archive.zip?sha="+composerCommitA, false); ok { + t.Errorf("tagged GitLab release rebuilt to %q, want only the listed archive", got) + } } func TestComposerRewriteMetadataCooldown(t *testing.T) {