Address comments

Author: hyperxpro

client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java

@@ -186,9 +186,6 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
                 if (keepBody) {
                     ensureBodyReplayable(request, bodyRepresentation, future.isStreamConsumed());
                     requestBuilder = request.toBuilder();
-                    // These include what the store held before this response, which would outrank what the
-                    // redirect just set; the store adds back below whatever matches the new URI.
-                    requestBuilder.resetCookies();
                     if (!sameBase) {
                         // An explicitly resolved address and virtual host belong to the previous target.
                         requestBuilder.setAddress(null);
@@ -202,6 +199,14 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
                             .setProxyServer(request.getProxyServer())
                             .setRangeOffset(request.getRangeOffset());
                 }
+                // Without a cookie store every cookie on the request is the caller's, so a same-origin hop keeps
+                // them. With one, they also hold what the store had before this response, which would outrank
+                // what it just set, so the hop carries only the store's cookies for the new URI, added below.
+                if (config.getCookieStore() == null && !stripAuth) {
+                    requestBuilder.setCookies(request.getCookies());
+                } else {
+                    requestBuilder.resetCookies();
+                }
 
                 requestBuilder.setMethod(switchToGet ? GET : originalMethod)
                         .setFollowRedirect(true)

client/src/test/java/org/asynchttpclient/netty/handler/intercept/RedirectCookieRotationTest.java

@@ -15,10 +15,12 @@
  */
 package org.asynchttpclient.netty.handler.intercept;
 
+import io.netty.handler.codec.http.cookie.DefaultCookie;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import org.asynchttpclient.AbstractBasicTest;
 import org.asynchttpclient.AsyncHttpClient;
+import org.asynchttpclient.AsyncHttpClientConfig;
 import org.asynchttpclient.BoundRequestBuilder;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.handler.AbstractHandler;
@@ -65,9 +67,18 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
                     case "/logout-303":
                         redirect(response, HttpServletResponse.SC_SEE_OTHER, "SID=; Path=/; Max-Age=0", "/home");
                         break;
+                    case "/logout-307":
+                        redirect(response, 307, "SID=; Path=/; Max-Age=0", "/home");
+                        break;
                     case "/p/a":
                         redirect(response, HttpServletResponse.SC_FOUND, null, "/q/b");
                         break;
+                    case "/see-other":
+                        redirect(response, HttpServletResponse.SC_SEE_OTHER, null, "/home");
+                        break;
+                    case "/elsewhere":
+                        redirect(response, HttpServletResponse.SC_FOUND, null, "http://127.0.0.1:" + port1 + "/home");
+                        break;
                     default:
                         String cookie = request.getHeader("Cookie");
                         if (cookie != null) {
@@ -80,16 +91,23 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
     }
 
     @Test
-    void aSessionRotatedByARedirectIsTheOneSent() throws Exception {
-        assertEquals("SID=new", afterSeeding(client -> client.prepareGet(url("/login"))), "GET, 302");
-        assertEquals("SID=new", afterSeeding(client -> client.preparePost(url("/login"))), "POST, 302 to GET");
-        assertEquals("SID=new", afterSeeding(client -> client.preparePost(url("/login-307"))), "POST, 307");
+    void aGetRedirectSendsTheSessionItRotated() throws Exception {
+        assertEquals("SID=new", afterSeeding(client -> client.prepareGet(url("/login"))));
     }
 
     @Test
-    void aCookieARedirectDeletedStaysDeleted() throws Exception {
-        assertNull(afterSeeding(client -> client.prepareGet(url("/logout"))), "GET, 302");
-        assertNull(afterSeeding(client -> client.preparePost(url("/logout-303"))), "POST, 303");
+    void a307SendsTheSessionItRotated() throws Exception {
+        assertEquals("SID=new", afterSeeding(client -> client.preparePost(url("/login-307"))));
+    }
+
+    @Test
+    void aGetRedirectDoesNotResendACookieItDeleted() throws Exception {
+        assertNull(afterSeeding(client -> client.prepareGet(url("/logout"))));
+    }
+
+    @Test
+    void a307DoesNotResendACookieItDeleted() throws Exception {
+        assertNull(afterSeeding(client -> client.preparePost(url("/logout-307"))));
     }
 
     @Test
@@ -98,13 +116,46 @@ void aPathScopedCookieDoesNotFollowARedirectOutOfItsPath() throws Exception {
         assertFalse(received != null && received.contains("P="), "sent to /q/b: " + received);
     }
 
+    // The next two already hold on main, where a redirect to GET is built from scratch; they keep it that way.
+
+    @Test
+    void aPostRedirectedToGetSendsTheSessionItRotated() throws Exception {
+        assertEquals("SID=new", afterSeeding(client -> client.preparePost(url("/login"))));
+    }
+
+    @Test
+    void a303DoesNotResendACookieItDeleted() throws Exception {
+        assertNull(afterSeeding(client -> client.preparePost(url("/logout-303"))));
+    }
+
+    // Without a cookie store every cookie on the request is the caller's own.
+
+    @Test
+    void withoutAStoreTheCallersCookieFollowsASameOriginRedirect() throws Exception {
+        assertEquals("X=1", withoutAStore(client -> client.prepareGet(url("/login"))), "GET, 302");
+        assertEquals("X=1", withoutAStore(client -> client.preparePost(url("/see-other"))), "POST, 303");
+    }
+
+    @Test
+    void withoutAStoreTheCallersCookieStaysBehindOnACrossOriginRedirect() throws Exception {
+        assertNull(withoutAStore(client -> client.prepareGet(url("/elsewhere"))));
+    }
+
     private String afterSeeding(Function<AsyncHttpClient, BoundRequestBuilder> request) throws Exception {
         try (AsyncHttpClient client = asyncHttpClient(config().setFollowRedirect(true))) {
             client.prepareGet(url("/seed")).execute().get(TIMEOUT, TimeUnit.SECONDS);
             return request.apply(client).execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE);
         }
     }
 
+    private String withoutAStore(Function<AsyncHttpClient, BoundRequestBuilder> request) throws Exception {
+        AsyncHttpClientConfig noStore = config().setFollowRedirect(true).setCookieStore(null).build();
+        try (AsyncHttpClient client = asyncHttpClient(noStore)) {
+            return request.apply(client).addCookie(new DefaultCookie("X", "1"))
+                    .execute().get(TIMEOUT, TimeUnit.SECONDS).getHeader(RECEIVED_COOKIE);
+        }
+    }
+
     private static void redirect(HttpServletResponse response, int status, String setCookie, String location) {
         if (setCookie != null) {
             response.addHeader("Set-Cookie", setCookie);