diff --git a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/CallerCookies.java b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/CallerCookies.java index b38b1685a..84eeb826f 100644 --- a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/CallerCookies.java +++ b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/CallerCookies.java @@ -18,14 +18,12 @@ import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.cookie.ClientCookieDecoder; import io.netty.handler.codec.http.cookie.Cookie; -import io.netty.handler.codec.http.cookie.ServerCookieDecoder; import org.asynchttpclient.Request; import org.asynchttpclient.RequestBuilder; import org.asynchttpclient.cookie.CookieStore; import org.asynchttpclient.uri.Uri; import java.util.ArrayList; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Locale; @@ -44,16 +42,28 @@ private CallerCookies() { } /** - * The request's cookies minus the ones the store put there: those this response set, rotated or deleted, - * and those the store still holds with the same value. A caller's cookie sharing only a name with a stored - * one stays the caller's. + * The request's cookie objects minus the ones the store put there: those this response set, rotated or + * deleted, and those the store still holds with the same value. A caller's cookie sharing only a name with + * a stored one stays the caller's. A Cookie header the caller set is not among these; see + * {@link #headerCookies}. */ - static List of(Request request, HttpResponse response, CookieStore cookieStore, Uri next, - ClientCookieDecoder cookieDecoder) { - List cookies = withHeaderCookies(request); + private static List of(Request request, CookieStore cookieStore, Set setByResponse) { + List cookies = request.getCookies(); if (cookies.isEmpty()) { return cookies; } + List stored = cookieStore.get(request.getUri()); + List callers = new ArrayList<>(cookies.size()); + for (Cookie cookie : cookies) { + if (!setByResponse.contains(cookie.name().toLowerCase(Locale.ROOT)) && !holdsSameValue(stored, cookie)) { + callers.add(cookie); + } + } + return callers; + } + + private static Set replacedNames(HttpResponse response, CookieStore cookieStore, Uri next, + 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. @@ -81,14 +91,7 @@ static List of(Request request, HttpResponse response, CookieStore cooki setByResponse.add(cookie.name().toLowerCase(Locale.ROOT)); } } - List stored = cookieStore.get(request.getUri()); - List callers = new ArrayList<>(cookies.size()); - for (Cookie cookie : cookies) { - if (!setByResponse.contains(cookie.name().toLowerCase(Locale.ROOT)) && !holdsSameValue(stored, cookie)) { - callers.add(cookie); - } - } - return callers; + return setByResponse; } /** @@ -96,28 +99,76 @@ static List of(Request request, HttpResponse response, CookieStore cooki */ static void refresh(RequestBuilder retry, Request request, HttpResponse response, CookieStore cookieStore, ClientCookieDecoder cookieDecoder) { - retry.setCookies(of(request, response, cookieStore, request.getUri(), cookieDecoder)); - retry.setHeader(COOKIE, Collections.emptyList()); - for (Cookie cookie : cookieStore.get(request.getUri())) { + refresh(retry, request, response, cookieStore, request.getUri(), cookieDecoder); + } + + static void refresh(RequestBuilder retry, Request request, HttpResponse response, CookieStore cookieStore, + Uri next, ClientCookieDecoder cookieDecoder) { + Set 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)) { retry.addCookieIfUnset(cookie); } } /** - * The request's cookies plus the pairs of a Cookie header the caller set. A hop drops the raw header and - * sends these instead, reconciled against the response, or the header would outrank a cookie it rotated. - * Only the request the caller built sends the header as written. + * The caller's Cookie header values minus the pairs this response replaced, kept as text: a lenient decode + * followed by strict encoding can reject a value that was sent on the first attempt. + *

+ * Every ';' ends a pair, inside quotes too: RFC 6265 does not allow one in a cookie value, and + * {@code NettyRequestFactory#mergeCookies} splits the same way when it decides which names the caller set. + * Honouring quotes here would let a stray '"' hide a replaced pair in its neighbour's value, to be resent + * stale while the merge still counts its name as the caller's and drops the store's new value. Only the + * replaced pairs are cut out, so the rest keeps its bytes and a header that loses none is returned as is. */ - private static List withHeaderCookies(Request request) { - List headers = request.getHeaders().getAll(COOKIE); - if (headers.isEmpty()) { - return request.getCookies(); + private static List headerCookies(List headers, Set setByResponse) { + if (setByResponse.isEmpty() || headers.isEmpty()) { + return headers; } - List cookies = new ArrayList<>(request.getCookies()); + List remaining = new ArrayList<>(headers.size()); for (String header : headers) { - cookies.addAll(ServerCookieDecoder.LAX.decodeAll(header)); + StringBuilder kept = new StringBuilder(header.length()); + boolean removed = false; + int start = 0; + while (start <= header.length()) { + int end = header.indexOf(';', start); + if (end < 0) { + end = header.length(); + } + int nameEnd = start; + while (nameEnd < end && header.charAt(nameEnd) != '=') { + nameEnd++; + } + String name = header.substring(start, nameEnd).trim(); + if (!name.isEmpty() && setByResponse.contains(name.toLowerCase(Locale.ROOT))) { + removed = true; + } else { + if (kept.length() > 0) { + kept.append(';'); + } + kept.append(header, start, end); + } + start = end + 1; + } + if (!removed) { + remaining.add(header); + continue; + } + // Cutting the first or last pair leaves the whitespace or ';' that separated it at that end. + int from = 0; + int to = kept.length(); + while (from < to && (kept.charAt(from) == ';' || kept.charAt(from) <= ' ')) { + from++; + } + while (to > from && (kept.charAt(to - 1) == ';' || kept.charAt(to - 1) <= ' ')) { + to--; + } + if (from < to) { + remaining.add(kept.substring(from, to)); + } } - return cookies; + return remaining; } private static boolean holdsSameValue(List stored, Cookie cookie) { diff --git a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java index a409bfb02..d1ec092fd 100644 --- a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java +++ b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java @@ -207,9 +207,8 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture CookieStore cookieStore = config.getCookieStore(); if (stripAuth) { requestBuilder.resetCookies(); - } else { - requestBuilder.setCookies(cookieStore == null - ? request.getCookies() : CallerCookies.of(request, response, cookieStore, newUri, cookieDecoder)); + } else if (cookieStore == null) { + requestBuilder.setCookies(request.getCookies()); } requestBuilder.setMethod(switchToGet ? GET : originalMethod) @@ -253,9 +252,13 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture final boolean initialConnectionKeepAlive = future.isKeepAlive(); if (cookieStore != null) { - // Update request's cookies assuming that cookie store is already updated by Interceptors - for (Cookie cookie : cookieStore.get(newUri)) { - requestBuilder.addCookieIfUnset(cookie); + if (stripAuth) { + // Only cookies scoped to the new URI may cross this security boundary. + for (Cookie cookie : cookieStore.get(newUri)) { + requestBuilder.addCookieIfUnset(cookie); + } + } else { + CallerCookies.refresh(requestBuilder, request, response, cookieStore, newUri, cookieDecoder); } } @@ -481,7 +484,7 @@ private static HttpHeaders propagatedHeaders(Request request, Realm realm, boole .remove(PROXY_AUTHORIZATION); } if (cookiesReconciled) { - // CallerCookies carries its pairs in the cookie list. + // Unless stripped above, CallerCookies.refresh puts it back minus the pairs this response replaced. headers.remove(COOKIE); } return headers; diff --git a/client/src/test/java/org/asynchttpclient/netty/handler/intercept/AuthRetryCookieTest.java b/client/src/test/java/org/asynchttpclient/netty/handler/intercept/AuthRetryCookieTest.java index 36d5a0e87..86034e01d 100644 --- a/client/src/test/java/org/asynchttpclient/netty/handler/intercept/AuthRetryCookieTest.java +++ b/client/src/test/java/org/asynchttpclient/netty/handler/intercept/AuthRetryCookieTest.java @@ -30,17 +30,23 @@ import java.util.HashSet; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import static org.asynchttpclient.Dsl.asyncHttpClient; import static org.asynchttpclient.Dsl.basicAuthRealm; +import static org.asynchttpclient.Dsl.digestAuthRealm; +import static org.asynchttpclient.Dsl.ntlmAuthRealm; +import static org.asynchttpclient.Dsl.proxyServer; import static org.junit.jupiter.api.Assertions.assertEquals; /** - * The retry after a 401 must carry the cookies the challenge set, not the ones the first attempt went out with. + * The retry after a 401 or 407 must carry the cookies the challenge set, not the ones the first attempt went out + * with. */ public class AuthRetryCookieTest extends AbstractBasicTest { private static final String RECEIVED_COOKIE = "received-cookie"; + private static final String NTLM_TYPE_2 = "TlRMTVNTUAACAAAAAAAAACgAAAABggAAU3J2Tm9uY2UAAAAAAAAAAA=="; @Override public AbstractHandler configureHandler() { @@ -48,12 +54,28 @@ public AbstractHandler configureHandler() { @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { + String authorization = request.getHeader("Authorization"); if ("/seed".equals(target)) { response.addHeader("Set-Cookie", "SID=old; Path=/"); - } else if (request.getHeader("Authorization") == null) { - response.addHeader("Set-Cookie", "SID=new; Path=/"); - response.setHeader("WWW-Authenticate", "Basic realm=\"test\""); - response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); + } else if ("/digest".equals(target) && authorization == null) { + challenge(response, HttpServletResponse.SC_UNAUTHORIZED, "WWW-Authenticate", "SID=new; Path=/", + "Digest realm=\"test\", nonce=\"dcd98b7102dd2f0e8b11d0f600bfb0c093\", qop=\"auth\""); + } else if ("/ntlm".equals(target) && authorization == null) { + challenge(response, HttpServletResponse.SC_UNAUTHORIZED, "WWW-Authenticate", "SID=mid; Path=/", + "NTLM"); + } else if ("/ntlm".equals(target) && authorization.startsWith("NTLM TlRMTVNTUAAB") + && "X=a b; SID=mid".equals(request.getHeader("Cookie"))) { + // The second round of the handshake rotates the session again. Only a first retry that + // carried the first rotation gets here; any other falls through and echoes what it sent. + challenge(response, HttpServletResponse.SC_UNAUTHORIZED, "WWW-Authenticate", "SID=new; Path=/", + "NTLM " + NTLM_TYPE_2); + } else if ("/proxied".equals(target) && request.getHeader("Proxy-Authorization") == null) { + // This server is the proxy too: the request is sent to it in absolute form. + challenge(response, HttpServletResponse.SC_PROXY_AUTHENTICATION_REQUIRED, "Proxy-Authenticate", + "SID=new; Path=/", "Basic realm=\"proxy\""); + } else if ("/protected".equals(target) && authorization == null) { + challenge(response, HttpServletResponse.SC_UNAUTHORIZED, "WWW-Authenticate", "SID=new; Path=/", + "Basic realm=\"test\""); } else { String cookie = request.getHeader("Cookie"); if (cookie != null) { @@ -87,6 +109,61 @@ void aCallerSetCookieHeaderDoesNotOutrankTheSessionTheChallengeSet() throws Exce } } + @Test + void aRawCookieWithANonStrictValueSurvivesTheRetry() throws Exception { + try (AsyncHttpClient client = asyncHttpClient()) { + String header = client.prepareGet(url("/protected")) + .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("; ")))); + } + } + + @Test + void aRawCookieSurvivesADigestRetryBesideTheSessionTheChallengeSet() throws Exception { + assertEquals("X=a b; SID=new", rawCookieOnRetry("SID=old; X=a b", client -> client.prepareGet(url("/digest")) + .setRealm(digestAuthRealm("user", "pass").setUsePreemptiveAuth(false)))); + } + + @Test + void aRawCookieSurvivesAProxyAuthenticationRetryBesideTheSessionTheChallengeSet() throws Exception { + assertEquals("X=a b; SID=new", rawCookieOnRetry("SID=old; X=a b", client -> client.prepareGet(url("/proxied")) + .setProxyServer(proxyServer("localhost", port2) + .setRealm(basicAuthRealm("user", "pass").setUsePreemptiveAuth(false))))); + } + + /** NTLM takes two challenges, and the retry after each carries what that one set. */ + @Test + void aRawCookieSurvivesEveryRoundOfAnNtlmHandshake() throws Exception { + assertEquals("X=a b; SID=new", rawCookieOnRetry("SID=old; X=a b", client -> client.prepareGet(url("/ntlm")) + .setRealm(ntlmAuthRealm("Zaphod", "Beeblebrox").setNtlmDomain("Ursa-Minor").setNtlmHost("LightCity")))); + } + + /** Every ';' ends a raw pair, so a quote does not hide the session the challenge set in its neighbour's value. */ + @Test + void aQuoteInARawValueDoesNotShieldTheSessionTheChallengeSet() throws Exception { + assertEquals("X=\"a; SID=new", rawCookieOnRetry("X=\"a; SID=old", client -> client.prepareGet(url("/protected")) + .setRealm(basicAuthRealm("user", "pass").setUsePreemptiveAuth(false)))); + assertEquals("X=a\"b; SID=new", rawCookieOnRetry("X=a\"b; SID=old", client -> client.prepareGet(url("/digest")) + .setRealm(digestAuthRealm("user", "pass").setUsePreemptiveAuth(false)))); + } + + private static String rawCookieOnRetry(String header, Function request) + throws Exception { + try (AsyncHttpClient client = asyncHttpClient()) { + return request.apply(client).setHeader("Cookie", header) + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + } + } + + private static void challenge(HttpServletResponse response, int status, String authenticate, String setCookie, + String challenge) { + response.addHeader("Set-Cookie", setCookie); + response.setHeader(authenticate, challenge); + response.setStatus(status); + } + private Set cookiesOnRetry(DefaultCookie callerCookie) throws Exception { try (AsyncHttpClient client = asyncHttpClient()) { client.prepareGet(url("/seed")).execute().get(TIMEOUT, TimeUnit.SECONDS); diff --git a/client/src/test/java/org/asynchttpclient/netty/handler/intercept/RedirectCookieRotationTest.java b/client/src/test/java/org/asynchttpclient/netty/handler/intercept/RedirectCookieRotationTest.java index 1580f2f07..d6bc1881a 100644 --- a/client/src/test/java/org/asynchttpclient/netty/handler/intercept/RedirectCookieRotationTest.java +++ b/client/src/test/java/org/asynchttpclient/netty/handler/intercept/RedirectCookieRotationTest.java @@ -204,6 +204,126 @@ void aCallerSetCookieHeaderDoesNotOutrankTheSessionTheRedirectRotated() throws E } } + @Test + void aRawCookieWithAValueTheStrictEncoderRejectsSurvivesARedirect() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + String received = client.prepareGet(url("/bounce")) + .setHeader("Cookie", "X=a b; flag; Y=1") + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + assertEquals("X=a b; flag; Y=1", received); + } + } + + @Test + void aRawCookieWithANonStrictValueSurvivesA307() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + String received = client.preparePost(url("/login-307")) + .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")), cookiesSent(received)); + } + } + + @Test + void aRotatedCookieDoesNotDisplaceAnUnrelatedRawValue() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + String received = client.prepareGet(url("/login")) + .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")), cookiesSent(received)); + } + } + + @Test + void rotationPreservesQuotedAndValuelessRawPairs() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + String received = client.prepareGet(url("/login")) + .setHeader("Cookie", "SID=old; X=\"a;b\"; flag; Y=a b") + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + assertEquals("X=\"a;b\"; flag; Y=a b; SID=new", received); + } + } + + @Test + void aRawCookieTheRedirectDeletedIsNotResent() throws Exception { + assertEquals("X=a b", withRawCookie("SID=old; X=a b", client -> client.prepareGet(url("/logout")))); + assertNull(withRawCookie("SID=old", client -> client.prepareGet(url("/logout")))); + } + + /** + * Every ';' ends a raw pair, as it does where the request factory merges the header: a quote must not hide a + * pair the redirect replaced in its neighbour's value, or the stale one is resent and the new one dropped. + */ + @Test + void aQuoteInARawValueDoesNotShieldTheSessionTheRedirectRotated() throws Exception { + assertEquals("X=a\"b; SID=new", withRawCookie("X=a\"b; SID=old", client -> client.prepareGet(url("/login")))); + assertEquals("X=\"a; SID=new", withRawCookie("X=\"a; SID=old\"", client -> client.prepareGet(url("/login")))); + } + + @Test + void aQuoteInARawValueDoesNotShieldACookieTheRedirectDeleted() throws Exception { + assertEquals("X=\"a", withRawCookie("X=\"a; SID=old", client -> client.prepareGet(url("/logout")))); + } + + /** Only the replaced pair is cut out; what is left, separators included, goes out as the caller wrote it. */ + @Test + void theRawPairsARotationKeepsAreSentByteForByte() throws Exception { + assertEquals("prefs={\"a\":\"x;y\"}; SID=new", + withRawCookie("prefs={\"a\":\"x;y\"}; SID=old", client -> client.prepareGet(url("/login")))); + assertEquals("A=1;B=2; SID=new", withRawCookie("A=1;B=2; SID=old", client -> client.prepareGet(url("/login")))); + assertEquals("A=1;B=2; SID=new", withRawCookie("A=1;SID=old;B=2", client -> client.prepareGet(url("/login")))); + assertEquals("A=1; SID=new", withRawCookie("SID=a; A=1; sid=b", client -> client.prepareGet(url("/login")))); + } + + @Test + void aRotationReachesEveryCookieHeaderTheCallerSet() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + String received = client.prepareGet(url("/login")) + .addHeader("Cookie", "X=a b; SID=old") + .addHeader("Cookie", "A=1;B=2") + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + assertEquals("X=a b; A=1;B=2; SID=new", received); + } + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + String received = client.prepareGet(url("/login")) + .addHeader("Cookie", "A=1;B=2") + .addHeader("Cookie", "SID=old") + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + assertEquals("A=1;B=2; SID=new", received); + } + } + + @Test + void aRawCookieSurvivesARotationWithTheLaxEncoder() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true).setUseLaxCookieEncoder(true))) { + String received = client.prepareGet(url("/login")) + .setHeader("Cookie", "flag; X=a b; SID=old") + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + assertEquals("flag; X=a b; SID=new", received); + } + } + + @Test + void rawCookieStillWinsOverAStoredCookieOfTheSameName() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + client.prepareGet(url("/seed")).execute().get(TIMEOUT, TimeUnit.SECONDS); + String received = client.prepareGet(url("/bounce")) + .setHeader("Cookie", "SID=mine; X=a b") + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + assertEquals("SID=mine; X=a b", received); + } + } + + @Test + void rawCookieDoesNotCrossAnOriginBoundary() throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + String received = client.prepareGet(url("/elsewhere")) + .setHeader("Cookie", "X=a b") + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + assertNull(received); + } + } + /** A Set-Cookie the store refused, or filed for another path, does not replace the caller's cookie. */ @Test void aSetCookieThatDoesNotReachTheNextHopLeavesTheCallersCookie() throws Exception { @@ -242,6 +362,14 @@ private String withCallerCookie(String name, String value, Function request) + throws Exception { + try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) { + return request.apply(client).setHeader("Cookie", header) + .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE); + } + } + private static Set cookiesSent(String header) { return header == null ? new HashSet<>() : new HashSet<>(Arrays.asList(header.split("; "))); }