Cap cookies retained per domain (RFC 6265 5.3)#2228
Open
pavel-ptashyts wants to merge 2 commits into
Open
Conversation
ThreadSafeCookieStore kept unbounded cookies per domain, so a server could grow the jar — and the per-request get(Uri) retrieval scan, which visits every cookie in the matching domain buckets — without bound. Bound it at MAX_COOKIES_PER_DOMAIN (200, well above browser per-domain limits of ~50-180 so it only trips under abuse, never for realistic usage). On add, once a domain's bucket exceeds the cap, evictExcessCookies drops expired entries first, then the oldest by creation time until back at the cap. Eviction uses the two-arg remove(key, value) so a cookie another thread just replaced under the same key is never collaterally removed; a benign race between concurrent adders may evict one extra, which is self-correcting. This bounds both memory and the retrieval scan regardless of how cookies are distributed across paths. No public API change: the cap is package-private, eviction is private, and getUnderlying() is unchanged. Adds tests that a flooded domain is capped at the limit, that cookies under the cap are all retained, and that the cap is per-domain (flooding one domain does not evict another's). The audit rated the underlying scan a non-issue for realistic cookie counts; this cap targets its stated pathological case (100+ cookies on one domain) and doubles as mild protection against cookie flooding.
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.
ThreadSafeCookieStore kept unbounded cookies per domain, so a server could grow the jar — and the per-request get(Uri) retrieval scan, which visits every cookie in the matching domain buckets — without bound. Bound it at MAX_COOKIES_PER_DOMAIN (200, well above browser per-domain limits of ~50-180 so it only trips under abuse, never for realistic usage). On add, once a domain's bucket exceeds the cap, evictExcessCookies drops expired entries first, then the oldest by creation time until back at the cap.
Eviction uses the two-arg remove(key, value) so a cookie another thread just replaced under the same key is never collaterally removed; a benign race between concurrent adders may evict one extra, which is self-correcting. This bounds both memory and the retrieval scan regardless of how cookies are distributed across paths.
No public API change: the cap is package-private, eviction is private, and getUnderlying() is unchanged. Adds tests that a flooded domain is capped at the limit, that cookies under the cap are all retained, and that the cap is per-domain (flooding one domain does not evict another's).
The audit rated the underlying scan a non-issue for realistic cookie counts; this cap targets its stated pathological case (100+ cookies on one domain) and doubles as mild protection against cookie flooding.