fix(ghrel): ask for octet-stream, and never cache a JSON "asset"

pkg/proxies/ghrel/ghrelproxy.go

@@ -287,10 +287,28 @@ func (p *Proxy) serveAPIAsset(w http.ResponseWriter, r *http.Request) {
 		upstream += "?" + r.URL.RawQuery
 	}
 
+	// Accept is NOT optional here. GitHub's asset endpoint serves the asset's
+	// METADATA as JSON by default and only returns the bytes when asked for
+	// application/octet-stream. Omitting it does not fail — it quietly returns
+	// a ~1.4 KB JSON document with a 200, which then gets cached under the
+	// tarball's key and served to every build. On 2026-09-21 that fed spc a
+	// JSON blob in place of zlib-1.3.2.tar.gz; only spc's own sha256 check
+	// turned it into a visible failure instead of a corrupt PHP binary.
+	//
+	// The client's own Accept is deliberately not forwarded. Whatever spc or
+	// curl happens to send, this path exists to return bytes, and the cache
+	// key does not vary on Accept — so honoring a caller that asked for JSON
+	// would poison the same key for everyone else.
 	req := pkgcache.Request{
 		Key:                "apiasset/" + strings.TrimPrefix(r.URL.Path, "/"),
 		URL:                upstream,
+		Accept:             "application/octet-stream",
 		DefaultContentType: "application/octet-stream",
+		// Belt and braces: if upstream answers with JSON anyway (a rate-limit
+		// body, an error document, a future API change), refuse it rather than
+		// storing it. A wrong-typed response on this path is never the
+		// artifact, and caching one is indistinguishable from corruption.
+		RejectContentTypes: []string{"application/json"},
 	}
 	if p.cfg.ImmutableAssets {
 		req.Immutable = true

pkg/proxies/ghrel/ghrelproxy_test.go

@@ -576,6 +576,73 @@ func TestAPIAssetEndpointServesBytesNotJSON(t *testing.T) {
 	}
 }
 
+// The asset endpoint is CONTENT-NEGOTIATED: GitHub returns the asset's
+// metadata as JSON unless the request asks for application/octet-stream. The
+// upstream above ignores Accept and always returns bytes, which is why it kept
+// passing while production served JSON — so this one behaves like GitHub.
+//
+// Regression for 2026-09-21: the proxy omitted Accept, cached 1.4 KB of
+// metadata JSON under zlib-1.3.2.tar.gz's key, and served it to every build.
+// It was spc's sha256 check, not anything here, that turned that into a
+// visible failure rather than a corrupt PHP binary.
+func TestAPIAssetSendsOctetStreamAccept(t *testing.T) {
+	t.Parallel()
+	payload := []byte(strings.Repeat("BINARY\x00\xff", 400))
+	const metadataJSON = `{"url":"https://api.github.com/repos/madler/zlib/releases/assets/357391855","name":"zlib-1.3.2.tar.gz"}`
+
+	var sawAccept atomic.Value
+	api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		sawAccept.Store(r.Header.Get("Accept"))
+		if r.Header.Get("Accept") != "application/octet-stream" {
+			// Exactly what GitHub does: a 200, with metadata, not the asset.
+			w.Header().Set("Content-Type", "application/json; charset=utf-8")
+			_, _ = io.WriteString(w, metadataJSON)
+			return
+		}
+		w.Header().Set("Content-Type", "application/octet-stream")
+		_, _ = w.Write(payload)
+	}))
+	defer api.Close()
+
+	p := startProxy(t, Config{APIUpstream: api.URL, DownloadUpstream: api.URL})
+	got := get(t, "http://"+p.Addr()+"/repos/madler/zlib/releases/assets/357391855")
+
+	if acc, _ := sawAccept.Load().(string); acc != "application/octet-stream" {
+		t.Errorf("upstream saw Accept %q, want application/octet-stream", acc)
+	}
+	if got.code != http.StatusOK {
+		t.Fatalf("status %d, want 200", got.code)
+	}
+	if string(got.data) != string(payload) {
+		t.Fatalf("served %d bytes, want the %d-byte asset; body starts %.60q",
+			len(got.data), len(payload), got.data)
+	}
+}
+
+// Second line of defence, independent of Accept: if upstream answers an asset
+// path with JSON anyway — a rate-limit body, an error document, an API change
+// — that response must not be cached or served. A 200 carrying the wrong media
+// type is the dangerous shape, because it looks like success everywhere except
+// in the bytes.
+func TestAPIAssetRefusesJSONResponse(t *testing.T) {
+	t.Parallel()
+	api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json; charset=utf-8")
+		_, _ = io.WriteString(w, `{"message":"API rate limit exceeded"}`)
+	}))
+	defer api.Close()
+
+	p := startProxy(t, Config{APIUpstream: api.URL, DownloadUpstream: api.URL})
+	got := get(t, "http://"+p.Addr()+"/repos/madler/zlib/releases/assets/357391855")
+
+	if got.code == http.StatusOK {
+		t.Fatalf("status 200 with a JSON body: the proxy served metadata as an artifact\n%.120q", got.data)
+	}
+	if strings.Contains(string(got.data), "rate limit") {
+		t.Errorf("upstream JSON was passed through to the caller: %.120q", got.data)
+	}
+}
+
 // 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.

pkg/proxies/pkgcache/fetch.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"log/slog"
 	"net/http"
+	"strings"
 	"time"
 )
 
@@ -57,6 +58,37 @@ type Request struct {
 	Accept string
 	// DefaultContentType is used when upstream does not supply one.
 	DefaultContentType string
+	// RejectContentTypes lists media types that must never be cached or
+	// served for this request, matched against the response's Content-Type
+	// ignoring parameters. Empty means accept whatever upstream sends.
+	//
+	// For artifact paths where a wrong type is PROOF the response is not the
+	// artifact. A registry that answers a tarball URL with
+	// application/json is handing back an error document or metadata, and a
+	// 200 makes it indistinguishable from success: it gets cached under the
+	// artifact's key and served to every subsequent job until someone
+	// notices. Checked BEFORE any bytes are written, so the caller's normal
+	// upstream-failure path handles it.
+	RejectContentTypes []string
+}
+
+// rejectedContentType reports whether ct is one of req's rejected media types.
+// Parameters are ignored — "application/json; charset=utf-8" matches
+// "application/json", which is how GitHub actually labels its API responses.
+func (req Request) rejectedContentType(ct string) bool {
+	if len(req.RejectContentTypes) == 0 || ct == "" {
+		return false
+	}
+	media := strings.TrimSpace(strings.ToLower(ct))
+	if i := strings.IndexByte(media, ';'); i >= 0 {
+		media = strings.TrimSpace(media[:i])
+	}
+	for _, bad := range req.RejectContentTypes {
+		if media == strings.TrimSpace(strings.ToLower(bad)) {
+			return true
+		}
+	}
+	return false
 }
 
 // serveCachedArtifact writes a cache hit to the response.
@@ -271,6 +303,13 @@ func (f *Fetcher) ServeArtifact(w http.ResponseWriter, r *http.Request, req Requ
 		return &NotFoundError{URL: req.URL}
 	case resp.StatusCode != http.StatusOK:
 		return fmt.Errorf("fetching %s: upstream status %d", req.URL, resp.StatusCode)
+	case req.rejectedContentType(resp.Header.Get("Content-Type")):
+		// A 200 carrying the wrong media type is the dangerous case: it looks
+		// like success everywhere except in the bytes. Bail before writing
+		// headers or opening a cache writer, so nothing is stored and the
+		// caller reports an upstream failure.
+		return fmt.Errorf("fetching %s: upstream returned %s, which cannot be this artifact",
+			req.URL, resp.Header.Get("Content-Type"))
 	}
 
 	meta := metaFrom(resp, req)