fix(ghrel): ask for octet-stream, and never cache a JSON "asset" - #204
Merged
Merged
Conversation
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.
|
ePHPm Preview — removed Preview deployment has been torn down. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 is worse, because a 502 stops the build and silent corruption does not.
Changes
serveAPIAssetsendsAccept: 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.RequestgrowsRejectContentTypes, checked before any header is written or cache writer opened. A 200 with the wrong media type on an artifact path is proof the response is not the artifact, and storing one is indistinguishable from corruption. ghrel rejectsapplication/json, so a rate-limit body or a future API change fails loudly rather than being cached.Why the existing test missed it
TestAPIAssetEndpointServesBytesNotJSONpassed the whole time — its fake upstream ignoresAcceptand always returns bytes. A proxy that never sends the header looks identical to one that does. The new test behaves like GitHub.Revert-verified both ways:
Acceptalone → fails with a 502 (the guard catching it).Acceptand the guard → reproduces production exactly:served 103 bytes, want the 3200-byte asset; body starts "{\"url\":\"https://api.github.com/repos/madler/zlib/releases/as".Full
./pkg/proxies/...suite passes.[ghrel_proxy]is disabled fleet-wide and the poisoned entries are purged from both Linux nodes. It stays off until a build passes end-to-end with this in.