From 18f940bfcbd78342e476e53f43676598906879e1 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Mon, 21 Sep 2026 18:58:15 -0700 Subject: [PATCH] fix(ghrel): ask for octet-stream, and never cache a JSON "asset" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub's release-asset endpoint is content-negotiated: it returns the asset's METADATA as JSON unless the request asks for application/octet-stream. The proxy did not ask. So it got a 200 carrying ~1.4 KB of JSON, cached it under zlib-1.3.2.tar.gz's key, and served it to every build on the node. spc caught it on the sha256 (php-sdk run 35675558294, "Hash mismatch for downloaded GitHub release asset of zlib"). Without that check this would have fed a JSON blob into a statically linked PHP binary instead of failing. This is the mirror image of #203. That one sent asset BYTES through the JSON rewriter and 502'd; this one sent an asset REQUEST without the header that makes it return bytes. Same endpoint, opposite direction, and the second was worse: a 502 stops the build, silent corruption does not. Two changes: - serveAPIAsset sends Accept: application/octet-stream. The client's own Accept is deliberately not forwarded — 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 key for everyone else. - pkgcache.Request grows RejectContentTypes, checked before any header or cache writer is opened. A 200 with the wrong media type on an artifact path is PROOF the response is not the artifact; storing one is indistinguishable from corruption. ghrel rejects application/json, so a rate-limit body or a future API change fails loudly instead of being cached. The existing asset test passed throughout because its fake upstream ignored Accept and always returned bytes. The new one behaves like GitHub. Revert-verified both ways: dropping Accept alone fails with a 502; dropping Accept and the guard together reproduces production exactly, serving 103 bytes of JSON where a 3200-byte asset was expected. --- pkg/proxies/ghrel/ghrelproxy.go | 18 ++++++++ pkg/proxies/ghrel/ghrelproxy_test.go | 67 ++++++++++++++++++++++++++++ pkg/proxies/pkgcache/fetch.go | 39 ++++++++++++++++ 3 files changed, 124 insertions(+) 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)