Allow for multiple Content-Length headers behavior to be configurable...

...and introduce HttpDecoderOption & HttpDecoderBuilder in doing so.

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. While it makes sense for Netty
to bias towards the safest option, Netty is used as a library for a
variety of different use cases, and there is usually no
one-size-fits-all for parsing behavior like this...

It seems like the ideal solution would be to default to the safest
behavior but to also make these types of decisions configurable. This is
further motivated by the fact that the HTTP RFCs are often vague or
ambiguous in many areas and we have seen other back-and-forth
conversations (e.g., netty#10003) based
on differing interpretations that would probably be most amicably
resolved by simply exposing better configuration.

Modifications

* Deprecate HttpRequestDecoder's telescoping constructors and introduce
AbstractHttpObjectDecoderBuilder and HttpRequestDecoderBuilder.
(The current HttpObjectDecoder implementations are only
minimally configurable, and any further configuration will not scale
well with the existing telescoping constructors.)
* Move default decoder values to the HttpObjectDecoder class and
reference them accordingly.
* Introduce HttpDecoderOption class. This option is similar in vein to
the ChannelOption class, where it is capable of exposing a wide
possibility of different configurations.
* Introduce the first decoder option:
MultipleContentLengthHeadersBehavior. This option sets the precedent of
using an interface for its implementation (as opposed to boolean/enum)
so that users can easily define their own implementations or even
"listen in" on implementations for logging purposes (without having to
override).
* Current available options are: ALWAYS_REJECT,
IF_DIFFERENT_REJECT_ELSE_DEDUPE, and IF_DIFFERENT_REJECT_ELSE_ALLOW (see
Javadoc for more).
* 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.
* Extend HttpObjectDecoder to accept a map of options and use these
options to resolve multiple Content-Length scenarios.
* In doing so, also apply this logic to HTTP/1.0 (in addition to 1.1).
Even though the cited RFCs were for 1.1, it seems that the risks they
are trying to mitigate could still apply to messages in 1.0 that specify
a content length.
* Also include HTTP/1.0 in the existing
"handleTransferEncodingChunkedWithContentLength" logic for the same
reason (this logic could also probably be better served by an
HttpDecoderOption in the future).
* Include new unit tests to test all of the possible combinations of
MultipleContentLengthHeadersBehavior and multiple content lengths.

Result

This is a backwards-compatible change with no functional change to the
existing behavior. Users will now be able to customize the behavior for
handling multiple content lengths, and we will be able to more easily
add additional options in the future if needed.

To limit the scope of this commit, it does not attempt to introduce the
builder or decoder options to the other relevant classes
(HttpResponseDecoder, HttpClientCodec, HttpServerCodec), but we may wish
to follow-up with those changes.

Author: Bennett-Lynch

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

@@ -111,7 +111,7 @@ public interface MultipleContentLengthHeadersBehavior {
          * that the {@link List} of values represents different header lines and has not been split on commas. A single
          * line may or may not have multiple, comma-separated values.
          */
-        long resolveContentLength(HttpHeaders headers, List<String> contentLengthFields);
+        long resolveContentLength(HttpHeaders headers, List<String> contentLengthFieldValues);
 
         /**
          * {@link #ALWAYS_REJECT} will always throw an {@link IllegalArgumentException} (and thus trigger a failed
@@ -124,7 +124,7 @@ public interface MultipleContentLengthHeadersBehavior {
         MultipleContentLengthHeadersBehavior ALWAYS_REJECT =
                 new MultipleContentLengthHeadersBehavior() {
                     @Override
-                    public long resolveContentLength(HttpHeaders headers, List<String> contentLengthFields) {
+                    public long resolveContentLength(HttpHeaders headers, List<String> contentLengthFieldValues) {
                         throw new IllegalArgumentException("Multiple Content-Length headers found");
                     }
                 };
@@ -139,8 +139,8 @@ public long resolveContentLength(HttpHeaders headers, List<String> contentLength
         MultipleContentLengthHeadersBehavior IF_DIFFERENT_REJECT_ELSE_DEDUPE =
                 new MultipleContentLengthHeadersBehavior() {
                     @Override
-                    public long resolveContentLength(HttpHeaders headers, List<String> contentLengthFields) {
-                        String contentLength = findAndEnforceUniqueContentLength(contentLengthFields);
+                    public long resolveContentLength(HttpHeaders headers, List<String> contentLengthFieldValues) {
+                        String contentLength = findAndEnforceUniqueContentLength(contentLengthFieldValues);
                         headers.set(HttpHeaderNames.CONTENT_LENGTH, contentLength);
                         return Long.parseLong(contentLength);
                     }
@@ -151,13 +151,13 @@ public long resolveContentLength(HttpHeaders headers, List<String> contentLength
          * all the same. When this occurs, the original {@link HttpHeaders} (and its multiple values) are left
          * untouched.
          * <p>
-         * This behavior is not offered as an explicit option in the RFC but may desired for pass-through behavior.
+         * This behavior is not offered as an explicit option in the RFC but may be desired for pass-through behavior.
          */
         MultipleContentLengthHeadersBehavior IF_DIFFERENT_REJECT_ELSE_ALLOW =
                 new MultipleContentLengthHeadersBehavior() {
                     @Override
-                    public long resolveContentLength(HttpHeaders headers, List<String> contentLengthFields) {
-                        String contentLength = findAndEnforceUniqueContentLength(contentLengthFields);
+                    public long resolveContentLength(HttpHeaders headers, List<String> contentLengthFieldValues) {
+                        String contentLength = findAndEnforceUniqueContentLength(contentLengthFieldValues);
                         return Long.parseLong(contentLength);
                     }
                 };

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

@@ -44,7 +44,6 @@ public class MultipleContentLengthHeadersBehaviorTest {
     private final boolean singleField;
 
     private EmbeddedChannel channel;
-    private HttpRequest request;
 
     @Parameters
     public static Collection<Object[]> parameters() {
@@ -83,7 +82,7 @@ public void setUp() {
     public void testMultipleContentLengthHeadersBehavior() {
         String requestStr = setupRequestString();
         assertThat(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)), is(true));
-        request = channel.readInbound();
+        HttpRequest request = channel.readInbound();
 
         if (behavior == MultipleContentLengthHeadersBehavior.ALWAYS_REJECT) {
             assertInvalid(request);
@@ -113,7 +112,7 @@ public void testMultipleContentLengthHeadersBehavior() {
 
     private String setupRequestString() {
         String firstValue = "1";
-        String secondValue = sameValue? firstValue : "2";
+        String secondValue = sameValue ? firstValue : "2";
         String contentLength;
         if (singleField) {
             contentLength = "Content-Length: " + firstValue + ", " + secondValue + "\r\n\r\n";
@@ -133,7 +132,7 @@ public void testDanglingComma() {
                             "Connection: close\n\n" +
                             "b";
         assertThat(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)), is(true));
-        request = channel.readInbound();
+        HttpRequest request = channel.readInbound();
         assertInvalid(request);
         assertThat(channel.finish(), is(false));
     }