diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java index 0082910fcc..66801cfe72 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java @@ -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 diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java index 3636c5c466..12702fb8c2 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java @@ -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; } /** diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java index 2421e5e88c..7100e67b82 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java @@ -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. diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java index 0484e8b8cc..7bc1cc990b 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java @@ -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 expected = Collections.singletonList("chunked"); assertEquals(expected, message.headers().getAll(HttpHeaderNames.TRANSFER_ENCODING)); } + + private static List allPossibleCasesOfContinue() { + final List cases = new ArrayList(); + 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); + } + }