From 00184eae41a5bee67ee2a24989f1cce1a20ff852 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Mon, 21 Sep 2026 07:43:32 -0700 Subject: [PATCH] fix(ghrel): route the asset-download endpoint to bytes, not the JSON rewriter The proxy treated every path under /repos/ as JSON metadata. One of them is not: GitHub serves asset BYTES from /repos/{owner}/{repo}/releases/assets/{id} asked with Accept: application/octet-stream. That is how a client fetches an asset from a PRIVATE repo, and it is what spc's GitHubRelease downloader uses -- so it was not an edge case, it was the main path for every `type: ghrel` artifact. serveMetadata ran that binary body through rewriteAssetURLs, json.Unmarshal failed, and the handler answered 502. The client saw only a failed download with nothing pointing at the proxy: Download artifact 'zlib' failed. Please check your internet connection Last failed command: curl -sfSL --max-time 3600 ... - Exit code: 22 That took out every php-sdk Linux build the day this proxy was first enabled (run 35611830865), after 8 retries each. The proxy was disabled on the fleet to restore builds; this is the fix. isAPIAssetPath matches that one shape and routes it to ServeArtifact against the API upstream, so the bytes stream and cache like any other artifact. Metadata paths are unaffected -- /releases, /releases/latest, /releases/tags/{tag} and /releases/assets (no id) all still go to the rewriter, which is pinned by a table test, because misrouting the other way would serve every release document as opaque bytes and silently stop rewriting asset URLs. Tests: a binary response under /repos/ served intact and cached (one upstream hit across two requests), plus the routing predicate. Revert-verified -- with the route removed the regression test reproduces the outage exactly, "status 502, want 200". --- pkg/proxies/ghrel/ghrelproxy.go | 56 +++++++++++++++++++++++++ pkg/proxies/ghrel/ghrelproxy_test.go | 62 ++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/pkg/proxies/ghrel/ghrelproxy.go b/pkg/proxies/ghrel/ghrelproxy.go index 337e405..f7a1ab0 100644 --- a/pkg/proxies/ghrel/ghrelproxy.go +++ b/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. // diff --git a/pkg/proxies/ghrel/ghrelproxy_test.go b/pkg/proxies/ghrel/ghrelproxy_test.go index 42cd696..2ddbf89 100644 --- a/pkg/proxies/ghrel/ghrelproxy_test.go +++ b/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) + } + } +}