diff --git a/pkg/proxies/ghrel/ghrelproxy.go b/pkg/proxies/ghrel/ghrelproxy.go index f7a1ab0..a1a9e3b 100644 --- a/pkg/proxies/ghrel/ghrelproxy.go +++ b/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 diff --git a/pkg/proxies/ghrel/ghrelproxy_test.go b/pkg/proxies/ghrel/ghrelproxy_test.go index 2ddbf89..2752a82 100644 --- a/pkg/proxies/ghrel/ghrelproxy_test.go +++ b/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. diff --git a/pkg/proxies/pkgcache/fetch.go b/pkg/proxies/pkgcache/fetch.go index 8af0dc9..4639b16 100644 --- a/pkg/proxies/pkgcache/fetch.go +++ b/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)