fix: serve a moved tag when its archive matches the recorded hash

pkg/handlers/handlers.go

@@ -53,7 +53,9 @@ type Handlers struct {
 	gh          *github.Client
 	pinProxyURL string
 	// pins caches the pinned commit by module@version: the proxy never changes it.
-	pins       *sync.Map
+	pins *sync.Map
+	// tagHashes caches the hash of moved tag archives by owner/repo@commit.
+	tagHashes  *sync.Map
 	httpClient *http.Client
 	tracer     trace.Tracer
 }
@@ -67,6 +69,7 @@ func New(store Storer, goProxy *goproxy.Client, gh *github.Client, pinProxyURL s
 		gh:          gh,
 		pinProxyURL: pinProxyURL,
 		pins:        &sync.Map{},
+		tagHashes:   &sync.Map{},
 		httpClient:  &http.Client{Timeout: 10 * time.Second},
 		tracer:      otel.GetTracerProvider().Tracer("handler"),
 	}

pkg/handlers/module.go

@@ -236,7 +236,7 @@ func (h Handlers) downloadGitHub(ctx context.Context, moduleName, version string
 
 		logger := log.With().Str("module_name", moduleName).Str("module_version", version).Logger()
 
-		_, errHash := h.store.GetHashByName(ctxDownload, moduleName, version)
+		ph, 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")
@@ -247,7 +247,12 @@ func (h Handlers) downloadGitHub(ctx context.Context, moduleName, version string
 
 		recorded := errHash == nil
 
-		request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version, recorded)
+		var recordedHash string
+		if recorded {
+			recordedHash = ph.Hash
+		}
+
+		request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version, recordedHash)
 		if err != nil {
 			span.RecordError(err)
 			logger.Error().Err(err).Msg("Failed to get archive link")
@@ -481,26 +486,37 @@ func (h Handlers) downloadGitHubFromAssets(ctx context.Context, moduleName, vers
 	}
 }
 
-func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version string, recorded bool) (*http.Request, error) {
+// getArchiveLinkRequest builds the request downloading the GitHub zipball to serve for moduleName@version.
+// recordedHash is the hash recorded for this version, empty if none.
+func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version, recordedHash string) (*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, recorded)
+	ref, err := h.archiveRef(ctx, owner, repoName, moduleName, version, recordedHash)
 	if err != nil {
 		span.RecordError(err)
 
 		return nil, fmt.Errorf("failed to get archive ref: %w", err)
 	}
 
+	req, err := h.zipballRequest(ctx, owner, repoName, ref)
+	if err != nil {
+		span.RecordError(err)
+
+		return nil, err
+	}
+
+	return req, nil
+}
+
+func (h Handlers) zipballRequest(ctx context.Context, owner, repoName, ref string) (*http.Request, error) {
 	opts := &github.RepositoryContentGetOptions{Ref: ref}
 
 	link, _, err := h.gh.Repositories.GetArchiveLink(ctx, owner, repoName, github.Zipball, opts, 3)
 	if err != nil {
-		span.RecordError(err)
-
 		return nil, fmt.Errorf("failed to get archive link: %w", err)
 	}
 

pkg/handlers/pinned.go

@@ -2,6 +2,7 @@ package handlers
 
 import (
 	"context"
+	"crypto/sha256"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -22,11 +23,12 @@ var errNotPinned = errors.New("no pinned commit")
 // and the pinned commit once the tag moved or was deleted.
 // Requesting the tag keeps the archive bytes: the zipball of an annotated tag is named after the tag object, not the 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.
+// a moved tag whose archive still matches the recorded hash is served, since validate accepts exactly those bytes.
 // Without an answer from the Go module proxy, it falls back to the tag, as before pinning.
 // 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) {
+// recordedHash is the hash recorded for this version, empty if none.
+func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, version, recordedHash string) (string, error) {
 	if h.pinProxyURL == "" {
 		return version, nil
 	}
@@ -54,18 +56,31 @@ func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, v
 	case unchanged:
 		return version, nil
 	case moved:
-		return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, sha, recorded)
+		return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, sha, recordedHash)
 	case deleted:
-		return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, "", recorded)
+		return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, "", recordedHash)
 	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) {
+func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, moduleName, version, pinned, tagCommit, recordedHash string) (string, error) {
 	logger := log.With().Str("module_name", moduleName).Str("module_version", version).Str("pinned_commit", pinned).Logger()
 
+	if tagCommit != "" && recordedHash != "" {
+		matches, err := h.tagMatchesRecorded(ctx, owner, repoName, version, tagCommit, recordedHash)
+		if err != nil {
+			logger.Warn().Err(err).Str("tag_commit", tagCommit).Msg("Failed to check the moved tag against the recorded hash, serving as if it does not match")
+		}
+
+		if matches {
+			logger.Info().Str("tag_commit", tagCommit).Msg("Tag moved after publication, serving the tag: its archive matches the recorded hash")
+
+			return version, nil
+		}
+	}
+
 	gone, err := h.commitGone(ctx, owner, repoName, pinned)
 	if err != nil {
 		return "", err
@@ -80,7 +95,7 @@ func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, module
 		logger.Info().Msg("Tag not found, serving the pinned commit")
 
 		return pinned, nil
-	case tagCommit != "" && recorded:
+	case tagCommit != "" && recordedHash != "":
 		logger.Warn().Str("tag_commit", tagCommit).Msg("Pinned commit gone from GitHub, serving the moved tag, checked against the recorded hash")
 
 		return version, nil
@@ -89,6 +104,36 @@ func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, module
 	}
 }
 
+// tagMatchesRecorded reports whether the zipball of the tag version, pointing at tagCommit, has the recorded hash.
+func (h Handlers) tagMatchesRecorded(ctx context.Context, owner, repoName, version, tagCommit, recordedHash string) (bool, error) {
+	// Cache the hash, not the answer: the recorded hash can be realigned.
+	// The zipball of an annotated tag names the tag object, so two tags on one commit differ.
+	key := owner + "/" + repoName + "@" + version + "@" + tagCommit
+
+	if cached, ok := h.tagHashes.Load(key); ok {
+		if sum, ok := cached.(string); ok {
+			return sum == recordedHash, nil
+		}
+	}
+
+	req, err := h.zipballRequest(ctx, owner, repoName, version)
+	if err != nil {
+		return false, fmt.Errorf("tag %s: %w", version, err)
+	}
+
+	hash := sha256.New()
+
+	if _, err = h.gh.Do(ctx, req, hash); err != nil {
+		return false, fmt.Errorf("download tag %s: %w", version, err)
+	}
+
+	sum := fmt.Sprintf("%x", hash.Sum(nil))
+
+	h.tagHashes.Store(key, sum)
+
+	return sum == recordedHash, nil
+}
+
 // 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.

pkg/handlers/pinned_test.go

@@ -1,6 +1,7 @@
 package handlers
 
 import (
+	"crypto/sha256"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -28,7 +29,7 @@ func TestArchiveRef(t *testing.T) {
 		rw.WriteHeader(http.StatusGone)
 	})
 
-	for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown", "movedgone", "deletedgone", "checkdown"} {
+	for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown", "movedgone", "deletedgone", "checkdown", "movedmatch", "archivedown", "archivedowngone"} {
 		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"}}`)
 		})
@@ -55,6 +56,9 @@ func TestArchiveRef(t *testing.T) {
 		}
 	}
 
+	// The URL of the fake GitHub, set once it starts: the zipball endpoint redirects to an absolute URL.
+	var ghURL string
+
 	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"))
@@ -64,28 +68,50 @@ func TestArchiveRef(t *testing.T) {
 	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"))
+	gh.HandleFunc("/repos/o/movedmatch/commits/v1.0.0", commit("other"))
+	gh.HandleFunc("/repos/o/archivedown/commits/v1.0.0", commit("other"))
+	gh.HandleFunc("/repos/o/archivedowngone/commits/v1.0.0", commit("other"))
 
 	// The pinned commit, still on GitHub or gone.
-	for _, repo := range []string{"moved", "deleted", "notfound"} {
+	for _, repo := range []string{"moved", "deleted", "notfound", "movedmatch", "archivedown"} {
 		gh.HandleFunc("/repos/o/"+repo+"/commits/pinned", commit("pinned"))
 	}
 
+	// The zipball of the moved tags.
+	for _, repo := range []string{"moved", "movedgone", "checkdown", "movedmatch"} {
+		gh.HandleFunc("/repos/o/"+repo+"/zipball/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) {
+			rw.Header().Set("Location", ghURL+"/archives/"+repo)
+			rw.WriteHeader(http.StatusFound)
+		})
+	}
+
+	gh.HandleFunc("/archives/", func(rw http.ResponseWriter, _ *http.Request) {
+		_, _ = fmt.Fprint(rw, "tag archive")
+	})
+	gh.HandleFunc("/repos/o/archivedown/zipball/v1.0.0", status(http.StatusServiceUnavailable))
+	gh.HandleFunc("/repos/o/archivedowngone/zipball/v1.0.0", status(http.StatusServiceUnavailable))
+
 	gh.HandleFunc("/repos/o/movedgone/commits/pinned", status(http.StatusUnprocessableEntity))
+	gh.HandleFunc("/repos/o/archivedowngone/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)
 
+	ghURL = ghSrv.URL
+
 	ghClient := github.NewClient(nil)
 	ghClient.BaseURL, _ = url.Parse(ghSrv.URL + "/")
 
 	h := New(nil, nil, ghClient, proxySrv.URL)
 
+	tagArchive := fmt.Sprintf("%x", sha256.Sum256([]byte("tag archive")))
+
 	testCases := []struct {
 		desc     string
 		repo     string
-		recorded bool
+		recorded string
 		ref      string
 		wantErr  bool
 	}{
@@ -99,12 +125,20 @@ func TestArchiveRef(t *testing.T) {
 		{repo: "proxydown", ref: "v1.0.0"},
 		{repo: "proxygone", ref: "v1.0.0"},
 		{repo: "Bad!Path", ref: "v1.0.0"},
+		// The pinned commit, the recorded hash is from other bytes.
+		{desc: "moved recorded", repo: "moved", recorded: "other", ref: "pinned"},
+		// validate accepts exactly the bytes of the moved tag.
+		{desc: "moved recorded from the tag", repo: "movedmatch", recorded: tagArchive, ref: "v1.0.0"},
+		{desc: "moved not recorded", repo: "movedmatch", ref: "pinned"},
+		// The check failed: served as if the tag does not match.
+		{desc: "tag archive down", repo: "archivedown", recorded: tagArchive, ref: "pinned"},
+		{desc: "tag archive down, pinned commit gone", repo: "archivedowngone", recorded: tagArchive, ref: "v1.0.0"},
 		// validate checks the moved tag against the recorded hash.
-		{desc: "movedgone recorded", repo: "movedgone", recorded: true, ref: "v1.0.0"},
+		{desc: "movedgone recorded", repo: "movedgone", recorded: "other", 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},
+		{desc: "deletedgone recorded", repo: "deletedgone", recorded: "other", wantErr: true},
+		{repo: "checkdown", recorded: "other", wantErr: true},
 	}
 
 	for _, test := range testCases {
@@ -150,8 +184,60 @@ 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", false)
+	ref, err := h.archiveRef(t.Context(), "o", "moved", "github.com/o/moved", "v1.0.0", "")
 	require.NoError(t, err)
 
 	assert.Equal(t, "v1.0.0", ref)
 }
+
+func TestTagMatchesRecorded_cached(t *testing.T) {
+	var downloads atomic.Int32
+
+	var ghURL string
+
+	gh := http.NewServeMux()
+	gh.HandleFunc("/repos/o/r/zipball/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) {
+		rw.Header().Set("Location", ghURL+"/archive")
+		rw.WriteHeader(http.StatusFound)
+	})
+	gh.HandleFunc("/repos/o/r/zipball/v1.0.1", func(rw http.ResponseWriter, _ *http.Request) {
+		rw.Header().Set("Location", ghURL+"/archive-v1.0.1")
+		rw.WriteHeader(http.StatusFound)
+	})
+	gh.HandleFunc("/archive", func(rw http.ResponseWriter, _ *http.Request) {
+		downloads.Add(1)
+		_, _ = fmt.Fprint(rw, "tag archive")
+	})
+	// An annotated tag on the same commit: its zipball names another tag object.
+	gh.HandleFunc("/archive-v1.0.1", func(rw http.ResponseWriter, _ *http.Request) {
+		downloads.Add(1)
+		_, _ = fmt.Fprint(rw, "other tag archive")
+	})
+
+	ghSrv := httptest.NewServer(gh)
+	t.Cleanup(ghSrv.Close)
+
+	ghURL = ghSrv.URL
+
+	ghClient := github.NewClient(nil)
+	ghClient.BaseURL, _ = url.Parse(ghSrv.URL + "/")
+
+	h := New(nil, nil, ghClient, "")
+
+	tagArchive := fmt.Sprintf("%x", sha256.Sum256([]byte("tag archive")))
+
+	matches, err := h.tagMatchesRecorded(t.Context(), "o", "r", "v1.0.0", "tagcommit", tagArchive)
+	require.NoError(t, err)
+	assert.True(t, matches)
+
+	// A realigned hash no longer matches the cached archive.
+	matches, err = h.tagMatchesRecorded(t.Context(), "o", "r", "v1.0.0", "tagcommit", "realigned")
+	require.NoError(t, err)
+	assert.False(t, matches)
+
+	matches, err = h.tagMatchesRecorded(t.Context(), "o", "r", "v1.0.1", "tagcommit", fmt.Sprintf("%x", sha256.Sum256([]byte("other tag archive"))))
+	require.NoError(t, err)
+	assert.True(t, matches)
+
+	assert.Equal(t, int32(2), downloads.Load())
+}