andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Add option to HttpObjectDecoder to allow duplicate Content-Lengths (#10349)

Browse Source 
Motivation:

Since https://github.com/netty/netty/pull/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
https://github.com/netty/netty/issues/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.
This commit is contained in:
Bennett Lynch 2020-07-06 01:25:13 -07:00 committed by Norman Maurer
parent f8b3a388ff
commit f7fa9fce72
6 changed files with 265 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -28,8 +28,10 @@ import java.util.List;
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_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}
@ -47,6 +49,8 @@ import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_MAX_INITIAL_
*/
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<>();
@ -64,14 +68,14 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
* {@code maxChunkSize (8192)}).
*/
public HttpClientCodec() {
this(DEFAULT_MAX_INITIAL_LINE_LENGTH, DEFAULT_MAX_HEADER_SIZE, false);
this(DEFAULT_MAX_INITIAL_LINE_LENGTH, DEFAULT_MAX_HEADER_SIZE, DEFAULT_FAIL_ON_MISSING_RESPONSE);
}
/**
* Creates a new instance with the specified decoder options.
*/
public HttpClientCodec(int maxInitialLineLength, int maxHeaderSize) {
this(maxInitialLineLength, maxHeaderSize, false);
this(maxInitialLineLength, maxHeaderSize, DEFAULT_FAIL_ON_MISSING_RESPONSE);
}
/**
@ -79,7 +83,7 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
*/
public HttpClientCodec(
int maxInitialLineLength, int maxHeaderSize, boolean failOnMissingResponse) {
this(maxInitialLineLength, maxHeaderSize, failOnMissingResponse, true);
this(maxInitialLineLength, maxHeaderSize, failOnMissingResponse, DEFAULT_VALIDATE_HEADERS);
}
/**
@ -88,7 +92,8 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
public HttpClientCodec(
int maxInitialLineLength, int maxHeaderSize, boolean failOnMissingResponse,
boolean validateHeaders) {
this(maxInitialLineLength, maxHeaderSize, failOnMissingResponse, validateHeaders, false);
this(maxInitialLineLength, maxHeaderSize, failOnMissingResponse, validateHeaders,
DEFAULT_PARSE_HTTP_AFTER_CONNECT_REQUEST);
}
/**
@ -106,19 +111,31 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
* Creates a new instance with the specified decoder options.
*/
public HttpClientCodec(
int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean failOnMissingResponse,
int maxInitialLineLength, int maxHeaderSize, boolean failOnMissingResponse,
boolean validateHeaders, int initialBufferSize) {
this(maxInitialLineLength, maxHeaderSize, maxChunkSize, failOnMissingResponse, validateHeaders,
initialBufferSize, false);
this(maxInitialLineLength, maxHeaderSize, failOnMissingResponse, validateHeaders,
initialBufferSize, DEFAULT_PARSE_HTTP_AFTER_CONNECT_REQUEST);
}
/**
* Creates a new instance with the specified decoder options.
*/
public HttpClientCodec(
int maxInitialLineLength, int maxHeaderSize, int maxChunkSize, boolean failOnMissingResponse,
int maxInitialLineLength, int maxHeaderSize, boolean failOnMissingResponse,
boolean validateHeaders, int initialBufferSize, boolean parseHttpAfterConnectRequest) {
init(new Decoder(maxInitialLineLength, maxHeaderSize, validateHeaders, initialBufferSize),
this(maxInitialLineLength, maxHeaderSize, failOnMissingResponse, validateHeaders,
initialBufferSize, parseHttpAfterConnectRequest, DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS);
}
/**
* Creates a new instance with the specified decoder options.
*/
public HttpClientCodec(
int maxInitialLineLength, int maxHeaderSize, boolean failOnMissingResponse,
boolean validateHeaders, int initialBufferSize, boolean parseHttpAfterConnectRequest,
boolean allowDuplicateContentLengths) {
init(new Decoder(maxInitialLineLength, maxHeaderSize, validateHeaders, initialBufferSize,
allowDuplicateContentLengths),
new Encoder());
this.parseHttpAfterConnectRequest = parseHttpAfterConnectRequest;
this.failOnMissingResponse = failOnMissingResponse;
@ -188,8 +205,9 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
}
Decoder(int maxInitialLineLength, int maxHeaderSize, boolean validateHeaders,
int initialBufferSize) {
super(maxInitialLineLength, maxHeaderSize, validateHeaders, initialBufferSize);
int initialBufferSize, boolean allowDuplicateContentLengths) {
super(maxInitialLineLength, maxHeaderSize, validateHeaders, initialBufferSize,
allowDuplicateContentLengths);
}
@Override

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

@ -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.ByteProcessor;
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,10 +39,11 @@ import java.util.List;
* <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
@ -48,15 +51,24 @@ import java.util.List;
* </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>
* </table>
*
* <h3>Parameters that control parsing behavior</h3>
* <table border="1">
* <tr>
* <td>{@code maxChunkSize}</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>
* <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>
*
@ -107,11 +119,14 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
public static final boolean DEFAULT_CHUNKED_SUPPORTED = true;
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 boolean chunkedSupported;
protected final boolean validateHeaders;
private final boolean allowDuplicateContentLengths;
private final HeaderParser headerParser;
private final LineParser lineParser;
@ -172,9 +187,20 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
this(maxInitialLineLength, maxHeaderSize, chunkedSupported, validateHeaders, DEFAULT_INITIAL_BUFFER_SIZE);
}
/**
* Creates a new instance with the specified parameters.
*/
protected HttpObjectDecoder(
int maxInitialLineLength, int maxHeaderSize,
boolean chunkedSupported, boolean validateHeaders, int initialBufferSize) {
this(maxInitialLineLength, maxHeaderSize, chunkedSupported, validateHeaders, initialBufferSize,
DEFAULT_ALLOW_DUPLICATE_CONTENT_LENGTHS);
}
protected HttpObjectDecoder(
int maxInitialLineLength, int maxHeaderSize,
boolean chunkedSupported, boolean validateHeaders, int initialBufferSize,
boolean allowDuplicateContentLengths) {
checkPositive(maxInitialLineLength, "maxInitialLineLength");
checkPositive(maxHeaderSize, "maxHeaderSize");
AppendableCharSequence seq = new AppendableCharSequence(initialBufferSize);
@ -182,6 +208,7 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
headerParser = new HeaderParser(seq, maxHeaderSize);
this.chunkedSupported = chunkedSupported;
this.validateHeaders = validateHeaders;
this.allowDuplicateContentLengths = allowDuplicateContentLengths;
}
@Override
@ -587,10 +614,9 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
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:
//
@ -604,17 +630,42 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder {
// 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;

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

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

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

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

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

@ -73,6 +73,16 @@ public final class HttpServerCodec extends CombinedChannelDuplexHandler<HttpRequ
new HttpServerResponseEncoder());
}
/**
* Creates a new instance with the specified decoder options.
*/
public HttpServerCodec(int maxInitialLineLength, int maxHeaderSize, boolean validateHeaders,
int initialBufferSize, boolean allowDuplicateContentLengths) {
init(new HttpServerRequestDecoder(maxInitialLineLength, maxHeaderSize, validateHeaders,
initialBufferSize, allowDuplicateContentLengths),
new HttpServerResponseEncoder());
}
/**
* Upgrades to another protocol from HTTP. Removes the {@link HttpRequestDecoder} and
* {@link HttpResponseEncoder} from the pipeline.
@ -105,6 +115,12 @@ public final class HttpServerCodec extends CombinedChannelDuplexHandler<HttpRequ
super.decode(context, buffer);
}
HttpServerRequestDecoder(int maxInitialLineLength, int maxHeaderSize,
boolean validateHeaders, int initialBufferSize, boolean allowDuplicateContentLengths) {
super(maxInitialLineLength, maxHeaderSize, validateHeaders, initialBufferSize,
allowDuplicateContentLengths);
}
@Override
protected void handlerAdded0(final ChannelHandlerContext ctx) {
context = new DelegatingChannelHandlerContext(ctx) {

142
codec-http/src/test/java/io/netty/handler/codec/http/MultipleContentLengthHeadersTest.java Normal file
View File

@ -0,0 +1,142 @@
/*
* Copyright 2020 The Netty Project
*
* The Netty Project licenses this file to you 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.netty.handler.codec.http;
import io.netty.buffer.Unpooled;
import io.netty.channel.embedded.EmbeddedChannel;
import io.netty.util.CharsetUtil;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import static io.netty.handler.codec.http.HttpObjectDecoder.DEFAULT_INITIAL_BUFFER_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;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.core.IsInstanceOf.instanceOf;
@RunWith(Parameterized.class)
public class MultipleContentLengthHeadersTest {
private final boolean allowDuplicateContentLengths;
private final boolean sameValue;
private final boolean singleField;
private EmbeddedChannel channel;
@Parameters
public static Collection<Object[]> parameters() {
return Arrays.asList(new Object[][] {
{ false, false, false },
{ false, false, true },
{ false, true, false },
{ false, true, true },
{ true, false, false },
{ true, false, true },
{ true, true, false },
{ true, true, true }
});
}
public MultipleContentLengthHeadersTest(
boolean allowDuplicateContentLengths, boolean sameValue, boolean singleField) {
this.allowDuplicateContentLengths = allowDuplicateContentLengths;
this.sameValue = sameValue;
this.singleField = singleField;
}
@Before
public void setUp() {
HttpRequestDecoder decoder = new HttpRequestDecoder(
DEFAULT_MAX_INITIAL_LINE_LENGTH,
DEFAULT_MAX_HEADER_SIZE,
DEFAULT_VALIDATE_HEADERS,
DEFAULT_INITIAL_BUFFER_SIZE,
allowDuplicateContentLengths);
channel = new EmbeddedChannel(decoder);
}
@Test
public void testMultipleContentLengthHeadersBehavior() {
String requestStr = setupRequestString();
assertThat(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)), is(true));
HttpRequest request = channel.readInbound();
if (allowDuplicateContentLengths) {
if (sameValue) {
assertValid(request);
List<String> contentLengths = request.headers().getAll(HttpHeaderNames.CONTENT_LENGTH);
assertThat(contentLengths, contains("1"));
LastHttpContent body = channel.readInbound();
assertThat(body.content().readableBytes(), is(1));
assertThat(body.content().readCharSequence(1, CharsetUtil.US_ASCII).toString(), is("a"));
} else {
assertInvalid(request);
}
} else {
assertInvalid(request);
}
assertThat(channel.finish(), is(false));
}
private String setupRequestString() {
String firstValue = "1";
String secondValue = sameValue ? firstValue : "2";
String contentLength;
if (singleField) {
contentLength = "Content-Length: " + firstValue + ", " + secondValue + "\r\n\r\n";
} else {
contentLength = "Content-Length: " + firstValue + "\r\n" +
"Content-Length: " + secondValue + "\r\n\r\n";
}
return "PUT /some/path HTTP/1.1\r\n" +
contentLength +
"ab";
}
@Test
public void testDanglingComma() {
String requestStr = "GET /some/path HTTP/1.1\r\n" +
"Content-Length: 1,\r\n" +
"Connection: close\n\n" +
"ab";
assertThat(channel.writeInbound(Unpooled.copiedBuffer(requestStr, CharsetUtil.US_ASCII)), is(true));
HttpRequest request = channel.readInbound();
assertInvalid(request);
assertThat(channel.finish(), is(false));
}
private static void assertValid(HttpRequest request) {
assertThat(request.decoderResult().isFailure(), is(false));
}
private static void assertInvalid(HttpRequest request) {
assertThat(request.decoderResult().isFailure(), is(true));
assertThat(request.decoderResult().cause(), instanceOf(IllegalArgumentException.class));
assertThat(request.decoderResult().cause().getMessage(),
containsString("Multiple Content-Length values found"));
}
}