fix(netty): keep caller Cookie headers as written on redirects and auth retries by mkurz · Pull Request #2360 · AsyncHttpClient/async-http-client

mkurz · 2026-10-07T08:09:14Z

Summary

On a redirect or an authentication retry, keep a caller-set Cookie header as text, removing only the pairs that the response replaced, instead of decoding it leniently and re-encoding it strictly.
Find those pairs by splitting at every ;, as NettyRequestFactory.mergeCookies does, so that a quote in front of a replaced pair cannot hide it and resend a stale session. Both now share one split.
Count a Set-Cookie deletion only where its Domain and Path apply, so that a deletion for another path no longer cuts the caller's cookie and lets an older stored one take its place.
No breaking API change: two helpers are added to HttpUtils. Follow-up to fd97636 (GHSA-2jwh-9rmr-j4xf).

Problem

Since fd97636, a caller-set Cookie header is kept when the cookie store also contributes cookies to a hop, with the pairs that the response replaced removed. The cookie store is enabled by default (ThreadSafeCookieStore), so this path covers ordinary clients. It applies to redirects and to authentication retries: 401 (Basic, Digest, NTLM) and proxy 407.

To find the replaced pairs, CallerCookies decoded the caller's header leniently and re-encoded the remaining pairs strictly. That broke headers that the first request had sent without complaint:

A value that the strict encoder rejects failed the redirect or retry, although the initial request had been sent.
Pairs could be reordered, and valueless or unbalanced-quote pairs were dropped.

Review also found that CallerCookies counted every Set-Cookie with Max-Age<=0 as replacing the caller's cookie of that name, whatever its Domain and Path. Set-Cookie: SID=; Path=/elsewhere; Max-Age=0 on a same-origin 302, 401 or 407 response cut the caller's SID=mine, whether it was set as a cookie object or in a Cookie header. If the store held a SID for the next request, that older value was sent in its place. main behaves the same way.

Change

Keep the caller-set header as text: remove only the pairs whose names the response replaced. Cookie objects are still encoded normally, and caller cookies are still stripped at cross-origin redirect boundaries.
Split at every ;, as mergeCookies does when it decides which names the caller set. RFC 6265 does not allow a ; in a cookie value, quoted or not. Keeping the text while splitting like the lenient decoder, which treats a double quote as opening a quoted string, would be unsafe. A header such as X=a"b; SID=old or X="a; SID=old would become a single pair named X. A SID that a redirect, 401 or 407 response rotated, or deleted with Max-Age=0, would stay in the header. mergeCookies would still count it as the caller's and drop the store's new value, so the next request would carry the stale session. Tests pin these cases.
One split for both: HttpUtils.cookiePairs and HttpUtils.cookiePairName now split and name the pairs for mergeCookies and CallerCookies. CallerCookies still folds names as the store does, while mergeCookies compares them as written; the Javadoc says so.
Cut out only the replaced pairs and keep the rest as written, separators included, so prefs={"a":"x;y"} or A=1;B=2 is not rewritten.
Return a header that loses no pair unchanged, instead of re-joining it with "; ". A header's pair names are checked before anything is built.
Count a deletion only where it applies: its Domain and Path must match the next request or the request it answered, as the store matches them (RFC 6265 sections 5.1.3 and 5.1.4). Cookie objects and Cookie header pairs follow the same rule.
- Matching only the next request is not enough. Take Set-Cookie: S=; Path=/app; Max-Age=0 on a logout that redirects to /login. The deleted cookie was sent with the logout request, either added by the store or as a caller's pair that may be a copy of it. Once it is gone from the store, it can no longer be told from a caller's own cookie, and it would follow the redirect.
- The store's other checks are not repeated, such as a public suffix as Domain or a Secure cookie over plaintext.
A Set-Cookie that is not a deletion counts only if it applies to the next request too, not just because a stored cookie for that request has the same value.
One store lookup per hop for the next request's cookies. A retry, which goes to the same URI again, reuses it for the current request.
Restore the Javadoc of CallerCookies.of. In Redirect30xInterceptor, remove a block that the new code made dead, and update a comment that described only the case without a cookie store.

Behavior changes

A caller-set Cookie header reaches the next hop of a redirect or authentication retry as written, minus the pairs the response replaced. Before, it could be reordered, lose pairs, or fail the hop.
A raw pair equal to a stored cookie now follows a same-origin redirect as written. Seed P=scoped; Path=/p, then GET /p/a with Cookie: P=scoped and a 302 to /q/b: main sends no P, because the pair counted as the store's and was dropped outside its Path. This change sends P=scoped, since the caller wrote it.
A deletion whose Domain or Path matches neither the request it answered nor the next one no longer removes the caller's cookie, as an object or in a Cookie header.
Unchanged: a deletion that does apply still removes the caller's cookie of that name. If the store keeps a cookie of that name for the next request, for another path, that one is sent instead, as on main and as a browser would.
Not covered, as on main: the store's other reasons to refuse a cookie are not repeated.
- A deletion that matches by Domain and Path but that the store refuses still removes the caller's cookie. Examples are a public suffix as Domain, or a Secure cookie over plaintext.
- So does a refused Set-Cookie that is not a deletion, if the store already holds its value for the next request.
- Covering these would need the store's own decision on each cookie.

Compatibility

No breaking API change. HttpUtils gains cookiePairs and cookiePairName.

AI disclosure

OpenAI Codex and Claude Code on behalf of Matthias Kurz. The first commit was written in two steps, the first with Codex and the second with Claude Code, and squashed; it carries both Co-Authored-By trailers per AGENTS.md. The review fixes were made with Claude Code, and their commits carry its Co-Authored-By trailer.

Test plan

New tests in RedirectCookieRotationTest and AuthRetryCookieTest cover:

redirects with cookie rotation, and Digest, proxy 407 and NTLM (both rounds) retries;
the lax encoder, several caller Cookie headers, and a raw pair deleted with Max-Age=0;
quotes in front of a replaced pair, and byte-exact kept text;
deletions:
- for another path, or for another Domain, on a redirect and on a 401 retry, for cookie objects and header pairs, with and without a stored cookie of that name;
- a logout that deletes a path-scoped cookie and redirects elsewhere, for cookie objects and header pairs, also beside a second stored cookie of that name;
a non-deleting Set-Cookie for another path with the value of a stored cookie;
a raw pair equal to a stored, path-scoped cookie, which follows a redirect out of its Path.

HttpUtilsTest covers the shared split. The proxy 407 test no longer sets a cookie, so it does not pin that a proxy's Set-Cookie is stored against the origin. It fails on main, where the strict encoder rejects the value.

Broken variants of the deletion rule, each run against RedirectCookieRotationTest and AuthRetryCookieTest:

Every deletion counted, as on main: 2 failures: the deletion for another path on a 401 retry, and the deletion for another Domain.
A deletion counted only if it applies to the next request: 2 failures, both logout tests: the deleted P=scoped follows the redirect.
The previous revision's rule, which kept a header pair while the store held a cookie of that name for the next request: 1 failure, the logout beside a second stored cookie: the caller's P=scoped follows instead of the stored P=root.
The Domain not checked: 1 failure, the deletion with Domain=example.org.
A non-deleting Set-Cookie counted without checking its Path: 1 failure, the Set-Cookie for /elsewhere with the value of the stored cookie.

Runs:

Each new commit compiles with its tests.
./mvnw -B -ntp -Dgpg.skip=true clean verify on JDK 11: 1,814 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used.
The cookie and redirect tests, compiled on JDK 11 and run on JDK 17, 21 and 25 (Maven Surefire's -Djvm): 270 tests each, 0 failures.
All four open AHC changes merged together (raw Cookie headers on retries, ResponseBodyControl.execute, suspend/resume order, demand-bounded decompression): they merge without conflicts, together and in every pair, and ./mvnw -B -ntp -Dgpg.skip=true clean install on JDK 11 passes with 1,867 tests, 0 failures, 0 errors, 11 skipped; Revapi passed.
Play WS against that merged build, Scala 2.13 and 3.3, JDK 17: all tests pass. A Play WS test sends Cookie: z=1; flash=stale; a=2; flag through a same-origin redirect that sets flash, with the cookie store enabled. The next hop receives z=1; a=2; flag; flash=redirect-cookie. Against AHC without this change, the valueless flag is dropped and the test fails.

A redirect or authentication retry decoded a caller-set Cookie header leniently, then re-encoded it strictly, whenever the cookie store also contributed cookies to the hop. Values accepted on the first hop could therefore fail on the next one, pairs could be reordered, and valueless or unbalanced-quote pairs were dropped. Keep caller-set header pairs as text and cut out only the pairs whose names the response replaced. The rest stays as written, separators included, so prefs={"a":"x;y"} or A=1;B=2 is not rewritten, and a header that loses no pair is returned unchanged. Cookie objects are still encoded normally, and caller cookies are still stripped at cross-origin redirect boundaries. Find the pairs by splitting on every ';', as NettyRequestFactory.mergeCookies does when it decides which names the caller set; RFC 6265 does not allow a ';' in a cookie value, quoted or not. Treating a double quote as opening a quoted string would let a header such as X=a"b; SID=old hide a SID that a redirect, 401 or 407 response rotated or deleted, so that the next request resent the stale session. Restore the Javadoc of CallerCookies.of, and fix the comment on the Cookie header in Redirect30xInterceptor, which still said the caller's pairs travel in the cookie list. Add tests for redirects with cookie rotation, Digest, proxy and NTLM retries, the lax encoder, several caller Cookie headers, a raw pair deleted with Max-Age=0, quotes in front of a replaced pair, and byte-exact kept text. Follow-up to fd97636 (GHSA-2jwh-9rmr-j4xf). OpenAI Codex and Claude Code on behalf of Matthias Kurz Co-Authored-By: Codex <codex@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

hyperxpro · 2026-10-21T21:35:40Z

+                                             ClientCookieDecoder cookieDecoder) {
        // Only what the store took counts: a Set-Cookie it refused, or one for another path, leaves the
        // caller's cookie of that name in place. Names folded as the store keys them, so a SID set here also
        // replaces a request's sid.


This doesn't hold for the maxAge() <= 0 branch below. It adds the name without checking Domain or Path, so Set-Cookie: SID=; Path=/elsewhere; Max-Age=0 on a same origin 302 cuts the caller's SID=mine while the store keeps its own SID. Same on a 401 retry, same for addCookie. We should only add the name when the deletion's path and domain match next, like the store does (RFC 6265 5.3 steps 6 and 11). Predates this PR, of() had the same branch, but now it hits raw headers directly. holdsSameValue below ignores path as well.

Confirmed, and fixed in 3f3b8de. A deletion now counts only if its Domain and Path match next or the request it answered, matched as the store matches them. Cookie objects and header pairs follow the same rule.

Matching only next would break a common case: Set-Cookie: S=; Path=/app; Max-Age=0 on a logout that redirects to /login. The deleted S was sent with the logout request, either added by the store or as a caller's pair that may be a copy of it. Once the store has deleted it, of() can no longer tell it from a caller's own cookie, so it would follow the redirect to /login. So a deletion that applies to the request it answered still counts.

holdsSameValue: a Set-Cookie that is not a deletion now also has to apply to next. A cookie set for /elsewhere with the value of a stored cookie for next no longer counts.

appliesTo repeats only the store's Domain and Path matching, not its other checks. A deletion that matches but that the store refuses, for example with a public suffix as Domain or as a Secure cookie over plaintext, still counts, as on main. The description lists that.

The tests cover:

your example on a 302 and on a 401 retry, for cookie objects and header pairs, with and without a stored SID;

a deletion for another Domain, which the store refuses;

the logout case for cookie objects and header pairs, also beside a second stored cookie of that name, which then follows the redirect.

Each of these variants fails at least one test: every deletion counted (as on main), only next matched, the Domain not checked, and a non-deleting Set-Cookie counted without checking its Path.

hyperxpro · 2026-10-21T21:35:40Z

+                        Uri next, ClientCookieDecoder cookieDecoder) {
+        Set<String> setByResponse = replacedNames(response, cookieStore, next, cookieDecoder);
+        retry.setCookies(of(request, cookieStore, setByResponse));
+        retry.setHeader(COOKIE, headerCookies(request.getHeaders().getAll(COOKIE), setByResponse));


Raw pairs no longer go through holdsSameValue(stored, ...). Before, a raw pair equal to a stored cookie counted as the store's and was dropped once the redirect left that cookie's Path. Now it follows as written. Seed P=scoped; Path=/p, GET /p/a with Cookie: P=scoped, 302 to /q/b: main sends no Cookie, this sends P=scoped. I think the new behaviour is the right one, but it needs a test and a line in the description.

Added in 0bac96e with your example: aRawPairEqualToAStoredCookieFollowsARedirectOutOfItsPath. /q/b receives P=scoped; SID=old, since the seed also stores SID; on main it receives only SID=old. The description now covers it under Behavior changes.

hyperxpro · 2026-10-21T21:35:40Z

+                    nameEnd++;
+                }
+                String name = header.substring(start, nameEnd).trim();
+                if (!name.isEmpty() && setByResponse.contains(name.toLowerCase(Locale.ROOT))) {


mergeCookies compares caller names case sensitive, this folds them. So sid=lower goes out on the first request next to the store's SID=upper and gets cut here once SID rotates. The fold matches the store so probably fine, but the javadoc says we split the same way as mergeCookies and we don't quite. Can we share the pair name loop between the two instead of keeping two copies in sync?

Done in d1d8552. HttpUtils.cookiePairs and HttpUtils.cookiePairName now split and name the pairs for both mergeCookies and CallerCookies. I kept the fold, since it matches the store. The Javadoc now says that the merge compares names as written while CallerCookies folds them, instead of claiming that both work the same way.

hyperxpro · 2026-10-21T21:35:40Z

+        List<String> remaining = new ArrayList<>(headers.size());
        for (String header : headers) {
-            cookies.addAll(ServerCookieDecoder.LAX.decodeAll(header));
+            StringBuilder kept = new StringBuilder(header.length());


We build kept and a trimmed name for every pair even when nothing gets removed and then throw it all away. Check the names first, or create the builder on the first removal.

Done in d1d8552. The pair names are checked first, and a header without a replaced pair is returned as is, before anything is built.

hyperxpro · 2026-10-21T21:35:40Z

+        Set<String> setByResponse = replacedNames(response, cookieStore, next, cookieDecoder);
+        retry.setCookies(of(request, cookieStore, setByResponse));
+        retry.setHeader(COOKIE, headerCookies(request.getHeaders().getAll(COOKIE), setByResponse));
+        for (Cookie cookie : cookieStore.get(next)) {


Third cookieStore.get on this hop, replacedNames and of already did one each. On a 401 retry all three are the same URI. Fetch once and pass it down.

Done in 0e4d781. refresh looks up next once and passes the list to the replacement check and to the final loop. A 401 or 407 retry reuses it for the current request, so it does one lookup. A redirect does a second one, for the request URI, and only if the request has cookie objects.

hyperxpro · 2026-10-21T21:35:40Z

@@ -481,7 +484,7 @@ private static HttpHeaders propagatedHeaders(Request request, Realm realm, boole
                    .remove(PROXY_AUTHORIZATION);
        }
        if (cookiesReconciled) {


This is dead now. CallerCookies.refresh sets Cookie again right after on the non stripping path, and the stripping path already removed it above. Drop the parameter and the block.

Removed in 68c2746, with the parameter.

hyperxpro · 2026-10-21T21:35:41Z

-                    requestBuilder.setCookies(cookieStore == null
-                            ? request.getCookies() : CallerCookies.of(request, response, cookieStore, newUri, cookieDecoder));
+                } else if (cookieStore == null) {
+                    requestBuilder.setCookies(request.getCookies());


The comment above only describes the cookieStore == null case now. With a store the split happens in CallerCookies.refresh further down.

Updated in 68c2746. It now says that without a store every cookie is the caller's, and that with one CallerCookies.refresh tells them apart below.

hyperxpro · 2026-10-21T21:35:41Z

+                    .setRealm(basicAuthRealm("user", "pass").setUsePreemptiveAuth(false))
+                    .setHeader("Cookie", "SID=old; X=a b")
+                    .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE);
+            assertEquals(new HashSet<>(Arrays.asList("X=a b", "SID=new")), new HashSet<>(Arrays.asList(header.split("; "))));


header is null when the retry sends no Cookie at all, so a regression here shows up as an NPE instead of the assertion diff. The helper at the bottom of the file already has the null check, use that.

Done in 21a2e1d. Both tests that split the header now go through a null-safe helper, which cookiesOnRetry uses too.

hyperxpro · 2026-10-21T21:35:41Z

+    }
+
+    @Test
+    void aRawCookieSurvivesAProxyAuthenticationRetryBesideTheSessionTheChallengeSet() throws Exception {


This pins that a Set-Cookie on the proxy's own 407 gets stored against the origin and replaces the caller's SID. That's existing Interceptors behaviour (only CONNECT is exempt), not sure we want a test cementing it. A 401 fixture covers the same reconciliation without it.

Agreed, the test shouldn't pin that. In 21a2e1d the 407 no longer sets a cookie, so the test only checks that the caller's Cookie header survives a proxy authentication retry as written. I kept it because it is the only test of a Cookie header on the 407 path, and it fails on main, where the strict encoder rejects X=a b.

A redirect or authentication retry drops the request's cookies that the response replaced, so that the store's current values are sent instead. Every Set-Cookie with Max-Age<=0 counted, whatever its Domain and Path. A deletion for another path then cut the caller's cookie, and the store's cookie of that name, if it held one, was sent in its place. Count a deletion only if its Domain and Path match the next request or the one it answered, as the store matches them. The latter keeps the cookie that the response deleted from following a redirect to another path: once gone from the store, the store's cookie can no longer be told from the caller's, and a caller's pair may be a copy of it. Cookie objects and Cookie header pairs follow the same rule. A Set-Cookie that is not a deletion counts only if it applies to the next request too, not just because a stored cookie for that request has the same value. The store's other checks, such as a public suffix as Domain or a Secure cookie over plaintext, are not repeated. A deletion that the store refuses for such a reason still counts, as before. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A pair of a caller's Cookie header now follows a same-origin redirect as written, even when the store holds a cookie with the same name and value. Before, such a pair counted as the store's, and was dropped once the redirect left that cookie's Path. Pin the new behavior: a caller who wrote the pair meant to send it. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

NettyRequestFactory and CallerCookies each split a caller's Cookie header into pairs and named them, in two loops that had to agree. Move the split and the naming to HttpUtils and use them in both. CallerCookies still folds names as the store does, while the merge compares them as written; the Javadoc now says so instead of claiming that both work the same way. CallerCookies also checks a header's pair names before it builds anything, so a header without a replaced pair is no longer copied. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

CallerCookies.refresh asked the store for the next request's cookies up to twice and for the current request's once, three lookups of the same URI on an authentication retry. Look up the next request's cookies once and pass them down, and reuse them for the current request on a retry. A redirect still needs both. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

propagatedHeaders removed the Cookie header whenever there was a cookie store. On a hop that strips credentials it is already gone, and on any other CallerCookies.refresh sets it again right after. Drop the block and its parameter, and say in the comment above that, with a store, CallerCookies tells the caller's cookies from the store's. Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Two tests split the received Cookie header without a null check, so a retry without any cookie failed with a NullPointerException instead of an assertion. Use a null-safe helper for both. The proxy authentication test pinned that a Set-Cookie on the proxy's own 407 is stored against the origin and replaces the caller's cookie, existing behavior that this change should not cement. Its 407 now sets no cookie, so the test checks only that the caller's Cookie header survives the retry as written. It fails on main, where the strict encoder rejects the value "a b". Claude Code on behalf of Matthias Kurz Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

mkurz · 2026-10-22T12:51:21Z

Thanks for the review. I've pushed six commits on top and updated the description. The deletion check differs a little from what you suggested; the first reply explains why. Replies are inline.

mkurz mentioned this pull request Oct 7, 2026

Upgrade to AHC 3.0.15 and decide streamed-body resumes through ResponseBodyControl.execute playframework/play-ws#1487

Draft

18 of 22 tasks

hyperxpro requested changes Oct 21, 2026

View reviewed changes

mkurz and others added 6 commits October 22, 2026 14:12

mkurz requested a review from hyperxpro October 22, 2026 14:02

Conversation

mkurz commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Change

Behavior changes

Compatibility

AI disclosure

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurz commented Oct 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mkurz commented Oct 7, 2026 •

edited

Loading