fix(ghrel): route the asset-download endpoint to bytes, not the JSON rewriter

pkg/proxies/ghrel/ghrelproxy.go

@@ -66,6 +66,7 @@ import (
 	"log/slog"
 	"net/http"
 	"net/url"
+	"regexp"
 	"strings"
 	"time"
 
@@ -238,6 +239,10 @@ func (p *Proxy) handle(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	switch {
+	// MUST precede the apiPrefix case. The asset-download endpoint lives
+	// under /repos/ too but returns BYTES, not JSON -- see isAPIAssetPath.
+	case isAPIAssetPath(r.URL.Path):
+		p.serveAPIAsset(w, r)
 	case strings.HasPrefix(r.URL.Path, apiPrefix):
 		p.serveMetadata(w, r)
 	case strings.HasPrefix(r.URL.Path, downloadPrefix):
@@ -247,6 +252,57 @@ func (p *Proxy) handle(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+// apiAssetRe matches GitHub's asset-DOWNLOAD endpoint:
+//
+//	/repos/{owner}/{repo}/releases/assets/{id}
+//
+// which is the one path under /repos/ that does not return JSON. Asked with
+// Accept: application/octet-stream it streams the asset bytes, and that is
+// exactly how a client fetches an asset from a private repository, so it
+// cannot simply be excluded.
+var apiAssetRe = regexp.MustCompile(`^/repos/[^/]+/[^/]+/releases/assets/[^/]+/?$`)
+
+// isAPIAssetPath reports whether a path is the asset-download endpoint.
+//
+// Getting this wrong is not subtle but IS silent-looking: serveMetadata runs
+// every /repos/ response through a JSON rewriter, so a binary body fails to
+// parse and the handler answers 502. The client sees a failed download with no
+// indication the proxy invented the failure. That took out every php-sdk Linux
+// build the day this proxy was first enabled (run 35611830865, "Download
+// artifact 'zlib' failed", curl exit 22 after 8 retries) -- zlib is a
+// `type: ghrel` artifact, and spc fetches its bytes from this endpoint rather
+// than from browser_download_url.
+func isAPIAssetPath(path string) bool {
+	return apiAssetRe.MatchString(path)
+}
+
+// serveAPIAsset streams asset bytes from the API asset endpoint, caching them.
+//
+// Same treatment as serveAsset -- the only difference is which upstream the
+// request goes to, because this path is on the API origin rather than the
+// download origin.
+func (p *Proxy) serveAPIAsset(w http.ResponseWriter, r *http.Request) {
+	upstream := p.cfg.APIUpstream + r.URL.Path
+	if r.URL.RawQuery != "" {
+		upstream += "?" + r.URL.RawQuery
+	}
+
+	req := pkgcache.Request{
+		Key:                "apiasset/" + strings.TrimPrefix(r.URL.Path, "/"),
+		URL:                upstream,
+		DefaultContentType: "application/octet-stream",
+	}
+	if p.cfg.ImmutableAssets {
+		req.Immutable = true
+	} else {
+		req.TTL = p.cfg.AssetTTL
+	}
+
+	if err := p.srv.Fetcher().ServeArtifact(w, r, req); err != nil {
+		p.writeFetchError(w, err, "release asset (api endpoint)")
+	}
+}
+
 // serveMetadata proxies a REST API request and rewrites the asset URLs in the
 // response so the caller comes back here for the bytes.
 //

pkg/proxies/ghrel/ghrelproxy_test.go

@@ -536,3 +536,65 @@ func TestNestedMetadataPathsAreBothCached(t *testing.T) {
 		t.Errorf("upstream hits = %d after re-reading both documents, want 2: one of them is not being cached", got)
 	}
 }
+
+// TestAPIAssetEndpointServesBytesNotJSON is the regression test for the bug
+// that took out every php-sdk Linux build the day this proxy was first
+// enabled.
+//
+// The asset-DOWNLOAD endpoint lives under /repos/ like the metadata endpoints,
+// but returns BYTES. Routing it to serveMetadata runs a binary body through a
+// JSON rewriter, which fails to parse and answers 502 -- and the client just
+// sees a failed download, with nothing pointing at the proxy. spc fetches
+// asset bytes from exactly this endpoint (it is how an asset is fetched from a
+// private repo), so `type: ghrel` artifacts all went through it.
+func TestAPIAssetEndpointServesBytesNotJSON(t *testing.T) {
+	t.Parallel()
+	payload := []byte(strings.Repeat("BINARY\x00\xff", 400))
+	var hits atomic.Int64
+	api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		hits.Add(1)
+		w.Header().Set("Content-Type", "application/octet-stream")
+		_, _ = w.Write(payload)
+	}))
+	defer api.Close()
+
+	p := startProxy(t, Config{APIUpstream: api.URL, DownloadUpstream: api.URL})
+	base := "http://" + p.Addr()
+	const path = "/repos/madler/zlib/releases/assets/12345"
+
+	for i := range 2 {
+		got := get(t, base+path)
+		if got.code != http.StatusOK {
+			t.Fatalf("attempt %d: status %d, want 200 (a 502 here is the proxy failing to parse bytes as JSON)", i, got.code)
+		}
+		if string(got.data) != string(payload) {
+			t.Fatalf("attempt %d: body mismatch (%d bytes, want %d)", i, len(got.data), len(payload))
+		}
+	}
+	if hits.Load() != 1 {
+		t.Errorf("upstream hits = %d, want 1: the asset was not cached", hits.Load())
+	}
+}
+
+// TestIsAPIAssetPath pins the routing predicate directly: metadata paths must
+// NOT be mistaken for assets, or every release lookup would be served as
+// opaque bytes and never rewritten.
+func TestIsAPIAssetPath(t *testing.T) {
+	t.Parallel()
+	for _, tc := range []struct {
+		path  string
+		asset bool
+	}{
+		{"/repos/acme/tool/releases/assets/42", true},
+		{"/repos/acme/tool/releases/assets/42/", true},
+		{"/repos/acme/tool/releases/latest", false},
+		{"/repos/acme/tool/releases", false},
+		{"/repos/acme/tool/releases/tags/v1.0.0", false},
+		{"/repos/acme/tool/releases/assets", false},
+		{"/download/acme/tool/releases/download/v1/x.tgz", false},
+	} {
+		if got := isAPIAssetPath(tc.path); got != tc.asset {
+			t.Errorf("isAPIAssetPath(%q) = %v, want %v", tc.path, got, tc.asset)
+		}
+	}
+}