RequestBodyBufferMiddleware: 411 No Content Length for GET requests?

[andig]

I'm doing some testing with Guzzle as client and I'm running into unexpected behaviour- 411 instead of qualified response.

In

http/src/Middleware/RequestBodyBufferMiddleware.php

Line 35 in abdf36e

return new Response(411, array('Content-Type' => 'text/plain'), 'No Content-Length header given');

an HTTP 411 is returned when Content-length is not set. According to https://stackoverflow.com/questions/8540931/what-is-the-correct-content-length-to-set-for-a-get-request this requirement should not be necessary.

A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests.

IMHO that would mean that the 411 should not be returned for GET requests and instead 0 assumed.

cc @WyriHaximus

[clue]

Thanks for reporting! A normal GETrequest does not specify a Content-Length request header and no Transfer-Encoding request header and as such has a defined size of 0 bytes and this above code should not trigger at all.

Requests that do use a body but do specify a Content-Length request header MUST use Transfer-Encoding: chunked. This is a very rare case however and is not currently supported by the RequestBodyBufferMiddleware.

Does this help with the issue you're seeing? Otherwise, can you provide some code for us to reproduce the problem you're seeing? Thank you!

[andig]

@clue here we go. It seems like an odd request that GuzzleHttp produces but thats what seems to cause the problem. The request uses Transfer-Encoding: chunked without Content-Length- is this what you've meant above?

request:

GET /entity.json HTTP/1.1
Host: 127.0.0.1:8080
Transfer-Encoding: chunked
Expect: 100-Continue
user-agent: Symfony/3.X
accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
accept-language: en-us,en;q=0.5
accept-charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7

response:

HTTP 411 Length Required
Content-Type: text/plain
X-Powered-By: React/alpha
Date: Wed, 04 Oct 2017 20:34:18 GMT
Content-Length: 30
Connection: close

under the hood the Server->handleRequest seems to be the problem since it replaces the body length 0 with length NULL.

It seems this is a special case where a chunked body of length 0 (or really a request that cannot have a body?) is removed and length 0 needs be kept?

Took me 4 hours now to get there- too tired to look for a fix...

For time being this is my workaround, probably invalid:

$request = $request->withoutHeader('Transfer-Encoding');
$request = $request->withoutHeader('Content-Length');

if ($request->getBody()->getSize() === 0) {
	$stream = new LengthLimitedStream($stream, 0);
}
else {
	$stream = new ChunkedDecoder($stream);
	$contentLength = null;
}

[clue]

@andig Thanks for digging into this and confirming my above assertion 👍

Apparently your above request uses the Transfer-Encoding: chunked request header despite sending an empty body (GET requests have no defined semantics for a request body and usually don't contain one).

This is indeed a valid request, but as stated above is currently not supported by the RequestBodyBufferMiddleware. This is done because this middleware handler tries to buffer the whole request in memory and with chunked requests, there's no way to tell now big the request body is going to be. This means that we would have to start buffering the whole request before hitting the limit and then reject everything, thus wasting precious bandwidth and resources. Note that this is a common issue and there's even a custom HTTP status code 411 (Length Required) and it looks like many web servers chose to reject these requests.

I'm not sure how relevant this issue is actually in the wild which is why we chose to not implement this in the first step. I suppose we would be happy to re-evaluate this and accept PRs 👍

As an alternative, have you tried explicitly assigning a Content-Length header with your HTTP client? I'm not sure if this is an option in your case, but it looks like you're implementing some sort of reverse proxy and should know the incoming Content-Length?

I hope this helps 👍

[andig]

The root cause for this issue in my setup is actually symfony/symfony#24437. The general behaviour of producing this kind of valid but ugly request is due to Symfony returning a stream when it shouldn't.

From what I understand it shouldn't be too difficult- if you accept potential memory consumption- to buffer requests of unknown length. I'll try to look into this, might be beyond my skill due to the involved stream magic.

[andig]

I'm toying with the idea of buffering chunked requests limited by buffer size (SizeLimitedStream) but I'm failing to create a test case that doesn't stall. Is something like this correct or rather where is it wrong?

$connector->connect($uri)->then(function (ConnectionInterface $conn) use ($loop) {
	$body = Psr7\stream_for('foo'); // psr stream
	$body = new React\Stream\ReadableResourceStream($body, $loop);  // psr->react stream 
	$body = new React\Http\ChunkedEncoder($body); // react stream
	$body = new React\Http\HttpBodyStream($body); // react->psr stream

	$psrRequest = (new Psr7\Request('GET', 'http://127.0.0.1/'))
		->withHeader('Transfer-Encoding', 'chunked')
		->withBody($body)
	;

	$conn->write(Psr7\str($psrRequest));
});

[WyriHaximus]

@andig @clue Imho we have a few options here:

1. Fit RequestBodyBufferMiddleware with a flag where it does accept a chunked encoded request and buffers the body but rejects it the moment it goes over the limit.
2. Add a new middleware that specifically handles this case.
3. Provide it through a third party me / @friends-of-reactphp / @andig
4. Keep as is.

My main concern is that this might be very common behavior among HTTP clients and we would be rejecting their requests thus blocking a possible big percentage of clients out there. In the end I want our users not to have to worry about these kind of things.

/cc @jsor what do you think?

[andig]

My main concern is that this might be very common behavior among HTTP clients and we would be rejecting their requests thus blocking a possible big percentage of clients out there.

I fully agree. Even more so as large parts of the infrastructure like the ChunkedDecoder are already implemented.

From what I've looked into 1)/2) needs collaboration of the server since it already removes those requests. This seems a bit ugly right now since the behaviour cannot be controlled solely by middleware. So also 3) would need help by the Server.

I've opened #235 for sake of discussion with what I've got so far.

[kelunik]

Does your client send a correct body in that case, so 0\r\n\r\n?

[andig]

Does your client send a correct body in that case, so 0\r\n\r\n?

I'll need to double-check the Guzzle internals for that case. Good question how to handle it if it doesn't?

[kelunik]

If it doesn't, then Guzzle / Symfony / whatever needs a bug fix anyway. The client could also just check the first chunk of the stream end see whether the stream is already at EOF at that point before sending transfer-encoding: chunked.

[andig]

The client could also just check the first chunk of the stream end see whether the stream is already at EOF at that point before sending transfer-encoding: chunked.

@kelunik Symfony returns php://input which has imho strange behaviour which seems to make this approach not feasible, at least in cli:

$file = fopen("php://input", "rb");
var_dump(feof($file)); // bool(false)
var_dump(fread($file, 4096)); // string(0) ""

[kelunik]

@andig What happens if you change the order of the var_dumps?

[andig]

If I fread() one or more bytes first its eof. Reading 0 bytes doesn't change it (original order at https://3v4l.org/nJdCT).

[kelunik]

You obviously have to read at least one byte. ;-)

[andig]

You obviously have to read at least one byte. ;-)

Or we could make this scenario work as the request is legal ;)

I've pushed two more commits to #235 that handle the OverflowException error of the underlying SizeLimitedStream. It's probably not particularly pretty but wfm right now. If this is something you'd consider going with I'd add tests. We'd also need a configurable buffer size- unfortunately this time as part of the Server, not the middleware.

[andig]

Update I've updated this once more to move the entire body decoding and handling logic to the RequestBodyBufferMiddleware. This avoids split responsibilities for buffer size and imho also gives more flexibility for other types of middleware handlers since they get more control over the naked request.

ping @WyriHaximus for another look

[kelunik]

Or we could make this scenario work as the request is legal ;)

That's of course preferred.

[WyriHaximus]

@andig Re: #120 (comment) I've had a call with @clue earlier this week regarding this and other issues and we're going to make one minor change to RequestBodyBufferMiddleware and that is to pass the max size onto the buffer function. (And remove the length header check of course.) There is a PR adding the argument required for it reactphp/promise-stream#3 so we can do that. We really appreciate your work on #235 but prefer this simpler way to go. Another thing is that the chunked encoding is already handled underwater by the server so moving it to the middleware would break that.

[andig]

There is a PR adding the argument required for it

@WyriHaximus any solution you think fit is fine with me. Regarding reactphp/promise-stream#3 and it's use I found it useful in #235 to distinguish between "any type of stream exception" and OverflowException in order to reply with HTTP 413. You probably have a solution for that but it's not obvious to me from reactphp/promise-stream#3 (i.e.- use a different type of exception?).

We really appreciate your work on #235 but prefer this simpler way to go

I'm happy that I can contribute smaller things and just a bit impatient for the finished server ;)

Another thing is that the chunked encoding is already handled underwater by the server so moving it to the middleware would break that.

I changed that because it was required with my approach. In hindsight I thought it might even be useful for extensibility but that could also come through subclassing the server rather than middlewares. And how often would you need to implement other transfer encodings...

I'd be happy to update this PR when reactphp/promise-stream#3 is in.

[andig]

I'm closing this in favour of #235 which I'll follow up with.