From dbff190d13e15cfd61b3b99158c2f6a6330ff242 Mon Sep 17 00:00:00 2001 From: Keith Lustria Date: Fri, 11 Aug 2023 00:12:44 -0700 Subject: [PATCH 1/4] Create a different config setting for inbound and outbound HTTP1 header validation for both client and server Description =========== Currently, Helidon 4 only support validate-headers config to validate headers for both inbound and outbound headers on both client and server. This change will bring separation on the configuration for both inbound and outbound headers on both client and server. The change will also remove performance impact for a singular config that would validate both outbound and inbound headers by default. By separating the configuration, only necessary headers will be validated by default. For example, the request headers on the client and the response headers on the server will not be validated by default because the user has control on their creation. On the other hand, the response headers on the client and the request headers on the server will be validated by default because the values they contain are unpredictable upon receipt and may contain malicious data. Following are new config options: 1. Client a. validate-response-header (inbound) - Validates the client response headers and will default to true because the content is unpredictable. b. validate-request-header (outbound) - Validates the client request headers and will default to false because the user has control on header creation. 2. Server b. validate-request-header (inbound) - Validates the server request headers and will default to true because the content is unpredictable.. a. validate-response-header (outbound) - Validates the client request headers and will default to false because the user has control on header creation. Details of the change: ===================== 1. Webserver a. Http1ConfigBlueprint - replace validateHeaders() with validateRequestHeaders() and validateResponseHeaders() to allow separate validation control of request headers and response headers, respectively. b. Http1Connection and HttpServerResponse - will process validateRequestHeaders() and validateResponseHeaders() on their corresponding areas of validation. 2. Webclient a. Http1ClientProtocolConfigBlueprint = replace validateHeaders() with validateRequestHeaders() and validateResponseHeaders() to allow separate validation control of request headers and response headers, respectively. b. Http1CallChainBase, Http1CallOutputStreamChain and Http1CallEntityChain - will process validateRequestHeaders() and validateResponseHeaders() on their corresponding areas of validation. c. Http1ClientTest - Header validation related Unit test are adjusted to use validateRequestHeaders() and validateResponseHeaders() instead of their predecessor validateHeaders() 3. Integration tests for Webserver - all files changed will perform various scenarios to validate request or response headers on the client side. 4. Integration tests for Webclient - all files changed will perform various scenarios to validate request or response headers on the client side. Documentation ============= This is a configuration change so documentation will be collected from javadoc comments in Http1ConfigBlueprint.java for the Webserver and Http1ClientProtocolConfigBlueprint.java for Http1 Webclient. Examples: 1. Http1 WebServer ``` ServerConnectionSelector http1 = Http1ConnectionSelector.builder() .config(Http1Config.builder() .validateRequestHeaders(true) .validateResponseHeaders(false) .build()) .build(); server.addConnectionSelector(http1); ``` 2. Http1 WebClient ``` Http1Client client = Http1Client.create(clientConfig -> clientConfig.baseUri(baseURI) .protocolConfig(it -> { it.validateRequestHeaders(true); it.validateResponseHeaders(false); }) ); ``` --- .../integration/webclient/webclient/pom.xml | 9 +- ...uestImplTest.java => Http1ClientTest.java} | 18 +- .../webclient/http1/ValidateHeadersTest.java | 229 ++++++++++++++++++ ...ntentEncodingDisabledNoValidationTest.java | 2 +- .../server/DisableValidateHeadersTest.java | 131 ++++++++++ .../server/ValidateRequestHeadersTest.java | 125 ++++++++++ .../server/ValidateResponseHeadersTest.java | 161 ++++++++++++ .../webclient/http1/Http1CallChainBase.java | 2 +- .../webclient/http1/Http1CallEntityChain.java | 2 +- .../http1/Http1CallOutputStreamChain.java | 2 +- .../Http1ClientProtocolConfigBlueprint.java | 17 +- ...uestImplTest.java => Http1ClientTest.java} | 40 ++- .../webserver/http1/Http1ConfigBlueprint.java | 20 +- .../nima/webserver/http1/Http1Connection.java | 13 +- .../webserver/http1/Http1ServerResponse.java | 33 ++- .../webserver/http1/ConnectionConfigTest.java | 6 +- .../src/test/resources/application.yaml | 3 +- 17 files changed, 765 insertions(+), 48 deletions(-) rename nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/{ClientRequestImplTest.java => Http1ClientTest.java} (97%) create mode 100644 nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java create mode 100644 nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/DisableValidateHeadersTest.java create mode 100644 nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateRequestHeadersTest.java create mode 100644 nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateResponseHeadersTest.java rename nima/webclient/http1/src/test/java/io/helidon/nima/webclient/http1/{ClientRequestImplTest.java => Http1ClientTest.java} (95%) diff --git a/nima/tests/integration/webclient/webclient/pom.xml b/nima/tests/integration/webclient/webclient/pom.xml index 570b0a31792..b6ae81b075d 100644 --- a/nima/tests/integration/webclient/webclient/pom.xml +++ b/nima/tests/integration/webclient/webclient/pom.xml @@ -32,10 +32,6 @@ io.helidon.nima.webclient helidon-nima-webclient - - io.helidon.nima.http2 - helidon-nima-http2-webclient - io.helidon.nima.webserver helidon-nima-webserver @@ -51,6 +47,11 @@ junit-jupiter-api test + + org.junit.jupiter + junit-jupiter-params + test + org.hamcrest hamcrest-all diff --git a/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ClientRequestImplTest.java b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java similarity index 97% rename from nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ClientRequestImplTest.java rename to nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java index ae6d75f3ac5..82945ea79d8 100644 --- a/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ClientRequestImplTest.java +++ b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java @@ -65,7 +65,7 @@ that is why this tests is in this module, but in the wrong package */ @ServerTest -class ClientRequestImplTest { +class Http1ClientTest { private static final Http.HeaderValue REQ_CHUNKED_HEADER = Http.Header.createCached( Http.Header.create("X-Req-Chunked"), "true"); private static final Http.HeaderValue REQ_EXPECT_100_HEADER_NAME = Http.Header.createCached( @@ -77,20 +77,20 @@ class ClientRequestImplTest { private final String baseURI; private final WebClient injectedHttp1client; - ClientRequestImplTest(WebServer webServer, WebClient client) { + Http1ClientTest(WebServer webServer, WebClient client) { baseURI = "http://localhost:" + webServer.port(); injectedHttp1client = client; } @SetUpRoute static void routing(HttpRules rules) { - rules.put("/test", ClientRequestImplTest::responseHandler); - rules.put("/redirectKeepMethod", ClientRequestImplTest::redirectKeepMethod); - rules.put("/redirect", ClientRequestImplTest::redirect); - rules.get("/afterRedirect", ClientRequestImplTest::afterRedirectGet); - rules.put("/afterRedirect", ClientRequestImplTest::afterRedirectPut); - rules.put("/chunkresponse", ClientRequestImplTest::chunkResponseHandler); - rules.put("/delayedEndpoint", ClientRequestImplTest::delayedHandler); + rules.put("/test", Http1ClientTest::responseHandler); + rules.put("/redirectKeepMethod", Http1ClientTest::redirectKeepMethod); + rules.put("/redirect", Http1ClientTest::redirect); + rules.get("/afterRedirect", Http1ClientTest::afterRedirectGet); + rules.put("/afterRedirect", Http1ClientTest::afterRedirectPut); + rules.put("/chunkresponse", Http1ClientTest::chunkResponseHandler); + rules.put("/delayedEndpoint", Http1ClientTest::delayedHandler); } @Test diff --git a/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java new file mode 100644 index 00000000000..797473b1399 --- /dev/null +++ b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java @@ -0,0 +1,229 @@ +/* + * 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.nima.webclient.http1; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.UncheckedIOException; +import java.net.SocketTimeoutException; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Stream; +import java.util.concurrent.TimeUnit; + +import io.helidon.common.GenericType; +import io.helidon.common.http.Headers; +import io.helidon.common.http.Http; +import io.helidon.common.http.Http.HeaderName; +import io.helidon.common.http.ServerRequestHeaders; +import io.helidon.nima.http.media.EntityReader; +import io.helidon.nima.http.media.EntityWriter; +import io.helidon.nima.http.media.MediaContext; +import io.helidon.nima.http.media.MediaContextConfig; +import io.helidon.nima.testing.junit5.webserver.ServerTest; +import io.helidon.nima.testing.junit5.webserver.SetUpRoute; +import io.helidon.nima.webclient.api.ClientConnection; +import io.helidon.nima.webclient.api.ClientResponseTyped; +import io.helidon.nima.webclient.api.HttpClientRequest; +import io.helidon.nima.webclient.api.HttpClientResponse; +import io.helidon.nima.webclient.api.Proxy; +import io.helidon.nima.webclient.api.WebClient; +import io.helidon.nima.webserver.WebServer; +import io.helidon.nima.webserver.WebServerConfig; +import io.helidon.nima.webserver.http.HttpRules; +import io.helidon.nima.webserver.http.ServerRequest; +import io.helidon.nima.webserver.http.ServerResponse; +import io.helidon.nima.webserver.http1.Http1Config; +import io.helidon.nima.webserver.http1.Http1ConnectionSelector; +import io.helidon.nima.webserver.spi.ServerConnectionSelector; + +import io.helidon.nima.testing.junit5.webserver.SetUpServer; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import static io.helidon.common.testing.http.junit5.HttpHeaderMatcher.hasHeader; +import static io.helidon.common.testing.http.junit5.HttpHeaderMatcher.noHeader; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.IsNot.not; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Test for validating client side outbound/inbound headers (request/response headers) + */ +@ServerTest +class ValidateHeadersTest { + public static final String VALID_HEADER_NAME = "Valid-Header-Name"; + public static final String VALID_HEADER_VALUE = "Valid-Header-Value"; + private final String baseURI; + + ValidateHeadersTest(WebServer webServer) { + baseURI = "http://localhost:" + webServer.port(); + } + + @SetUpServer + static void server(WebServerConfig.Builder server) { + ServerConnectionSelector http1 = Http1ConnectionSelector.builder() + .config(Http1Config.builder() + .validateRequestHeaders(false) + .validateResponseHeaders(false) + .build()) + .build(); + + server.addConnectionSelector(http1); + } + + @SetUpRoute + static void routing(HttpRules rules) { + rules.put("/test", ValidateHeadersTest::headerValidationHandler); + } + + @ParameterizedTest + @MethodSource("customHeaders") + void testRequestHeaders(String headerName, String headerValue, boolean expectsValid) { + Http1Client client = Http1Client.create(clientConfig -> clientConfig.baseUri(baseURI) + .protocolConfig(it -> { + it.validateRequestHeaders(true); + it.validateResponseHeaders(false); + }) + ); + Http1ClientRequest request = client.put(baseURI + "/test"); + request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + if (expectsValid) { + HttpClientResponse response = request.request(); + assertThat(response.status(), is(Http.Status.OK_200)); + } else { + assertThrows(IllegalArgumentException.class, () -> request.request()); + } + } + + @ParameterizedTest + @MethodSource("customHeaders") + void testResponsetHeaders(String headerName, String headerValue, boolean expectsValid) { + Http1Client client = Http1Client.create(clientConfig -> clientConfig.baseUri(baseURI) + .protocolConfig(it -> { + it.validateRequestHeaders(false); + it.validateResponseHeaders(true); + }) + ); + Http1ClientRequest request = client.put(baseURI + "/test"); + request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + if (expectsValid) { + HttpClientResponse response = request.request(); + assertThat(response.status(), is(Http.Status.OK_200)); + String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + assertThat(responseHeaderValue, is(headerValue.trim())); + } else { + assertThrows(IllegalArgumentException.class, () -> request.request()); + } + } + + @ParameterizedTest + @MethodSource("customHeaders") + void testOutputStreamResponsetHeaders(String headerName, String headerValue, boolean expectsValid) { + Http1Client client = Http1Client.create(clientConfig -> clientConfig.baseUri(baseURI) + .protocolConfig(it -> { + it.validateRequestHeaders(false); + it.validateResponseHeaders(true); + }) + .sendExpectContinue(false) + ); + Http1ClientRequest request = client.put(baseURI + "/test"); + request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + if (expectsValid) { + HttpClientResponse response = request.outputStream(it -> { + it.write("Foo Bar".getBytes(StandardCharsets.UTF_8)); + it.close(); + }); + assertThat(response.status(), is(Http.Status.OK_200)); + String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + assertThat(responseHeaderValue, is(headerValue.trim())); + } else { + assertThrows( + IllegalArgumentException.class, () -> request.outputStream(it -> { + it.write("Foo Bar".getBytes(StandardCharsets.UTF_8)); + it.close(); + }) + ); + } + } + + @ParameterizedTest + @MethodSource("customHeaders") + void testDisableHeaderValidation(String headerName, String headerValue, boolean expectsValid) { + Http1Client client = Http1Client.create(clientConfig -> clientConfig.baseUri(baseURI) + .protocolConfig(it -> { + it.validateRequestHeaders(false); + it.validateResponseHeaders(false); + }) + ); + Http1ClientRequest request = client.put(baseURI + "/test"); + request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + HttpClientResponse response = request.request(); + assertThat(response.status(), is(Http.Status.OK_200)); + String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + assertThat(responseHeaderValue, is(headerValue.trim())); + } + + private static void headerValidationHandler(ServerRequest request, ServerResponse response) { + ServerRequestHeaders headers = request.headers(); + request.headers().toMap().forEach((k, v) -> { + if (k.contains("Header")) { + response.headers().add(Http.Header.create(Http.Header.create(k), v)); + } + }); + response.send("any"); + } + + private static Stream customHeaders() { + return Stream.of( + // Invalid header names + arguments("Header\u001aName", VALID_HEADER_VALUE, false), + arguments("Header\u000EName", VALID_HEADER_VALUE, false), + arguments("", VALID_HEADER_VALUE, false), + arguments("{Header=Name}", VALID_HEADER_VALUE, false), + arguments("\"HeaderName\"", VALID_HEADER_VALUE, false), + arguments("[\\HeaderName]", VALID_HEADER_VALUE, false), + arguments("@Header,Name;", VALID_HEADER_VALUE, false), + // Valid header names + arguments("!#$Custom~%&\'*Header+^`|", VALID_HEADER_VALUE, true), + arguments("Custom_0-9_a-z_A-Z_Header", VALID_HEADER_VALUE, true), + // Valid header values + arguments(VALID_HEADER_NAME, "Header Value", true), + arguments(VALID_HEADER_NAME, "HeaderValue1\u0009, Header=Value2", true), + arguments(VALID_HEADER_NAME, "Header\tValue", true), + // Invalid header values + arguments(VALID_HEADER_NAME, "HeaderV\u001calue1", false), + arguments(VALID_HEADER_NAME, "HeaderValue1, Header\u007fValue", false), + arguments(VALID_HEADER_NAME, "HeaderValue1\u001f, HeaderValue2", false) + ); + } +} diff --git a/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ContentEncodingDisabledNoValidationTest.java b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ContentEncodingDisabledNoValidationTest.java index 06c1fdb1a1b..6d1f97b3b7c 100644 --- a/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ContentEncodingDisabledNoValidationTest.java +++ b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ContentEncodingDisabledNoValidationTest.java @@ -51,7 +51,7 @@ static void server(WebServerConfig.Builder server) { ServerConnectionSelector http1 = Http1ConnectionSelector.builder() .config(http1Config -> http1Config // Headers validation is disabled - .validateHeaders(false)) + .validateRequestHeaders(false)) .build(); server.addConnectionSelector(http1) diff --git a/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/DisableValidateHeadersTest.java b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/DisableValidateHeadersTest.java new file mode 100644 index 00000000000..ef920b93eee --- /dev/null +++ b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/DisableValidateHeadersTest.java @@ -0,0 +1,131 @@ +/* + * Copyright (c) 2022, 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.nima.tests.integration.server; + +import java.util.stream.Stream; + +import io.helidon.common.http.Http; +import io.helidon.common.http.Http.Header; +import io.helidon.common.http.Http.HeaderName; +import io.helidon.common.http.ServerRequestHeaders; +import io.helidon.nima.testing.junit5.webserver.ServerTest; +import io.helidon.nima.testing.junit5.webserver.SetUpRoute; +import io.helidon.nima.testing.junit5.webserver.SetUpServer; +import io.helidon.nima.webclient.api.HttpClientResponse; +import io.helidon.nima.webclient.http1.Http1Client; +import io.helidon.nima.webclient.http1.Http1ClientRequest; +import io.helidon.nima.webclient.http1.Http1ClientResponse; +import io.helidon.nima.webserver.WebServerConfig; +import io.helidon.nima.webserver.http.HttpRules; +import io.helidon.nima.webserver.http.ServerRequest; +import io.helidon.nima.webserver.http.ServerResponse; +import io.helidon.nima.webserver.http1.Http1Config; +import io.helidon.nima.webserver.http1.Http1ConnectionSelector; +import io.helidon.nima.webserver.spi.ServerConnectionSelector; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +/** + * Test when header validation is disabled. Expectation is that the server will not fail but the client will. + */ +@ServerTest +class DisableValidateHeadersTest { + public static final String HEADER_NAME_VALUE_DELIMETER = "->"; + public static final String VALID_HEADER_NAME = "Valid-Header-Name"; + public static final String VALID_HEADER_VALUE = "Valid-Header-Value"; + private final Http1Client client; + + DisableValidateHeadersTest(Http1Client client) { + this.client = client; + } + + @SetUpServer + static void server(WebServerConfig.Builder server) { + ServerConnectionSelector http1 = Http1ConnectionSelector.builder() + .config(Http1Config.builder() + .validateRequestHeaders(false) + .validateResponseHeaders(false) + .build()) + .build(); + + server.addConnectionSelector(http1); + } + + @SetUpRoute + static void routing(HttpRules rules) { + rules.get("/test", DisableValidateHeadersTest::disabledHeaderValidationHandler); + } + + @ParameterizedTest + @MethodSource("customHeaders") + void testHeaders(String headerName, String headerValue, boolean expectsValid) { + Http1ClientRequest request = client.get("/test"); + request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + if (expectsValid) { + HttpClientResponse response = request.request(); + assertThat(response.status(), is(Http.Status.OK_200)); + String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + assertThat(responseHeaderValue, is(headerValue.trim())); + } else { + assertThrows(IllegalArgumentException.class, () -> request.request()); + } + } + + private static void disabledHeaderValidationHandler(ServerRequest request, ServerResponse response) { + ServerRequestHeaders headers = request.headers(); + request.headers().toMap().forEach((k, v) -> { + if (k.contains("Header")) { + response.headers().add(Http.Header.create(Http.Header.create(k), v)); + } + }); + response.send("any"); + } + + private static Stream customHeaders() { + return Stream.of( + // Invalid header names + arguments("Header\u001aName", VALID_HEADER_VALUE, false), + arguments("Header\u000EName", VALID_HEADER_VALUE, false), + arguments("(Header:Name)", VALID_HEADER_VALUE, false), + arguments("", VALID_HEADER_VALUE, false), + arguments("{Header=Name}", VALID_HEADER_VALUE, false), + arguments("\"HeaderName\"", VALID_HEADER_VALUE, false), + arguments("[\\HeaderName]", VALID_HEADER_VALUE, false), + arguments("@Header,Name;", VALID_HEADER_VALUE, false), + // Valid header names + arguments("!#$Custom~%&\'*Header+^`|", VALID_HEADER_VALUE, true), + arguments("Custom_0-9_a-z_A-Z_Header", VALID_HEADER_VALUE, true), + // Valid header values + arguments(VALID_HEADER_NAME, "Header Value", true), + arguments(VALID_HEADER_NAME, "HeaderValue1\u0009, Header=Value2", true), + arguments(VALID_HEADER_NAME, "Header\tValue", true), + // Invalid header values + arguments(VALID_HEADER_NAME, "HeaderV\u001calue1", false), + arguments(VALID_HEADER_NAME, "HeaderValue1, Header\u007fValue", false), + arguments(VALID_HEADER_NAME, "HeaderValue1\u001f, HeaderValue2", false) + ); + } +} diff --git a/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateRequestHeadersTest.java b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateRequestHeadersTest.java new file mode 100644 index 00000000000..51c9bb86d0b --- /dev/null +++ b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateRequestHeadersTest.java @@ -0,0 +1,125 @@ +/* + * Copyright (c) 2022, 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.nima.tests.integration.server; + +import java.util.stream.Stream; + +import io.helidon.common.http.Http; +import io.helidon.common.http.Http.Header; +import io.helidon.common.http.Http.HeaderName; +import io.helidon.common.http.ServerRequestHeaders; +import io.helidon.nima.testing.junit5.webserver.ServerTest; +import io.helidon.nima.testing.junit5.webserver.SetUpRoute; +import io.helidon.nima.testing.junit5.webserver.SetUpServer; +import io.helidon.nima.webclient.api.HttpClientResponse; +import io.helidon.nima.webclient.http1.Http1Client; +import io.helidon.nima.webclient.http1.Http1ClientRequest; +import io.helidon.nima.webclient.http1.Http1ClientResponse; +import io.helidon.nima.webserver.WebServerConfig; +import io.helidon.nima.webserver.http.HttpRules; +import io.helidon.nima.webserver.http.ServerRequest; +import io.helidon.nima.webserver.http.ServerResponse; +import io.helidon.nima.webserver.http1.Http1Config; +import io.helidon.nima.webserver.http1.Http1ConnectionSelector; +import io.helidon.nima.webserver.spi.ServerConnectionSelector; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +/** + * Test for validating inbound headers (request headers) + */ +@ServerTest +class ValidateRequestHeadersTest { + public static final String VALID_HEADER_NAME = "Valid-Header-Name"; + public static final String VALID_HEADER_VALUE = "Valid-Header-Value"; + private final Http1Client client; + + ValidateRequestHeadersTest(Http1Client client) { + this.client = client; + } + + @SetUpServer + static void server(WebServerConfig.Builder server) { + ServerConnectionSelector http1 = Http1ConnectionSelector.builder() + .config(Http1Config.builder() + .validateRequestHeaders(true) + .validateResponseHeaders(false) + .build()) + .build(); + + server.addConnectionSelector(http1); + } + + @SetUpRoute + static void routing(HttpRules rules) { + rules.get("/test", ValidateRequestHeadersTest::requestHandler); + } + + @ParameterizedTest + @MethodSource("requestHeaders") + void testHeadersFromResponse(String headerName, String headerValue, boolean expectsValid) { + Http1ClientRequest request = client.get("/test"); + request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + HttpClientResponse response = request.submit("any"); + if (expectsValid) { + assertThat(response.status(), is(Http.Status.OK_200)); + } else { + assertThat(response.status(), not(Http.Status.OK_200)); + } + } + + private static void requestHandler(ServerRequest request, ServerResponse response) { + ServerRequestHeaders headers = request.headers(); + response.send("any"); + } + + private static Stream requestHeaders() { + return Stream.of( + // Invalid header names + arguments("Header\u001aName", VALID_HEADER_VALUE, false), + arguments("Header\u000EName", VALID_HEADER_VALUE, false), + arguments("HeaderName\r\n", VALID_HEADER_VALUE, false), + arguments("(Header:Name)", VALID_HEADER_VALUE, false), + arguments("", VALID_HEADER_VALUE, false), + arguments("{Header=Name}", VALID_HEADER_VALUE, false), + arguments("\"HeaderName\"", VALID_HEADER_VALUE, false), + arguments("[\\HeaderName]", VALID_HEADER_VALUE, false), + arguments("@Header,Name;", VALID_HEADER_VALUE, false), + // Valid header names + arguments("!#$Custom~%&\'*Header+^`|", VALID_HEADER_VALUE, true), + arguments("Custom_0-9_a-z_A-Z_Header", VALID_HEADER_VALUE, true), + // Valid header values + arguments(VALID_HEADER_NAME, "Header Value", true), + arguments(VALID_HEADER_NAME, "HeaderValue1\u0009, Header=Value2", true), + arguments(VALID_HEADER_NAME, "Header\tValue", true), + // arguments(VALID_HEADER_NAME, " Header Value ", true), + // Invalid header values + arguments(VALID_HEADER_NAME, "H\u001ceaderValue1", false), + arguments(VALID_HEADER_NAME, "HeaderValue1, Header\u007fValue", false), + arguments(VALID_HEADER_NAME, "HeaderValue1\u001f, HeaderValue2", false) + ); + } +} diff --git a/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateResponseHeadersTest.java b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateResponseHeadersTest.java new file mode 100644 index 00000000000..d18061ba695 --- /dev/null +++ b/nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ValidateResponseHeadersTest.java @@ -0,0 +1,161 @@ +/* + * Copyright (c) 2022, 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.nima.tests.integration.server; + +import java.io.IOException; +import java.io.OutputStream; +import java.io.UncheckedIOException; +import java.util.stream.Stream; + +import io.helidon.common.http.Http; +import io.helidon.common.http.Http.Header; +import io.helidon.common.http.Http.HeaderName; +import io.helidon.common.http.ServerRequestHeaders; +import io.helidon.nima.testing.junit5.webserver.ServerTest; +import io.helidon.nima.testing.junit5.webserver.SetUpRoute; +import io.helidon.nima.testing.junit5.webserver.SetUpServer; +import io.helidon.nima.webclient.api.HttpClientResponse; +import io.helidon.nima.webclient.http1.Http1Client; +import io.helidon.nima.webclient.http1.Http1ClientRequest; +import io.helidon.nima.webclient.http1.Http1ClientResponse; +import io.helidon.nima.webserver.WebServerConfig; +import io.helidon.nima.webserver.http.HttpRules; +import io.helidon.nima.webserver.http.ServerRequest; +import io.helidon.nima.webserver.http.ServerResponse; +import io.helidon.nima.webserver.http1.Http1Config; +import io.helidon.nima.webserver.http1.Http1ConnectionSelector; +import io.helidon.nima.webserver.spi.ServerConnectionSelector; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +/** + * Test for validating outbound headers (response headers) + */ +@ServerTest +class ValidateResponseHeadersTest { + public static final String HEADER_NAME_VALUE_DELIMETER = "->"; + public static final String VALID_HEADER_NAME = "Valid-Header-Name"; + public static final String VALID_HEADER_VALUE = "Valid-Header-Value"; + private final Http1Client client; + + ValidateResponseHeadersTest(Http1Client client) { + this.client = client; + } + + @SetUpServer + static void server(WebServerConfig.Builder server) { + ServerConnectionSelector http1 = Http1ConnectionSelector.builder() + .config(Http1Config.builder() + .validateRequestHeaders(false) + .validateResponseHeaders(true) + .build()) + .build(); + + server.addConnectionSelector(http1); + } + + @SetUpRoute + static void routing(HttpRules rules) { + rules.get("/test", ValidateResponseHeadersTest::responseHandler) + .get("/testOutputStream", ValidateResponseHeadersTest::responseHandlerForOutputStream); + } + + @ParameterizedTest + @MethodSource("responseHeaders") + void testHeadersFromResponse(String headerName, String headerValue, boolean expectsValid) { + String headerNameAndValue = headerName + HEADER_NAME_VALUE_DELIMETER + headerValue; + Http1ClientRequest request = client.get("/test"); + HttpClientResponse response = request.submit(headerNameAndValue); + if (expectsValid) { + assertThat(response.status(), is(Http.Status.OK_200)); + String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + assertThat(responseHeaderValue, is(headerValue.trim())); + } else { + assertThat(response.status(), not(Http.Status.OK_200)); + } + } + + @ParameterizedTest + @MethodSource("responseHeaders") + void testHeadersFromResponseOutputStream(String headerName, String headerValue, boolean expectsValid) { + String headerNameAndValue = headerName + HEADER_NAME_VALUE_DELIMETER + headerValue; + Http1ClientRequest request = client.get("/testOutputStream"); + HttpClientResponse response = request.submit(headerNameAndValue); + if (expectsValid) { + assertThat(response.status(), is(Http.Status.OK_200)); + String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + assertThat(responseHeaderValue, is(headerValue.trim())); + } else { + assertThat(response.status(), not(Http.Status.OK_200)); + } + } + + private static void responseHandler(ServerRequest request, ServerResponse response) { + setHeader(request, response); + response.send("any"); + } + + private static void responseHandlerForOutputStream(ServerRequest request, ServerResponse response) { + setHeader(request, response); + try (OutputStream outputStream = response.outputStream()) { + outputStream.write("any".getBytes()); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + private static void setHeader(ServerRequest request, ServerResponse response) { + ServerRequestHeaders headers = request.headers(); + String[] header = request.content().as(String.class).split(HEADER_NAME_VALUE_DELIMETER); + response.headers().add(Http.Header.create(Http.Header.create(header[0]), header[1])); + } + + private static Stream responseHeaders() { + return Stream.of( + // Invalid header names + arguments("Header\u001aName", VALID_HEADER_VALUE, false), + arguments("Header\u000EName", VALID_HEADER_VALUE, false), + arguments("HeaderName\r\n", VALID_HEADER_VALUE, false), + arguments("(Header:Name)", VALID_HEADER_VALUE, false), + arguments("", VALID_HEADER_VALUE, false), + arguments("{Header=Name}", VALID_HEADER_VALUE, false), + arguments("\"HeaderName\"", VALID_HEADER_VALUE, false), + arguments("[\\HeaderName]", VALID_HEADER_VALUE, false), + arguments("@Header,Name;", VALID_HEADER_VALUE, false), + // Valid header names + arguments("!#$Custom~%&\'*Header+^`|", VALID_HEADER_VALUE, true), + arguments("Custom_0-9_a-z_A-Z_Header", VALID_HEADER_VALUE, true), + // Valid header values + arguments(VALID_HEADER_NAME, "Header Value", true), + arguments(VALID_HEADER_NAME, "HeaderValue1\u0009, Header=Value2", true), + arguments(VALID_HEADER_NAME, "Header\tValue", true), + // Invalid header values + arguments(VALID_HEADER_NAME, "H\u001ceaderValue1", false), + arguments(VALID_HEADER_NAME, "HeaderValue1, Header\u007fValue", false), + arguments(VALID_HEADER_NAME, "HeaderValue1\u001f, HeaderValue2", false) + ); + } +} diff --git a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallChainBase.java b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallChainBase.java index 5d1fd8e7726..7f04cf98e00 100644 --- a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallChainBase.java +++ b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallChainBase.java @@ -138,7 +138,7 @@ void prologue(BufferData nonEntityData, WebClientServiceRequest request, ClientU ClientResponseHeaders readHeaders(DataReader reader) { WritableHeaders writable = Http1HeadersParser.readHeaders(reader, protocolConfig.maxHeaderSize(), - protocolConfig.validateHeaders()); + protocolConfig.validateResponseHeaders()); return ClientResponseHeaders.create(writable, clientConfig.mediaTypeParserMode()); } diff --git a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallEntityChain.java b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallEntityChain.java index 8dcc25942fa..7c2f6f6e414 100644 --- a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallEntityChain.java +++ b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallEntityChain.java @@ -71,7 +71,7 @@ public WebClientServiceResponse doProceed(ClientConnection connection, headers.set(Http.Header.create(Http.Header.CONTENT_LENGTH, entityBytes.length)); - writeHeaders(headers, writeBuffer, protocolConfig().validateHeaders()); + writeHeaders(headers, writeBuffer, protocolConfig().validateRequestHeaders()); // we have completed writing the headers whenSent.complete(serviceRequest); diff --git a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallOutputStreamChain.java b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallOutputStreamChain.java index af281bde8db..f9cf72ed360 100644 --- a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallOutputStreamChain.java +++ b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1CallOutputStreamChain.java @@ -242,7 +242,7 @@ private void sendPrologueAndHeader() { writer.writeNow(prologue); BufferData headerBuffer = BufferData.growing(128); - writeHeaders(headers, headerBuffer, protocolConfig.validateHeaders()); + writeHeaders(headers, headerBuffer, protocolConfig.validateRequestHeaders()); writer.writeNow(headerBuffer); whenSent.complete(request); diff --git a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1ClientProtocolConfigBlueprint.java b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1ClientProtocolConfigBlueprint.java index 45dd3f7171c..ceafef3da4f 100644 --- a/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1ClientProtocolConfigBlueprint.java +++ b/nima/webclient/http1/src/main/java/io/helidon/nima/webclient/http1/Http1ClientProtocolConfigBlueprint.java @@ -62,13 +62,24 @@ default String type() { int maxStatusLineLength(); /** - * Sets whether the header format is validated or not. + * Sets whether the request header format is validated or not. + *

+ * Defaults to {@code false} as user has control on the header creation. + *

+ * + * @return whether request header validation should be enabled + */ + @ConfiguredOption("false") + boolean validateRequestHeaders(); + + /** + * Sets whether the response header format is validated or not. *

* Defaults to {@code true}. *

* - * @return whether header validation should be enabled + * @return whether response header validation should be enabled */ @ConfiguredOption("true") - boolean validateHeaders(); + boolean validateResponseHeaders(); } diff --git a/nima/webclient/http1/src/test/java/io/helidon/nima/webclient/http1/ClientRequestImplTest.java b/nima/webclient/http1/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java similarity index 95% rename from nima/webclient/http1/src/test/java/io/helidon/nima/webclient/http1/ClientRequestImplTest.java rename to nima/webclient/http1/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java index bcb30fbd1ef..6445bf77034 100644 --- a/nima/webclient/http1/src/test/java/io/helidon/nima/webclient/http1/ClientRequestImplTest.java +++ b/nima/webclient/http1/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java @@ -62,7 +62,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.params.provider.Arguments.arguments; -class ClientRequestImplTest { +class Http1ClientTest { public static final String VALID_HEADER_VALUE = "Valid-Header-Value"; public static final String VALID_HEADER_NAME = "Valid-Header-Name"; public static final String BAD_HEADER_PATH = "/badHeader"; @@ -240,7 +240,13 @@ void testRelativeUris(boolean relativeUris, boolean outputStream, String request @ParameterizedTest @MethodSource("headerValues") void testHeaderValues(List headerValues, boolean expectsValid) { - Http1ClientRequest request = client.get("http://localhost:" + dummyPort + "/test"); + Http1Client clientValidateRequestHeaders = Http1Client.builder() + .protocolConfig(it -> { + it.validateRequestHeaders(true); + it.validateResponseHeaders(false); + }) + .build(); + Http1ClientRequest request = clientValidateRequestHeaders.get("http://localhost:" + dummyPort + "/test"); request.header(Http.Header.create(Http.Header.create("HeaderName"), headerValues)); request.connection(new FakeHttp1ClientConnection()); if (expectsValid) { @@ -254,7 +260,13 @@ void testHeaderValues(List headerValues, boolean expectsValid) { @ParameterizedTest @MethodSource("headers") void testHeaders(Http.HeaderValue header, boolean expectsValid) { - Http1ClientRequest request = client.get("http://localhost:" + dummyPort + "/test"); + Http1Client clientValidateRequestHeaders = Http1Client.builder() + .protocolConfig(it -> { + it.validateRequestHeaders(true); + it.validateResponseHeaders(false); + }) + .build(); + Http1ClientRequest request = clientValidateRequestHeaders.get("http://localhost:" + dummyPort + "/test"); request.connection(new FakeHttp1ClientConnection()); request.header(header); if (expectsValid) { @@ -268,10 +280,13 @@ void testHeaders(Http.HeaderValue header, boolean expectsValid) { @ParameterizedTest @MethodSource("headers") void testDisableHeaderValidation(Http.HeaderValue header, boolean expectsValid) { - Http1Client clientWithNoHeaderValidation = Http1Client.builder() - .protocolConfig(it -> it.validateHeaders(false)) + Http1Client clientWithDisabledHeaderValidation = Http1Client.builder() + .protocolConfig(it -> { + it.validateRequestHeaders(false); + it.validateResponseHeaders(false); + }) .build(); - Http1ClientRequest request = clientWithNoHeaderValidation.put("http://localhost:" + dummyPort + "/test"); + Http1ClientRequest request = clientWithDisabledHeaderValidation.put("http://localhost:" + dummyPort + "/test"); request.header(header); request.connection(new FakeHttp1ClientConnection()); HttpClientResponse response = request.submit("Sending Something"); @@ -285,7 +300,13 @@ void testDisableHeaderValidation(Http.HeaderValue header, boolean expectsValid) @ParameterizedTest @MethodSource("responseHeaders") void testHeadersFromResponse(String headerName, String headerValue, boolean expectsValid) { - Http1ClientRequest request = client.get("http://localhost:" + dummyPort + BAD_HEADER_PATH); + Http1Client clientValidateResponseHeaders = Http1Client.builder() + .protocolConfig(it -> { + it.validateRequestHeaders(false); + it.validateResponseHeaders(true); + }) + .build(); + Http1ClientRequest request = clientValidateResponseHeaders.get("http://localhost:" + dummyPort + BAD_HEADER_PATH); request.connection(new FakeHttp1ClientConnection()); String headerNameAndValue = headerName + HEADER_NAME_VALUE_DELIMETER + headerValue; if (expectsValid) { @@ -302,7 +323,10 @@ void testHeadersFromResponse(String headerName, String headerValue, boolean expe @MethodSource("responseHeadersForDisabledValidation") void testDisableValidationForHeadersFromResponse(String headerName, String headerValue) { Http1Client clientWithNoHeaderValidation = Http1Client.builder() - .protocolConfig(it -> it.validateHeaders(false)) + .protocolConfig(it -> { + it.validateRequestHeaders(false); + it.validateResponseHeaders(false); + }) .build(); Http1ClientRequest request = clientWithNoHeaderValidation.put("http://localhost:" + dummyPort + BAD_HEADER_PATH); request.connection(new FakeHttp1ClientConnection()); diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ConfigBlueprint.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ConfigBlueprint.java index 2f3f9e200b8..c7270aac065 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ConfigBlueprint.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ConfigBlueprint.java @@ -60,11 +60,29 @@ interface Http1ConfigBlueprint extends ProtocolConfig { * are validated by format * (content length is always validated as it is part of protocol processing (other headers may be validated if * features use them)). + *

+ * Defaults to {@code true}. + *

* * @return whether to validate headers */ @ConfiguredOption("true") - boolean validateHeaders(); + boolean validateRequestHeaders(); + + /** + * Whether to validate headers. + * If set to false, any value is accepted, otherwise validates headers + known headers + * are validated by format + * (content length is always validated as it is part of protocol processing (other headers may be validated if + * features use them)). + *

+ * Defaults to {@code false} as user has control on the header creation. + *

+ * + * @return whether to validate headers + */ + @ConfiguredOption("false") + boolean validateResponseHeaders(); /** * If set to false, any path is accepted (even containing illegal characters). diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java index 6b4137f806a..cefa85798ea 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java @@ -104,7 +104,7 @@ public class Http1Connection implements ServerConnection, InterruptableTask headers) { writer, request, !request.headers() - .contains(HeaderValues.CONNECTION_CLOSE)); + .contains(HeaderValues.CONNECTION_CLOSE), + http1Config.validateResponseHeaders()); routing.route(ctx, request, response); // we have handled a request without request entity @@ -355,7 +356,7 @@ private void route(HttpPrologue prologue, WritableHeaders headers) { } } else { // Check whether Content-Encoding header is present when headers validation is enabled - if (http1Config.validateHeaders() && headers.contains(Http.Header.CONTENT_ENCODING)) { + if (http1Config.validateRequestHeaders() && headers.contains(Http.Header.CONTENT_ENCODING)) { throw RequestException.builder() .type(EventType.BAD_REQUEST) .request(DirectTransportRequest.create(prologue, headers)) @@ -382,7 +383,8 @@ private void route(HttpPrologue prologue, WritableHeaders headers) { writer, request, !request.headers() - .contains(HeaderValues.CONNECTION_CLOSE)); + .contains(HeaderValues.CONNECTION_CLOSE), + http1Config.validateResponseHeaders()); routing.route(ctx, request, response); @@ -436,7 +438,8 @@ private void handleRequestException(RequestException e) { byte[] message = response.entity().orElse(BufferData.EMPTY_BYTES); headers.set(Http.Header.create(Http.Header.CONTENT_LENGTH, String.valueOf(message.length))); - Http1ServerResponse.nonEntityBytes(headers, response.status(), buffer, response.keepAlive()); + Http1ServerResponse.nonEntityBytes(headers, response.status(), buffer, response.keepAlive(), + http1Config.validateResponseHeaders()); if (message.length != 0) { buffer.write(message); } diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java index 12a3d98a054..4ac41059ca3 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1ServerResponse.java @@ -78,12 +78,14 @@ class Http1ServerResponse extends ServerResponseBase { private ClosingBufferedOutputStream outputStream; private long entitySize; private String streamResult = ""; + private final boolean validateHeaders; Http1ServerResponse(ConnectionContext ctx, Http1ConnectionListener sendListener, DataWriter dataWriter, Http1ServerRequest request, - boolean keepAlive) { + boolean keepAlive, + boolean validateHeaders) { super(ctx, request); this.ctx = ctx; @@ -92,12 +94,14 @@ class Http1ServerResponse extends ServerResponseBase { this.request = request; this.headers = ServerResponseHeaders.create(); this.keepAlive = keepAlive; + this.validateHeaders = validateHeaders; } static void nonEntityBytes(ServerResponseHeaders headers, Http.Status status, BufferData buffer, - boolean keepAlive) { + boolean keepAlive, + boolean validateHeaders) { // first write status if (status == null || status == Http.Status.OK_200) { @@ -126,7 +130,7 @@ static void nonEntityBytes(ServerResponseHeaders headers, } // write headers followed by empty line - writeHeaders(headers, buffer); + writeHeaders(headers, buffer, validateHeaders); buffer.write('\r'); // "\r\n" - empty line after headers buffer.write('\n'); @@ -177,7 +181,8 @@ public OutputStream outputStream() { ctx, sendListener, request, - keepAlive); + keepAlive, + validateHeaders); int writeBufferSize = ctx.listenerContext().config().writeBufferSize(); outputStream = new ClosingBufferedOutputStream(bos, writeBufferSize); @@ -269,8 +274,11 @@ private void handleSinkData(Object data, MediaType mediaType) { } } - private static void writeHeaders(Headers headers, BufferData buffer) { + private static void writeHeaders(Headers headers, BufferData buffer, boolean validate) { for (HeaderValue header : headers) { + if (validate) { + header.validate(); + } header.writeHttp1Header(buffer); } } @@ -295,7 +303,7 @@ private BufferData responseBuffer(byte[] bytes) { // give some space for code and headers + entity BufferData responseBuffer = BufferData.growing(256 + bytes.length); - nonEntityBytes(headers, status(), responseBuffer, keepAlive); + nonEntityBytes(headers, status(), responseBuffer, keepAlive, validateHeaders); if (bytes.length > 0) { responseBuffer.write(bytes); } @@ -326,6 +334,7 @@ private static class BlockingOutputStream extends OutputStream { private boolean firstByte = true; private long responseBytesTotal; private boolean closing = false; + private boolean validateHeaders = false; private BlockingOutputStream(ServerResponseHeaders headers, WritableHeaders trailers, @@ -336,7 +345,8 @@ private BlockingOutputStream(ServerResponseHeaders headers, ConnectionContext ctx, Http1ConnectionListener sendListener, Http1ServerRequest request, - boolean keepAlive) { + boolean keepAlive, + boolean validateHeaders) { this.headers = headers; this.trailers = trailers; this.status = status; @@ -350,6 +360,7 @@ private BlockingOutputStream(ServerResponseHeaders headers, this.request = request; this.keepAlive = keepAlive; this.forcedChunked = headers.contains(HeaderValues.TRANSFER_ENCODING_CHUNKED); + this.validateHeaders = validateHeaders; } @Override @@ -424,7 +435,7 @@ void commit() { trailers.set(STREAM_STATUS_NAME, String.valueOf(status.get().code())); trailers.set(STREAM_RESULT_NAME, streamResult.get()); BufferData buffer = BufferData.growing(128); - writeHeaders(trailers, buffer); + writeHeaders(trailers, buffer, this.validateHeaders); buffer.write('\r'); // "\r\n" - empty line after headers buffer.write('\n'); dataWriter.write(buffer); @@ -460,7 +471,7 @@ private void write(BufferData buffer) throws IOException { sendListener.headers(ctx, headers); // write headers and payload part in one buffer to avoid TCP/ACK delay problems BufferData growing = BufferData.growing(256 + buffer.available()); - nonEntityBytes(headers, status.get(), growing, keepAlive); + nonEntityBytes(headers, status.get(), growing, keepAlive, validateHeaders); // check not exceeding content-length bytesWritten += buffer.available(); checkContentLength(buffer); @@ -513,7 +524,7 @@ private void sendFirstChunkOnly() { // at this moment, we must send headers sendListener.headers(ctx, headers); BufferData bufferData = BufferData.growing(contentLength + 256); - nonEntityBytes(headers, status.get(), bufferData, keepAlive); + nonEntityBytes(headers, status.get(), bufferData, keepAlive, validateHeaders); if (firstBuffer != null) { bufferData.write(firstBuffer); @@ -544,7 +555,7 @@ private void sendHeadersAndPrepare() { // at this moment, we must send headers sendListener.headers(ctx, headers); BufferData bufferData = BufferData.growing(256); - nonEntityBytes(headers, status.get(), bufferData, keepAlive); + nonEntityBytes(headers, status.get(), bufferData, keepAlive, validateHeaders); sendListener.data(ctx, bufferData); responseBytesTotal += bufferData.available(); dataWriter.write(bufferData); diff --git a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java index 1880c92f155..084a311d6b6 100644 --- a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java +++ b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java @@ -48,13 +48,15 @@ void testConnectionConfig() { assertThat(http1Config.maxPrologueLength(), is(4096)); assertThat(http1Config.maxHeadersSize(), is(8192)); assertThat(http1Config.validatePath(), is(true)); - assertThat(http1Config.validateHeaders(), is(true)); + assertThat(http1Config.validateRequestHeaders(), is(true)); + assertThat(http1Config.validateResponseHeaders(), is(false)); http1Config = http1Configs.get("other"); assertThat(http1Config.maxPrologueLength(), is(81)); assertThat(http1Config.maxHeadersSize(), is(42)); assertThat(http1Config.validatePath(), is(false)); - assertThat(http1Config.validateHeaders(), is(false)); + assertThat(http1Config.validateRequestHeaders(), is(false)); + assertThat(http1Config.validateResponseHeaders(), is(true)); } } diff --git a/nima/webserver/webserver/src/test/resources/application.yaml b/nima/webserver/webserver/src/test/resources/application.yaml index 4e33aee74c6..7cf1c19f59c 100644 --- a/nima/webserver/webserver/src/test/resources/application.yaml +++ b/nima/webserver/webserver/src/test/resources/application.yaml @@ -31,7 +31,8 @@ server: protocols: providers: http_1_1: - validate-headers: false + validate-request-headers: false + validate-response-headers: true validate-path: false max-prologue-length: 81 max-headers-size: 42 From 32313d9bae3ba2bd22dc561e95218a4d606da390 Mon Sep 17 00:00:00 2001 From: Keith Lustria Date: Fri, 11 Aug 2023 14:26:23 -0700 Subject: [PATCH 2/4] Rename ClientRequestImplTest class to Http1ClientTest as this is not focused to ClientRequestImplTest anymore --- .../java/io/helidon/nima/webclient/http1/Http1ClientTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java index 872a4c66cbe..b9dff251393 100644 --- a/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java +++ b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/Http1ClientTest.java @@ -65,7 +65,7 @@ that is why this tests is in this module, but in the wrong package */ @ServerTest -class ClientRequestImplTest { +class Http1ClientTest { private static final Http.Header REQ_CHUNKED_HEADER = Http.Headers.createCached( Http.HeaderNames.create("X-Req-Chunked"), "true"); private static final Http.Header REQ_EXPECT_100_HEADER_NAME = Http.Headers.createCached( From 7967092732e5ff0d5e1654726d362e76683e3425 Mon Sep 17 00:00:00 2001 From: Keith Lustria Date: Fri, 11 Aug 2023 14:31:27 -0700 Subject: [PATCH 3/4] Update ValidateHeadersTest in tests/integration/webclient/webclient to use the new Headers and HeaderNames apis --- .../webclient/http1/ValidateHeadersTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java index 797473b1399..5ba77ce59d9 100644 --- a/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java +++ b/nima/tests/integration/webclient/webclient/src/test/java/io/helidon/nima/webclient/http1/ValidateHeadersTest.java @@ -116,7 +116,7 @@ void testRequestHeaders(String headerName, String headerValue, boolean expectsVa }) ); Http1ClientRequest request = client.put(baseURI + "/test"); - request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + request.header(Http.Headers.create(Http.HeaderNames.create(headerName), headerValue)); if (expectsValid) { HttpClientResponse response = request.request(); assertThat(response.status(), is(Http.Status.OK_200)); @@ -135,11 +135,11 @@ void testResponsetHeaders(String headerName, String headerValue, boolean expects }) ); Http1ClientRequest request = client.put(baseURI + "/test"); - request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + request.header(Http.Headers.create(Http.HeaderNames.create(headerName), headerValue)); if (expectsValid) { HttpClientResponse response = request.request(); assertThat(response.status(), is(Http.Status.OK_200)); - String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + String responseHeaderValue = response.headers().get(Http.HeaderNames.create(headerName)).values(); assertThat(responseHeaderValue, is(headerValue.trim())); } else { assertThrows(IllegalArgumentException.class, () -> request.request()); @@ -157,14 +157,14 @@ void testOutputStreamResponsetHeaders(String headerName, String headerValue, boo .sendExpectContinue(false) ); Http1ClientRequest request = client.put(baseURI + "/test"); - request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + request.header(Http.Headers.create(Http.HeaderNames.create(headerName), headerValue)); if (expectsValid) { HttpClientResponse response = request.outputStream(it -> { it.write("Foo Bar".getBytes(StandardCharsets.UTF_8)); it.close(); }); assertThat(response.status(), is(Http.Status.OK_200)); - String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + String responseHeaderValue = response.headers().get(Http.HeaderNames.create(headerName)).values(); assertThat(responseHeaderValue, is(headerValue.trim())); } else { assertThrows( @@ -186,10 +186,10 @@ void testDisableHeaderValidation(String headerName, String headerValue, boolean }) ); Http1ClientRequest request = client.put(baseURI + "/test"); - request.header(Http.Header.create(Http.Header.create(headerName), headerValue)); + request.header(Http.Headers.create(Http.HeaderNames.create(headerName), headerValue)); HttpClientResponse response = request.request(); assertThat(response.status(), is(Http.Status.OK_200)); - String responseHeaderValue = response.headers().get(Http.Header.create(headerName)).values(); + String responseHeaderValue = response.headers().get(Http.HeaderNames.create(headerName)).values(); assertThat(responseHeaderValue, is(headerValue.trim())); } @@ -197,7 +197,7 @@ private static void headerValidationHandler(ServerRequest request, ServerRespons ServerRequestHeaders headers = request.headers(); request.headers().toMap().forEach((k, v) -> { if (k.contains("Header")) { - response.headers().add(Http.Header.create(Http.Header.create(k), v)); + response.headers().add(Http.Headers.create(Http.HeaderNames.create(k), v)); } }); response.send("any"); From 44ee17e72b5ec3fadd2bbcf4c0fe07841ea6ac94 Mon Sep 17 00:00:00 2001 From: Keith Lustria Date: Sun, 13 Aug 2023 20:23:26 -0700 Subject: [PATCH 4/4] Fix inadvertent removal of http2 webclient dependency in the webclient integration test --- nima/tests/integration/webclient/webclient/pom.xml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/nima/tests/integration/webclient/webclient/pom.xml b/nima/tests/integration/webclient/webclient/pom.xml index b6ae81b075d..d250a03b898 100644 --- a/nima/tests/integration/webclient/webclient/pom.xml +++ b/nima/tests/integration/webclient/webclient/pom.xml @@ -32,11 +32,20 @@ io.helidon.nima.webclient helidon-nima-webclient + + io.helidon.nima.http2 + helidon-nima-http2-webclient + io.helidon.nima.webserver helidon-nima-webserver test + + io.helidon.nima.testing.junit5 + helidon-nima-testing-junit5-http2 + test + io.helidon.nima.testing.junit5 helidon-nima-testing-junit5-webserver