diff --git a/vertx-core/src/main/asciidoc/http.adoc b/vertx-core/src/main/asciidoc/http.adoc index f739da3effa..46eb1dbd6a7 100644 --- a/vertx-core/src/main/asciidoc/http.adoc +++ b/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: diff --git a/vertx-core/src/main/java/io/vertx/core/http/ClientRedirectConfig.java b/vertx-core/src/main/java/io/vertx/core/http/ClientRedirectConfig.java index 5a5e154496f..53b43716179 100644 --- a/vertx-core/src/main/java/io/vertx/core/http/ClientRedirectConfig.java +++ b/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 diff --git a/vertx-core/src/main/java/io/vertx/core/http/HttpClientOptions.java b/vertx-core/src/main/java/io/vertx/core/http/HttpClientOptions.java index 7c27cbcbd50..0078044dc89 100755 --- a/vertx-core/src/main/java/io/vertx/core/http/HttpClientOptions.java +++ b/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 diff --git a/vertx-core/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java b/vertx-core/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java index 8fd5e4e7a72..65ded944d28 100644 --- a/vertx-core/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java +++ b/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 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 { 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 handler; synchronized (this) { @@ -452,7 +468,9 @@ private void handleEarlyHints(MultiMap headers) { void handleResponse(Promise 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> handler = redirectHandler; if (handler != null) { ContextInternal prev = context.beginDispatch(); @@ -538,23 +556,22 @@ private Future 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) { diff --git a/vertx-core/src/test/java/io/vertx/tests/http/HttpTest.java b/vertx-core/src/test/java/io/vertx/tests/http/HttpTest.java index d0a4741c991..5526ac33c8e 100644 --- a/vertx-core/src/test/java/io/vertx/tests/http/HttpTest.java +++ b/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);