fix(proxy): clamp oversized n query param instead of returning 400#4856
Conversation
PR distribution#4353 made MaxTags (default 1000) a hard ceiling on the `n` query parameter — anything larger and the handler returns 400 PAGINATION_NUMBER_INVALID before the request ever reaches storage or the proxy tag service. That broke clients like Renovate which use n=10000 against pull-through caches. distribution#4846 fixed a related 500 in proxy mode but not this 400, so users reported the regression still persisted. The OCI distribution-spec describes pagination differently: a server MAY return fewer than `n` results "when the total number of tags attached to the repository is less than <int> or a Link header is provided" — otherwise it MUST include `<int>` results. In other words, the right answer for "client asked for more than we'll serve" is `maxtags` results plus a Link header, not a rejection. PAGINATION_NUMBER_INVALID isn't among the 14 error codes the spec defines, either. Drop the oversized-n rejection and clamp to MaxTags instead; the existing Link-header path already handles continuation correctly. Malformed (non-integer) and negative `n` values keep returning 400, since the spec defines `n` as a non-negative integer and those requests are genuinely invalid. Verified end-to-end against registry-1.docker.io in proxy mode: n=10000 now returns the tag list (or a clamped page with Link) instead of 400. Also restores pre-3.1.0 behavior for Renovate-style clients without needing proxy-specific logic. Spec reference: https://github.com/opencontainers/distribution-spec/blob/main/spec.md#listing-tags Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
|
PTAL @thaJeztah |
| limit = parsedMax | ||
| // Per the OCI distribution-spec, a server MAY return fewer than n | ||
| // results when a Link header is provided for continuation. Clamp to | ||
| // MaxTags instead of rejecting oversized requests. | ||
| if maxTags := th.App.Config.Tags.MaxTags; maxTags > 0 && limit > maxTags { | ||
| limit = maxTags | ||
| } |
There was a problem hiding this comment.
I have a feeling there may still be a logical error here; if q.Get("n") is 0, we set limit to 0 (parsedMax) here.
We'll probably want to use the default in that case, not 0, or what's the expectation if it's a literal 0?
Not entirely sure when the -1 sentinel value should be used (or if that one should be kept until we get anything other than 0)
There was a problem hiding this comment.
Good eye, but n=0 is actually explicit in the spec:
When n is zero, this endpoint MUST return an empty list, and MUST NOT include a Link header.
The handler already honors this via the if limit == 0 { moreEntries = false } branch further down -- List() is skipped, filled stays empty, and no Link header is emitted.
I verified against the latest build of proxy:
GET /v2/library/alpine/tags/list?n=0
HTTP/1.1 200 OK
{"name":"library/alpine","tags":[]}
The -1 sentinel is only meaningful when n is absent ( i.e. "list all"), which is a different case from n=0 ("give me zero"). Falling back to the default on n=0 would actually violate the spec's MUST
There was a problem hiding this comment.
Ah, thanks! Yes, "zero" values tend to be ambiguous, but looks indeed like it's called out in the spec 👍
davidspek
left a comment
There was a problem hiding this comment.
Approving to unblock :)
#4353 made
MaxTags(default 1000) a hard ceiling on thenquery parameter — anything larger and the handler returns400 PAGINATION_NUMBER_INVALIDbefore the request ever reaches storage or the proxy tag service. That broke clients like Renovate, which usen=10000against pull-through caches. #4846 fixed a related 500 in proxy mode, but not this400, so users reported the regression still persisted.The OCI distribution-spec describes pagination differently: a server MAY return fewer than
nresults "when the total number of tags attached to the repository is less than or aLinkheader is provided" — otherwise it MUST include<int>results. In other words, the right answer for "client asked for more than we'll serve" ismaxtagsresults plus aLinkheader, not a rejection.PAGINATION_NUMBER_INVALIDisn't among the 14 error codes the spec defines, either.Drop the oversized-n rejection and clamp to
MaxTagsinstead; the existingLink-header path already handles continuation correctly. Malformed (non-integer) and negativenvalues keep returning 400, since the spec definesnas a non-negative integer and those requests are genuinely invalid.This restores pre-3.1.0 behavior for Renovate-style clients without needing proxy-specific logic.