Repository navigation
Fail closed in gem, Hex and PyPI cooldown - #585
Conversation
|
This conflicts with main in |
RubyGems, Hex and PyPI keep publish times outside the metadata clients read, so cooldown fetches them from a second API. When that fetch failed, was rate limited, or returned something that didn't parse, the compact index, Hex package or simple page was served unfiltered, and a Hex package that couldn't be re-encoded was passed on as upstream sent it. A flaky or rate-limited timestamp API let every version in cooldown through. These now fail with 502, or with 503 and Retry-After when the rate limit gate held the request, as the Go handler already does. PyPI file downloads check the same way. A PyPI package the JSON API answers 404 for still has no times and is served unfiltered, as before. npm, Composer, pub.dev and PyPI JSON metadata that couldn't be rewritten was also passed on unfiltered unless the package had denied versions. It now fails with 502 whenever cooldown applies too. npm tarball downloads whose packument couldn't be fetched skipped the cooldown check and downloaded straight from upstream; they now fail. All of this only applies when cooldown covers the package: packages exempted with a duration of 0 skip the timestamp lookups, and the gem and Hex handlers serve them through the metadata cache as when cooldown is off.
With cooldown on, the Hex handler read the /packages response, its decompressed payload and the Hex API's JSON without a limit, and the gem handler decoded the versions API's JSON the same way. A misbehaving upstream could make the proxy hold any amount in memory, and a small gzip payload could expand without bound. These reads now go through ReadMetadata, so metadata_max_size applies to each, and to the Hex payload after decompression as the NuGet handler does. A response over the limit fails the request with 502.
899aa81 to
aa0460a
Compare
|
Rebased onto main, CI is green. |
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Failing closed is the right call, and the handler changes look right to me. One problem with downloads, though.
PyPIHandler.handleDownload checks cooldown before it looks in the cache, so with cooldown on, every file download fetches /pypi/<name>/json first. cache_metadata is off by default, so when pypi.org is down or rate limiting, files that are already cached now get 502/503 instead of being served from the cache. npm mostly avoids this through the stored PublishedAt, but routed packages and versions cached before their publish time was stored go through the same path in NPMHandler.handleDownload.
When the publish times can't be read, serve the file if it's already cached, and fail with 502/503 only on a cache miss. That keeps fail-closed for anything not fetched yet, and the proxy keeps serving what it has while the upstream is unreachable. Please add a test with a cached file and a failing JSON API for both handlers.
Smaller thing: the comment block in internal/handler/cooldown_fail_closed.go between the import and cooldownApplies isn't attached to anything and repeats the doc comments below it. Please delete it.
PyPI and npm downloads look up the publish time before the cache. When that lookup fails, a file that is already cached is now served, and only a cache miss fails with 502 or 503. The proxy keeps serving what it has while the upstream is unreachable, and nothing new is fetched unchecked. Also drops the unattached comment block in cooldown_fail_closed.go.
|
Done in 224bce8: when the publish times can't be read, PyPI and npm downloads now serve the file if it's already cached and fail with 502/503 only on a cache miss. |
simonchrz
added
the
breaking
RubyGems, Hex and PyPI keep publish times outside the metadata clients read, so cooldown fetches them from a second API (the RubyGems versions API, the Hex HTTP API, the PyPI JSON API). When that fetch failed, was held by the rate limit gate, or returned something that didn't parse, the compact index, Hex package or simple page was served unfiltered. A flaky or rate-limited timestamp API let every version in cooldown through. This makes them fail closed, as the Go handler already does.
Retry-Afterwhen the rate limit gate held the request. A PyPI file that is already cached is still served; only one that would have to be fetched fails. A PyPI package the JSON API answers 404 for still has no times and is served unfiltered, as before.0skip the timestamp lookups, and the gem and Hex handlers serve them through the metadata cache as when cooldown is off.A second commit puts the Hex handler's reads (the
/packagesresponse, its decompressed payload, the Hex API JSON) and the gem versions API JSON behindReadMetadata, sometadata_max_sizeapplies to them, after decompression for the Hex payload as the NuGet handler does.The cooldown section of docs/configuration.md describes the new behaviour.
Tests:
TestGemCooldownFailsClosed,TestHexCooldownFailsClosed,TestPyPICooldownFailsClosed,TestPyPIDownloadCooldownFailsClosed,TestNPMDownloadCooldownFailsClosed,TestCooldownRewriteFailureFailsClosed,TestDownloadCooldownErrorServesCachedFile,TestHexCooldownMetadataSizeLimit,TestGemCooldownVersionsSizeLimit.TestGemCompactIndexVersionsAPIFailsnow expects 502.The npm download change overlaps with #582, which refuses uncached tarballs whose packument can't be read; whichever lands second needs a small rebase.