Send the request body again when a redirection keeps the request method

vertx-core/src/main/asciidoc/http.adoc

@@ -1612,8 +1612,8 @@ The client can be configured to follow HTTP redirections provided by the `Locati
 * a `301`, `302`, `307` or `308` status code along with an HTTP GET, HEAD, or QUERY method
 * a `303` status code, in addition the directed request perform an HTTP GET method
 
-When redirecting a `QUERY` request, the request body is buffered in memory and resent to the redirected location.
-By default, the maximum size of this buffer is limited to `4KB` (configured via {@link io.vertx.core.http.ClientRedirectConfig#setMaxBufferedSize(int)}). If the body size exceeds this limit, the redirection is not followed.
+When a redirection keeps the request method, e.g. a `307` or `308` redirection of a `QUERY` request or of a `POST` request followed by a custom redirect handler, the request body is buffered in memory and sent again to the redirected location.
+By default, the maximum size of this buffer is limited to `4KB` (configured via {@link io.vertx.core.http.ClientRedirectConfig#setMaxBufferedSize(int)}). If the body size exceeds this limit, redirections other than `303` are not followed.
 
 Here's an example:
 

vertx-core/src/main/java/io/vertx/core/http/ClientRedirectConfig.java

@@ -65,14 +65,14 @@ public ClientRedirectConfig setMaxRedirects(int maxRedirects) {
   }
 
   /**
-   * @return the maximum size in bytes of the redirect buffer when redirecting QUERY requests
+   * @return the maximum size in bytes of the request body buffered in case a redirection keeps the request method
    */
   public int getMaxBufferedSize() {
     return maxBufferedSize;
   }
 
   /**
-   * Set the maximum size of the redirect buffer in bytes when redirecting QUERY requests.
+   * Set the maximum size in bytes of the request body buffered in case a redirection keeps the request method, e.g. a {@code 307} or {@code 308} redirection of a {@code POST} or {@code QUERY} request.
    *
    * @param maxBufferedSize the maximum buffer size
    * @return a reference to this, so the API can be used fluently

vertx-core/src/main/java/io/vertx/core/http/HttpClientOptions.java

@@ -933,14 +933,14 @@ public HttpClientOptions setMaxRedirects(int maxRedirects) {
   }
 
   /**
-   * @return the maximum size in bytes of the redirect buffer when redirecting QUERY requests
+   * @return the maximum size in bytes of the request body buffered in case a redirection keeps the request method
    */
   public int getMaxRedirectBufferedSize() {
     return maxRedirectBufferedSize;
   }
 
   /**
-   * Set the maximum size of the redirect buffer in bytes when redirecting QUERY requests.
+   * Set the maximum size in bytes of the request body buffered in case a redirection keeps the request method, e.g. a {@code 307} or {@code 308} redirection of a {@code POST} or {@code QUERY} request.
    *
    * @param maxRedirectBufferedSize the maximum buffer size
    * @return a reference to this, so the API can be used fluently

vertx-core/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java

@@ -63,6 +63,7 @@ public class HttpClientRequestImpl extends HttpClientRequestBase implements Http
   private String traceOperation;
   private int maxRedirectBufferSize = HttpClientOptions.DEFAULT_MAX_REDIRECT_BUFFERED_SIZE;
   private List<Buffer> bodyBuffer;
+  private boolean bodyBufferDiscarded;
 
   public HttpClientRequestImpl(HostAndPort authority, HttpConnection connection, HttpClientStream stream) {
     super(authority, connection, stream, stream.context().promise(), HttpMethod.GET, "/");
@@ -403,33 +404,48 @@ private void handleNextRequest(HttpClientRequest next, Promise<HttpClientRespons
     next.setMaxRedirects(maxRedirects);
     HttpClientRequestImpl nextImpl = (HttpClientRequestImpl) next;
     nextImpl.numberOfRedirections = numberOfRedirections + 1;
-    nextImpl.bodyBuffer = bodyBuffer;
     endFuture.onComplete(ar -> {
       if (ar.succeeded()) {
         if (timeoutMs > 0) {
           next.idleTimeout(timeoutMs);
         }
-        if (next.getMethod() == HttpMethod.QUERY && bodyBuffer != null && !bodyBuffer.isEmpty()) {
-          Buffer redirectBody;
-          if (bodyBuffer.size() == 1) {
-            redirectBody = bodyBuffer.get(0);
-          } else {
-            CompositeByteBuf composite = Unpooled.compositeBuffer();
-            for (Buffer b : bodyBuffer) {
-              composite.addComponent(true, ((BufferInternal) b).getByteBuf());
+        if (getMethod().equals(next.getMethod()) && canRedirectBody(getMethod())) {
+          // The redirection keeps the method, so the body is sent again
+          if (bodyBufferDiscarded) {
+            handler.tryFail(new VertxException("Cannot follow the " + getMethod() + " redirection: the request body " +
+              "exceeds the maximum size that can be buffered for redirections (" + maxRedirectBufferSize + " bytes)", true));
+            next.reset(0);
+            return;
+          }
+          if (bodyBuffer != null && !bodyBuffer.isEmpty()) {
+            Buffer redirectBody;
+            if (bodyBuffer.size() == 1) {
+              redirectBody = bodyBuffer.get(0);
+            } else {
+              CompositeByteBuf composite = Unpooled.compositeBuffer();
+              for (Buffer b : bodyBuffer) {
+                composite.addComponent(true, ((BufferInternal) b).getByteBuf());
+              }
+              redirectBody = BufferInternal.buffer(composite);
             }
-            redirectBody = BufferInternal.buffer(composite);
+            next.end(redirectBody);
+            return;
           }
-          next.end(redirectBody);
-        } else {
-          next.end();
         }
+        next.end();
       } else {
         next.reset(0);
       }
     });
   }
 
+  /**
+   * @return whether the body of a request using {@code method} is sent again when a redirection keeps the method
+   */
+  private static boolean canRedirectBody(HttpMethod method) {
+    return method != HttpMethod.GET && method != HttpMethod.HEAD && method != HttpMethod.CONNECT;
+  }
+
   private void handleContinue(Void v) {
     Handler<Void> handler;
     synchronized (this) {
@@ -452,7 +468,9 @@ private void handleEarlyHints(MultiMap headers) {
 
   void handleResponse(Promise<HttpClientResponse> promise, HttpClientResponse resp, long timeoutMs) {
     int statusCode = resp.statusCode();
-    if (followRedirects && numberOfRedirections < maxRedirects && statusCode >= 300 && statusCode < 400) {
+    if (followRedirects && numberOfRedirections < maxRedirects && statusCode >= 300 && statusCode < 400
+      && !(bodyBufferDiscarded && statusCode != 303)) {
+      // A redirection other than 303 may keep the method and therefore needs the body, which could not be buffered
       Function<HttpClientResponse, Future<HttpClientRequest>> handler = redirectHandler;
       if (handler != null) {
         ContextInternal prev = context.beginDispatch();
@@ -538,23 +556,22 @@ private Future<Void> doWrite(Buffer buff, boolean end, boolean connect) {
       if (trailersSent) {
         return context.failedFuture(new IllegalStateException("Request already complete"));
       }
-      if (followRedirects && getMethod() == HttpMethod.QUERY) {
-        if (buff != null) {
-          int currentLen = 0;
-          if (bodyBuffer != null) {
-            for (Buffer b : bodyBuffer) {
-              currentLen += b.length();
-            }
+      if (followRedirects && buff != null && !bodyBufferDiscarded && canRedirectBody(getMethod())) {
+        // Keep a copy of the body in case a redirection keeps the method, e.g. 307 and 308
+        int currentLen = 0;
+        if (bodyBuffer != null) {
+          for (Buffer b : bodyBuffer) {
+            currentLen += b.length();
           }
-          if (currentLen + buff.length() > maxRedirectBufferSize) {
-            followRedirects = false;
-            bodyBuffer = null;
-          } else {
-            if (bodyBuffer == null) {
-              bodyBuffer = new ArrayList<>();
-            }
-            bodyBuffer.add(buff.copy());
+        }
+        if (currentLen + buff.length() > maxRedirectBufferSize) {
+          bodyBufferDiscarded = true;
+          bodyBuffer = null;
+        } else {
+          if (bodyBuffer == null) {
+            bodyBuffer = new ArrayList<>();
           }
+          bodyBuffer.add(buff.copy());
         }
       }
       if (!headersSent) {

vertx-core/src/test/java/io/vertx/tests/http/HttpTest.java

@@ -3518,6 +3518,165 @@ public void testFollowRedirectQueryWithConfiguredBodyExceedingLimit() throws Exc
       .await();
   }
 
+  @Test
+  public void testFollowRedirectPostWithBodyOn307() throws Exception {
+    testFollowRedirectWithBodyKeepingMethod(HttpMethod.POST, 307, 2048);
+  }
+
+  @Test
+  public void testFollowRedirectPutWithBodyOn308() throws Exception {
+    testFollowRedirectWithBodyKeepingMethod(HttpMethod.PUT, 308, 2048);
+  }
+
+  @Test
+  public void testFollowRedirectPostWithMultipleBuffersOn307() throws Exception {
+    Buffer chunk1 = TestUtils.randomBuffer(1024);
+    Buffer chunk2 = TestUtils.randomBuffer(1024);
+    Buffer expected = Buffer.buffer().appendBuffer(chunk1).appendBuffer(chunk2);
+    server.requestHandler(req -> {
+      if ("/".equals(req.path())) {
+        assertEquals(HttpMethod.POST, req.method());
+        req.body().onComplete(TestUtils.onSuccess(body -> {
+          assertEquals(expected, body);
+          String scheme = req.connection().isSsl() ? "https" : "http";
+          req.response().setStatusCode(307).putHeader(HttpHeaders.LOCATION, scheme + "://" + config.host() + ":" + config.port() + "/whatever").end();
+        }));
+      } else if ("/whatever".equals(req.path())) {
+        req.body().onComplete(TestUtils.onSuccess(body -> {
+          // Fail with a status code instead of an assertion so that a missing body does not hang the test
+          req.response().setStatusCode(req.method() == HttpMethod.POST && expected.equals(body) ? 200 : 400).end();
+        }));
+      } else {
+        req.response().setStatusCode(404).end();
+      }
+    });
+    startServer(testAddress);
+    client = keepMethodRedirectClient();
+    RequestOptions opts = new RequestOptions()
+      .setMethod(POST)
+      .setHost(config.host())
+      .setPort(config.port());
+    client.request(opts).compose(req -> {
+        req.setFollowRedirects(true);
+        req.setChunked(true);
+        req.write(chunk1);
+        return req.end(chunk2).compose(v -> req.response()).expecting(HttpResponseExpectation.SC_OK);
+      })
+      .await();
+  }
+
+  @Test
+  public void testFollowRedirectPostWithBodyExceedingLimitOn307() throws Exception {
+    // The redirection keeps the method and the body could not be buffered: the redirection is not followed
+    Buffer expected = TestUtils.randomBuffer(4 * 1024 + 1);
+    server.requestHandler(req -> {
+      if ("/".equals(req.path())) {
+        assertEquals(HttpMethod.POST, req.method());
+        req.body().onComplete(TestUtils.onSuccess(body -> {
+          assertEquals(expected, body);
+          String scheme = req.connection().isSsl() ? "https" : "http";
+          req.response().setStatusCode(307).putHeader(HttpHeaders.LOCATION, scheme + "://" + config.host() + ":" + config.port() + "/whatever").end();
+        }));
+      } else {
+        // Fail with a status code instead of an assertion so that a wrongly followed redirection does not hang the test
+        req.response().setStatusCode(500).end();
+      }
+    });
+    startServer(testAddress);
+    client = keepMethodRedirectClient();
+    RequestOptions opts = new RequestOptions()
+      .setMethod(POST)
+      .setHost(config.host())
+      .setPort(config.port());
+    client.request(opts).compose(req -> req
+        .setFollowRedirects(true)
+        .send(expected)
+        .expecting(HttpResponseExpectation.status(307)))
+      .await();
+  }
+
+  @Test
+  public void testFollowRedirectPostWithBodyExceedingLimitOn303() throws Exception {
+    // The redirection changes the method to GET and does not need the body: the redirection is followed
+    Buffer expected = TestUtils.randomBuffer(4 * 1024 + 1);
+    server.requestHandler(req -> {
+      if ("/".equals(req.path())) {
+        assertEquals(HttpMethod.POST, req.method());
+        req.body().onComplete(TestUtils.onSuccess(body -> {
+          assertEquals(expected, body);
+          String scheme = req.connection().isSsl() ? "https" : "http";
+          req.response().setStatusCode(303).putHeader(HttpHeaders.LOCATION, scheme + "://" + config.host() + ":" + config.port() + "/whatever").end();
+        }));
+      } else if ("/whatever".equals(req.path())) {
+        assertEquals(HttpMethod.GET, req.method());
+        assertNull(req.getHeader(HttpHeaders.CONTENT_LENGTH));
+        req.response().end();
+      } else {
+        req.response().setStatusCode(404).end();
+      }
+    });
+    startServer(testAddress);
+    RequestOptions opts = new RequestOptions()
+      .setMethod(POST)
+      .setHost(config.host())
+      .setPort(config.port());
+    client.request(opts).compose(req -> req
+        .setFollowRedirects(true)
+        .send(expected)
+        .expecting(HttpResponseExpectation.SC_OK))
+      .await();
+  }
+
+  private void testFollowRedirectWithBodyKeepingMethod(HttpMethod method, int statusCode, int bodySize) throws Exception {
+    Buffer expected = TestUtils.randomBuffer(bodySize);
+    server.requestHandler(req -> {
+      if ("/".equals(req.path())) {
+        assertEquals(method, req.method());
+        req.body().onComplete(TestUtils.onSuccess(body -> {
+          assertEquals(expected, body);
+          String scheme = req.connection().isSsl() ? "https" : "http";
+          req.response().setStatusCode(statusCode).putHeader(HttpHeaders.LOCATION, scheme + "://" + config.host() + ":" + config.port() + "/whatever").end();
+        }));
+      } else if ("/whatever".equals(req.path())) {
+        req.body().onComplete(TestUtils.onSuccess(body -> {
+          // Fail with a status code instead of an assertion so that a missing body does not hang the test
+          req.response().setStatusCode(req.method() == method && expected.equals(body) ? 200 : 400).end();
+        }));
+      } else {
+        req.response().setStatusCode(404).end();
+      }
+    });
+    startServer(testAddress);
+    client = keepMethodRedirectClient();
+    RequestOptions opts = new RequestOptions()
+      .setMethod(method)
+      .setHost(config.host())
+      .setPort(config.port());
+    client.request(opts).compose(req -> req
+        .setFollowRedirects(true)
+        .send(expected)
+        .expecting(HttpResponseExpectation.SC_OK))
+      .await();
+  }
+
+  /**
+   * A client following 307 and 308 redirections with the same method, like an HTTP client compliant with RFC 9110 would do.
+   */
+  private HttpClientAgent keepMethodRedirectClient() {
+    return httpClientBuilder()
+      .withRedirectHandler(resp -> {
+        int status = resp.statusCode();
+        String location = resp.getHeader(HttpHeaders.LOCATION);
+        if ((status == 307 || status == 308) && location != null) {
+          return Future.succeededFuture(new RequestOptions()
+            .setMethod(resp.request().getMethod())
+            .setAbsoluteURI(location));
+        }
+        return null;
+      })
+      .build();
+  }
+
   @Test
   public void testFollowRedirectQueryWithMultipleBuffers() throws Exception {
     Buffer chunk1 = TestUtils.randomBuffer(1024);