Add option to HttpObjectDecoder to allow duplicate Content-Lengths (n…

…etty#10349)


Motivation:

Since netty#9865 (Netty 4.1.44) the
default behavior of the HttpObjectDecoder has been to reject any HTTP
message that is found to have multiple Content-Length headers when
decoding. This behavior is well-justified as per the risks outlined in
netty#9861, however, we can see from the
cited RFC section that there are multiple possible options offered for
responding to this scenario:

> If a message is received that has multiple Content-Length header
> fields with field-values consisting of the same decimal value, or a
> single Content-Length header field with a field value containing a
> list of identical decimal values (e.g., "Content-Length: 42, 42"),
> indicating that duplicate Content-Length header fields have been
> generated or combined by an upstream message processor, then the
> recipient MUST either reject the message as invalid or replace the
> duplicated field-values with a single valid Content-Length field
> containing that decimal value prior to determining the message body
> length or forwarding the message.

https://tools.ietf.org/html/rfc7230#section-3.3.2

Netty opted for the first option (rejecting as invalid), which seems
like the safest, but the second option (replacing duplicate values with
a single value) is also valid behavior.

Modifications:

* Introduce "allowDuplicateContentLengths" parameter to
HttpObjectDecoder (defaulting to false).
* When set to true, will allow multiple Content-Length headers only if
they are all the same value. The duplicated field-values will be
replaced with a single valid Content-Length field.
* Add new parameterized test class for testing different variations of
multiple Content-Length headers.

Result:

This is a backwards-compatible change with no functional change to the
existing behavior.

Note that the existing logic would result in NumberFormatExceptions
for header values like "Content-Length: 42, 42". The new logic correctly
reports these as IllegalArgumentException with the proper error message.

Additionally note that this behavior is only applied to HTTP/1.1, but I
suspect that we may want to expand that to include HTTP/1.0 as well...
That behavior is not modified here to minimize the scope of this change.

codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java

@@ -28,9 +28,11 @@
 import java.util.Queue;
 import java.util.concurrent.atomic.AtomicLong;
 
+import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS;
 import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_MAX_CHUNK_SIZE;
 import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_MAX_HEADER_SIZE;
 import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_MAX_INITIAL_LINE_LENGTH;
+import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_VALIDATE_HEADERS;
 
 /**
  * A combination of {@link HttpRequestEncoder} and {@link HttpResponseDecoder}
@@ -48,6 +50,8 @@
  */
 public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResponseDecoder, HttpRequestEncoder>
         implements HttpClientUpgradeHandler.SourceCodec {
+    public static final boolean DEFAULT_FAIL_ON_MISSING_RESPONSE = false;
+    public static final boolean DEFAULT_PARSE_HTTP_AFTER_CONNECT_REQUEST = false;
 
     /** A queue that is used for correlating a request and a response. */
     private final Queue<HttpMethod> queue = new ArrayDeque<HttpMethod>();
@@ -65,22 +69,23 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
      * {@code maxChunkSize (8192)}).
      */
     public HttpClientCodec() {
-        this(DEFAULT_MAX_INITIAL_LINE_LENGTH, DEFAULT_MAX_HEADER_SIZE, DEFAULT_MAX_CHUNK_SIZE, false);
+        this(DEFAULT_MAX_INITIAL_LINE_LENGTH, DEFAULT_MAX_HEADER_SIZE, DEFAULT_MAX_CHUNK_SIZE,
+             DEFAULT_FAIL_ON_MISSING_RESPONSE);
     }
 
     /**
      * Creates a new instance with the specified decoder options.
      */
     public HttpClientCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize) {
-        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, false);
+        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, DEFAULT_FAIL_ON_MISSING_RESPONSE);
     }
 
     /**
      * Creates a new instance with the specified decoder options.
      */
     public HttpClientCodec(
             int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean failOnMissingResponse) {
-        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, failOnMissingResponse, true);
+        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, failOnMissingResponse, DEFAULT_VALIDATE_HEADERS);
     }
 
     /**
@@ -89,7 +94,8 @@ public HttpClientCodec(
     public HttpClientCodec(
             int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean failOnMissingResponse,
             boolean validateHeaders) {
-        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, failOnMissingResponse, validateHeaders, false);
+        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, failOnMissingResponse, validateHeaders,
+             DEFAULT_PARSE_HTTP_AFTER_CONNECT_REQUEST);
     }
 
     /**
@@ -110,7 +116,7 @@ public HttpClientCodec(
             int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean failOnMissingResponse,
             boolean validateHeaders, int initialBufferSize) {
         this(maxInitialLineLength, maxHeaderSize, maxChunkSize, failOnMissingResponse, validateHeaders,
-                initialBufferSize, false);
+             initialBufferSize, DEFAULT_PARSE_HTTP_AFTER_CONNECT_REQUEST);
     }
 
     /**
@@ -119,7 +125,19 @@ public HttpClientCodec(
     public HttpClientCodec(
             int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean failOnMissingResponse,
             boolean validateHeaders, int initialBufferSize, boolean parseHttpAfterConnectRequest) {
-        init(new Decoder(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize),
+        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, failOnMissingResponse, validateHeaders,
+             initialBufferSize, parseHttpAfterConnectRequest, DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS);
+    }
+
+    /**
+     * Creates a new instance with the specified decoder options.
+     */
+    public HttpClientCodec(
+            int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean failOnMissingResponse,
+            boolean validateHeaders, int initialBufferSize, boolean parseHttpAfterConnectRequest,
+            boolean allowDuplicateContentLengths) {
+        init(new Decoder(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize,
+                         allowDuplicateContentLengths),
              new Encoder());
         this.parseHttpAfterConnectRequest = parseHttpAfterConnectRequest;
         this.failOnMissingResponse = failOnMissingResponse;
@@ -186,8 +204,9 @@ private final class Decoder extends HttpResponseDecoder {
         }
 
         Decoder(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean validateHeaders,
-                int initialBufferSize) {
-            super(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize);
+                int initialBufferSize, boolean allowDuplicateContentLengths) {
+            super(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize,
+                  allowDuplicateContentLengths);
         }
 
         @Override

codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java

@@ -16,6 +16,7 @@
 package io.netty.handler.codec.http;
 
 import static io.netty.util.internal.ObjectUtil.checkPositive;
+import static io.netty.util.internal.StringUtil.COMMA;
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
@@ -29,6 +30,7 @@
 import io.netty.util.internal.AppendableCharSequence;
 
 import java.util.List;
+import java.util.regex.Pattern;
 
 /**
  * Decodes {@link ByteBuf}s into {@link HttpMessage}s and
@@ -37,29 +39,47 @@
  * <h3>Parameters that prevents excessive memory consumption</h3>
  * <table border="1">
  * <tr>
- * <th>Name</th><th>Meaning</th>
+ * <th>Name</th><th>Default value</th><th>Meaning</th>
  * </tr>
  * <tr>
  * <td>{@code maxInitialLineLength}</td>
+ * <td>{@value #DEFAULT_MAX_INITIAL_LINE_LENGTH}</td>
  * <td>The maximum length of the initial line
  *     (e.g. {@code "GET / HTTP/1.0"} or {@code "HTTP/1.0 200 OK"})
  *     If the length of the initial line exceeds this value, a
  *     {@link TooLongFrameException} will be raised.</td>
  * </tr>
  * <tr>
  * <td>{@code maxHeaderSize}</td>
+ * <td>{@value #DEFAULT_MAX_HEADER_SIZE}</td>
  * <td>The maximum length of all headers.  If the sum of the length of each
  *     header exceeds this value, a {@link TooLongFrameException} will be raised.</td>
  * </tr>
  * <tr>
  * <td>{@code maxChunkSize}</td>
+ * <td>{@value #DEFAULT_MAX_CHUNK_SIZE}</td>
  * <td>The maximum length of the content or each chunk.  If the content length
  *     (or the length of each chunk) exceeds this value, the content or chunk
  *     will be split into multiple {@link HttpContent}s whose length is
  *     {@code maxChunkSize} at maximum.</td>
  * </tr>
  * </table>
  *
+ * <h3>Parameters that control parsing behavior</h3>
+ * <table border="1">
+ * <tr>
+ * <th>Name</th><th>Default value</th><th>Meaning</th>
+ * </tr>
+ * <tr>
+ * <td>{@code allowDuplicateContentLengths}</td>
+ * <td>{@value #DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS}</td>
+ * <td>When set to {@code false}, will reject any messages that contain multiple Content-Length header fields.
+ *     When set to {@code true}, will allow multiple Content-Length headers only if they are all the same decimal value.
+ *     The duplicated field-values will be replaced with a single valid Content-Length field.
+ *     See <a href="https://tools.ietf.org/html/rfc7230#section-3.3.2">RFC 7230, Section 3.3.2</a>.</td>
+ * </tr>
+ * </table>
+ *
  * <h3>Chunked Content</h3>
  *
  * If the content of an HTTP message is greater than {@code maxChunkSize} or
@@ -108,12 +128,15 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
     public static final int DEFAULT_MAX_CHUNK_SIZE = 8192;
     public static final boolean DEFAULT_VALIDATE_HEADERS = true;
     public static final int DEFAULT_INITIAL_BUFFER_SIZE = 128;
+    public static final boolean DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS = false;
 
     private static final String EMPTY_VALUE = "";
+    private static final Pattern COMMA_PATTERN = Pattern.compile(",");
 
     private final int maxChunkSize;
     private final boolean chunkedSupported;
     protected final boolean validateHeaders;
+    private final boolean allowDuplicateContentLengths;
     private final HeaderParser headerParser;
     private final LineParser lineParser;
 
@@ -176,9 +199,20 @@ protected HttpObjectDecoder(
              DEFAULT_INITIAL_BUFFER_SIZE);
     }
 
+    /**
+     * Creates a new instance with the specified parameters.
+     */
     protected HttpObjectDecoder(
             int maxInitialLineLength, int maxHeaderSize, int maxChunkSize,
             boolean chunkedSupported, boolean validateHeaders, int initialBufferSize) {
+        this(maxInitialLineLength, maxHeaderSize, maxChunkSize, chunkedSupported, validateHeaders, initialBufferSize,
+             DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS);
+    }
+
+    protected HttpObjectDecoder(
+            int maxInitialLineLength, int maxHeaderSize, int maxChunkSize,
+            boolean chunkedSupported, boolean validateHeaders, int initialBufferSize,
+            boolean allowDuplicateContentLengths) {
         checkPositive(maxInitialLineLength, "maxInitialLineLength");
         checkPositive(maxHeaderSize, "maxHeaderSize");
         checkPositive(maxChunkSize, "maxChunkSize");
@@ -189,6 +223,7 @@ protected HttpObjectDecoder(
         this.maxChunkSize = maxChunkSize;
         this.chunkedSupported = chunkedSupported;
         this.validateHeaders = validateHeaders;
+        this.allowDuplicateContentLengths = allowDuplicateContentLengths;
     }
 
     @Override
@@ -594,10 +629,9 @@ private State readHeaders(ByteBuf buffer) {
         name = null;
         value = null;
 
-        List<String> values = headers.getAll(HttpHeaderNames.CONTENT_LENGTH);
-        int contentLengthValuesCount = values.size();
+        List<String> contentLengthFields = headers.getAll(HttpHeaderNames.CONTENT_LENGTH);
 
-        if (contentLengthValuesCount > 0) {
+        if (!contentLengthFields.isEmpty()) {
             // Guard against multiple Content-Length headers as stated in
             // https://tools.ietf.org/html/rfc7230#section-3.3.2:
             //
@@ -611,17 +645,42 @@ private State readHeaders(ByteBuf buffer) {
             //   duplicated field-values with a single valid Content-Length field
             //   containing that decimal value prior to determining the message body
             //   length or forwarding the message.
-            if (contentLengthValuesCount > 1 && message.protocolVersion() == HttpVersion.HTTP_1_1) {
-                throw new IllegalArgumentException("Multiple Content-Length headers found");
+            boolean multipleContentLengths =
+                    contentLengthFields.size() > 1 || contentLengthFields.get(0).indexOf(COMMA) >= 0;
+            if (multipleContentLengths && message.protocolVersion() == HttpVersion.HTTP_1_1) {
+                if (allowDuplicateContentLengths) {
+                    // Find and enforce that all Content-Length values are the same
+                    String firstValue = null;
+                    for (String field : contentLengthFields) {
+                        String[] tokens = COMMA_PATTERN.split(field, -1);
+                        for (String token : tokens) {
+                            String trimmed = token.trim();
+                            if (firstValue == null) {
+                                firstValue = trimmed;
+                            } else if (!trimmed.equals(firstValue)) {
+                                throw new IllegalArgumentException(
+                                        "Multiple Content-Length values found: " + contentLengthFields);
+                            }
+                        }
+                    }
+                    // Replace the duplicated field-values with a single valid Content-Length field
+                    headers.set(HttpHeaderNames.CONTENT_LENGTH, firstValue);
+                    contentLength = Long.parseLong(firstValue);
+                } else {
+                    // Reject the message as invalid
+                    throw new IllegalArgumentException(
+                            "Multiple Content-Length values found: " + contentLengthFields);
+                }
+            } else {
+                contentLength = Long.parseLong(contentLengthFields.get(0));
             }
-            contentLength = Long.parseLong(values.get(0));
         }
 
         if (isContentAlwaysEmpty(message)) {
             HttpUtil.setTransferEncodingChunked(message, false);
             return State.SKIP_CONTROL_CHARS;
         } else if (HttpUtil.isTransferEncodingChunked(message)) {
-            if (contentLengthValuesCount > 0 && message.protocolVersion() == HttpVersion.HTTP_1_1) {
+            if (!contentLengthFields.isEmpty() && message.protocolVersion() == HttpVersion.HTTP_1_1) {
                 handleTransferEncodingChunkedWithContentLength(message);
             }
             return State.READ_CHUNK_SIZE;

codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java

@@ -82,6 +82,13 @@ public HttpRequestDecoder(
               initialBufferSize);
     }
 
+    public HttpRequestDecoder(
+            int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean validateHeaders,
+            int initialBufferSize, boolean allowDuplicateContentLengths) {
+        super(maxInitialLineLength, maxHeaderSize, maxChunkSize, DEFAULT_CHUNKED_SUPPORTED, validateHeaders,
+              initialBufferSize, allowDuplicateContentLengths);
+    }
+
     @Override
     protected HttpMessage createMessage(String[] initialLine) throws Exception {
         return new DefaultHttpRequest(

codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java

@@ -113,6 +113,13 @@ public HttpResponseDecoder(
               initialBufferSize);
     }
 
+    public HttpResponseDecoder(
+            int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean validateHeaders,
+            int initialBufferSize, boolean allowDuplicateContentLengths) {
+        super(maxInitialLineLength, maxHeaderSize, maxChunkSize, DEFAULT_CHUNKED_SUPPORTED, validateHeaders,
+              initialBufferSize, allowDuplicateContentLengths);
+    }
+
     @Override
     protected HttpMessage createMessage(String[] initialLine) {
         return new DefaultHttpResponse(

codec-http/src/main/java/io/netty/handler/codec/http/HttpServerCodec.java

@@ -75,6 +75,16 @@ public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunk
           new HttpServerResponseEncoder());
     }
 
+    /**
+     * Creates a new instance with the specified decoder options.
+     */
+    public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean validateHeaders,
+                           int initialBufferSize, boolean allowDuplicateContentLengths) {
+        init(new HttpServerRequestDecoder(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders,
+                                          initialBufferSize, allowDuplicateContentLengths),
+             new HttpServerResponseEncoder());
+    }
+
     /**
      * Upgrades to another protocol from HTTP. Removes the {@link HttpRequestDecoder} and
      * {@link HttpResponseEncoder} from the pipeline.
@@ -101,6 +111,12 @@ private final class HttpServerRequestDecoder extends HttpRequestDecoder {
             super(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize);
         }
 
+        HttpServerRequestDecoder(int maxInitialLineLength, int maxHeaderSize, int maxChunkSize,
+                                 boolean validateHeaders, int initialBufferSize, boolean allowDuplicateContentLengths) {
+            super(maxInitialLineLength, maxHeaderSize, maxChunkSize, validateHeaders, initialBufferSize,
+                  allowDuplicateContentLengths);
+        }
+
         @Override
         protected void decode(ChannelHandlerContext ctx, ByteBuf buffer, List<Object> out) throws Exception {
             int oldSize = out.size();