Repository navigation
RequestBodyBufferMiddleware: 411 No Content Length for GET requests? #232
Description
Activity
Thanks for reporting! A normal
GETrequest does not specify aContent-Lengthrequest header and noTransfer-Encodingrequest header and as such has a defined size of0bytes and this above code should not trigger at all.Requests that do use a body but do specify a
Content-Lengthrequest header MUST useTransfer-Encoding: chunked. This is a very rare case however and is not currently supported by theRequestBodyBufferMiddleware.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!
@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: chunkedwithoutContent-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.7response:
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: closeunder the hood the
Server->handleRequestseems 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; }@andig Thanks for digging into this and confirming my above assertion 👍
Apparently your above request uses the
Transfer-Encoding: chunkedrequest header despite sending an empty body (GETrequests 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 code411(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-Lengthheader 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 incomingContent-Length?I hope this helps 👍
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.
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)); });@andig @clue Imho we have a few options here:
- Fit
RequestBodyBufferMiddlewarewith a flag where it does accept achunkedencoded request and buffers the body but rejects it the moment it goes over the limit. - Add a new middleware that specifically handles this case.
- Provide it through a third party me / @friends-of-reactphp / @andig
- 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?
- Fit
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
ChunkedDecoderare 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.
Does your client send a correct body in that case, so
0\r\n\r\n?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?
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.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://inputwhich 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) ""If I
fread()one or more bytes first itseof. Reading 0 bytes doesn't change it (original order at https://3v4l.org/nJdCT).You obviously have to read at least one byte. ;-)
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
OverflowExceptionerror of the underlyingSizeLimitedStream. 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 theServer, not the middleware.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
Or we could make this scenario work as the request is legal ;)
That's of course preferred.
@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
RequestBodyBufferMiddlewareand that is to pass the max size onto thebufferfunction. (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.Reacted by andigThere 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
OverflowExceptionin 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.
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
Content-lengthis 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.IMHO that would mean that the 411 should not be returned for GET requests and instead 0 assumed.
cc @WyriHaximus