From 19535c1ccc3a013bddc3ff3fed6e79b5210bbab4 Mon Sep 17 00:00:00 2001 From: Bohdan Yurov Date: Wed, 16 Nov 2016 14:00:32 +0200 Subject: [PATCH 1/5] Fixes #80 issue: now we support scenario when first buffer exceeds maxSize in RequestHeaderParser --- src/RequestHeaderParser.php | 61 +++++++++++++++++++++++------ tests/RequestHeaderParserTest.php | 64 +++++++++++++++++++++++++++---- 2 files changed, 106 insertions(+), 19 deletions(-) diff --git a/src/RequestHeaderParser.php b/src/RequestHeaderParser.php index ca9d57f7..a0c32d20 100644 --- a/src/RequestHeaderParser.php +++ b/src/RequestHeaderParser.php @@ -12,26 +12,55 @@ */ class RequestHeaderParser extends EventEmitter { + /** + * While HTTP does not strictly specify max length for headers, + * most HTTP-server implementations use 8K. There are some using + * 4K though (eg nginx use system page size, that is usually around + * 4096 bytes) + */ + const DEFAULT_HEADER_SIZE = 4096; + + /** + * Data buffer used to emit "data" event if we have read + * part of body while seeking headers end (\r\n\r\n) + * + * @var string + */ private $buffer = ''; - private $maxSize = 4096; + + /** + * Maximum headers length + * + * @var int + */ + private $maxSize; + + /** + * @param int $maxSize + */ + public function __construct( + $maxSize = self::DEFAULT_HEADER_SIZE + ) { + $this->maxSize = $maxSize; + } public function feed($data) { - if (strlen($this->buffer) + strlen($data) > $this->maxSize) { - $this->emit('error', array(new \OverflowException("Maximum header size of {$this->maxSize} exceeded."), $this)); - $this->removeAllListeners(); - return; - } + try { + $this->buffer .= $data; - $this->buffer .= $data; + $headerLen = strpos($this->buffer, "\r\n\r\n"); - if (false !== strpos($this->buffer, "\r\n\r\n")) { - try { - $this->parseAndEmitRequest(); - } catch (Exception $exception) { - $this->emit('error', [$exception]); + if (($headerLen ?: strlen($this->buffer)) > $this->maxSize) { + throw new \OverflowException("Maximum header size of {$this->maxSize} exceeded."); } + if (false !== $headerLen) { + $this->parseAndEmitRequest(); + $this->removeAllListeners(); + } + } catch (Exception $exception) { + $this->emit('error', [$exception, $this]); $this->removeAllListeners(); } } @@ -72,4 +101,12 @@ public function parseRequest($data) return array($request, $bodyBuffer); } + + /** + * @return int + */ + public function getMaxHeadersSize() + { + return $this->maxSize; + } } diff --git a/tests/RequestHeaderParserTest.php b/tests/RequestHeaderParserTest.php index 807af42c..1f2bf156 100644 --- a/tests/RequestHeaderParserTest.php +++ b/tests/RequestHeaderParserTest.php @@ -96,12 +96,16 @@ public function testHeadersEventShouldParsePathAndQueryString() $this->assertSame($headers, $request->getHeaders()); } - public function testHeaderOverflowShouldEmitError() - { - $error = null; - $passedParser = null; + /** + * @param RequestHeaderParser $parser + * @param string $data + */ + private function checkHeaderOverflowShouldEmitError( + RequestHeaderParser $parser, + $data + ) { + $error = $passedParser = null; - $parser = new RequestHeaderParser(); $parser->on('headers', $this->expectCallableNever()); $parser->on('error', function ($message, $parser) use (&$error, &$passedParser) { $error = $message; @@ -111,16 +115,37 @@ public function testHeaderOverflowShouldEmitError() $this->assertSame(1, count($parser->listeners('headers'))); $this->assertSame(1, count($parser->listeners('error'))); - $data = str_repeat('A', 4097); $parser->feed($data); $this->assertInstanceOf('OverflowException', $error); - $this->assertSame('Maximum header size of 4096 exceeded.', $error->getMessage()); + $this->assertSame( + sprintf('Maximum header size of %d exceeded.', $parser->getMaxHeadersSize()), + $error->getMessage() + ); $this->assertSame($parser, $passedParser); $this->assertSame(0, count($parser->listeners('headers'))); $this->assertSame(0, count($parser->listeners('error'))); } + public function testHeaderOverflowShouldEmitErrorDefault() + { + /* + * This checks that exception is thrown if buffer exceeds header + * max length, but header end is still missing + */ + $data = str_repeat('A', RequestHeaderParser::DEFAULT_HEADER_SIZE + 1); + $this->checkHeaderOverflowShouldEmitError(new RequestHeaderParser(), $data); + + /* + * This checks that exception is thrown if header size really exceeded + */ + $data = str_pad($data . "\r\n\r\n", RequestHeaderParser::DEFAULT_HEADER_SIZE * 2, 'B'); + $this->checkHeaderOverflowShouldEmitError(new RequestHeaderParser(), $data); + + $data = str_repeat('A', 8096 + 1); + $this->checkHeaderOverflowShouldEmitError(new RequestHeaderParser(8096), $data); + } + public function testGuzzleRequestParseException() { $error = null; @@ -142,6 +167,31 @@ public function testGuzzleRequestParseException() $this->assertSame(0, count($parser->listeners('error'))); } + /** + * Checks if HeaderParser supports data chunks of size + * bigger, than @see RequestHeaderParser::$maxSize option + * + * @url https://github.com/reactphp/http/issues/80 + */ + public function testHugeBodyFirstChunk() + { + $bodyBuffer = null; + + $parser = new RequestHeaderParser(); + $parser->on('headers', function ($parsedRequest, $parsedBodyBuffer) use (&$bodyBuffer) { + $bodyBuffer = $parsedBodyBuffer; + }); + $parser->on('error', $this->expectCallableNever()); + + $data = $this->createGetRequest(); + $headLength = strlen($data); + $totalLength = 4096 * 16; + $data = str_pad($data, $totalLength, "\0x00"); + $parser->feed($data); + + $this->assertEquals($totalLength - $headLength, strlen($bodyBuffer)); + } + private function createGetRequest() { $data = "GET / HTTP/1.1\r\n"; From f74d6df03485359ddaf0da2f2f32f2cbd8cf3da1 Mon Sep 17 00:00:00 2001 From: Bohdan Yurov Date: Wed, 16 Nov 2016 14:14:42 +0200 Subject: [PATCH 2/5] Fixes #81 issue: data listener is removed if HeaderParser emits error --- src/Server.php | 8 +++++++- tests/ServerTest.php | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/Server.php b/src/Server.php index e1abfd8e..49535060 100644 --- a/src/Server.php +++ b/src/Server.php @@ -42,7 +42,13 @@ public function __construct(SocketServerInterface $io) }); }); - $conn->on('data', array($parser, 'feed')); + $listener = [$parser, 'feed']; + $conn->on('data', $listener); + $parser->on('error', function() use ($conn, $listener) { + // TODO: return 400 response + $conn->removeListener('data', $listener); + $this->emit('error', func_get_args()); + }); }); } diff --git a/tests/ServerTest.php b/tests/ServerTest.php index 65c13ccb..552b562f 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -2,6 +2,7 @@ namespace React\Tests\Http; +use React\Http\RequestHeaderParser; use React\Http\Server; class ServerTest extends TestCase @@ -66,6 +67,28 @@ public function testResponseContainsPoweredByHeader() $this->assertContains("\r\nX-Powered-By: React/alpha\r\n", $conn->getData()); } + public function testParserErrorEmitted() + { + $io = new ServerStub(); + + $error = null; + $server = new Server($io); + $server->on('headers', $this->expectCallableNever()); + $server->on('error', function ($message) use (&$error) { + $error = $message; + }); + + $conn = new ConnectionStub(); + $io->emit('connection', [$conn]); + + $data = $this->createGetRequest(); + $data = str_pad($data, 4096 * 4); + $conn->emit('data', [$data]); + + $this->assertInstanceOf('OverflowException', $error); + $this->assertEquals('', $conn->getData()); + } + private function createGetRequest() { $data = "GET / HTTP/1.1\r\n"; From 36fd2a61985629427f151153d73cbb0087138502 Mon Sep 17 00:00:00 2001 From: Bohdan Yurov Date: Wed, 16 Nov 2016 14:17:16 +0200 Subject: [PATCH 3/5] Composer.json update --- composer.json | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index c186927d..da6705dc 100644 --- a/composer.json +++ b/composer.json @@ -1,5 +1,8 @@ { - "name": "react/http", + "name": "werkint/reactphp-http", + "replace": { + "react/http": "self.version" + }, "description": "Library for building an evented http server.", "keywords": ["http"], "license": "MIT", From 409089f27a77b33884b8f64057a08e368cb22684 Mon Sep 17 00:00:00 2001 From: Bohdan Yurov Date: Wed, 16 Nov 2016 15:38:50 +0200 Subject: [PATCH 4/5] replace fix --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index da6705dc..99d1f0f5 100644 --- a/composer.json +++ b/composer.json @@ -1,7 +1,7 @@ { "name": "werkint/reactphp-http", "replace": { - "react/http": "self.version" + "react/http": "v0.4.2" }, "description": "Library for building an evented http server.", "keywords": ["http"], From f126189040a04ae6325a9b393f614735f7359ec6 Mon Sep 17 00:00:00 2001 From: Bohdan Yurov Date: Tue, 29 Nov 2016 16:04:00 +0200 Subject: [PATCH 5/5] Fixes #84 issue: pause/resume on request not working --- src/Server.php | 13 ++++++----- tests/ServerTest.php | 55 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/Server.php b/src/Server.php index e1abfd8e..a8290d28 100644 --- a/src/Server.php +++ b/src/Server.php @@ -22,6 +22,13 @@ public function __construct(SocketServerInterface $io) $parser = new RequestHeaderParser(); $parser->on('headers', function (Request $request, $bodyBuffer) use ($conn, $parser) { + $request->on('pause', function () use ($conn) { + $conn->pause(); + }); + $request->on('resume', function () use ($conn) { + $conn->resume(); + }); + // attach remote ip to the request as metadata $request->remoteAddress = $conn->getRemoteAddress(); @@ -34,12 +41,6 @@ public function __construct(SocketServerInterface $io) $conn->on('data', function ($data) use ($request) { $request->emit('data', array($data)); }); - $request->on('pause', function () use ($conn) { - $conn->emit('pause'); - }); - $request->on('resume', function () use ($conn) { - $conn->emit('resume'); - }); }); $conn->on('data', array($parser, 'feed')); diff --git a/tests/ServerTest.php b/tests/ServerTest.php index 65c13ccb..2b49c6f6 100644 --- a/tests/ServerTest.php +++ b/tests/ServerTest.php @@ -2,6 +2,7 @@ namespace React\Tests\Http; +use React\Http\Request; use React\Http\Server; class ServerTest extends TestCase @@ -66,6 +67,60 @@ public function testResponseContainsPoweredByHeader() $this->assertContains("\r\nX-Powered-By: React/alpha\r\n", $conn->getData()); } + public function testParserErrorEmitted() + { + $io = new ServerStub(); + + $error = null; + $server = new Server($io); + $server->on('headers', $this->expectCallableNever()); + $server->on('error', function ($message) use (&$error) { + $error = $message; + }); + + $conn = new ConnectionStub(); + $io->emit('connection', [$conn]); + + $data = $this->createGetRequest(); + $data = str_pad($data, 4096 * 4); + $conn->emit('data', [$data]); + + $this->assertInstanceOf('OverflowException', $error); + $this->assertEquals('', $conn->getData()); + } + + /** + * @url https://github.com/reactphp/http/issues/84 + */ + public function testPauseResume() + { + $io = new ServerStub(); + + $server = new Server($io); + $called = false; + $server->on('request', function (Request $request) use (&$called) { + $called = true; + + $request->emit('pause'); + $request->emit('resume'); + }); + + /** @var ConnectionStub|\PHPUnit_Framework_MockObject_MockObject $conn */ + $conn = $this->getMock(ConnectionStub::class, ['pause', 'resume'], [], '', false, false); + $conn->expects($this->once()) + ->method('pause') + ; + $conn->expects($this->once()) + ->method('resume') + ; + $io->emit('connection', [$conn]); + + $data = $this->createGetRequest(); + $conn->emit('data', [$data]); + + $this->assertTrue($called); + } + private function createGetRequest() { $data = "GET / HTTP/1.1\r\n";