Remove "Content-Length" when decoding HTTP/1.1 message with both "Tra… (#10003)

Motivation

As part of a recent commit for issue
https://github.com/netty/netty/issues/9861 the HttpObjectDecoder was
changed to throw an IllegalArgumentException (and produce a failed
decoder result) when decoding a message with both "Transfer-Encoding:
chunked" and "Content-Length".

While it seems correct for Netty to try to sanitize these types of
messages, the spec explicitly mentions that the Content-Length header
should be *removed* in this scenario.

Both Nginx 1.15.9 and Tomcat 9.0.31 also opt to remove the header:
b693d7c198/java/org/apache/coyote/http11/Http11Processor.java (L747-L755)
0ad4393e30/src/http/ngx_http_request.c (L1946-L1953)

Modifications

* Change the default behavior from throwing an IllegalArgumentException
to removing the "Content-Length" header
* Extract the behavior to a new protected method,
handleChunkedEncodingWithContentLength(), that can be overridden to
change this behavior (or capture metrics)

Result

Messages of this nature will now be successfully decoded and have their
"Content-Length" header removed, rather than creating invalid messages
(decoder result failures). Users will be allowed to override and
configure this behavior.

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

@@ -628,23 +628,9 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
             HttpUtil.setTransferEncodingChunked(message, false);
             return State.SKIP_CONTROL_CHARS;
         } else if (HttpUtil.isTransferEncodingChunked(message)) {
-            // See https://tools.ietf.org/html/rfc7230#section-3.3.3
-            //
-            //       If a message is received with both a Transfer-Encoding and a
-            //       Content-Length header field, the Transfer-Encoding overrides the
-            //       Content-Length.  Such a message might indicate an attempt to
-            //       perform request smuggling (Section 9.5) or response splitting
-            //       (Section 9.4) and ought to be handled as an error.  A sender MUST
-            //       remove the received Content-Length field prior to forwarding such
-            //       a message downstream.
-            //
-            // This is also what http_parser does:
-            // https://github.com/nodejs/http-parser/blob/v2.9.2/http_parser.c#L1769
             if (contentLengthValuesCount > 0 && message.protocolVersion() == HttpVersion.HTTP_1_1) {
-                throw new IllegalArgumentException(
+                handleTransferEncodingChunkedWithContentLength(message);
-                        "Both 'Content-Length: " + contentLength + "' and 'Transfer-Encoding: chunked' found");
             }
-
             return State.READ_CHUNK_SIZE;
         } else if (contentLength() >= 0) {
             return State.READ_FIXED_LENGTH_CONTENT;
@@ -653,6 +639,32 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
         }
     }
 
+    /**
+     * Invoked when a message with both a "Transfer-Encoding: chunked" and a "Content-Length" header field is detected.
+     * The default behavior is to <i>remove</i> the Content-Length field, but this method could be overridden
+     * to change the behavior (to, e.g., throw an exception and produce an invalid message).
+     * <p>
+     * See: https://tools.ietf.org/html/rfc7230#section-3.3.3
+     * <pre>
+     *     If a message is received with both a Transfer-Encoding and a
+     *     Content-Length header field, the Transfer-Encoding overrides the
+     *     Content-Length.  Such a message might indicate an attempt to
+     *     perform request smuggling (Section 9.5) or response splitting
+     *     (Section 9.4) and ought to be handled as an error.  A sender MUST
+     *     remove the received Content-Length field prior to forwarding such
+     *     a message downstream.
+     * </pre>
+     * Also see:
+     * https://github.com/apache/tomcat/blob/b693d7c1981fa7f51e58bc8c8e72e3fe80b7b773/
+     * java/org/apache/coyote/http11/Http11Processor.java#L747-L755
+     * https://github.com/nginx/nginx/blob/0ad4393e30c119d250415cb769e3d8bc8dce5186/
+     * src/http/ngx_http_request.c#L1946-L1953
+     */
+    protected void handleTransferEncodingChunkedWithContentLength(HttpMessage message) {
+        message.headers().remove(HttpHeaderNames.CONTENT_LENGTH);
+        contentLength = Long.MIN_VALUE;
+    }
+
     private long contentLength() {
         if (contentLength == Long.MIN_VALUE) {
             contentLength = HttpUtil.getContentLength(message, -1L);

codec-http/src/test/java/io/netty/handler/codec/http/HttpRequestDecoderTest.java

@@ -408,7 +408,15 @@ public class HttpRequestDecoderTest {
                 "Content-Length: 5\r\n" +
                 "Transfer-Encoding: chunked\r\n\r\n" +
                 "0\r\n\r\n";
-        testInvalidHeaders0(requestStr);
+
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestDecoder());
+        assertTrue(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)));
+        HttpRequest request = channel.readInbound();
+        assertFalse(request.decoderResult().isFailure());
+        assertTrue(request.headers().contains("Transfer-Encoding", "chunked", false));
+        assertFalse(request.headers().contains("Content-Length"));
+        LastHttpContent c = channel.readInbound();
+        assertFalse(channel.finish());
     }
 
     private static void testInvalidHeaders0(String requestStr) {