Validate http request and response headers when using the webclient (#…

…6515)

* Hard reset of branch from latest

* Validate Webclient HTTP request and response headers. Change includes the following:

1. The main method for validating header value now exists in HTTP HeaderValue, so it can be reused in other places outside of WebClient, if needed.
2. HttpCallChainBase.writeHeaders will validate both the header name and values for the HTTP request using 3. Http.HeaderValue.validate().
3. Http1HeadersParser.readHeaders used for parsing response headers now includes validation of header name and values using Http.HeaderValue.validate().
4. Extensive unit test for each of the new features added.

* Remove validator from Lazy String and use HttpValue.validate() to validate header name and value after they are parsed in Http1HeadersParser.readHeaders().

Note: HttpToken.validate() used to validate when headerName is parsed in Http1HeadersParser.readHeaderName() was removed as it is already performed in the above change.

* Minor Http.HeaderValue.validate refactoring

common/buffers/src/main/java/io/helidon/common/buffers/LazyString.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 Oracle and/or its affiliates.
+ * 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.

common/buffers/src/test/java/io/helidon/common/buffers/LazyStringTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 Oracle and/or its affiliates.
+ * 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.

common/http/pom.xml

@@ -74,6 +74,11 @@
             <artifactId>junit-jupiter-api</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <artifactId>junit-jupiter-params</artifactId>
+            <groupId>org.junit.jupiter</groupId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.hamcrest</groupId>
             <artifactId>hamcrest-all</artifactId>

common/http/src/main/java/io/helidon/common/http/Http.java

@@ -238,7 +238,6 @@ public static Method create(String name) {
             return method;
         }
 
-
         /**
          * Create a predicate for the provided methods.
          *
@@ -960,6 +959,41 @@ default void writeHttp1Header(BufferData buffer) {
             }
         }
 
+        /**
+         * Check validity of header name and values.
+         *
+         * @throws IllegalArgumentException in case the HeaderValue is not valid
+         */
+        default void validate() throws IllegalArgumentException {
+            String name = name();
+            // validate that header name only contains valid characters
+            HttpToken.validate(name);
+            // Validate header value
+            validateValue(name, values());
+        }
+
+
+        // validate header value based on https://www.rfc-editor.org/rfc/rfc7230#section-3.2 and throws IllegalArgumentException
+        // if invalid.
+        private static void validateValue(String name, String value) throws IllegalArgumentException {
+            char[] vChars = value.toCharArray();
+            int vLength = vChars.length;
+            for (int i = 0; i < vLength; i++) {
+                char vChar = vChars[i];
+                if (i == 0) {
+                    if (vChar < '!' || vChar == '\u007f') {
+                        throw new IllegalArgumentException("First character of the header value is invalid"
+                                                                   + " for header '" + name + "'");
+                    }
+                } else {
+                    if (vChar < ' ' && vChar != '\t' || vChar == '\u007f') {
+                        throw new IllegalArgumentException("Character at position " + (i + 1) + " of the header value is invalid"
+                                                                   + " for header '" + name + "'");
+                    }
+                }
+            }
+        }
+
         private void writeHeader(BufferData buffer, byte[] nameBytes, byte[] valueBytes) {
             // header name
             buffer.write(nameBytes);

common/http/src/main/java/io/helidon/common/http/Http1HeadersParser.java

@@ -67,7 +67,11 @@ public static WritableHeaders<?> readHeaders(DataReader reader, int maxHeadersSi
             reader.skip(2);
             maxLength -= eol + 1;
 
-            headers.add(Http.Header.create(header, value));
+            Http.HeaderValue headerValue = Http.Header.create(header, value);
+            headers.add(headerValue);
+            if (validate) {
+                headerValue.validate();
+            }
             if (maxLength < 0) {
                 throw new IllegalStateException("Header size exceeded");
             }
@@ -113,9 +117,6 @@ private static Http.HeaderName readHeaderName(DataReader reader,
         }
 
         String headerName = reader.readAsciiString(col);
-        if (validate) {
-            HttpToken.validate(headerName);
-        }
         Http.HeaderName header = Http.Header.create(headerName);
         reader.skip(1); // skip the colon character
 

common/http/src/test/java/io/helidon/common/http/Http1HeadersParserTest.java

@@ -17,16 +17,27 @@
 package io.helidon.common.http;
 
 import java.nio.charset.StandardCharsets;
+import java.util.stream.Stream;
 
 import io.helidon.common.buffers.DataReader;
 
+import org.junit.jupiter.api.Assertions;
 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.hasItems;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
 
 class Http1HeadersParserTest {
+    private static String CUSTOM_HEADER_NAME = "Custom-Header-Name";
+    private static String CUSTOM_HEADER_VALUE = "Custom-Header-Value";
+    public static final String VALID_HEADER_VALUE = "Valid-Header-Value";
+    public static final String VALID_HEADER_NAME = "Valid-Header-Name";
+
     @Test
     void testHeadersAreCaseInsensitive() {
         DataReader reader = new DataReader(() -> (
@@ -44,6 +55,66 @@ void testHeadersAreCaseInsensitive() {
         testHeader(headers, "HeADer", "hv1", "hv2", "hv3");
     }
 
+    @ParameterizedTest
+    @MethodSource("headers")
+    void testHeadersWithValidationEnabled(String headerName, String headerValue, boolean expectsValid) {
+        // retrieve headers with validation enabled
+        WritableHeaders<?> headers;
+        if (expectsValid) {
+            headers = getHeaders(headerName, headerValue, true);
+            String responseHeaderValue = headers.get(Http.Header.create(headerName)).values();
+            // returned header values WhiteSpaces are trimmed so need to be tested with trimmed values
+            assertThat(responseHeaderValue, is(headerValue.trim()));
+        } else {
+            Assertions.assertThrows(IllegalArgumentException.class,
+                                    () -> getHeaders(headerName, headerValue, true));
+        }
+    }
+
+    @ParameterizedTest
+    @MethodSource("headers")
+    void testHeadersWithValidationDisabled(String headerValue) {
+        // retrieve headers without validating
+        WritableHeaders<?> headers = getHeaders(CUSTOM_HEADER_NAME, headerValue, false);
+        String responseHeaderValue = headers.get(Http.Header.create(CUSTOM_HEADER_NAME)).values();
+        // returned header values WhiteSpaces are trimmed so need to be tested with trimmed values
+        assertThat(responseHeaderValue, is(headerValue.trim()));
+    }
+
+    private static WritableHeaders<?> getHeaders(String headerName, String headerValue, boolean validate) {
+        DataReader reader =
+                new DataReader(() -> (headerName + ":" + headerValue + "\r\n" + "\r\n").getBytes(StandardCharsets.US_ASCII));
+        return Http1HeadersParser.readHeaders(reader, 1024, validate);
+    }
+
+    private static Stream<Arguments> headers() {
+        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("<Header?Name>", 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)
+        );
+    }
+
+
     private void testHeader(Headers headers, String header, String... values) {
         Http.HeaderName headerName = Http.Header.create(header);
         assertThat("Headers should contain header: " + headerName.lowerCase(),
@@ -53,4 +124,4 @@ private void testHeader(Headers headers, String header, String... values) {
                    headers.get(headerName).allValues(),
                    hasItems(values));
     }
-}
+}

common/http/src/test/java/io/helidon/common/http/HttpTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2022 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 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.
@@ -16,13 +16,20 @@
 
 package io.helidon.common.http;
 
+import java.util.stream.Stream;
+
+import org.junit.jupiter.api.Assertions;
 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 io.helidon.common.http.Http.Status.TEMPORARY_REDIRECT_307;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.sameInstance;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
 
 /**
  * Unit test for {@link Http}.
@@ -50,4 +57,36 @@ void testResposneStatusCustomReason() {
         assertThat(rs.code(), is(TEMPORARY_REDIRECT_307.code()));
         assertThat(rs.family(), is(TEMPORARY_REDIRECT_307.family()));
     }
+
+    @ParameterizedTest
+    @MethodSource("headers")
+    void testHeaderValidation(String headerName, String headerValues, boolean expectsValid) {
+        Http.HeaderValue header = Http.Header.create(Http.Header.create(headerName), headerValues);
+        if (expectsValid) {
+            header.validate();
+        } else {
+            Assertions.assertThrows(IllegalArgumentException.class, () -> header.validate());
+        }
+    }
+
+    private static Stream<Arguments> headers() {
+        return Stream.of(
+                // Valid headers
+                arguments("!#$Custom~%&\'*Header+^`|", "!Header\tValue~", true),
+                arguments("Custom_0-9_a-z_A-Z_Header", "\u0080Header Value\u00ff", true),
+                // Invalid headers
+                arguments("Valid-Header-Name", "H\u001ceaderValue1", false),
+                arguments("Valid-Header-Name", "HeaderValue1, Header\u007fValue", false),
+                arguments("Valid-Header-Name", "HeaderValue1\u001f, HeaderValue2", false),
+                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("<Header?Name>", "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)
+        );
+    }
 }

nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/HttpCallChainBase.java

@@ -47,8 +47,11 @@ abstract class HttpCallChainBase implements WebClientService.Chain {
         this.tls = tls;
     }
 
-    static void writeHeaders(Headers headers, BufferData bufferData) {
+    static void writeHeaders(Headers headers, BufferData bufferData, boolean validate) {
         for (Http.HeaderValue header : headers) {
+            if (validate) {
+                header.validate();
+            }
             header.writeHttp1Header(bufferData);
         }
         bufferData.write(Bytes.CR_BYTE);

nima/webclient/webclient/src/main/java/io/helidon/nima/webclient/http1/HttpCallEntityChain.java

@@ -28,15 +28,13 @@
 import io.helidon.common.http.Http;
 import io.helidon.nima.common.tls.Tls;
 import io.helidon.nima.http.media.EntityWriter;
-import io.helidon.nima.http.media.MediaContext;
 import io.helidon.nima.webclient.ClientConnection;
 import io.helidon.nima.webclient.WebClientServiceRequest;
 import io.helidon.nima.webclient.WebClientServiceResponse;
 
 class HttpCallEntityChain extends HttpCallChainBase {
 
-    private final MediaContext mediaContext;
-    private final int maxStatusLineLength;
+    private final Http1ClientConfig clientConfig;
     private final CompletableFuture<WebClientServiceRequest> whenSent;
     private final CompletableFuture<WebClientServiceResponse> whenComplete;
     private final Object entity;
@@ -48,8 +46,7 @@ class HttpCallEntityChain extends HttpCallChainBase {
                         CompletableFuture<WebClientServiceResponse> whenComplete,
                         Object entity) {
         super(clientConfig, connection, tls);
-        this.mediaContext = clientConfig.mediaContext();
-        this.maxStatusLineLength = clientConfig.maxStatusLineLength();
+        this.clientConfig = clientConfig;
         this.whenSent = whenSent;
         this.whenComplete = whenComplete;
         this.entity = entity;
@@ -71,8 +68,7 @@ public WebClientServiceResponse doProceed(ClientConnection connection,
 
         headers.set(Http.Header.create(Http.Header.CONTENT_LENGTH, entityBytes.length));
 
-        // todo validate request headers
-        writeHeaders(headers, writeBuffer);
+        writeHeaders(headers, writeBuffer, clientConfig.validateHeaders());
         // we have completed writing the headers
         whenSent.complete(serviceRequest);
 
@@ -81,7 +77,7 @@ public WebClientServiceResponse doProceed(ClientConnection connection,
         }
         writer.write(writeBuffer);
 
-        Http.Status responseStatus = Http1StatusParser.readStatus(reader, maxStatusLineLength);
+        Http.Status responseStatus = Http1StatusParser.readStatus(reader, clientConfig.maxStatusLineLength());
         ClientResponseHeaders responseHeaders = readHeaders(reader);
 
         return WebClientServiceResponse.builder()
@@ -99,7 +95,7 @@ byte[] entityBytes(Object entity, ClientRequestHeaders headers) {
             return (byte[]) entity;
         }
         GenericType<Object> genericType = GenericType.create(entity);
-        EntityWriter<Object> writer = mediaContext.writer(genericType, headers);
+        EntityWriter<Object> writer = clientConfig.mediaContext().writer(genericType, headers);
 
         // todo this should use output stream of client, but that would require delaying header write
         // to first byte written