Repository: netty/netty
Issue: Possible wrong behavior in `HttpResponseDecoder`/`HttpRequestDecoder` for large header/initline/content

##### Netty Version

4.0, 4.1, 5.0
##### Description

[`HttpResponseDecoder`](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java#L118) and [`HttpRequestDecoder`](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java#L87) in the event when the max configured sizes for HTTP initial line, headers or content is breached, sends a `DefaultHttpResponse` and `DefaultHttpRequest` respectively. After this [`HttpObjectDecoder`](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java#L369) gets into `BAD_MESSAGE` state and ignores any other data received on this connection.
The combination of the above two behaviors, means that the decoded response/request are not complete (absence of sending `LastHTTPContent`). So, any code, waiting for a complete message will have to additionally check for decoder result to follow the correct semantics of HTTP.
##### Question

Is there a reason why we do not want to raise an exception in the pipeline instead of propagating an invalid HTTP message? 
If yes, then, should we send a `DefaultFullHttpResponse`/`DefaultFullHttpRequest` instead so atleast the semantics of a complete HTTP request/response is honored?

I can send a PR for this if I understand what should be the correct behavior here. 

_PS: If the issue description is not clear, I can create a quick test for it._


Issue comments:
That does sound a bit fishy...I can't give history for why this was done but I'm guessing the intention was to send something to the other end (as opposed to relying on timeouts or other failure detection mechanisms)?  No matter what the intention was I agree based upon your description it seems like we are not meeting that now.  If the content can be well formed w.r.t HTTP (either we have not sent any headers, we aren't providing a conflicting content length or any other headers, or we can still gracefully fail within the HTTP protocol) then I think it would make sense to provide the response immediately.  If there is a chance the message would violate the protocol then either see if we can mitigate this or potentially throw an exception in the pipeline.

On a related note...
- HTTP/2 provides feedback to the peer for a similar situation via a RST_STREAM frame [here](https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapter.java#L273).
- SPDY behaves in a similar manner [here](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpDecoder.java#L327) and throws an exception into the pipeline.  I'm not as familiar with SPDY so I'm not sure if there is a mechanism setup to catch the exception and provide feedback to the peer.

> I can't give history for why this was done but I'm guessing the intention was to send something to the other end 

Yeah I think so too, although the current approach creates an awkward usage pattern of having to look at  `decoderResult()` in `HttpObject`. This is easy to overlook and get into receiving incomplete HTTP messages.

IMO, we should raise an exception and have the user decide how to handle this. 

I am not so sure if it is better to ignore all future content on the connection (today's behavior) or ignore it till the end of response (if exists) and then reuse the connection for further requests. The reason for reuse being that it's the receiver end's configuration to limit the header/initline/content size and not a part of the protocol, so theoretically, it is not a bad HTTP message. However, I can understand the scenario of a DOS attack if we keep reading content on the server till eternity.
On the contrary, if we choose to ignore all future content on the connection (as it is today), isn't it better to just close the connection as opposed to keep reading from the connection to ignore?

Thanks for providing HTTP/2, SPDY context. HTTP/2 then is equivalent to closing connection on HTTP/1

@normanmaurer @trustin thoughts?

> On the contrary, if we choose to ignore all future content on the connection (as it is today), isn't it better to just close the connection as opposed to keep reading from the connection to ignore?

Closing the connection will indeed provide some notification to the peer but one thing to be mindful of is pipelining.  Is it possible the user may want to consider the current request bad, but future messages on the same connection may be OK?  This may be a bit of an esoteric boundary case as it depends upon the ability to detect when the current message ends and the next message begins (after the max length has been exceeded).

> HTTP/2 then is equivalent to closing connection on HTTP/1

I think there are a few subtle differences:
1. There is rational as to what happened at the application layer for HTTP/2 as opposed to shutdown from the transport layer.
2. Currently we are closing the connection in HTTP/2 but potentially we could allow other existing streams being processed to complete.

Hi @NiteshKant thanks for filing an issue.

Some history:

The HTTP spec requires us to introspect the message to determine the appropriate response code to send back to the peer. For example, there are two different codes depending on whether the initial line was too long or the headers block was too large (414 URI Too Long vs 431 Request Header Fields Too Large).

Further, when introducing SPDY we wanted to be able to share channel handlers between HTTP and SPDY (such as those responsible for retuning a 400 vs 414 vs 431 response). This meant that we couldn't signal these by sending different types of exceptions up the pipeline, since we would need to the Stream-Id to send the response on and we didn't want to invalidate other requests on the connection.

On to FullHttpMessage:

This change was put into place before the message objects were refactored (back when the 4.x branches still used HttpMessage/HttpChunk) and thus before the Full\* classes were introduced. I think modifying the decoder to emit these objects as "Full" messages sounds feasible, but I'd like to see how this interacts with Channel Handlers designed to be shared between both HTTP and SPDY / HTTP/2 connections.

Thanks @jpinner for the input!

> For example, there are two different codes depending on whether the initial line was too long or the headers block was too large (414 URI Too Long vs 431 Request Header Fields Too Large).

Glad you mentioned it, as there isn't a way today to do that (unless one wants to inspect the exception message string). `HttpObjectDecoder` should provide that information while calling `createInvalidMessage()` IMO.

> Further, when introducing SPDY we wanted to be able to share channel handlers between HTTP and SPDY (such as those responsible for retuning a 400 vs 414 vs 431 response). This meant that we couldn't signal these by sending different types of exceptions up the pipeline, since we would need to the Stream-Id to send the response on and we didn't want to invalidate other requests on the connection.

I am a bit confused about this. Is this still true in terms of reusing channel handlers between SPDY and HTTP? I did not see that in the examples & SPDY code. 

>  I think modifying the decoder to emit these objects as "Full" messages sounds feasible, but I'd like to see how this interacts with Channel Handlers designed to be shared between both HTTP and SPDY / HTTP/2 connections.

Great, can you point me to a test/example that I can use to validate if it is working fine?

Based on the inputs, I will categorize the issue in two behaviors:

##### Incomplete `HttpRequest`/`HttpResponse`

This is the fact that [`HttpResponseDecoder`](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java#L118) and [`HttpRequestDecoder`](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java#L87) are not returning a Full\* message. 
It looks like, returning a Full\* message should be a viable solution, provided we can validate SPDY/HTTP/2 usecases.

##### All data ignored by `HttpObjectDecoder` after these errors

This is because [`HttpObjectDecoder`](https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java#L369) gets into `BAD_MESSAGE` state and ignores any other data received on this connection.

It looks like this is an undesirable behavior specifically for HTTP pipelining and keep-alive connections in general. Is there any consensus on a need for fixing this in some way?

@NiteshKant 

> Glad you mentioned it, as there isn't a way today to do that (unless one wants to inspect the exception message string). HttpObjectDecoder should provide that information while calling createInvalidMessage() IMO.

One way to do this is to check and see if the URL is "/bad-request" -- if it is then createMessage(initialLine) was never called. But yes, this is hacky -- I have a better object decoder sitting on the back-burner that I should open source but have never gotten around to it.

> I am a bit confused about this. Is this still true in terms of reusing channel handlers between SPDY and HTTP? I did not see that in the examples & SPDY code.

A common way to set up the pipeline is to add a handler that responds to the NPN to select between HTTP and SPDY decoders, and then to add common handlers after that. If you do that, then exceptions on the pipeline need to correspond to connection-level errors not stream/request-level errors.

To your issue categorizations, yes the fact that we aren't returning "Full" messages could be fixed. As for HttpObjectDecoder itself ignoring future data, it has to because it hasn't read enough of the message to determine proper framing. In all common deployments, these sorts of parsing errors all cause connection closure over HTTP/1.1 connections.

> A common way to set up the pipeline is to add a handler that responds to the NPN to select between HTTP and SPDY decoders, and then to add common handlers after that. If you do that, then exceptions on the pipeline need to correspond to connection-level errors not stream/request-level errors.

Got it, thanks for the explanation.

> In all common deployments, these sorts of parsing errors all cause connection closure over HTTP/1.1 connections.

Agreed that connection closure is a better behavior. If I look at it purely from HTTP/1.1 point of view, I see the following behavior in client and server:

##### Server

Server receives such a request and emits a special `HttpRequest` message in the pipeline. As part of handling this request, typically a server would send a response with a header "Connection: Close" to indicate to the sender that the connection should not be reused. Upon close by the sender, the connection closes.

##### Client

Client receives such a response and emits a special `HttpResponse` message in the pipeline. As part of handling this response, the client is expected to close the connection.

In both the above cases, failure to follow the expected action, would result in an active connection, data on which is silently swallowed by `HttpObjectDecoder`. Should we fix this behavior such that the channel handler does the correct thing for HTTP/1.1 and allows override of this behavior per protocol?

What I am referring to is having an abstract method in `HttpObjectDecoder` like:

``` java
protected abstract void onInvalidMessage(Reason invalidationReason);
```

which is implemented in `HttpRequestDecoder` and `HttpResponseDecoder` to do the correct thing. Any other implementation or an extension of these classes can then override this behavior as needed.

If this makes sense, I can work on a PR to implement it.

>  I'd like to see how this interacts with Channel Handlers designed to be shared between both HTTP and SPDY / HTTP/2 connections.

I'm not sure we have to be too concerned about HTTP/2.  HTTP/2 provides translation from HTTP/2 frame events to HTTP/1.x objects [InboundHttp2ToHttpAdapter](https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpAdapter.java#L39) and translation from HTTP/1.x objects to HTTP/2 frames [HttpToHttp2ConnectionHandler](https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpToHttp2ConnectionHandler.java#L29).  Other than these classes (and some utility support classes) I don't think there is much dependency between HTTP/2 and HTTP/1.x.

> I have a better object decoder sitting on the back-burner that I should open source but have never gotten around to it.

@jpinner - Does your object decoder address this issue in a similar manner to @NiteshKant's proposal?  What other improvements have you made? What is your anticipated time frame for making your improvements available as a PR?

@NiteshKant - I would be interested in seeing a PR.  Unless @jpinner's solution already accounts for this and it can be made available within a reasonable time frame.

@Scottmitch we have an internal HTTP/2 implementation that we use for 3.x and 4.0. As for our object decoder, we've made slight changes to the HTTP objects themselves so that "DecoderResult" is a proper future that can have listeners attached to it. We've also changed how bodies are handled -- we use something similar to the Stream proposal for Netty 5. Let me see how quickly I can open source these to at least provide them as a reference in this discussion.

@NiteshKant the problem with having the decoder have an "onInvalidMessage" that closes the connection is that when acting as a server, it would need to first write a response to the socket. To do this it would have to sit after the object encoder on the pipeline which places an ordering dependency and breaks the HttpServerCodec handler.

Can we start with a simple change that emits "Full" invalid messages?

PR #3379 does the change to emit Full\* invalid messages from the decoders in question.

I will try and put together a PR, attempting to fix the `HttpObjectDecoder` on similar lines as I proposed so that we can discuss the options, even if we do not go that route.

The simple change that emits Full\* on an invalid message has been merged.  Thanks for great discussion, folks.

If I understood correctly, the problem is that there's no easy way to determine if the message is bad because of protocol violation or because of too large headers.  In case the header is too large, we do not need to enter the `BAD_MESSAGE` state but just skip the traffic until the next request/response.

As some folks have mentioned, `DecoderResult` has some limitation and it stems from the lack of proper streaming construct. I hope to introduce/prototype a new construct that could solve this problem altogether, so please feel free to add more input to #859 


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java
index bae740864f5..0fb0339e8ff 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java
@@ -84,7 +84,7 @@ protected HttpMessage createMessage(String[] initialLine) throws Exception {
 
     @Override
     protected HttpMessage createInvalidMessage() {
-        return new DefaultHttpRequest(HttpVersion.HTTP_1_0, HttpMethod.GET, "/bad-request", validateHeaders);
+        return new DefaultFullHttpRequest(HttpVersion.HTTP_1_0, HttpMethod.GET, "/bad-request", validateHeaders);
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java
index 982f98286b1..bbb747057b3 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java
@@ -115,7 +115,7 @@ protected HttpMessage createMessage(String[] initialLine) {
 
     @Override
     protected HttpMessage createInvalidMessage() {
-        return new DefaultHttpResponse(HttpVersion.HTTP_1_0, UNKNOWN_STATUS, validateHeaders);
+        return new DefaultFullHttpResponse(HttpVersion.HTTP_1_0, UNKNOWN_STATUS, validateHeaders);
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java
index 2222ee91c5c..6be4d403a68 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java
@@ -17,6 +17,7 @@
 
 import io.netty.buffer.ByteBuf;
 import io.netty.handler.codec.TooLongFrameException;
+import io.netty.handler.codec.http.DefaultFullHttpRequest;
 import io.netty.handler.codec.http.DefaultHttpRequest;
 import io.netty.handler.codec.http.HttpMessage;
 import io.netty.handler.codec.http.HttpRequest;
@@ -78,7 +79,7 @@ protected HttpMessage createMessage(String[] initialLine) throws Exception {
 
     @Override
     protected HttpMessage createInvalidMessage() {
-        return new DefaultHttpRequest(RtspVersions.RTSP_1_0, RtspMethods.OPTIONS, "/bad-request", validateHeaders);
+        return new DefaultFullHttpRequest(RtspVersions.RTSP_1_0, RtspMethods.OPTIONS, "/bad-request", validateHeaders);
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java
index cdf49902a43..0234c6773b7 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java
@@ -17,6 +17,7 @@
 
 import io.netty.buffer.ByteBuf;
 import io.netty.handler.codec.TooLongFrameException;
+import io.netty.handler.codec.http.DefaultFullHttpResponse;
 import io.netty.handler.codec.http.DefaultHttpResponse;
 import io.netty.handler.codec.http.HttpMessage;
 import io.netty.handler.codec.http.HttpResponse;
@@ -83,7 +84,7 @@ protected HttpMessage createMessage(String[] initialLine) throws Exception {
 
     @Override
     protected HttpMessage createInvalidMessage() {
-        return new DefaultHttpResponse(RtspVersions.RTSP_1_0, UNKNOWN_STATUS, validateHeaders);
+        return new DefaultFullHttpResponse(RtspVersions.RTSP_1_0, UNKNOWN_STATUS, validateHeaders);
     }
 
     @Override


Test patch: