From 81dc27987286da700b64af091a13cc1cb33d27ed Mon Sep 17 00:00:00 2001 From: Tomas Langer Date: Mon, 20 Nov 2023 19:07:28 +0100 Subject: [PATCH] Headers on server cannot be set after entity was sent, or output stream was requested (throws IllegalStateException) Content length is removed from headers before being sent if expecting chunked encoding in HTTP/1.1 Signed-off-by: Tomas Langer --- .../webserver/http2/Http2ServerResponse.java | 6 + .../OutputStreamAndContentLengthTest.java | 180 ++++++++++++++++++ .../webserver/http/ServerResponse.java | 3 + .../webserver/http1/Http1ServerResponse.java | 13 ++ 4 files changed, 202 insertions(+) create mode 100644 webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/OutputStreamAndContentLengthTest.java diff --git a/webserver/http2/src/main/java/io/helidon/webserver/http2/Http2ServerResponse.java b/webserver/http2/src/main/java/io/helidon/webserver/http2/Http2ServerResponse.java index 03ed6e48bd9..40328c7ba68 100644 --- a/webserver/http2/src/main/java/io/helidon/webserver/http2/Http2ServerResponse.java +++ b/webserver/http2/src/main/java/io/helidon/webserver/http2/Http2ServerResponse.java @@ -62,6 +62,12 @@ class Http2ServerResponse extends ServerResponseBase { @Override public Http2ServerResponse header(Header header) { + if (streamingEntity) { + throw new IllegalStateException("Cannot set response header after requesting output stream."); + } + if (isSent()) { + throw new IllegalStateException("Cannot set response header after response was already sent."); + } headers.set(header); return this; } diff --git a/webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/OutputStreamAndContentLengthTest.java b/webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/OutputStreamAndContentLengthTest.java new file mode 100644 index 00000000000..72f99e615be --- /dev/null +++ b/webserver/tests/webserver/src/test/java/io/helidon/webserver/tests/OutputStreamAndContentLengthTest.java @@ -0,0 +1,180 @@ +/* + * Copyright (c) 2023 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.helidon.webserver.tests; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.Random; + +import io.helidon.http.HeaderNames; +import io.helidon.http.HeaderValues; +import io.helidon.http.InternalServerException; +import io.helidon.http.Status; +import io.helidon.webclient.api.ClientResponseTyped; +import io.helidon.webclient.api.WebClient; +import io.helidon.webserver.http.HttpRules; +import io.helidon.webserver.http.ServerRequest; +import io.helidon.webserver.http.ServerResponse; +import io.helidon.webserver.testing.junit5.ServerTest; +import io.helidon.webserver.testing.junit5.SetUpRoute; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static io.helidon.common.testing.http.junit5.HttpHeaderMatcher.hasHeader; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + +@ServerTest +class OutputStreamAndContentLengthTest { + private static byte[] bytes; + private static byte[] smallBytes; + private final WebClient client; + + OutputStreamAndContentLengthTest(WebClient client) { + this.client = client; + } + + @SetUpRoute + static void routing(HttpRules rules) { + rules.post("/chunked", OutputStreamAndContentLengthTest::chunked) + .post("/content-length/good", OutputStreamAndContentLengthTest::goodContentLength) + .post("/content-length/out-of-order", OutputStreamAndContentLengthTest::outOfOrderContentLength) + .post("/content-length/out-of-order-small", OutputStreamAndContentLengthTest::outOfOrderContentLengthSmall); + } + + @BeforeEach + void beforeEach() { + OutputStreamAndContentLengthTest.bytes = new byte[1_000_000]; // 1 MBi + new Random().nextBytes(bytes); + OutputStreamAndContentLengthTest.smallBytes = new byte[32]; + new Random().nextBytes(smallBytes); + } + + @AfterEach + void afterEach() { + OutputStreamAndContentLengthTest.bytes = null; + OutputStreamAndContentLengthTest.smallBytes = null; + } + + @Test + void testOutputStreamChunked() { + ClientResponseTyped response = client.post("/chunked") + .outputStream(it -> { + try (it) { + new ByteArrayInputStream(bytes).transferTo(it); + } + }, byte[].class); + + assertThat(response.headers(), hasHeader(HeaderValues.TRANSFER_ENCODING_CHUNKED)); + assertThat(response.headers(), not(hasHeader(HeaderNames.CONTENT_LENGTH))); + assertThat(response.status(), is(Status.OK_200)); + assertThat(response.entity(), is(bytes)); + } + + @Test + void testOutputStreamWithContentLength() { + ClientResponseTyped response = client.post("/content-length/good") + .outputStream(it -> { + try (it) { + new ByteArrayInputStream(bytes).transferTo(it); + } + }, byte[].class); + + assertThat(response.headers(), not(hasHeader(HeaderValues.TRANSFER_ENCODING_CHUNKED))); + assertThat(response.headers(), hasHeader(HeaderNames.CONTENT_LENGTH)); + assertThat(response.status(), is(Status.OK_200)); + assertThat(response.entity(), is(bytes)); + } + + @Test + void testOutputStreamOutOfOrder() { + ClientResponseTyped response = client.post("/content-length/out-of-order") + .outputStream(it -> { + try (it) { + new ByteArrayInputStream(bytes).transferTo(it); + } + }, byte[].class); + + assertThat(response.headers(), hasHeader(HeaderValues.TRANSFER_ENCODING_CHUNKED)); + assertThat(response.headers(), not(hasHeader(HeaderNames.CONTENT_LENGTH))); + assertThat(response.status(), is(Status.OK_200)); + assertThat(response.entity(), is(bytes)); + } + + @Test + void testOutputStreamOutOfOrderSmallEntity() { + // this entity must be small enough to trigger content length optimization + ClientResponseTyped response = client.post("/content-length/out-of-order-small") + .outputStream(it -> { + try (it) { + new ByteArrayInputStream(smallBytes).transferTo(it); + } + }, byte[].class); + + assertThat(response.headers(), hasHeader(HeaderValues.TRANSFER_ENCODING_CHUNKED)); + assertThat(response.headers(), not(hasHeader(HeaderNames.CONTENT_LENGTH))); + assertThat(response.status(), is(Status.OK_200)); + assertThat(response.entity(), is(smallBytes)); + } + + private static void chunked(ServerRequest req, ServerResponse res) throws IOException { + try (OutputStream out = res.outputStream(); InputStream in = req.content().inputStream()) { + in.transferTo(out); + } + + } + + private static void goodContentLength(ServerRequest req, ServerResponse res) throws IOException { + // should not be chunked, as we have a content length + res.contentLength(bytes.length); + try (OutputStream out = res.outputStream(); InputStream in = req.content().inputStream()) { + in.transferTo(out); + } + } + + private static void outOfOrderContentLength(ServerRequest req, ServerResponse res) throws IOException { + // should transfer all data and not fail + try (OutputStream out = res.outputStream(); InputStream in = req.content().inputStream()) { + in.transferTo(out); + } + try { + res.contentLength(bytes.length); + throw new InternalServerException("Content length cannot be set after stream was requested", new RuntimeException()); + } catch (IllegalStateException ignored) { + // this is expected + } + } + + private static void outOfOrderContentLengthSmall(ServerRequest req, ServerResponse res) throws IOException { + // should transfer all data and not fail + OutputStream out = res.outputStream(); // intentionally not closing output stream + try (InputStream in = req.content().inputStream()) { + in.transferTo(out); + } + try { + res.contentLength(smallBytes.length); + throw new InternalServerException("Content length cannot be set after stream was requested", new RuntimeException()); + } catch (IllegalStateException ignored) { + // this is expected + } + } +} diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/http/ServerResponse.java b/webserver/webserver/src/main/java/io/helidon/webserver/http/ServerResponse.java index 53e9602afa7..0e29c829150 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/http/ServerResponse.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/http/ServerResponse.java @@ -90,9 +90,12 @@ default ServerResponse header(String name, String... values) { /** * Set header with a value. + * Headers cannot be set after {@link #outputStream()} method is called, or after the response was sent. * * @param header header value * @return this instance + * @throws java.lang.IllegalStateException in case a header is set after output stream was requested, + * or the response was sent * @see HeaderName */ ServerResponse header(Header header); diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java b/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java index 81d39e4ca98..4875d34c41b 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/http1/Http1ServerResponse.java @@ -53,6 +53,7 @@ * An HTTP/1 server response. */ class Http1ServerResponse extends ServerResponseBase { + private static final System.Logger LOGGER = System.getLogger(Http1ServerResponse.class.getName()); private static final byte[] HTTP_BYTES = "HTTP/1.1 ".getBytes(StandardCharsets.UTF_8); private static final byte[] OK_200 = "HTTP/1.1 200 OK\r\n".getBytes(StandardCharsets.UTF_8); private static final byte[] DATE = "Date: ".getBytes(StandardCharsets.UTF_8); @@ -140,6 +141,12 @@ static void nonEntityBytes(ServerResponseHeaders headers, @Override public Http1ServerResponse header(Header header) { + if (streamingEntity) { + throw new IllegalStateException("Cannot set response header after requesting output stream."); + } + if (isSent()) { + throw new IllegalStateException("Cannot set response header after response was already sent."); + } this.headers.set(header); return this; } @@ -588,6 +595,12 @@ private void write(BufferData buffer) throws IOException { // proper stream with multiple buffers, write status amd headers headers.add(STREAM_TRAILERS); } + // this is chunked encoding, if anybody managed to set content length, it would break everything + if (headers.contains(HeaderNames.CONTENT_LENGTH)) { + LOGGER.log(System.Logger.Level.WARNING, "Content length was set after stream was requested, " + + "the response is already chunked, cannot use content-length"); + headers.remove(HeaderNames.CONTENT_LENGTH); + } sendHeadersAndPrepare(); firstByte = false; BufferData combined = BufferData.create(firstBuffer, buffer);