From d235feb06f5bfb4309016ebde9a03ddd677078aa Mon Sep 17 00:00:00 2001 From: Pavel Ptashyts <49400901+pavel-ptashyts@users.noreply.github.com> Date: Thu, 2 Jul 2026 10:58:18 +0200 Subject: [PATCH] Cap cookies retained per domain (RFC 6265 5.3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../cookie/ThreadSafeCookieStore.java | 33 ++++++++++++++++ .../cookie/ThreadSafeCookieStoreGetTest.java | 38 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index ab0e63235..e1e69b5a9 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -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> 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 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 innerMap) { + innerMap.entrySet().removeIf(entry -> hasCookieExpired(entry.getValue().cookie, entry.getValue().createdAt)); + while (innerMap.size() > MAX_COOKIES_PER_DOMAIN) { + Map.Entry oldest = null; + for (Map.Entry entry : innerMap.entrySet()) { + if (oldest == null || entry.getValue().createdAt < oldest.getValue().createdAt) { + oldest = entry; + } + } + if (oldest == null) { + break; + } + innerMap.remove(oldest.getKey(), oldest.getValue()); } } diff --git a/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java b/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java index 6234549fa..1e70957e4 100644 --- a/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java +++ b/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java @@ -112,6 +112,44 @@ public void returnsMultipleDistinctCookiesAtSameDomainPath() { assertEquals(setOf("ALPHA=AV", "BETA=BV"), namesValues(store.get(uri))); } + @Test + 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();