RequestBodyBufferMiddleware should not reject requests that exceed post_max_size

[clue]

If you try out example 12 (introduced via #252), you can see that the server currently rejects the request with a rather ugly error page if you exceed the maximum size of 100 KiB. PHP's default behavior is actually to just ignore the request body and still process things normally otherwise. This also allows the consumer to create a custom error message.

See http://php.net/manual/en/ini.core.php#ini.post-max-size

[WyriHaximus]

Actually this happens in RequestBodyBufferMiddleware, I'll craft a PR for updating that middleware instead 👍

[WyriHaximus]

PR up at #263

[acasademont]

Hi! I stumbled upon this today and I was expecting a 413 on the server. What would be the best way to detect that the request has exceeded the buffer size compared to a legit empty body? By comparing the request size header with the actual body length? Thx!

[clue]

@acasademont You're right, the current middleware implementation doesn't reject excessive requests by design (to be in line with PHP's implementation). Instead, excessive requests will be forwarded like requests with an empty request body.

I don't think we currently expose a difference between empty and excessive requests, but I can totally see some value in this. Have you tried implementing this as a custom middleware or do you think the existing middleware implementation needs some addition?

[acasademont]

@clue Nopes, I just increased the buffer size :p

On PHP you get a warning if the request body size exceeds the post_max_size setting (https://github.com/php/php-src/blob/1b2ec73c1d9175769c3ad4dd40825546851287bc/main/SAPI.c#L253), here it's a bit silent, I spent quite a lot of time debugging one of our big requests and to fully understand what was going on and why the body was blank.

So if we copy PHP behaviour we should copy it with the warning too (I'm not sure how a warning could be implemented here 😅), if not, I'd rather go back to the 413 error to avoid confusion.

Also, I was a bit confused too by the fact that post_max_size is not honored, it's capped at 64KB.

[clue]

So if we copy PHP behaviour we should copy it with the warning too (I'm not sure how a warning could be implemented here sweat_smile), if not, I'd rather go back to the 413 error to avoid confusion.

It's an interesting thought, but this contradicts the original suggestion in this ticket and the implementation in #263, so I'm not sure about the BC implications here. If you feel this is worth investigating, perhaps let's create a follow-up ticket instead of resurrecting this old one?

Also, I was a bit confused too by the fact that post_max_size is not honored, it's capped at 64KB.

Yeah, this is a recent change introduced via #371 which also includes the rationale for this. I agree this can be confusing and this is definitely something we can improve. Let me know if you have any thoughts on this.

[mmoreram]

I found this issue, looking for exactly the same as @acasademont

I understand the purpose for bypassing an empty body when this is overflown, but by doing that, it's clearly impossible to detect when that happens. In my case, I'd like to limit the body size, and report a 413 when this body is overflown. So I still think that, and after reading all your comments, that allowing custom behavior would be nice.

About the implementation, an optional callback in the constructor would be enough. This callback would be executed when the limit is reached, instead of passing to the next middleware as exposed

new RequestBodyBufferMiddleware($this->maxBufferSize, function() {
    throw new Exception('buffer limit reached - 413');
})

By default, this value would be null, exposing the same behavior as it's at the moment.

if ($error instanceof OverflowException) {
    if ($this->limitReachedCallback) {
        $this->limitReachedCallback();
    }
    
    return Stream\first($body, 'close')->then(function () use ($stack, $request) {
        return $stack($request);
    });
}

This would be an elegant way of exposing customization.

[gitneko]

@mmoreram I'm not sure where you're coming from that it's impossible to detect. There are two possible "return types" that the request body buffer middleware has - either the request body is a PSR stream or a ReactPHP readable stream. If it's a ReactPHP readable stream, then the request body was clearly too large and you can throw HTTP 413.

All you need to do is to provide a middleware after the request body buffer middleware which checks whether the request body is a ReactPHP readable stream - and if it is, the buffer middleware didn't buffer because the body is too large.

http/src/Middleware/RequestBodyBufferMiddleware.php

Lines 53 to 65 in 2a9fc79

	return Stream\buffer($body, $sizeLimit)->then(function ($buffer) use ($request, $stack) {
	$request = $request->withBody(new BufferedBody($buffer));
	return $stack($request);
	}, function ($error) use ($stack, $request, $body) {
	// On buffer overflow keep the request body stream in,
	// but ignore the contents and wait for the close event
	// before passing the request on to the next middleware.
	if ($error instanceof OverflowException) {
	return Stream\first($body, 'close')->then(function () use ($stack, $request) {
	return $stack($request);
	});
	}

My ReactPHP HTTP server is a bit complex and I've been doing some things differently (including invoking middlewares), but I've been using this "middleware" after the request body buffer middleware for months without issues.

function (ServerRequestInterface $request) {
    // The buffer middleware leaves the stream intact on overflow
    if($request->getBody() instanceof ReadableStreamInterface) {
        throw new RequestEntityTooLarge();
    }
    
    return $request;
}

[clue]

@gitneko That's an interesting find! Note that you're relying on undocumented behavior here, the fact that this currently seems to return a specific body instance is undocumented and may change in the future.

@mmoreram I agree that this can be useful for some use cases. Afaict this should also be possible from within a dedicated middleware handler similar to what @gitneko suggested, so I don't think this requires an addition to the existing middleware implementation.

Incoming requests may or may not have a body (an empty body equals "no body"). If the request contains a body, it will have a Content-Length header (most likely) or a Transfer-Encoding: chunked header (much less likely).

In the former case, you know the exact size of bytes, so it's trivial to reject a request that exceeds your threshold with a 413 (Payload Too Large) error response. In the latter case, there's no way to know the size in advance, so the only way to detect you're over your threshold is by buffering or streaming and counting. In this case (and because it's a feature that's very rarely used), it's also quite common to use a 411 (Length Required) error response.

[mmoreram]

Thanks @clue and @gitneko for your explanation :)