Uh oh!
There was an error while loading. Please reload this page.
Cap cookies retained per domain (RFC 6265 5.3) - #2256
Merged
hyperxpro merged 10 commits intoJul 18, 2026
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.
…ions, expired-first test Review feedback on the per-domain cookie cap (ThreadSafeCookieStore): - Order eviction by a monotonic per-store AtomicLong stamp (StoredCookie.seq) instead of createdAt. Creation time is millisecond-granular (ties randomly under a flood) and wall-clock based (an NTP step backward reorders it); the sequence is strict, tie-free, and clock-independent. - Replace the O(n)-rescan-per-removal while loop with a single pass that drops expired entries and collects survivors, sorting by seq only when still over the cap. Concurrent evictors now pick the same seq-ordered victims, so the two-arg (identity) remove()s no-op instead of over-evicting; documented that the bucket may briefly sit below the cap but never grows unbounded. - Fix the RFC pointer: the per-domain limit is §5.5 (Implementation Limits), the eviction order is §5.3's 'remove excess cookies' prose (no 'step 12'), and note the deliberate deviation from least-recently-accessed tie-breaking. - Document that the two-arg remove is identity-based (StoredCookie has no equals()), a load-bearing invariant. - Add a test that a mix of expired and live cookies over the cap evicts the expired ones first, leaving every live cookie in place.
pavel-ptashyts
commented
Jul 18, 2026
ContributorAuthor
@hyperxpro I created new PR as you asked. Also I did fix for all comments from prev PR |
hyperxpro
approved these changes
Jul 18, 2026
Uh oh!
There was an error while loading. Please reload this page.
hyperxpro
commented
Jul 18, 2026
Member
Thanks a lot! |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.