Fixes #8770 - Review whether to send request body in redirects. (#8775)

* Fixes #8770 - Review whether to send request body in redirects.

Now the original request body is re-sent only if the redirect status code is 307 or 308.
In the other cases, it is a redirect to a GET method, so the Location is followed without resending the body, and the content headers are removed.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-client/src/main/java/org/eclipse/jetty/client/HttpRedirector.java

@@ -28,7 +28,9 @@ import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.client.api.Result;
 import org.eclipse.jetty.client.util.BufferingResponseListener;
+import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
+import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.util.NanoTime;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -38,7 +40,7 @@ import org.slf4j.LoggerFactory;
  * <p>
  * Applications can disable redirection via {@link Request#followRedirects(boolean)}
  * and then rely on this class to perform the redirect in a simpler way, for example:
- * <pre>
+ * <pre>{@code
  * HttpRedirector redirector = new HttpRedirector(httpClient);
  *
  * Request request = httpClient.newRequest("http://host/path").followRedirects(false);
@@ -53,7 +55,7 @@ import org.slf4j.LoggerFactory;
  *     request = result.getRequest();
  *     response = result.getResponse();
  * }
- * </pre>
+ * }</pre>
  */
 public class HttpRedirector
 {
@@ -85,11 +87,11 @@ public class HttpRedirector
     {
         switch (response.getStatus())
         {
-            case 301:
+            case HttpStatus.MOVED_PERMANENTLY_301:
-            case 302:
+            case HttpStatus.MOVED_TEMPORARILY_302:
-            case 303:
+            case HttpStatus.SEE_OTHER_303:
-            case 307:
+            case HttpStatus.TEMPORARY_REDIRECT_307:
-            case 308:
+            case HttpStatus.PERMANENT_REDIRECT_308:
                 return true;
             default:
                 return false;
@@ -191,7 +193,7 @@ public class HttpRedirector
         int status = response.getStatus();
         switch (status)
         {
-            case 301:
+            case HttpStatus.MOVED_PERMANENTLY_301:
             {
                 String method = request.getMethod();
                 if (HttpMethod.GET.is(method) || HttpMethod.HEAD.is(method) || HttpMethod.PUT.is(method))
@@ -201,7 +203,7 @@ public class HttpRedirector
                 fail(request, response, new HttpResponseException("HTTP protocol violation: received 301 for non GET/HEAD/POST/PUT request", response));
                 return null;
             }
-            case 302:
+            case HttpStatus.MOVED_TEMPORARILY_302:
             {
                 String method = request.getMethod();
                 if (HttpMethod.HEAD.is(method) || HttpMethod.PUT.is(method))
@@ -209,7 +211,7 @@ public class HttpRedirector
                 else
                     return redirect(request, response, listener, newURI, HttpMethod.GET.asString());
             }
-            case 303:
+            case HttpStatus.SEE_OTHER_303:
             {
                 String method = request.getMethod();
                 if (HttpMethod.HEAD.is(method))
@@ -217,8 +219,8 @@ public class HttpRedirector
                 else
                     return redirect(request, response, listener, newURI, HttpMethod.GET.asString());
             }
-            case 307:
+            case HttpStatus.TEMPORARY_REDIRECT_307:
-            case 308:
+            case HttpStatus.PERMANENT_REDIRECT_308:
             {
                 // Keep same method
                 return redirect(request, response, listener, newURI, request.getMethod());
@@ -310,6 +312,30 @@ public class HttpRedirector
         {
             Request redirect = client.copyRequest(httpRequest, location);
 
+            // Use the given method.
+            redirect.method(method);
+
+            if (HttpMethod.GET.is(method))
+            {
+                redirect.body(null);
+                redirect.headers(headers ->
+                {
+                    headers.remove(HttpHeader.CONTENT_LENGTH);
+                    headers.remove(HttpHeader.CONTENT_TYPE);
+                });
+            }
+
+            Request.Content body = redirect.getBody();
+            if (body != null && !body.isReproducible())
+            {
+                if (LOG.isDebugEnabled())
+                    LOG.debug("Could not redirect to {}, request body is not reproducible", location);
+                HttpConversation conversation = httpRequest.getConversation();
+                conversation.updateResponseListeners(null);
+                notifier.forwardSuccessComplete(conversation.getResponseListeners(), httpRequest, response);
+                return null;
+            }
+
             // Adjust the timeout of the new request, taking into account the
             // timeout of the previous request and the time already elapsed.
             long timeoutNanoTime = httpRequest.getTimeoutNanoTime();
@@ -328,9 +354,6 @@ public class HttpRedirector
                 }
             }
 
-            // Use given method
-            redirect.method(method);
-
             redirect.onRequestBegin(request ->
             {
                 Throwable cause = httpRequest.getAbortCause();

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java

@@ -33,6 +33,7 @@ import javax.servlet.http.HttpServletResponse;
 import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.client.api.Result;
+import org.eclipse.jetty.client.util.AsyncRequestContent;
 import org.eclipse.jetty.client.util.ByteBufferRequestContent;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
@@ -163,6 +164,49 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
         assertArrayEquals(data, response.getContent());
     }
 
+    @ParameterizedTest
+    @ArgumentsSource(ScenarioProvider.class)
+    public void test303WithRequestContentNotReproducible(Scenario scenario) throws Exception
+    {
+        start(scenario, new RedirectHandler());
+
+        byte[] data = new byte[]{0, 1, 2, 3, 4, 5, 6, 7};
+        AsyncRequestContent body = new AsyncRequestContent(ByteBuffer.wrap(data));
+        body.close();
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+            .scheme(scenario.getScheme())
+            .method(HttpMethod.POST)
+            .path("/303/localhost/done")
+            .body(body)
+            .timeout(5, TimeUnit.SECONDS)
+            .send();
+        assertNotNull(response);
+        assertEquals(200, response.getStatus());
+        assertFalse(response.getHeaders().contains(HttpHeader.LOCATION));
+        assertEquals(0, response.getContent().length);
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(ScenarioProvider.class)
+    public void test307WithRequestContentNotReproducible(Scenario scenario) throws Exception
+    {
+        start(scenario, new RedirectHandler());
+
+        byte[] data = new byte[]{0, 1, 2, 3, 4, 5, 6, 7};
+        AsyncRequestContent body = new AsyncRequestContent(ByteBuffer.wrap(data));
+        body.close();
+        ContentResponse response = client.newRequest("localhost", connector.getLocalPort())
+            .scheme(scenario.getScheme())
+            .method(HttpMethod.POST)
+            .path("/307/localhost/done")
+            .body(body)
+            .timeout(5, TimeUnit.SECONDS)
+            .send();
+        assertNotNull(response);
+        assertEquals(307, response.getStatus());
+        assertTrue(response.getHeaders().contains(HttpHeader.LOCATION));
+    }
+
     @ParameterizedTest
     @ArgumentsSource(ScenarioProvider.class)
     public void testMaxRedirections(Scenario scenario) throws Exception
@@ -704,7 +748,7 @@ public class HttpClientRedirectTest extends AbstractHttpClientServerTest
                 location += "/" + path;
 
                 if (Boolean.parseBoolean(request.getParameter("decode")))
-                    location = URLDecoder.decode(location, "UTF-8");
+                    location = URLDecoder.decode(location, StandardCharsets.UTF_8);
 
                 response.setHeader("Location", location);
 

tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientContinueTest.java

@@ -238,9 +238,9 @@ public class HttpClientContinueTest extends AbstractTest<TransportScenario>
                 }
                 else
                 {
-                    // Send 100-Continue and consume the content
+                    // Send 100-Continue and consume the content.
-                    IO.copy(request.getInputStream(), new ByteArrayOutputStream());
+                    IO.copy(request.getInputStream(), OutputStream.nullOutputStream());
-                    // Send a redirect
+                    // Send a redirect.
                     response.sendRedirect("/done");
                 }
             }