-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Cap cookies retained per domain (RFC 6265 5.3) #2228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
d235feb
f9dbd49
ead99dd
baf4319
f01be09
6b2af33
96197b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,12 @@ | |
|
|
||
| public final class ThreadSafeCookieStore implements CookieStore { | ||
|
|
||
| // RFC 6265 §5.3 (step 12) lets a user agent cap the cookies it retains. Bounding cookies per domain | ||
| // keeps a server from growing the jar — and the per-request retrieval scan in get(Uri) — without bound. | ||
| // Chosen generously (well above browser per-domain limits of ~50–180) so it only trips under abuse, | ||
| // never for realistic usage. Package-private for tests. | ||
| static final int MAX_COOKIES_PER_DOMAIN = 200; | ||
|
|
||
| private final Map<String, Map<CookieKey, StoredCookie>> cookieJar = new ConcurrentHashMap<>(); | ||
| private final AtomicInteger counter = new AtomicInteger(); | ||
|
|
||
|
|
@@ -196,6 +202,33 @@ private void add(String requestDomain, String requestPath, Cookie cookie) { | |
| } else { | ||
| final Map<CookieKey, StoredCookie> innerMap = cookieJar.computeIfAbsent(keyDomain, domain -> new ConcurrentHashMap<>()); | ||
| innerMap.put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE)); | ||
| if (innerMap.size() > MAX_COOKIES_PER_DOMAIN) { | ||
| evictExcessCookies(innerMap); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Bounds a single domain's cookie bucket at {@link #MAX_COOKIES_PER_DOMAIN} (RFC 6265 §5.3 step 12). | ||
| * Evicts expired entries first, then the oldest by creation time until back at the cap. Called from | ||
| * {@link #add} right after an insert pushes the bucket over the cap, so it normally removes a single | ||
| * entry. Concurrency-safe: entries are dropped with the two-arg {@code 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. | ||
| */ | ||
| private static void evictExcessCookies(Map<CookieKey, StoredCookie> innerMap) { | ||
| innerMap.entrySet().removeIf(entry -> hasCookieExpired(entry.getValue().cookie, entry.getValue().createdAt)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this re-scans all ~200 entries and calls System.currentTimeMillis() on each, on every over-cap add, even when nothing is expired. Fine at n=200, just flagging that the 200/201 oscillation means every steady-state add pays a full eviction pass with no low-water hysteresis. Not worth optimizing on its own, but the single-pass sequence-number approach would fold this in. |
||
| while (innerMap.size() > MAX_COOKIES_PER_DOMAIN) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two threads can both observe size == 201, both enter here, and each remove a different oldest entry, so the bucket drops to 199 and we silently evict a valid in-cap cookie. Cookie saves run on Netty IO threads, so two concurrent same-origin responses hitting add() is realistic. It's bounded and the victims are the oldest anyway, so not a blocker, but the PR note ("may evict one extra, self-correcting") undersells it: the store can sit below the cap, not just at it. The sequence-number idea below shrinks this window. |
||
| Map.Entry<CookieKey, StoredCookie> oldest = null; | ||
| for (Map.Entry<CookieKey, StoredCookie> entry : innerMap.entrySet()) { | ||
| if (oldest == null || entry.getValue().createdAt < oldest.getValue().createdAt) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ordering by createdAt is weak. It's millisecond granularity, so under a flood most cookies land in the same millisecond and "oldest" becomes whatever ConcurrentHashMap iteration hits first, effectively random. It's also wall-clock based, so an NTP step backward breaks the order. A per-store AtomicLong stamped onto each StoredCookie at construction gives a strict, tie-free order at basically zero cost, and lets you grab the k smallest in a single pass instead of this O(n) rescan on every removal. That would also tighten the over-eviction race above |
||
| oldest = entry; | ||
| } | ||
| } | ||
| if (oldest == null) { | ||
| break; | ||
| } | ||
| innerMap.remove(oldest.getKey(), oldest.getValue()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call on the two-arg remove. Add a one-liner noting it only works because StoredCookie has no equals(), so this is identity based and won't clobber a cookie another thread just re-put under the same key. That invariant is load-bearing and easy to break by accident. |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -112,6 +112,44 @@ public void returnsMultipleDistinctCookiesAtSameDomainPath() { | |
| assertEquals(setOf("ALPHA=AV", "BETA=BV"), namesValues(store.get(uri))); | ||
| } | ||
|
|
||
| @Test | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One gap: consider a test that mixes expired and live cookies over the cap and asserts eviction drops the expired ones first, to lock in the RFC priority order the code implements. |
||
| public void perDomainCookieCountIsCappedUnderFlood() { | ||
| ThreadSafeCookieStore store = new ThreadSafeCookieStore(); | ||
| Uri uri = Uri.create("http://www.foo.com/"); | ||
| int flood = ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN + 50; | ||
| for (int i = 0; i < flood; i++) { | ||
| store.add(uri, ClientCookieDecoder.LAX.decode("c" + i + "=v" + i + "; Domain=www.foo.com; Path=/")); | ||
| } | ||
| assertEquals(ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN, store.getUnderlying().get("www.foo.com").size(), | ||
| "a single domain's cookies must be capped at MAX_COOKIES_PER_DOMAIN"); | ||
| } | ||
|
|
||
| @Test | ||
| public void cookiesUnderTheCapAreAllRetained() { | ||
| ThreadSafeCookieStore store = new ThreadSafeCookieStore(); | ||
| Uri uri = Uri.create("http://www.foo.com/"); | ||
| for (int i = 0; i < 20; i++) { | ||
| store.add(uri, ClientCookieDecoder.LAX.decode("c" + i + "=v" + i + "; Domain=www.foo.com; Path=/")); | ||
| } | ||
| assertEquals(20, store.getUnderlying().get("www.foo.com").size(), "nothing is evicted below the cap"); | ||
| assertEquals(20, store.get(uri).size()); | ||
| } | ||
|
|
||
| @Test | ||
| public void capIsPerDomainNotGlobal() { | ||
| ThreadSafeCookieStore store = new ThreadSafeCookieStore(); | ||
| Uri foo = Uri.create("http://www.foo.com/"); | ||
| for (int i = 0; i < ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN + 20; i++) { | ||
| store.add(foo, ClientCookieDecoder.LAX.decode("c" + i + "=v" + i + "; Domain=www.foo.com; Path=/")); | ||
| } | ||
| store.add(Uri.create("http://www.bar.com/"), | ||
| ClientCookieDecoder.LAX.decode("only=1; Domain=www.bar.com; Path=/")); | ||
|
|
||
| assertEquals(ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN, store.getUnderlying().get("www.foo.com").size()); | ||
| assertEquals(1, store.getUnderlying().get("www.bar.com").size(), | ||
| "flooding one domain must not evict another domain's cookies"); | ||
| } | ||
|
|
||
| @Test | ||
| public void returnsEmptyForUnknownDomain() { | ||
| ThreadSafeCookieStore store = new ThreadSafeCookieStore(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFC pointer is off. §5.3's storage-model algorithm has no numbered "step 12". The per-domain number limit lives in §5.5 (Implementation Limits, "at least 50 cookies per domain"), and the eviction behavior is the unnumbered "remove excess cookies" prose in §5.3. Also worth noting the RFC breaks ties by least-recently-accessed, not creation time, so we're deviating here on purpose. Suggest citing §5.5 plus §5.3 and saying so in one line. Same fix applies to the javadoc on evictExcessCookies.