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

Correct expect header handling

Browse Source 
Motivation:

Today, the HTTP codec in Netty responds to HTTP/1.1 requests containing
an "expect: 100-continue" header and a content-length that exceeds the
max content length for the server with a 417 status (Expectation
Failed). This is a violation of the HTTP specification. The purpose of
this commit is to address this situation by modifying the HTTP codec to
respond in this situation with a 413 status (Request Entity Too
Large). Additionally, the HTTP codec ignores expectations in the expect
header that are currently unsupported. This commit also addresses this
situation by responding with a 417 status.

Handling the expect header is tricky business as the specification (RFC
2616) is more complicated than it needs to be. The specification defines
the legitimate values for this header as "100-continue" and defines the
notion of expectatation extensions. Further, the specification defines a
417 status (Expectation Failed) and this is where implementations go
astray. The intent of the specification was for servers to respond with
417 status when they do not support the expectation in the expect
header.

The key sentence from the specification follows:

    The server MUST respond with a 417 (Expectation Failed) status if
    any of the expectations cannot be met or, if there are other
    problems with the request, some other 4xx status.

That is, a server should respond with a 417 status if and only if there
is an expectation that the server does not support (whether it be
100-continue, or another expectation extension), and should respond with
another 4xx status code if the expectation is supported but there is
something else wrong with the request.

Modifications:

This commit modifies the HTTP codec by changing the handling for the
expect header in the HTTP object aggregator. In particular, the codec
will now respond with 417 status if any expectation other than
100-continue is present in the expect header, the codec will respond
with 413 status if the 100-continue expectation is present in the expect
header and the content-length is larger than the max content length for
the aggregator, and otherwise the codec will respond with 100 status.

Result:

The HTTP codec can now be used to correctly reply to clients that send a
100-continue expectation with a content-length that is too large for the
server with a 413 status, and servers that use the HTTP codec will now
no longer ignore expectations that are not supported (any value other
than 100-continue).
This commit is contained in:
Jason Tedor 2017-02-14 12:09:52 -05:00 committed by Norman Maurer
parent b7acae03f2
commit c92565d5c7
4 changed files with 179 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java
View File

@ -156,14 +156,19 @@ public class HttpObjectAggregator
@Override
protected Object newContinueResponse(HttpMessage start, int maxContentLength, ChannelPipeline pipeline) {
if (HttpUtil.is100ContinueExpected(start)) {
if (HttpUtil.isUnsupportedExpectation(start)) {
// if the request contains an unsupported expectation, we return 417
pipeline.fireUserEventTriggered(HttpExpectationFailedEvent.INSTANCE);
return EXPECTATION_FAILED.retainedDuplicate();
} else if (HttpUtil.is100ContinueExpected(start)) {
// if the request contains 100-continue but the content-length is too large, we return 413
if (getContentLength(start, -1L) <= maxContentLength) {
return CONTINUE.retainedDuplicate();
}
pipeline.fireUserEventTriggered(HttpExpectationFailedEvent.INSTANCE);
return EXPECTATION_FAILED.retainedDuplicate();
return TOO_LARGE.retainedDuplicate();
}
return null;
}
@ -174,8 +179,11 @@ public class HttpObjectAggregator
@Override
protected boolean ignoreContentAfterContinueResponse(Object msg) {
return msg instanceof HttpResponse &&
((HttpResponse) msg).status().code() == HttpResponseStatus.EXPECTATION_FAILED.code();
if (msg instanceof HttpResponse) {
final HttpResponse httpResponse = (HttpResponse) msg;
return httpResponse.status().codeClass().equals(HttpStatusClass.CLIENT_ERROR);
}
return false;
}
@Override

50
codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java
View File

@ -251,31 +251,49 @@ public final class HttpUtil {
}
/**
* Returns {@code true} if and only if the specified message contains the
* {@code "Expect: 100-continue"} header.
* Returns {@code true} if and only if the specified message contains an expect header and the only expectation
* present is the 100-continue expectation. Note that this method returns {@code false} if the expect header is
* not valid for the message (e.g., the message is a response, or the version on the message is HTTP/1.0).
*
* @param message the message
* @return {@code true} if and only if the expectation 100-continue is present and it is the only expectation
* present
*/
public static boolean is100ContinueExpected(HttpMessage message) {
// Expect: 100-continue is for requests only.
if (!(message instanceof HttpRequest)) {
if (!isExpectHeaderValid(message)) {
return false;
}
// It works only on HTTP/1.1 or later.
if (message.protocolVersion().compareTo(HttpVersion.HTTP_1_1) < 0) {
final String expectValue = message.headers().get(HttpHeaderNames.EXPECT);
// unquoted tokens in the expect header are case-insensitive, thus 100-continue is case insensitive
return HttpHeaderValues.CONTINUE.toString().equalsIgnoreCase(expectValue);
}
/**
* Returns {@code true} if the specified message contains an expect header specifying an expectation that is not
* supported. Note that this method returns {@code false} if the expect header is not valid for the message
* (e.g., the message is a response, or the version on the message is HTTP/1.0).
*
* @param message the message
* @return {@code true} if and only if an expectation is present that is not supported
*/
static boolean isUnsupportedExpectation(HttpMessage message) {
if (!isExpectHeaderValid(message)) {
return false;
}
// In most cases, there will be one or zero 'Expect' header.
CharSequence value = message.headers().get(HttpHeaderNames.EXPECT);
if (value == null) {
return false;
}
if (HttpHeaderValues.CONTINUE.contentEqualsIgnoreCase(value)) {
return true;
}
final String expectValue = message.headers().get(HttpHeaderNames.EXPECT);
return expectValue != null && !HttpHeaderValues.CONTINUE.toString().equalsIgnoreCase(expectValue);
}
// Multiple 'Expect' headers. Search through them.
return message.headers().contains(HttpHeaderNames.EXPECT, HttpHeaderValues.CONTINUE, true);
private static boolean isExpectHeaderValid(final HttpMessage message) {
/*
* Expect: 100-continue is for requests only and it works only on HTTP/1.1 or later. Note further that RFC 7231
* section 5.1.1 says "A server that receives a 100-continue expectation in an HTTP/1.0 request MUST ignore
* that expectation."
*/
return message instanceof HttpRequest &&
message.protocolVersion().compareTo(HttpVersion.HTTP_1_1) >= 0;
}
/**

58
codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java
View File

@ -293,9 +293,9 @@ public class HttpObjectAggregatorTest {
// Send a request with 100-continue + large Content-Length header value.
assertFalse(embedder.writeInbound(message));
// The aggregator should respond with '417.'
// The aggregator should respond with '413.'
FullHttpResponse response = embedder.readOutbound();
assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.status());
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH));
// An ill-behaving client could continue to send data without a respect, and such data should be discarded.
@ -324,6 +324,52 @@ public class HttpObjectAggregatorTest {
assertFalse(embedder.finish());
}
@Test
public void testUnsupportedExpectHeaderExpectation() {
runUnsupportedExceptHeaderExceptionTest(true);
runUnsupportedExceptHeaderExceptionTest(false);
}
private void runUnsupportedExceptHeaderExceptionTest(final boolean close) {
final HttpObjectAggregator aggregator;
final int maxContentLength = 4;
if (close) {
aggregator = new HttpObjectAggregator(maxContentLength, true);
} else {
aggregator = new HttpObjectAggregator(maxContentLength);
}
final EmbeddedChannel embedder = new EmbeddedChannel(new HttpRequestDecoder(), aggregator);
assertFalse(embedder.writeInbound(Unpooled.copiedBuffer(
"GET / HTTP/1.1\r\n" +
"Expect: chocolate=yummy\r\n" +
"Content-Length: 100\r\n\r\n", CharsetUtil.US_ASCII)));
assertNull(embedder.readInbound());
final FullHttpResponse response = (FullHttpResponse) embedder.readOutbound();
assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.status());
assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH));
response.release();
if (close) {
assertFalse(embedder.isOpen());
} else {
// keep-alive is on by default in HTTP/1.1, so the connection should be still alive
assertTrue(embedder.isOpen());
// the decoder should be reset by the aggregator at this point and be able to decode the next request
assertTrue(embedder.writeInbound(Unpooled.copiedBuffer("GET / HTTP/1.1\r\n\r\n", CharsetUtil.US_ASCII)));
final FullHttpRequest request = (FullHttpRequest) embedder.readInbound();
assertThat(request.method(), is(HttpMethod.GET));
assertThat(request.uri(), is("/"));
assertThat(request.content().readableBytes(), is(0));
request.release();
}
assertFalse(embedder.finish());
}
@Test
public void testOversizedRequestWith100ContinueAndDecoder() {
EmbeddedChannel embedder = new EmbeddedChannel(new HttpRequestDecoder(), new HttpObjectAggregator(4));
@ -335,7 +381,7 @@ public class HttpObjectAggregatorTest {
assertNull(embedder.readInbound());
FullHttpResponse response = (FullHttpResponse) embedder.readOutbound();
assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.status());
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH));
// Keep-alive is on by default in HTTP/1.1, so the connection should be still alive.
@ -364,7 +410,7 @@ public class HttpObjectAggregatorTest {
assertNull(embedder.readInbound());
FullHttpResponse response = (FullHttpResponse) embedder.readOutbound();
assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.status());
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH));
// We are forcing the connection closed if an expectation is exceeded.
@ -388,9 +434,9 @@ public class HttpObjectAggregatorTest {
// Send a request with 100-continue + large Content-Length header value.
assertFalse(embedder.writeInbound(message));
// The aggregator should respond with '417'.
// The aggregator should respond with '413'.
FullHttpResponse response = embedder.readOutbound();
assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.status());
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
assertEquals("0", response.headers().get(HttpHeaderNames.CONTENT_LENGTH));
// An ill-behaving client could continue to send data without a respect, and such data should be discarded.

80
codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java
View File

@ -16,8 +16,10 @@
package io.netty.handler.codec.http;
import io.netty.util.CharsetUtil;
import io.netty.util.ReferenceCountUtil;
import org.junit.Test;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
@ -118,4 +120,82 @@ public class HttpUtilTest {
List<String> expected = Collections.singletonList("chunked");
assertEquals(expected, message.headers().getAll(HttpHeaderNames.TRANSFER_ENCODING));
}
private static List<String> allPossibleCasesOfContinue() {
final List<String> cases = new ArrayList<String>();
final String c = "continue";
for (int i = 0; i < Math.pow(2, c.length()); i++) {
final StringBuilder sb = new StringBuilder(c.length());
int j = i;
int k = 0;
while (j > 0) {
if ((j & 1) == 1) {
sb.append(Character.toUpperCase(c.charAt(k++)));
} else {
sb.append(c.charAt(k++));
}
j >>= 1;
}
for (; k < c.length(); k++) {
sb.append(c.charAt(k));
}
cases.add(sb.toString());
}
return cases;
}
@Test
public void testIs100Continue() {
// test all possible cases of 100-continue
for (final String continueCase : allPossibleCasesOfContinue()) {
run100ContinueTest(HttpVersion.HTTP_1_1, "100-" + continueCase, true);
}
run100ContinueTest(HttpVersion.HTTP_1_1, null, false);
run100ContinueTest(HttpVersion.HTTP_1_1, "chocolate=yummy", false);
run100ContinueTest(HttpVersion.HTTP_1_0, "100-continue", false);
final HttpMessage message = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
message.headers().set(HttpHeaderNames.EXPECT, "100-continue");
run100ContinueTest(message, false);
}
private void run100ContinueTest(final HttpVersion version, final String expectations, boolean expect) {
final HttpMessage message = new DefaultFullHttpRequest(version, HttpMethod.GET, "/");
if (expectations != null) {
message.headers().set(HttpHeaderNames.EXPECT, expectations);
}
run100ContinueTest(message, expect);
}
private void run100ContinueTest(final HttpMessage message, final boolean expected) {
assertEquals(expected, HttpUtil.is100ContinueExpected(message));
ReferenceCountUtil.release(message);
}
@Test
public void testContainsUnsupportedExpectation() {
// test all possible cases of 100-continue
for (final String continueCase : allPossibleCasesOfContinue()) {
runUnsupportedExpectationTest(HttpVersion.HTTP_1_1, "100-" + continueCase, false);
}
runUnsupportedExpectationTest(HttpVersion.HTTP_1_1, null, false);
runUnsupportedExpectationTest(HttpVersion.HTTP_1_1, "chocolate=yummy", true);
runUnsupportedExpectationTest(HttpVersion.HTTP_1_0, "100-continue", false);
final HttpMessage message = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
message.headers().set("Expect", "100-continue");
runUnsupportedExpectationTest(message, false);
}
private void runUnsupportedExpectationTest(final HttpVersion version, final String expectations, boolean expect) {
final HttpMessage message = new DefaultFullHttpRequest(version, HttpMethod.GET, "/");
if (expectations != null) {
message.headers().set("Expect", expectations);
}
runUnsupportedExpectationTest(message, expect);
}
private void runUnsupportedExpectationTest(final HttpMessage message, final boolean expected) {
assertEquals(expected, HttpUtil.isUnsupportedExpectation(message));
ReferenceCountUtil.release(message);
}
}