Merge pull request #263 from WyriHaximus/254

RequestBodyParserMiddleware does not reject requests that exceed post_max_size

Author: jsor

README.md

@@ -694,8 +694,14 @@ configuration.
 configuration in most cases.)
 
 Any incoming request that has a request body that exceeds this limit will be
-rejected with a `413` (Request Entity Too Large) error message without calling
-the next middleware handlers.
+accepted, but its request body will be discarded (empty request body).
+This is done in order to avoid having to keep an incoming request with an
+excessive size (for example, think of a 2 GB file upload) in memory.
+This allows the next middleware handler to still handle this request, but it
+will see an empty request body.
+This is similar to PHP's default behavior, where the body will not be parsed
+if this limit is exceeded. However, unlike PHP's default behavior, the raw
+request body is not available via `php://input`.
 
 The `RequestBodyBufferMiddleware` will buffer requests with bodies of known size 
 (i.e. with `Content-Length` header specified) as well as requests with bodies of 
@@ -782,6 +788,8 @@ See also [example #12](examples) for more details.
   handler as given in the example above.
   This previous middleware handler is also responsible for rejecting incoming
   requests that exceed allowed message sizes (such as big file uploads).
+  The [`RequestBodyBufferMiddleware`](#requestbodybuffermiddleware) used above
+  simply discards excessive request bodies, resulting in an empty body.
   If you use this middleware without buffering first, it will try to parse an
   empty (streaming) body and may thus assume an empty data structure.
   See also [`RequestBodyBufferMiddleware`](#requestbodybuffermiddleware) for

composer.json

@@ -10,7 +10,7 @@
         "react/stream": "^1.0 || ^0.7.1",
         "react/promise": "^2.3 || ^1.2.1",
         "evenement/evenement": "^3.0 || ^2.0 || ^1.0",
-        "react/promise-stream": "^1.0 || ^0.1.2"
+        "react/promise-stream": "^1.1"
     },
     "autoload": {
         "psr-4": {

examples/12-upload.php

@@ -119,7 +119,7 @@
 
 // buffer and parse HTTP request body before running our request handler
 $server = new StreamingServer(new MiddlewareRunner(array(
-    new RequestBodyBufferMiddleware(100000), // 100 KB max
+    new RequestBodyBufferMiddleware(100000), // 100 KB max, ignore body otherwise
     new RequestBodyParserMiddleware(),
     $handler
 )));

src/Middleware/RequestBodyBufferMiddleware.php

@@ -2,12 +2,11 @@
 
 namespace React\Http\Middleware;
 
+use OverflowException;
 use Psr\Http\Message\ServerRequestInterface;
-use React\Http\Response;
 use React\Promise\Stream;
 use React\Stream\ReadableStreamInterface;
 use RingCentral\Psr7\BufferStream;
-use OverflowException;
 
 final class RequestBodyBufferMiddleware
 {
@@ -32,25 +31,36 @@ public function __invoke(ServerRequestInterface $request, $stack)
     {
         $body = $request->getBody();
 
-        // request body of known size exceeding limit
-        if ($body->getSize() > $this->sizeLimit) {
-            return new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit');
-        }
-
+        // skip if body is already buffered
         if (!$body instanceof ReadableStreamInterface) {
+            // replace with empty buffer if size limit is exceeded
+            if ($body->getSize() > $this->sizeLimit) {
+                $request = $request->withBody(new BufferStream(0));
+            }
+
             return $stack($request);
         }
 
-        return Stream\buffer($body, $this->sizeLimit)->then(function ($buffer) use ($request, $stack) {
+        // request body of known size exceeding limit
+        $sizeLimit = $this->sizeLimit;
+        if ($body->getSize() > $this->sizeLimit) {
+            $sizeLimit = 0;
+        }
+
+        return Stream\buffer($body, $sizeLimit)->then(function ($buffer) use ($request, $stack) {
             $stream = new BufferStream(strlen($buffer));
             $stream->write($buffer);
             $request = $request->withBody($stream);
 
             return $stack($request);
-        }, function($error) {
-            // request body of unknown size exceeding limit during buffering
+        }, 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 new Response(413, array('Content-Type' => 'text/plain'), 'Request body exceeds allowed limit');
+                return Stream\first($body, 'close')->then(function () use ($stack, $request) {
+                    return $stack($request);
+                });
             }
 
             throw $error;

tests/Middleware/RequestBodyBufferMiddlewareTest.php

@@ -2,15 +2,16 @@
 
 namespace React\Tests\Http\Middleware;
 
+use Clue\React\Block;
 use Psr\Http\Message\ServerRequestInterface;
 use React\EventLoop\Factory;
 use React\Http\Io\HttpBodyStream;
 use React\Http\Io\ServerRequest;
 use React\Http\Middleware\RequestBodyBufferMiddleware;
+use React\Http\Response;
 use React\Stream\ThroughStream;
 use React\Tests\Http\TestCase;
 use RingCentral\Psr7\BufferStream;
-use Clue\React\Block;
 
 final class RequestBodyBufferMiddlewareTest extends TestCase
 {
@@ -37,6 +38,7 @@ function (ServerRequestInterface $request) use (&$exposedRequest) {
         $stream->write('world');
         $stream->end('!');
 
+        $this->assertSame(11, $exposedRequest->getBody()->getSize());
         $this->assertSame('helloworld!', $exposedRequest->getBody()->getContents());
     }
 
@@ -62,13 +64,14 @@ function (ServerRequestInterface $request) use (&$exposedRequest) {
             }
         );
 
+        $this->assertSame($size, $exposedRequest->getBody()->getSize());
         $this->assertSame($body, $exposedRequest->getBody()->getContents());
     }
 
-    public function testExcessiveSizeImmediatelyReturnsError413ForKnownSize()
+    public function testKnownExcessiveSizedBodyIsDisgardedTheRequestIsPassedDownToTheNextMiddleware()
     {
         $loop = Factory::create();
-        
+
         $stream = new ThroughStream();
         $stream->end('aa');
         $serverRequest = new ServerRequest(
@@ -79,17 +82,40 @@ public function testExcessiveSizeImmediatelyReturnsError413ForKnownSize()
         );
 
         $buffer = new RequestBodyBufferMiddleware(1);
-        $response = $buffer(
+        $response = Block\await($buffer(
             $serverRequest,
             function (ServerRequestInterface $request) {
-                return $request;
+                return new Response(200, array(), $request->getBody()->getContents());
             }
+        ), $loop);
+
+        $this->assertSame(200, $response->getStatusCode());
+        $this->assertSame('', $response->getBody()->getContents());
+    }
+
+    public function testAlreadyBufferedExceedingSizeResolvesImmediatelyWithEmptyBody()
+    {
+        $serverRequest = new ServerRequest(
+            'GET',
+            'https://example.com/',
+            array(),
+            'hello'
         );
 
-        $this->assertSame(413, $response->getStatusCode());
+        $exposedRequest = null;
+        $buffer = new RequestBodyBufferMiddleware(1);
+        $buffer(
+            $serverRequest,
+            function (ServerRequestInterface $request) use (&$exposedRequest) {
+                $exposedRequest = $request;
+            }
+        );
+
+        $this->assertSame(0, $exposedRequest->getBody()->getSize());
+        $this->assertSame('', $exposedRequest->getBody()->getContents());
     }
 
-    public function testExcessiveSizeReturnsError413()
+    public function testExcessiveSizeBodyIsDiscardedAndTheRequestIsPassedDownToTheNextMiddleware()
     {
         $loop = Factory::create();
 
@@ -105,23 +131,19 @@ public function testExcessiveSizeReturnsError413()
         $promise = $buffer(
             $serverRequest,
             function (ServerRequestInterface $request) {
-                return $request;
+                return new Response(200, array(), $request->getBody()->getContents());
             }
         );
 
         $stream->end('aa');
 
-        $exposedResponse = null;
-        $promise->then(
-            function($response) use (&$exposedResponse) {
-                $exposedResponse = $response;
-            },
+        $exposedResponse = Block\await($promise->then(
+            null,
             $this->expectCallableNever()
-        );
-
-        $this->assertSame(413, $exposedResponse->getStatusCode());
+        ), $loop);
 
-        Block\await($promise, $loop);
+        $this->assertSame(200, $exposedResponse->getStatusCode());
+        $this->assertSame('', $exposedResponse->getBody()->getContents());
     }
 
     /**
@@ -130,7 +152,7 @@ function($response) use (&$exposedResponse) {
     public function testBufferingErrorThrows()
     {
         $loop = Factory::create();
-        
+
         $stream = new ThroughStream();
         $serverRequest = new ServerRequest(
             'GET',
@@ -151,4 +173,30 @@ function (ServerRequestInterface $request) {
 
         Block\await($promise, $loop);
     }
+
+    public function testFullBodyStreamedBeforeCallingNextMiddleware()
+    {
+        $promiseResolved = false;
+        $middleware = new RequestBodyBufferMiddleware(3);
+        $stream = new ThroughStream();
+        $serverRequest = new ServerRequest(
+            'GET',
+            'https://example.com/',
+            array(),
+            new HttpBodyStream($stream, null)
+        );
+
+        $middleware($serverRequest, function () {
+            return new Response();
+        })->then(function () use (&$promiseResolved) {
+            $promiseResolved = true;
+        });
+
+        $stream->write('aaa');
+        $this->assertFalse($promiseResolved);
+        $stream->write('aaa');
+        $this->assertFalse($promiseResolved);
+        $stream->end('aaa');
+        $this->assertTrue($promiseResolved);
+    }
 }