From 7cd496bd95d00991b1e163fa1dfafc691edd9540 Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Thu, 15 Feb 2024 16:18:10 -0800 Subject: [PATCH] Validate content length early during HttpServerResponse#sendFile In `HttpServerResponse#sendFile`, when the offset is greater than the file length, the calculated content length is negative. Currently, the value is not validated until after the head (response status and other headers) is written. An application might have downstream processing that would act on the `IllegalArgumentException` thrown when validating the content length. However, it is too late for the downstream processor to write a different response status (for example a 400 BAD REQUEST), to signal the client that the request is not valid. The problem is exacerbated when the downstream processor tries to write a status code, because the `IllegalStateException` exception will be thrown because the `Response head is already sent`. This leaves the connection hanging on the server side, while the client is expecting content. --- .../core/http/impl/Http1xServerResponse.java | 13 +++++ .../core/http/impl/Http2ServerResponse.java | 11 +++- .../io/vertx/core/http/Http2ServerTest.java | 3 - .../java/io/vertx/core/http/HttpTest.java | 58 +++++++++++++++++++ 4 files changed, 81 insertions(+), 4 deletions(-) diff --git a/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java b/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java index 0f9e7e5156d..7a2fed16a4f 100644 --- a/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java +++ b/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java @@ -559,6 +559,19 @@ private void doSendFile(String filename, long offset, long length, Handler resultHandler.handle(Future.failedFuture(exception))); + } else { + log.error("Invalid offset: " + offset); + } + return; + } + bytesWritten = contentLength; if (!headers.contains(HttpHeaders.CONTENT_TYPE)) { String contentType = MimeMapping.getMimeTypeForFilename(filename); diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java b/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java index c4245336e2f..71c6d075210 100644 --- a/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java +++ b/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java @@ -619,7 +619,16 @@ public HttpServerResponse sendFile(String filename, long offset, long length, Ha HttpUtils.resolveFile(stream.vertx, filename, offset, length, ar -> { if (ar.succeeded()) { AsyncFile file = ar.result(); - long contentLength = Math.min(length, file.getReadLength()); + long fileLength = file.getReadLength(); + long contentLength = Math.min(length, fileLength); + + // fail early before status code/headers are written to the response + if (contentLength < 0) { + Exception exception = new IllegalArgumentException("offset : " + offset + " is larger than the requested file length : " + fileLength); + h.handle(Future.failedFuture(exception)); + return; + } + if (headers.get(HttpHeaderNames.CONTENT_LENGTH) == null) { putHeader(HttpHeaderNames.CONTENT_LENGTH, String.valueOf(contentLength)); } diff --git a/src/test/java/io/vertx/core/http/Http2ServerTest.java b/src/test/java/io/vertx/core/http/Http2ServerTest.java index e8253128739..ef62504b770 100644 --- a/src/test/java/io/vertx/core/http/Http2ServerTest.java +++ b/src/test/java/io/vertx/core/http/Http2ServerTest.java @@ -58,7 +58,6 @@ import io.vertx.core.buffer.Buffer; import io.vertx.core.http.impl.Http1xOrH2CHandler; import io.vertx.core.http.impl.HttpUtils; -import io.vertx.core.http.impl.headers.HeadersMultiMap; import io.vertx.core.impl.Utils; import io.vertx.core.streams.ReadStream; import io.vertx.core.streams.WriteStream; @@ -74,9 +73,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.net.InetSocketAddress; -import java.nio.channels.ClosedChannelException; import java.nio.charset.StandardCharsets; -import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; import java.util.Collections; diff --git a/src/test/java/io/vertx/core/http/HttpTest.java b/src/test/java/io/vertx/core/http/HttpTest.java index 9f369da2332..6aee9d8683c 100644 --- a/src/test/java/io/vertx/core/http/HttpTest.java +++ b/src/test/java/io/vertx/core/http/HttpTest.java @@ -2167,6 +2167,64 @@ public void testSendRangeFileFromClasspath() { await(); } + @Test + public void testSendZeroRangeFileFromClasspath() { + server.requestHandler(res -> { + // hosts_config.txt is 23 bytes + res.response().sendFile("hosts_config.txt", 23, 0); + }).listen(testAddress, onSuccess(res -> { + client.request(requestOptions) + .compose(HttpClientRequest::send) + .compose(resp -> { + assertEquals(String.valueOf(0), resp.headers().get("Content-Length")); + return resp.body(); + }) + .onComplete(onSuccess(body -> { + assertEquals("", body.toString()); + assertEquals(0, body.toString().length()); + testComplete(); + })); + })); + await(); + } + + @Test + public void testSendOffsetIsHigherThanFileLengthForFileFromClasspath() { + server.requestHandler(res -> { + try { + // hosts_config.txt is 23 bytes + res.response().sendFile("hosts_config.txt", 33, 10) + .onFailure(throwable -> { + try { + res.response().setStatusCode(500).end(); + } catch (IllegalStateException illegalStateException) { + // should not reach here, the response should not be sent if the offset is negative + fail(illegalStateException); + } + }); + } catch (Exception ex) { + // a route.route().failureHandler() would handle failures during + // handling of the request. Here we simulate that scenario, and during + // handling of failure we would like to set a status code for example. + try { + res.response().setStatusCode(505).end(); + fail("Should not reach here, failures should be handled by res.response().onFailure() above"); + } catch (IllegalStateException exceptionWhenTheResponseIsAlreadySent) { + // should not reach here, the response should not be sent if the offset is negative + fail(exceptionWhenTheResponseIsAlreadySent); + } + } + }).listen(testAddress, onSuccess(res -> { + client.request(requestOptions) + .compose(HttpClientRequest::send) + .onComplete(onSuccess(response -> { + assertEquals(500, response.statusCode()); + testComplete(); + })); + })); + await(); + } + @Test public void test100ContinueHandledAutomatically() { Buffer toSend = TestUtils.randomBuffer(1000);