From 328fe95e8eed6c8677ac7e8168baf99da923dcc3 Mon Sep 17 00:00:00 2001 From: Cees-Jan Kiewiet Date: Wed, 22 Nov 2017 21:46:06 +0100 Subject: [PATCH 1/3] RequestBodyParserMiddleware does not reject requests that exceed post_max_size --- README.md | 3 +- examples/12-upload.php | 2 +- .../RequestBodyBufferMiddleware.php | 24 +++++--- .../RequestBodyBufferMiddlewareTest.php | 58 +++++++++++++------ 4 files changed, 60 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index d667f196..2ea87e96 100644 --- a/README.md +++ b/README.md @@ -694,8 +694,7 @@ 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 their request body will not be added to the request. The `RequestBodyBufferMiddleware` will buffer requests with bodies of known size (i.e. with `Content-Length` header specified) as well as requests with bodies of diff --git a/examples/12-upload.php b/examples/12-upload.php index 68c810ff..3fa82ec9 100644 --- a/examples/12-upload.php +++ b/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 ))); diff --git a/src/Middleware/RequestBodyBufferMiddleware.php b/src/Middleware/RequestBodyBufferMiddleware.php index b1d0ff47..030681fb 100644 --- a/src/Middleware/RequestBodyBufferMiddleware.php +++ b/src/Middleware/RequestBodyBufferMiddleware.php @@ -2,12 +2,12 @@ namespace React\Http\Middleware; +use OverflowException; use Psr\Http\Message\ServerRequestInterface; -use React\Http\Response; +use React\Promise\Promise; use React\Promise\Stream; use React\Stream\ReadableStreamInterface; use RingCentral\Psr7\BufferStream; -use OverflowException; final class RequestBodyBufferMiddleware { @@ -30,27 +30,37 @@ public function __construct($sizeLimit = null) public function __invoke(ServerRequestInterface $request, $stack) { + $sizeLimit = $this->sizeLimit; $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'); + $sizeLimit = 0; } if (!$body instanceof ReadableStreamInterface) { return $stack($request); } - return Stream\buffer($body, $this->sizeLimit)->then(function ($buffer) use ($request, $stack) { + 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 new Promise(function ($resolve, $reject) use ($stack, $request, $body) { + $body->on('error', function ($error) use ($reject) { + $reject($error); + }); + $body->on('close', function () use ($stack, $request, $resolve) { + $resolve($stack($request)); + }); + }); } throw $error; diff --git a/tests/Middleware/RequestBodyBufferMiddlewareTest.php b/tests/Middleware/RequestBodyBufferMiddlewareTest.php index 0e682435..2e480991 100644 --- a/tests/Middleware/RequestBodyBufferMiddlewareTest.php +++ b/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 { @@ -65,7 +66,7 @@ function (ServerRequestInterface $request) use (&$exposedRequest) { $this->assertSame($body, $exposedRequest->getBody()->getContents()); } - public function testExcessiveSizeImmediatelyReturnsError413ForKnownSize() + public function testKnownExcessiveSizedBodyIsDisgardedTheRequestIsPassedDownToTheNextMiddleware() { $loop = Factory::create(); @@ -79,17 +80,18 @@ 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(413, $response->getStatusCode()); + $this->assertSame(200, $response->getStatusCode()); + $this->assertSame('', $response->getBody()->getContents()); } - public function testExcessiveSizeReturnsError413() + public function testExcessiveSizeBodyIsDiscardedAndTheRequestIsPassedDownToTheNextMiddleware() { $loop = Factory::create(); @@ -105,23 +107,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()); } /** @@ -151,4 +149,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); + } } From 7c9a6ec397868ae5336571b77971b7a9f049d203 Mon Sep 17 00:00:00 2001 From: Cees-Jan Kiewiet Date: Tue, 28 Nov 2017 17:42:23 +0100 Subject: [PATCH 2/3] Use Stream\first() instead of wrapping our own promise around it --- composer.json | 2 +- src/Middleware/RequestBodyBufferMiddleware.php | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/composer.json b/composer.json index f6a4fe24..c3d46a9f 100644 --- a/composer.json +++ b/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": { diff --git a/src/Middleware/RequestBodyBufferMiddleware.php b/src/Middleware/RequestBodyBufferMiddleware.php index 030681fb..c7ec5289 100644 --- a/src/Middleware/RequestBodyBufferMiddleware.php +++ b/src/Middleware/RequestBodyBufferMiddleware.php @@ -4,7 +4,6 @@ use OverflowException; use Psr\Http\Message\ServerRequestInterface; -use React\Promise\Promise; use React\Promise\Stream; use React\Stream\ReadableStreamInterface; use RingCentral\Psr7\BufferStream; @@ -53,13 +52,8 @@ public function __invoke(ServerRequestInterface $request, $stack) // 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 Promise(function ($resolve, $reject) use ($stack, $request, $body) { - $body->on('error', function ($error) use ($reject) { - $reject($error); - }); - $body->on('close', function () use ($stack, $request, $resolve) { - $resolve($stack($request)); - }); + return Stream\first($body, 'close')->then(function () use ($stack, $request) { + return $stack($request); }); } From 802ee34158ef42d1bf6abea62040f86432333421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 8 Dec 2017 14:10:47 +0100 Subject: [PATCH 3/3] Discard buffered request body if size limit is exceeded --- README.md | 11 +++++++- .../RequestBodyBufferMiddleware.php | 16 +++++++---- .../RequestBodyBufferMiddlewareTest.php | 28 +++++++++++++++++-- 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 2ea87e96..7ee90d93 100644 --- a/README.md +++ b/README.md @@ -694,7 +694,14 @@ configuration. configuration in most cases.) Any incoming request that has a request body that exceeds this limit will be -accepted but their request body will not be added to the request. +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 @@ -781,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 diff --git a/src/Middleware/RequestBodyBufferMiddleware.php b/src/Middleware/RequestBodyBufferMiddleware.php index c7ec5289..7ae3f894 100644 --- a/src/Middleware/RequestBodyBufferMiddleware.php +++ b/src/Middleware/RequestBodyBufferMiddleware.php @@ -29,18 +29,24 @@ public function __construct($sizeLimit = null) public function __invoke(ServerRequestInterface $request, $stack) { - $sizeLimit = $this->sizeLimit; $body = $request->getBody(); + // 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); + } + // request body of known size exceeding limit + $sizeLimit = $this->sizeLimit; if ($body->getSize() > $this->sizeLimit) { $sizeLimit = 0; } - if (!$body instanceof ReadableStreamInterface) { - return $stack($request); - } - return Stream\buffer($body, $sizeLimit)->then(function ($buffer) use ($request, $stack) { $stream = new BufferStream(strlen($buffer)); $stream->write($buffer); diff --git a/tests/Middleware/RequestBodyBufferMiddlewareTest.php b/tests/Middleware/RequestBodyBufferMiddlewareTest.php index 2e480991..91bd14cc 100644 --- a/tests/Middleware/RequestBodyBufferMiddlewareTest.php +++ b/tests/Middleware/RequestBodyBufferMiddlewareTest.php @@ -38,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()); } @@ -63,13 +64,14 @@ function (ServerRequestInterface $request) use (&$exposedRequest) { } ); + $this->assertSame($size, $exposedRequest->getBody()->getSize()); $this->assertSame($body, $exposedRequest->getBody()->getContents()); } public function testKnownExcessiveSizedBodyIsDisgardedTheRequestIsPassedDownToTheNextMiddleware() { $loop = Factory::create(); - + $stream = new ThroughStream(); $stream->end('aa'); $serverRequest = new ServerRequest( @@ -91,6 +93,28 @@ function (ServerRequestInterface $request) { $this->assertSame('', $response->getBody()->getContents()); } + public function testAlreadyBufferedExceedingSizeResolvesImmediatelyWithEmptyBody() + { + $serverRequest = new ServerRequest( + 'GET', + 'https://example.com/', + array(), + 'hello' + ); + + $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 testExcessiveSizeBodyIsDiscardedAndTheRequestIsPassedDownToTheNextMiddleware() { $loop = Factory::create(); @@ -128,7 +152,7 @@ function (ServerRequestInterface $request) { public function testBufferingErrorThrows() { $loop = Factory::create(); - + $stream = new ThroughStream(); $serverRequest = new ServerRequest( 'GET',