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) + } + } +}