Only cache real pub archives on download

internal/handler/pub.go

@@ -5,17 +5,24 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
+	"regexp"
 	"strings"
 	"time"
 
+	"github.com/git-pkgs/proxy/internal/database"
 	"github.com/git-pkgs/proxy/internal/retention"
 )
 
 const (
-	pubUpstream  = "https://pub.dev"
-	pubPathParts = 2 // name + version in path split by /versions/
+	pubUpstream   = "https://pub.dev"
+	pubPathParts  = 2 // name + version in path split by /versions/
+	pubArchiveExt = ".tar.gz"
 )
 
+// pubPackageName matches pub package names, which have to be Dart identifiers.
+var pubPackageName = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
+
 // Packages are stored under pkg:pub/{name} with the name from the download
 // path, and purl keeps pub names as they are, so package rules use the
 // default key mapping.
@@ -69,19 +76,27 @@ func (h *PubHandler) handleDownload(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	name := parts[0]
-	version := strings.TrimSuffix(parts[1], ".tar.gz")
+	// Only the archive is cached. Without the suffix the upstream path is the
+	// HTML version page on pub.dev, which must not land under the archive key.
+	version, ok := strings.CutSuffix(parts[1], pubArchiveExt)
+	if !ok {
+		http.Error(w, "not found", http.StatusNotFound)
+		return
+	}
 
-	if name == "" || version == "" {
+	if !pubPackageName.MatchString(name) || version == "" || strings.Contains(version, "/") {
 		http.Error(w, "invalid request", http.StatusBadRequest)
 		return
 	}
 
-	filename := fmt.Sprintf("%s-%s.tar.gz", name, version)
+	filename := name + "-" + version + pubArchiveExt
 
 	h.proxy.Logger.Info("pub download request",
 		"name", name, "version", version)
 
-	downloadURL := h.upstreamURL + r.URL.Path
+	downloadURL := fmt.Sprintf("%s/packages/%s/versions/%s%s",
+		h.upstreamURL, url.PathEscape(name), url.PathEscape(version), pubArchiveExt)
+	h.discardVersionPage(name, version, filename)
 	result, err := h.proxy.GetOrFetchArtifactFromURL(
 		r.Context(), "pub", name, version, filename, downloadURL,
 	)
@@ -93,6 +108,32 @@ func (h *PubHandler) handleDownload(w http.ResponseWriter, r *http.Request) {
 	ServeArtifactRequest(w, r, result)
 }
 
+// discardVersionPage clears a cached archive that was fetched from a path
+// without the archive suffix. Proxy versions before the suffix check cached
+// pub.dev's HTML version page under the archive's name that way.
+func (h *PubHandler) discardVersionPage(name, version, filename string) {
+	_, versionPURL, err := packagePURLStrings("pub", name, version)
+	if err != nil {
+		return
+	}
+	record, err := h.proxy.DB.GetArtifact(versionPURL, filename)
+	if err != nil {
+		h.proxy.Logger.Warn("failed to read pub cache record",
+			"purl", versionPURL, "filename", filename, "error", err)
+		return
+	}
+	if record == nil || !record.StoragePath.Valid || record.Source == database.SourceLocal ||
+		strings.HasSuffix(record.UpstreamURL, pubArchiveExt) {
+		return
+	}
+	h.proxy.Logger.Warn("cached pub archive was fetched without the archive suffix, discarding",
+		"purl", versionPURL, "filename", filename, "upstream_url", record.UpstreamURL)
+	if err := h.proxy.DB.DiscardArtifact(versionPURL, filename, record.StoragePath.String); err != nil {
+		h.proxy.Logger.Warn("failed to discard pub cache record",
+			"purl", versionPURL, "filename", filename, "error", err)
+	}
+}
+
 // handlePackageMetadata proxies package metadata and rewrites archive URLs.
 func (h *PubHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Request) {
 	name := r.PathValue("name")

internal/handler/pub_test.go

@@ -2,11 +2,16 @@ package handler
 
 import (
 	"encoding/json"
+	"io"
 	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/git-pkgs/cooldown"
+	"github.com/git-pkgs/registries/fetch"
 )
 
 func TestPubRewriteMetadata(t *testing.T) {
@@ -92,3 +97,112 @@ func TestPubRewriteMetadataCooldown(t *testing.T) {
 		t.Errorf("latest version = %v, want 1.0.0", latest["version"])
 	}
 }
+
+// Without the .tar.gz suffix pub.dev answers with the HTML version page, which
+// would otherwise be cached under the archive's key.
+func TestPubDownloadRequiresArchiveSuffix(t *testing.T) {
+	proxy, db, _, fetcher := setupTestProxy(t)
+	fetcher.artifact = &fetch.Artifact{
+		Body:        io.NopCloser(strings.NewReader("<html>version page</html>")),
+		ContentType: "text/html",
+	}
+	h := NewPubHandlerWithUpstream(proxy, "http://localhost", "https://pub.example.com")
+
+	req := httptest.NewRequest(http.MethodGet, "/packages/http/versions/1.2.0", nil)
+	w := httptest.NewRecorder()
+	h.Routes().ServeHTTP(w, req)
+
+	if w.Code != http.StatusNotFound {
+		t.Errorf("status = %d, want %d", w.Code, http.StatusNotFound)
+	}
+	if fetcher.fetchCalled {
+		t.Errorf("upstream fetched %q", fetcher.fetchedURL)
+	}
+	art, err := db.GetArtifact("pkg:pub/http@1.2.0", "http-1.2.0.tar.gz")
+	if err != nil {
+		t.Fatalf("GetArtifact: %v", err)
+	}
+	if art != nil {
+		t.Errorf("artifact cached from %q", art.UpstreamURL)
+	}
+}
+
+func TestPubDownloadRejectsEncodedQueryInName(t *testing.T) {
+	proxy, _, _, fetcher := setupTestProxy(t)
+	fetcher.artifact = &fetch.Artifact{
+		Body:        io.NopCloser(strings.NewReader("archive")),
+		ContentType: "application/octet-stream",
+	}
+	h := NewPubHandlerWithUpstream(proxy, "http://localhost", "https://pub.example.com")
+
+	for _, path := range []string{
+		"/packages/http%3Fx/versions/1.2.0.tar.gz",
+		"/packages/http%23x/versions/1.2.0.tar.gz",
+		"/packages/http/versions/1.2.0%3Fx.tar.gz",
+	} {
+		fetcher.fetchCalled = false
+		fetcher.fetchedURL = ""
+		req := httptest.NewRequest(http.MethodGet, path, nil)
+		w := httptest.NewRecorder()
+		h.Routes().ServeHTTP(w, req)
+
+		if strings.ContainsAny(fetcher.fetchedURL, "?#") {
+			t.Errorf("%s: upstream URL %q carries a query or fragment", path, fetcher.fetchedURL)
+		}
+		if strings.Contains(path, "/http%") && fetcher.fetchCalled {
+			t.Errorf("%s: invalid package name reached upstream as %q", path, fetcher.fetchedURL)
+		}
+	}
+}
+
+// Older versions cached the HTML version page as the archive. Such an entry
+// is fetched again instead of being served.
+func TestPubDownloadRefetchesCachedVersionPage(t *testing.T) {
+	proxy, db, store, fetcher := setupTestProxy(t)
+	seedPackage(t, db, store, "pub", "http", "1.2.0", "http-1.2.0.tar.gz", "<html>version page</html>")
+	art, err := db.GetArtifact("pkg:pub/http@1.2.0", "http-1.2.0.tar.gz")
+	if err != nil || art == nil {
+		t.Fatalf("GetArtifact: %v", err)
+	}
+	art.UpstreamURL = "https://pub.dev/packages/http/versions/1.2.0"
+	if err := db.UpsertArtifact(art); err != nil {
+		t.Fatalf("UpsertArtifact: %v", err)
+	}
+	fetcher.artifact = &fetch.Artifact{
+		Body:        io.NopCloser(strings.NewReader("archive")),
+		ContentType: "application/octet-stream",
+	}
+	h := NewPubHandlerWithUpstream(proxy, "http://localhost", "https://pub.example.com")
+
+	req := httptest.NewRequest(http.MethodGet, "/packages/http/versions/1.2.0.tar.gz", nil)
+	w := httptest.NewRecorder()
+	h.Routes().ServeHTTP(w, req)
+
+	if !fetcher.fetchCalled {
+		t.Fatal("cached version page was served without fetching upstream")
+	}
+	if got := w.Body.String(); got != "archive" {
+		t.Errorf("body = %q, want %q", got, "archive")
+	}
+}
+
+func TestPubDownloadServesCachedArchive(t *testing.T) {
+	proxy, db, store, fetcher := setupTestProxy(t)
+	seedPackage(t, db, store, "pub", "http", "1.2.0", "http-1.2.0.tar.gz", "cached archive")
+	fetcher.artifact = &fetch.Artifact{
+		Body:        io.NopCloser(strings.NewReader("refetched")),
+		ContentType: "application/octet-stream",
+	}
+	h := NewPubHandlerWithUpstream(proxy, "http://localhost", "https://pub.example.com")
+
+	req := httptest.NewRequest(http.MethodGet, "/packages/http/versions/1.2.0.tar.gz", nil)
+	w := httptest.NewRecorder()
+	h.Routes().ServeHTTP(w, req)
+
+	if fetcher.fetchCalled {
+		t.Errorf("cached archive fetched again from %q", fetcher.fetchedURL)
+	}
+	if got := w.Body.String(); got != "cached archive" {
+		t.Errorf("body = %q, want %q", got, "cached archive")
+	}
+}