From 0b3efb5f655083ddb246657eb53adee47b0692a4 Mon Sep 17 00:00:00 2001 From: mmatur Date: Tue, 27 Oct 2026 12:01:33 +0100 Subject: [PATCH] fix: serve the moved tag when the pinned commit is gone and a hash is recorded --- pkg/handlers/module.go | 22 +++++++------- pkg/handlers/pinned.go | 54 +++++++++++++++++++++++++++++------ pkg/handlers/pinned_test.go | 57 ++++++++++++++++++++++++++----------- 3 files changed, 98 insertions(+), 35 deletions(-) diff --git a/pkg/handlers/module.go b/pkg/handlers/module.go index 73c8b8b..e3ebab8 100644 --- a/pkg/handlers/module.go +++ b/pkg/handlers/module.go @@ -236,25 +236,27 @@ func (h Handlers) downloadGitHub(ctx context.Context, moduleName, version string logger := log.With().Str("module_name", moduleName).Str("module_version", version).Logger() - request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version) - if err != nil { - span.RecordError(err) - logger.Error().Err(err).Msg("Failed to get archive link") + _, errHash := h.store.GetHashByName(ctxDownload, moduleName, version) + if errHash != nil && !errors.As(errHash, &db.NotFoundError{}) { + span.RecordError(errHash) + logger.Error().Err(errHash).Msg("Failed to get plugin hash") JSONErrorf(rw, http.StatusInternalServerError, "Failed to get plugin %s@%s", moduleName, version) return } - _, err = h.store.GetHashByName(ctxDownload, moduleName, version) - if err != nil && !errors.As(err, &db.NotFoundError{}) { + recorded := errHash == nil + + request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version, recorded) + if err != nil { span.RecordError(err) - logger.Error().Err(err).Msg("Failed to get plugin hash") + logger.Error().Err(err).Msg("Failed to get archive link") JSONErrorf(rw, http.StatusInternalServerError, "Failed to get plugin %s@%s", moduleName, version) return } - if err == nil { + if recorded { _, err = h.gh.Do(ctxDownload, request, rw) if err != nil { span.RecordError(err) @@ -479,14 +481,14 @@ func (h Handlers) downloadGitHubFromAssets(ctx context.Context, moduleName, vers } } -func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version string) (*http.Request, error) { +func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version string, recorded bool) (*http.Request, error) { ctx, span := h.tracer.Start(ctx, "handler_getArchiveLinkRequest") defer span.End() owner, repoName := path.Split(strings.TrimPrefix(moduleName, "github.com/")) owner = strings.TrimSuffix(owner, "/") - ref, err := h.archiveRef(ctx, owner, repoName, moduleName, version) + ref, err := h.archiveRef(ctx, owner, repoName, moduleName, version, recorded) if err != nil { span.RecordError(err) diff --git a/pkg/handlers/pinned.go b/pkg/handlers/pinned.go index 06ec53b..b644c3b 100644 --- a/pkg/handlers/pinned.go +++ b/pkg/handlers/pinned.go @@ -24,7 +24,9 @@ var errNotPinned = errors.New("no pinned commit") // The pinned commit of a moved annotated tag cannot give those bytes back, nor match a hash recorded after the move: // those hashes are realigned with `piceus realign --only`, reviewed one by one. // Without an answer from the Go module proxy, it falls back to the tag, as before pinning. -func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, version string) (string, error) { +// When GitHub no longer has the pinned commit, the moved tag is served only if a hash is recorded: +// validate then rejects any other bytes, and no hash is ever recorded from code the proxy did not pin. +func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, version string, recorded bool) (string, error) { if h.pinProxyURL == "" { return version, nil } @@ -52,19 +54,53 @@ func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, v case unchanged: return version, nil case moved: - log.Info().Str("module_name", moduleName).Str("module_version", version). - Str("tag_commit", sha).Str("pinned_commit", pinned). - Msg("Tag moved after publication, serving the pinned commit") + return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, sha, recorded) + case deleted: + return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, "", recorded) + default: + return "", fmt.Errorf("resolve tag %s: %w", version, err) + } +} + +// refAfterTagChange returns the ref to serve once the tag moved to tagCommit, or was deleted when tagCommit is empty. +func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, moduleName, version, pinned, tagCommit string, recorded bool) (string, error) { + logger := log.With().Str("module_name", moduleName).Str("module_version", version).Str("pinned_commit", pinned).Logger() + + gone, err := h.commitGone(ctx, owner, repoName, pinned) + if err != nil { + return "", err + } + + switch { + case !gone && tagCommit != "": + logger.Info().Str("tag_commit", tagCommit).Msg("Tag moved after publication, serving the pinned commit") return pinned, nil - case deleted: - log.Info().Str("module_name", moduleName).Str("module_version", version). - Str("pinned_commit", pinned).Int("status", resp.StatusCode). - Msg("Tag not found, serving the pinned commit") + case !gone: + logger.Info().Msg("Tag not found, serving the pinned commit") return pinned, nil + case tagCommit != "" && recorded: + logger.Warn().Str("tag_commit", tagCommit).Msg("Pinned commit gone from GitHub, serving the moved tag, checked against the recorded hash") + + return version, nil default: - return "", fmt.Errorf("resolve tag %s: %w", version, err) + return "", fmt.Errorf("pinned commit %s of %s@%s is gone from GitHub", pinned, moduleName, version) + } +} + +// commitGone reports whether GitHub no longer has the commit sha. +func (h Handlers) commitGone(ctx context.Context, owner, repoName, sha string) (bool, error) { + // A conditional request answered 304 is not counted in the GitHub rate limit. + _, resp, err := h.gh.Repositories.GetCommitSHA1(ctx, owner, repoName, sha, sha) + + switch { + case err == nil, resp != nil && resp.StatusCode == http.StatusNotModified: + return false, nil + case resp != nil && (resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusUnprocessableEntity): + return true, nil + default: + return false, fmt.Errorf("check commit %s: %w", sha, err) } } diff --git a/pkg/handlers/pinned_test.go b/pkg/handlers/pinned_test.go index 40fa321..022ffb8 100644 --- a/pkg/handlers/pinned_test.go +++ b/pkg/handlers/pinned_test.go @@ -28,7 +28,7 @@ func TestArchiveRef(t *testing.T) { rw.WriteHeader(http.StatusGone) }) - for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown"} { + for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown", "movedgone", "deletedgone", "checkdown"} { proxy.HandleFunc("/github.com/o/"+repo+"/@v/v1.0.0.info", func(rw http.ResponseWriter, _ *http.Request) { _, _ = fmt.Fprint(rw, `{"Version":"v1.0.0","Origin":{"VCS":"git","Hash":"pinned"}}`) }) @@ -49,18 +49,30 @@ func TestArchiveRef(t *testing.T) { } } + status := func(code int) http.HandlerFunc { + return func(rw http.ResponseWriter, _ *http.Request) { + rw.WriteHeader(code) + } + } + gh := http.NewServeMux() gh.HandleFunc("/repos/o/stable/commits/v1.0.0", commit("pinned")) gh.HandleFunc("/repos/o/moved/commits/v1.0.0", commit("other")) - gh.HandleFunc("/repos/o/deleted/commits/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) { - rw.WriteHeader(http.StatusUnprocessableEntity) - }) - gh.HandleFunc("/repos/o/notfound/commits/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) { - rw.WriteHeader(http.StatusNotFound) - }) - gh.HandleFunc("/repos/o/ghdown/commits/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) { - rw.WriteHeader(http.StatusServiceUnavailable) - }) + gh.HandleFunc("/repos/o/deleted/commits/v1.0.0", status(http.StatusUnprocessableEntity)) + gh.HandleFunc("/repos/o/notfound/commits/v1.0.0", status(http.StatusNotFound)) + gh.HandleFunc("/repos/o/ghdown/commits/v1.0.0", status(http.StatusServiceUnavailable)) + gh.HandleFunc("/repos/o/movedgone/commits/v1.0.0", commit("other")) + gh.HandleFunc("/repos/o/deletedgone/commits/v1.0.0", status(http.StatusUnprocessableEntity)) + gh.HandleFunc("/repos/o/checkdown/commits/v1.0.0", commit("other")) + + // The pinned commit, still on GitHub or gone. + for _, repo := range []string{"moved", "deleted", "notfound"} { + gh.HandleFunc("/repos/o/"+repo+"/commits/pinned", commit("pinned")) + } + + gh.HandleFunc("/repos/o/movedgone/commits/pinned", status(http.StatusUnprocessableEntity)) + gh.HandleFunc("/repos/o/deletedgone/commits/pinned", status(http.StatusUnprocessableEntity)) + gh.HandleFunc("/repos/o/checkdown/commits/pinned", status(http.StatusServiceUnavailable)) ghSrv := httptest.NewServer(gh) t.Cleanup(ghSrv.Close) @@ -71,9 +83,11 @@ func TestArchiveRef(t *testing.T) { h := New(nil, nil, ghClient, proxySrv.URL) testCases := []struct { - repo string - ref string - wantErr bool + desc string + repo string + recorded bool + ref string + wantErr bool }{ {repo: "unknown", ref: "v1.0.0"}, {repo: "noorigin", ref: "v1.0.0"}, @@ -85,11 +99,22 @@ func TestArchiveRef(t *testing.T) { {repo: "proxydown", ref: "v1.0.0"}, {repo: "proxygone", ref: "v1.0.0"}, {repo: "Bad!Path", ref: "v1.0.0"}, + // validate checks the moved tag against the recorded hash. + {desc: "movedgone recorded", repo: "movedgone", recorded: true, ref: "v1.0.0"}, + // Serving the moved tag would record a hash from code the proxy did not pin. + {desc: "movedgone not recorded", repo: "movedgone", wantErr: true}, + {desc: "deletedgone recorded", repo: "deletedgone", recorded: true, wantErr: true}, + {repo: "checkdown", recorded: true, wantErr: true}, } for _, test := range testCases { - t.Run(test.repo, func(t *testing.T) { - ref, err := h.archiveRef(t.Context(), "o", test.repo, "github.com/o/"+test.repo, "v1.0.0") + desc := test.desc + if desc == "" { + desc = test.repo + } + + t.Run(desc, func(t *testing.T) { + ref, err := h.archiveRef(t.Context(), "o", test.repo, "github.com/o/"+test.repo, "v1.0.0", test.recorded) if test.wantErr { require.Error(t, err) @@ -125,7 +150,7 @@ func TestPinnedCommit_cached(t *testing.T) { func TestArchiveRef_noPinProxy(t *testing.T) { h := New(nil, nil, github.NewClient(nil), "") - ref, err := h.archiveRef(t.Context(), "o", "moved", "github.com/o/moved", "v1.0.0") + ref, err := h.archiveRef(t.Context(), "o", "moved", "github.com/o/moved", "v1.0.0", false) require.NoError(t, err) assert.Equal(t, "v1.0.0", ref)