From 0255a0ae731ad31b5b6c4d0980eb0a5f9f55635b Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Mon, 3 Aug 2015 15:09:44 -0700 Subject: [PATCH] HttpObjectAggregator doesn't check content-length header Motivation: The HttpObjectAggregator always responds with a 100-continue response. It should check the Content-Length header to see if the content length is OK, and if not responds with a 417. Modifications: - HttpObjectAggregator checks the Content-Length header in the case of a 100-continue. Result: HttpObjectAggregator responds with 417 if content is known to be too big. --- .../http/HttpExpectationFailedEvent.java | 25 +++ .../codec/http/HttpObjectAggregator.java | 60 ++++++-- .../handler/codec/http/HttpObjectDecoder.java | 10 ++ .../codec/http/HttpObjectAggregatorTest.java | 142 ++++++++++++++++++ 4 files changed, 224 insertions(+), 13 deletions(-) create mode 100644 codec-http/src/main/java/io/netty/handler/codec/http/HttpExpectationFailedEvent.java diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpExpectationFailedEvent.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpExpectationFailedEvent.java new file mode 100644 index 0000000000..5c9f91fb10 --- /dev/null +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpExpectationFailedEvent.java @@ -0,0 +1,25 @@ +/* + * Copyright 2015 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; + +/** + * A user event designed to communicate that a expectation has failed and there should be no expectation that a + * body will follow. + */ +public final class HttpExpectationFailedEvent { + public static final HttpExpectationFailedEvent INSTANCE = new HttpExpectationFailedEvent(); + private HttpExpectationFailedEvent() { } +} 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 415e2a849d..bd812c73c3 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 @@ -18,7 +18,6 @@ package io.netty.handler.codec.http; import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBufHolder; import io.netty.buffer.CompositeByteBuf; -import io.netty.buffer.DefaultByteBufHolder; import io.netty.buffer.Unpooled; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelFutureListener; @@ -28,10 +27,13 @@ import io.netty.channel.ChannelPipeline; import io.netty.handler.codec.DecoderResult; import io.netty.handler.codec.MessageToMessageDecoder; import io.netty.handler.codec.TooLongFrameException; +import io.netty.handler.codec.http.HttpHeaders.Names; import java.util.List; -import static io.netty.handler.codec.http.HttpHeaders.*; +import static io.netty.handler.codec.http.HttpHeaders.is100ContinueExpected; +import static io.netty.handler.codec.http.HttpHeaders.isContentLengthSet; +import static io.netty.handler.codec.http.HttpHeaders.removeTransferEncodingChunked; /** * A {@link ChannelHandler} that aggregates an {@link HttpMessage} @@ -56,10 +58,17 @@ public class HttpObjectAggregator extends MessageToMessageDecoder { public static final int DEFAULT_MAX_COMPOSITEBUFFER_COMPONENTS = 1024; private static final FullHttpResponse CONTINUE = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.CONTINUE, Unpooled.EMPTY_BUFFER); + private static final FullHttpResponse EXPECTATION_FAILED = new DefaultFullHttpResponse( + HttpVersion.HTTP_1_1, HttpResponseStatus.EXPECTATION_FAILED, Unpooled.EMPTY_BUFFER); + + static { + HttpHeaders.setContentLength(EXPECTATION_FAILED, 0); + } private final int maxContentLength; private AggregatedFullHttpMessage currentMessage; private boolean tooLongFrameFound; + private final boolean closeOnExpectationFailed; private int maxCumulationBufferComponents = DEFAULT_MAX_COMPOSITEBUFFER_COMPONENTS; private ChannelHandlerContext ctx; @@ -73,14 +82,26 @@ public class HttpObjectAggregator extends MessageToMessageDecoder { * a {@link TooLongFrameException} will be raised. */ public HttpObjectAggregator(int maxContentLength) { - if (maxContentLength <= 0) { - throw new IllegalArgumentException( - "maxContentLength must be a positive integer: " + - maxContentLength); - } - this.maxContentLength = maxContentLength; + this(maxContentLength, false); } + /** + * Creates a new instance. + * @param maxContentLength + * the maximum length of the aggregated content in bytes. + * If the length of the aggregated content exceeds this value, + * a {@link TooLongFrameException} will be raised. + * @param closeOnExpectationFailed If a 100-continue response is detected but the content length is too large + * then {@code true} means close the connection. otherwise the connection will remain open and data will be + * consumed and discarded until the next request is received. + */ + public HttpObjectAggregator(int maxContentLength, boolean closeOnExpectationFailed) { + if (maxContentLength <= 0) { + throw new IllegalArgumentException("maxContentLength must be a positive integer: " + maxContentLength); + } + this.maxContentLength = maxContentLength; + this.closeOnExpectationFailed = closeOnExpectationFailed; + } /** * Returns the maximum number of components in the cumulation buffer. If the number of * the components in the cumulation buffer exceeds this value, the components of the @@ -124,12 +145,25 @@ public class HttpObjectAggregator extends MessageToMessageDecoder { HttpMessage m = (HttpMessage) msg; // Handle the 'Expect: 100-continue' header if necessary. - // TODO: Respond with 413 Request Entity Too Large - // and discard the traffic or close the connection. - // No need to notify the upstream handlers - just log. - // If decoding a response, just throw an exception. if (is100ContinueExpected(m)) { - ctx.writeAndFlush(CONTINUE).addListener(new ChannelFutureListener() { + if (HttpHeaders.getContentLength(m, 0) > maxContentLength) { + tooLongFrameFound = true; + final ChannelFuture future = ctx.writeAndFlush(EXPECTATION_FAILED.duplicate().retain()); + future.addListener(new ChannelFutureListener() { + @Override + public void operationComplete(ChannelFuture future) throws Exception { + if (!future.isSuccess()) { + ctx.fireExceptionCaught(future.cause()); + } + } + }); + if (closeOnExpectationFailed) { + future.addListener(ChannelFutureListener.CLOSE); + } + ctx.pipeline().fireUserEventTriggered(HttpExpectationFailedEvent.INSTANCE); + return; + } + ctx.writeAndFlush(CONTINUE.duplicate().retain()).addListener(new ChannelFutureListener() { @Override public void operationComplete(ChannelFuture future) throws Exception { if (!future.isSuccess()) { diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index 76cee53669..d155d8e450 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -433,6 +433,16 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { // This method is responsible for ending requests in some situations and must be called // when the input has been shutdown. super.channelInactive(ctx); + } else if (evt instanceof HttpExpectationFailedEvent) { + switch (currentState) { + case READ_FIXED_LENGTH_CONTENT: + case READ_VARIABLE_LENGTH_CONTENT: + case READ_CHUNK_SIZE: + reset(); + break; + default: + break; + } } super.userEventTriggered(ctx, evt); } 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 4e096c7392..5ad01a1ea9 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 @@ -209,4 +209,146 @@ public class HttpObjectAggregatorTest { assertNull(ch.readInbound()); ch.finish(); } + + @Test + public void testOversizedRequestWith100Continue() { + EmbeddedChannel embedder = new EmbeddedChannel(new HttpObjectAggregator(8)); + + // send an oversized request with 100 continue + HttpRequest message = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost"); + HttpHeaders.set100ContinueExpected(message, true); + HttpHeaders.setContentLength(message, 16); + + HttpContent chunk1 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("some", CharsetUtil.US_ASCII))); + HttpContent chunk2 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("test", CharsetUtil.US_ASCII))); + HttpContent chunk3 = LastHttpContent.EMPTY_LAST_CONTENT; + + // Send a request with 100-continue + large Content-Length header value. + assertFalse(embedder.writeInbound(message)); + + // The aggregator should respond with '417.' + FullHttpResponse response = (FullHttpResponse) embedder.readOutbound(); + assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.getStatus()); + assertEquals("0", response.headers().get(HttpHeaders.Names.CONTENT_LENGTH)); + + // An ill-behaving client could continue to send data without a respect, and such data should be discarded. + assertFalse(embedder.writeInbound(chunk1)); + + // The aggregator should not close the connection because keep-alive is on. + assertTrue(embedder.isOpen()); + + // Now send a valid request. + HttpRequest message2 = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost"); + + assertFalse(embedder.writeInbound(message2)); + assertFalse(embedder.writeInbound(chunk2)); + assertTrue(embedder.writeInbound(chunk3)); + + FullHttpRequest fullMsg = (FullHttpRequest) embedder.readInbound(); + assertNotNull(fullMsg); + + assertEquals( + chunk2.content().readableBytes() + chunk3.content().readableBytes(), + HttpHeaders.getContentLength(fullMsg)); + + assertEquals(HttpHeaders.getContentLength(fullMsg), fullMsg.content().readableBytes()); + + fullMsg.release(); + assertFalse(embedder.finish()); + } + + @Test + public void testOversizedRequestWith100ContinueAndDecoder() { + EmbeddedChannel embedder = new EmbeddedChannel(new HttpRequestDecoder(), new HttpObjectAggregator(4)); + embedder.writeInbound(Unpooled.copiedBuffer( + "PUT /upload HTTP/1.1\r\n" + + "Expect: 100-continue\r\n" + + "Content-Length: 100\r\n\r\n", CharsetUtil.US_ASCII)); + + assertNull(embedder.readInbound()); + + FullHttpResponse response = (FullHttpResponse) embedder.readOutbound(); + assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.getStatus()); + assertEquals("0", response.headers().get(HttpHeaders.Names.CONTENT_LENGTH)); + + // 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. + embedder.writeInbound(Unpooled.copiedBuffer("GET /max-upload-size HTTP/1.1\r\n\r\n", CharsetUtil.US_ASCII)); + + FullHttpRequest request = (FullHttpRequest) embedder.readInbound(); + assertThat(request.getMethod(), is(HttpMethod.GET)); + assertThat(request.getUri(), is("/max-upload-size")); + assertThat(request.content().readableBytes(), is(0)); + request.release(); + + assertFalse(embedder.finish()); + } + + @Test + public void testOversizedRequestWith100ContinueAndDecoderCloseConnection() { + EmbeddedChannel embedder = new EmbeddedChannel(new HttpRequestDecoder(), new HttpObjectAggregator(4, true)); + embedder.writeInbound(Unpooled.copiedBuffer( + "PUT /upload HTTP/1.1\r\n" + + "Expect: 100-continue\r\n" + + "Content-Length: 100\r\n\r\n", CharsetUtil.US_ASCII)); + + assertNull(embedder.readInbound()); + + FullHttpResponse response = (FullHttpResponse) embedder.readOutbound(); + assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.getStatus()); + assertEquals("0", response.headers().get(HttpHeaders.Names.CONTENT_LENGTH)); + + // We are forcing the connection closed if an expectation is exceeded. + assertFalse(embedder.isOpen()); + assertFalse(embedder.finish()); + } + + @Test + public void testRequestAfterOversized100ContinueAndDecoder() { + EmbeddedChannel embedder = new EmbeddedChannel(new HttpRequestDecoder(), new HttpObjectAggregator(15)); + + // Write first request with Expect: 100-continue + HttpRequest message = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost"); + HttpHeaders.set100ContinueExpected(message, true); + HttpHeaders.setContentLength(message, 16); + + HttpContent chunk1 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("some", CharsetUtil.US_ASCII))); + HttpContent chunk2 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("test", CharsetUtil.US_ASCII))); + HttpContent chunk3 = LastHttpContent.EMPTY_LAST_CONTENT; + + // Send a request with 100-continue + large Content-Length header value. + assertFalse(embedder.writeInbound(message)); + + // The aggregator should respond with '417' + FullHttpResponse response = (FullHttpResponse) embedder.readOutbound(); + assertEquals(HttpResponseStatus.EXPECTATION_FAILED, response.getStatus()); + assertEquals("0", response.headers().get(HttpHeaders.Names.CONTENT_LENGTH)); + + // An ill-behaving client could continue to send data without a respect, and such data should be discarded. + assertFalse(embedder.writeInbound(chunk1)); + + // The aggregator should not close the connection because keep-alive is on. + assertTrue(embedder.isOpen()); + + // Now send a valid request. + HttpRequest message2 = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost"); + + assertFalse(embedder.writeInbound(message2)); + assertFalse(embedder.writeInbound(chunk2)); + assertTrue(embedder.writeInbound(chunk3)); + + FullHttpRequest fullMsg = (FullHttpRequest) embedder.readInbound(); + assertNotNull(fullMsg); + + assertEquals( + chunk2.content().readableBytes() + chunk3.content().readableBytes(), + HttpHeaders.getContentLength(fullMsg)); + + assertEquals(HttpHeaders.getContentLength(fullMsg), fullMsg.content().readableBytes()); + + fullMsg.release(); + assertFalse(embedder.finish()); + } }