Don't close the connection whenever Expect: 100-continue is missing.
Motivation: The 4.1.0-Beta3 implementation of HttpObjectAggregator.handleOversizedMessage closes the connection if the client sent oversized chunked data with no Expect: 100-continue header. This causes a broken pipe or "connection reset by peer" error in some clients (tested on Firefox 31 OS X 10.9.5, async-http-client 1.8.14). This part of the HTTP 1.1 spec (below) seems to say that in this scenario the connection should not be closed (unless the intention is to be very strict about how data should be sent). http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html "If an origin server receives a request that does not include an Expect request-header field with the "100-continue" expectation, the request includes a request body, and the server responds with a final status code before reading the entire request body from the transport connection, then the server SHOULD NOT close the transport connection until it has read the entire request, or until the client closes the connection. Otherwise, the client might not reliably receive the response message. However, this requirement is not be construed as preventing a server from defending itself against denial-of-service attacks, or from badly broken client implementations." Modifications: Change HttpObjectAggregator.handleOversizedMessage to close the connection only if keep-alive is off and Expect: 100-continue is missing. Update test to reflect the change. Result: Broken pipe and connection reset errors on the client are avoided when oversized data is sent.
This commit is contained in:
parent
222d258d6e
commit
ac882d56e9
@ -161,10 +161,9 @@ public class HttpObjectAggregator
|
|||||||
});
|
});
|
||||||
|
|
||||||
// If the client started to send data already, close because it's impossible to recover.
|
// If the client started to send data already, close because it's impossible to recover.
|
||||||
// If 'Expect: 100-continue' is missing, close becuase it's impossible to recover.
|
// If keep-alive is off and 'Expect: 100-continue' is missing, no need to leave the connection open.
|
||||||
// If keep-alive is off, no need to leave the connection open.
|
|
||||||
if (oversized instanceof FullHttpMessage ||
|
if (oversized instanceof FullHttpMessage ||
|
||||||
!HttpHeaderUtil.is100ContinueExpected(oversized) || !HttpHeaderUtil.isKeepAlive(oversized)) {
|
(!HttpHeaderUtil.is100ContinueExpected(oversized) && !HttpHeaderUtil.isKeepAlive(oversized))) {
|
||||||
future.addListener(ChannelFutureListener.CLOSE);
|
future.addListener(ChannelFutureListener.CLOSE);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -223,6 +223,53 @@ public class HttpObjectAggregatorTest {
|
|||||||
assertFalse(embedder.finish());
|
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");
|
||||||
|
HttpHeaderUtil.set100ContinueExpected(message, true);
|
||||||
|
HttpHeaderUtil.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 agregator should respond with '413 Request Entity Too Large.'
|
||||||
|
FullHttpResponse response = embedder.readOutbound();
|
||||||
|
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
|
||||||
|
assertEquals("0", response.headers().get(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 = embedder.readInbound();
|
||||||
|
assertNotNull(fullMsg);
|
||||||
|
|
||||||
|
assertEquals(
|
||||||
|
chunk2.content().readableBytes() + chunk3.content().readableBytes(),
|
||||||
|
HttpHeaderUtil.getContentLength(fullMsg));
|
||||||
|
|
||||||
|
assertEquals(HttpHeaderUtil.getContentLength(fullMsg), fullMsg.content().readableBytes());
|
||||||
|
|
||||||
|
fullMsg.release();
|
||||||
|
assertFalse(embedder.finish());
|
||||||
|
}
|
||||||
|
|
||||||
private static void checkOversizedRequest(HttpRequest message) {
|
private static void checkOversizedRequest(HttpRequest message) {
|
||||||
EmbeddedChannel embedder = new EmbeddedChannel(new HttpObjectAggregator(4));
|
EmbeddedChannel embedder = new EmbeddedChannel(new HttpObjectAggregator(4));
|
||||||
|
|
||||||
@ -230,8 +277,25 @@ public class HttpObjectAggregatorTest {
|
|||||||
HttpResponse response = embedder.readOutbound();
|
HttpResponse response = embedder.readOutbound();
|
||||||
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
|
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
|
||||||
assertEquals("0", response.headers().get(Names.CONTENT_LENGTH));
|
assertEquals("0", response.headers().get(Names.CONTENT_LENGTH));
|
||||||
|
|
||||||
|
if (serverShouldCloseConnection(message)) {
|
||||||
assertFalse(embedder.isOpen());
|
assertFalse(embedder.isOpen());
|
||||||
assertFalse(embedder.finish());
|
assertFalse(embedder.finish());
|
||||||
|
} else {
|
||||||
|
assertTrue(embedder.isOpen());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
private static boolean serverShouldCloseConnection(HttpRequest message) {
|
||||||
|
// The connection should only be kept open if Expect: 100-continue is set,
|
||||||
|
// or if keep-alive is on.
|
||||||
|
if (HttpHeaderUtil.is100ContinueExpected(message)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
if (HttpHeaderUtil.isKeepAlive(message)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
Loading…
Reference in New Issue
Block a user