Skip to content

fix(netty): keep caller Cookie headers as written on redirects and auth retries #2360

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.

Sign up for GitHub

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

Open
mkurz wants to merge 1 commit into
base: main
Choose a base branch
from
+300 −41
Open

fix(netty): keep caller Cookie headers as written on redirects and auth retries #2360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 80 additions & 29 deletions client/src/main/java/org/asynchttpclient/netty/handler/intercept/CallerCookies.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<Cookie> of(Request request, HttpResponse response, CookieStore cookieStore, Uri next,
ClientCookieDecoder cookieDecoder) {
List<Cookie> cookies = withHeaderCookies(request);
private static List<Cookie> of(Request request, CookieStore cookieStore, Set<String> setByResponse) {
List<Cookie> cookies = request.getCookies();
if (cookies.isEmpty()) {
return cookies;
}
List<Cookie> stored = cookieStore.get(request.getUri());
List<Cookie> 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<String> 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.
Expand Down Expand Up @@ -81,43 +91,84 @@ static List<Cookie> of(Request request, HttpResponse response, CookieStore cooki
setByResponse.add(cookie.name().toLowerCase(Locale.ROOT));
}
}
List<Cookie> stored = cookieStore.get(request.getUri());
List<Cookie> 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;
}

/**
* For a retry of the same request: the caller's cookies, then the store's current ones.
*/
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<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)) {
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.
* <p>
* 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<Cookie> withHeaderCookies(Request request) {
List<String> headers = request.getHeaders().getAll(COOKIE);
if (headers.isEmpty()) {
return request.getCookies();
private static List<String> headerCookies(List<String> headers, Set<String> setByResponse) {
if (setByResponse.isEmpty() || headers.isEmpty()) {
return headers;
}
List<Cookie> cookies = new ArrayList<>(request.getCookies());
List<String> 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<Cookie> stored, Cookie cookie) {
Expand Down
17 changes: 10 additions & 7 deletions client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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;
Expand Down
87 changes: 82 additions & 5 deletions client/src/test/java/org/asynchttpclient/netty/handler/intercept/AuthRetryCookieTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,30 +30,52 @@
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() {
return new AbstractHandler() {
@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) {
Expand Down Expand Up @@ -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<AsyncHttpClient, BoundRequestBuilder> 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<String> cookiesOnRetry(DefaultCookie callerCookie) throws Exception {
try (AsyncHttpClient client = asyncHttpClient()) {
client.prepareGet(url("/seed")).execute().get(TIMEOUT, TimeUnit.SECONDS);
Expand Down
Loading
Loading