Framework: Uploads that exceed post_max_size causing misleading exceptions

[JacobBennett]

• Laravel Version: 5.3 although pretty sure it affects all versions
• PHP Version: 5.6 | 7.0

Description:

Issue #8202 does a great job of describing the problem that is taking place, there is one additional snag that makes this a bit more complicated though. The heart of the problem lies in the fact that

If the file I was attempted to upload was greater than my post_max_size - PHP clears the super globals of $_POST and $_FILE - but still sends the server the POST request (which is now empty).

The current VerifyPostSize middleware fixes this problem when the type of request is a POST. However, when attempting to use Laravel's form method spoofing, the _method key gets wiped out as well causing the framework to throw a MethodNotAllowedHTTPException.

For example, assume the following:

// web.php
Route::post('file', 'FileController@create');
Route::patch('file/{file}', 'FileController@update');

For the first route, a file that is too large will hit the RouteCollection and resolve the correct Route to use, it then goes through the middleware stack until it hits the VerifyPostSize middleware and throws the PostTooLargeException.

For the second route, a file that is too large will wipe out the _method key, which causes Laravel to think we are again sending a normal POST request. This causes the RouteCollection@match to fail when trying to find a matching Route to use, eventually throwing the MethodNotAllowedHTTPException from Line 205.

Steps To Reproduce:

Try to upload a file that is larger than your max_post_size to a route that matches a PATCH or PUT verb. This will throw a MethodNotAllowedHTTPException instead of the preferred PostTooLargeException.

Proposed Solution

Although it seems annoying to run this on every request, I don't know any other way to catch it than to put it at the top of RouteCollection@match (link). We would essentially relocate the VerifyPostMiddleware code and place it at the top of the RouteCollection match method.

This would also eliminate the need for the middleware as it would apply to each request that came through.

Looking for feedback on if this is a viable solution. @taylorotwell @TheShiftExchange.

Would be happy to do the PR as well.

[laurencei]

It's been a while since I dug into this problem. I'm trying to remember it all now.

So are you proposing that we include;

if ($_SERVER['CONTENT_LENGTH'] > ini_get('post_max_size')) {
   Throw new PostTooLargeException;
}

At the top of RouteCollection@match?

Maybe somewhere even higher in the stack on every call?

@taylorotwell - basically anytime ($_SERVER['CONTENT_LENGTH'] > ini_get('post_max_size')) is true - then the entire body of the request is invalid - so really the only valid option to do is gracefully abort. @JacobBennett is correct that in this situation you'll be sent down the wrong request route.

I dont know what the performance overhead of ini_get() is (considering it will be on every single laravel cycle)? One option is to call and cache that number in some sort of secret variable that is cached as part of config:cache - then there should be no change to framework performance?

[JacobBennett]

We could really put it anywhere in the stack I suppose as long as it is before the process that does the route matching.

[patrickcarlohickman]

Not tested, but what about adding the VerifyPostSize middleware to your $middleware array in app/Http/Kernel.php?

    /**
     * The application's global HTTP middleware stack.
     *
     * These middleware are run during every request to your application.
     *
     * @var array
     */
    protected $middleware = [
        \Illuminate\Foundation\Http\Middleware\CheckForMaintenanceMode::class,
        \Illuminate\Foundation\Http\Middleware\VerifyPostSize::class,
    ];

Illuminate/Foundation/Http/Kernel::sendRequestThroughRouter() shows:

        return (new Pipeline($this->app))
                    ->send($request)
                    ->through($this->app->shouldSkipMiddleware() ? [] : $this->middleware)
                    ->then($this->dispatchToRouter());

It looks to me like the "global" middleware defined in your Kernel is processed before the request is handed off to the router.

[laurencei]

@JacobBennett - I believe this is fixed in 5.4

In your App\Http\Kernel your middleware should be defined as the following (which is now standard in new 5.4 apps):

protected $middleware = [
    ...
    \Illuminate\Foundation\Http\Middleware\ValidatePostSize::class,
    ...
];

This global middleware runs before the router does any matching - so will correctly catch and identify this situation from my testing.

[JacobBennett]

👍