From affba40d16e899dcf08bd7086fc933c143a1dab1 Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Sun, 7 Feb 2016 13:40:38 -0500 Subject: [PATCH 1/6] Client refactor Class is now re-entrant instead of keeping state Remove non-specified default RFC headers Accept strict URI type to cut down on error handling --- src/Handshake/ClientNegotiator.php | 89 ++++++++++-------------------- src/Handshake/ResponseVerifier.php | 10 +--- tests/ab/clientRunner.php | 26 ++++----- 3 files changed, 44 insertions(+), 81 deletions(-) diff --git a/src/Handshake/ClientNegotiator.php b/src/Handshake/ClientNegotiator.php index b4a4e16..c393194 100644 --- a/src/Handshake/ClientNegotiator.php +++ b/src/Handshake/ClientNegotiator.php @@ -1,83 +1,50 @@ 'Upgrade' - , 'Cache-Control' => 'no-cache' - , 'Pragma' => 'no-cache' - , 'Upgrade' => 'websocket' - , 'Sec-WebSocket-Version' => 13 - , 'User-Agent' => "RatchetRFC/0.0.0" - ]; + /** + * @var ResponseVerifier + */ + private $verifier; - /** @var Request */ - public $request; + /** + * @var \Psr\Http\Message\RequestInterface + */ + public $defaultHeader; - /** @var Response */ - public $response; + function __construct() { + $this->verifier = new ResponseVerifier; - /** @var ResponseVerifier */ - public $verifier; - - private $websocketKey = ''; - - function __construct($path = null) - { - if (!is_string($path)) $path = "/"; - $request = new Request("GET", $path); - - $request = $request->withUri(new Uri("ws://127.0.0.1:9001" . $path)); - - $this->request = $request; - - $this->verifier = new ResponseVerifier(); - - $this->websocketKey = $this->generateKey(); + $this->defaultHeader = new Request('GET', '', [ + 'Connection' => 'Upgrade' + , 'Upgrade' => 'websocket' + , 'Sec-WebSocket-Version' => 13 + , 'User-Agent' => "RatchetRFC/0.0.0" + ]); } - public function addRequiredHeaders() { - foreach ($this->defaultHeaders as $k => $v) { - // remove any header that is there now - $this->request = $this->request->withoutHeader($k); - $this->request = $this->request->withHeader($k, $v); - } - $this->request = $this->request->withoutHeader("Sec-WebSocket-Key"); - $this->request = $this->request->withHeader("Sec-WebSocket-Key", $this->websocketKey); - $this->request = $this->request->withoutHeader("Host") - ->withHeader("Host", $this->request->getUri()->getHost() . ":" . $this->request->getUri()->getPort()); + public function generateRequest(UriInterface $uri) { + return $this->defaultHeader->withUri($uri) + ->withoutHeader("Sec-WebSocket-Key") + ->withHeader("Sec-WebSocket-Key", $this->generateKey()); } - public function getRequest() { - $this->addRequiredHeaders(); - return $this->request; + public function validateResponse(RequestInterface $request, ResponseInterface $response) { + return $this->verifier->verifyAll($request, $response); } - public function getResponse() { - return $this->response; - } - - public function validateResponse(Response $response) { - $this->response = $response; - - return $this->verifier->verifyAll($this->getRequest(), $response); - } - - protected function generateKey() { + public function generateKey() { $chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwzyz1234567890+/='; $charRange = strlen($chars) - 1; $key = ''; - for ($i = 0;$i < 16;$i++) { + for ($i = 0; $i < 16; $i++) { $key .= $chars[mt_rand(0, $charRange)]; } + return base64_encode($key); } diff --git a/src/Handshake/ResponseVerifier.php b/src/Handshake/ResponseVerifier.php index c1bd67a..58d05be 100644 --- a/src/Handshake/ResponseVerifier.php +++ b/src/Handshake/ResponseVerifier.php @@ -1,14 +1,10 @@ verifyStatus($response->getStatusCode()); diff --git a/tests/ab/clientRunner.php b/tests/ab/clientRunner.php index ee8897f..bb58832 100644 --- a/tests/ab/clientRunner.php +++ b/tests/ab/clientRunner.php @@ -1,7 +1,7 @@ create($testServer, 9001)->then(function (\React\Stream\Stream $stream) use ($deferred) { - $cn = new \Ratchet\RFC6455\Handshake\ClientNegotiator("/getCaseCount"); - $cnRequest = $cn->getRequest(); + $cn = new \Ratchet\RFC6455\Handshake\ClientNegotiator(); + $cnRequest = $cn->generateRequest(new Uri('ws://127.0.0.1:9001/getCaseCount')); $rawResponse = ""; $response = null; @@ -57,7 +57,7 @@ function getTestCases() { /** @var \Ratchet\RFC6455\Messaging\Streaming\MessageStreamer $ms */ $ms = null; - $stream->on('data', function ($data) use ($stream, &$rawResponse, &$response, &$ms, $cn, $deferred, &$context) { + $stream->on('data', function ($data) use ($stream, &$rawResponse, &$response, &$ms, $cn, $deferred, &$context, $cnRequest) { if ($response === null) { $rawResponse .= $data; $pos = strpos($rawResponse, "\r\n\r\n"); @@ -66,7 +66,7 @@ function getTestCases() { $rawResponse = substr($rawResponse, 0, $pos + 4); $response = \GuzzleHttp\Psr7\parse_response($rawResponse); - if (!$cn->validateResponse($response)) { + if (!$cn->validateResponse($cnRequest, $response)) { $stream->end(); $deferred->reject(); } else { @@ -105,15 +105,15 @@ function runTest($case) $deferred = new Deferred(); $factory->create($testServer, 9001)->then(function (\React\Stream\Stream $stream) use ($deferred, $casePath, $case) { - $cn = new \Ratchet\RFC6455\Handshake\ClientNegotiator($casePath); - $cnRequest = $cn->getRequest(); + $cn = new \Ratchet\RFC6455\Handshake\ClientNegotiator(); + $cnRequest = $cn->generateRequest(new Uri('ws://127.0.0.1:9001' . $casePath)); $rawResponse = ""; $response = null; $ms = null; - $stream->on('data', function ($data) use ($stream, &$rawResponse, &$response, &$ms, $cn, $deferred, &$context) { + $stream->on('data', function ($data) use ($stream, &$rawResponse, &$response, &$ms, $cn, $deferred, &$context, $cnRequest) { if ($response === null) { $rawResponse .= $data; $pos = strpos($rawResponse, "\r\n\r\n"); @@ -122,7 +122,7 @@ function runTest($case) $rawResponse = substr($rawResponse, 0, $pos + 4); $response = \GuzzleHttp\Psr7\parse_response($rawResponse); - if (!$cn->validateResponse($response)) { + if (!$cn->validateResponse($cnRequest, $response)) { $stream->end(); $deferred->reject(); } else { @@ -155,8 +155,8 @@ function createReport() { $factory->create($testServer, 9001)->then(function (\React\Stream\Stream $stream) use ($deferred) { $reportPath = "/updateReports?agent=" . AGENT . "&shutdownOnComplete=true"; - $cn = new \Ratchet\RFC6455\Handshake\ClientNegotiator($reportPath); - $cnRequest = $cn->getRequest(); + $cn = new \Ratchet\RFC6455\Handshake\ClientNegotiator(); + $cnRequest = $cn->generateRequest(new Uri('ws://127.0.0.1:9001' . $reportPath)); $rawResponse = ""; $response = null; @@ -164,7 +164,7 @@ function createReport() { /** @var \Ratchet\RFC6455\Messaging\Streaming\MessageStreamer $ms */ $ms = null; - $stream->on('data', function ($data) use ($stream, &$rawResponse, &$response, &$ms, $cn, $deferred, &$context) { + $stream->on('data', function ($data) use ($stream, &$rawResponse, &$response, &$ms, $cn, $deferred, &$context, $cnRequest) { if ($response === null) { $rawResponse .= $data; $pos = strpos($rawResponse, "\r\n\r\n"); @@ -173,7 +173,7 @@ function createReport() { $rawResponse = substr($rawResponse, 0, $pos + 4); $response = \GuzzleHttp\Psr7\parse_response($rawResponse); - if (!$cn->validateResponse($response)) { + if (!$cn->validateResponse($cnRequest, $response)) { $stream->end(); $deferred->reject(); } else { From 1e828bf7d4b50eecbd2a152eaf8c52315f03d715 Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Mon, 8 Feb 2016 07:50:17 -0500 Subject: [PATCH 2/6] Client negotiation cleanup --- src/Handshake/ClientNegotiator.php | 9 ++++++--- src/Handshake/ClientNegotiatorInterface.php | 11 ----------- src/Handshake/ResponseVerifier.php | 14 +++++++------- 3 files changed, 13 insertions(+), 21 deletions(-) delete mode 100644 src/Handshake/ClientNegotiatorInterface.php diff --git a/src/Handshake/ClientNegotiator.php b/src/Handshake/ClientNegotiator.php index c393194..ca93669 100644 --- a/src/Handshake/ClientNegotiator.php +++ b/src/Handshake/ClientNegotiator.php @@ -5,7 +5,7 @@ use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\UriInterface; use GuzzleHttp\Psr7\Request; -class ClientNegotiator implements ClientNegotiatorInterface { +class ClientNegotiator { /** * @var ResponseVerifier */ @@ -22,7 +22,7 @@ class ClientNegotiator implements ClientNegotiatorInterface { $this->defaultHeader = new Request('GET', '', [ 'Connection' => 'Upgrade' , 'Upgrade' => 'websocket' - , 'Sec-WebSocket-Version' => 13 + , 'Sec-WebSocket-Version' => $this->getVersion() , 'User-Agent' => "RatchetRFC/0.0.0" ]); } @@ -48,4 +48,7 @@ class ClientNegotiator implements ClientNegotiatorInterface { return base64_encode($key); } -} \ No newline at end of file + public function getVersion() { + return 13; + } +} \ No newline at end of file diff --git a/src/Handshake/ClientNegotiatorInterface.php b/src/Handshake/ClientNegotiatorInterface.php deleted file mode 100644 index c95c1ac..0000000 --- a/src/Handshake/ClientNegotiatorInterface.php +++ /dev/null @@ -1,11 +0,0 @@ -verifyUpgrade($response->getHeader('Upgrade')); $passes += (int)$this->verifyConnection($response->getHeader('Connection')); $passes += (int)$this->verifySecWebSocketAccept( - $response->getHeader('Sec-WebSocket-Accept'), - $request->getHeader('sec-websocket-key') - ); + $response->getHeader('Sec-WebSocket-Accept') + , $request->getHeader('Sec-WebSocket-Accept') + ); - return (4 == $passes); + return (4 === $passes); } public function verifyStatus($status) { - return ($status == 101); + return ((int)$status === 101); } public function verifyUpgrade(array $upgrade) { @@ -34,10 +34,10 @@ class ResponseVerifier { return ( 1 === count($swa) && 1 === count($key) && - $swa[0] == $this->sign($key[0])); + $swa[0] === $this->sign($key[0])); } public function sign($key) { - return base64_encode(sha1($key . Negotiator::GUID, true)); + return base64_encode(sha1($key . NegotiatorInterface::GUID, true)); } } \ No newline at end of file From 4095a7ed6ef5203437f0d05cccf2f3f00efd4693 Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Mon, 8 Feb 2016 07:51:54 -0500 Subject: [PATCH 3/6] Change scope, defensive --- src/Handshake/ClientNegotiator.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Handshake/ClientNegotiator.php b/src/Handshake/ClientNegotiator.php index ca93669..8726c8f 100644 --- a/src/Handshake/ClientNegotiator.php +++ b/src/Handshake/ClientNegotiator.php @@ -14,7 +14,7 @@ class ClientNegotiator { /** * @var \Psr\Http\Message\RequestInterface */ - public $defaultHeader; + private $defaultHeader; function __construct() { $this->verifier = new ResponseVerifier; @@ -29,7 +29,6 @@ class ClientNegotiator { public function generateRequest(UriInterface $uri) { return $this->defaultHeader->withUri($uri) - ->withoutHeader("Sec-WebSocket-Key") ->withHeader("Sec-WebSocket-Key", $this->generateKey()); } From cd89941a49f7b94959de708226188963f2f06c4d Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Mon, 8 Feb 2016 07:55:35 -0500 Subject: [PATCH 4/6] Formatting --- src/Handshake/ResponseVerifier.php | 3 ++- tests/ab/fuzzingclient.json | 23 ++++++++++++----------- tests/ab/fuzzingserver.json | 14 +++++++------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/Handshake/ResponseVerifier.php b/src/Handshake/ResponseVerifier.php index 6fa54ae..63909db 100644 --- a/src/Handshake/ResponseVerifier.php +++ b/src/Handshake/ResponseVerifier.php @@ -34,7 +34,8 @@ class ResponseVerifier { return ( 1 === count($swa) && 1 === count($key) && - $swa[0] === $this->sign($key[0])); + $swa[0] === $this->sign($key[0]) + ); } public function sign($key) { diff --git a/tests/ab/fuzzingclient.json b/tests/ab/fuzzingclient.json index 75b1cc9..d2fd0d0 100644 --- a/tests/ab/fuzzingclient.json +++ b/tests/ab/fuzzingclient.json @@ -1,13 +1,14 @@ { - "options": {"failByDrop": false}, - "outdir": "./reports/servers", - - "servers": [{ - "agent": "RatchetRFC/0.1.0", - "url": "ws://localhost:9001", - "options": {"version": 18} - }], - "cases": ["*"], - "exclude-cases": ["12.*","13.*"], - "exclude-agent-cases": {} + "options": { + "failByDrop": false + } + , "outdir": "./reports/servers" + , "servers": [{ + "agent": "RatchetRFC/0.1.0" + , "url": "ws://localhost:9001" + , "options": {"version": 18} + }] + , "cases": ["*"] + , "exclude-cases": ["6.4.*", "12.*","13.*"] + , "exclude-agent-cases": {} } diff --git a/tests/ab/fuzzingserver.json b/tests/ab/fuzzingserver.json index 70db183..0422560 100644 --- a/tests/ab/fuzzingserver.json +++ b/tests/ab/fuzzingserver.json @@ -1,10 +1,10 @@ { "url": "ws://127.0.0.1:9001" - , "options": { - "failByDrop": false + , "options": { + "failByDrop": false + } + , "outdir": "./reports/clients" + , "cases": ["*"] + , "exclude-cases": ["6.4.*", "12.*", "13.*"] + , "exclude-agent-cases": {} } - , "outdir": "./reports/clients" - , "cases": ["*"] - , "exclude-cases": ["12.*", "13.*"] - , "exclude-agent-cases": {} -} \ No newline at end of file From 0f4df7fed5d1436e617641f0b477a55b4204db2a Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Mon, 8 Feb 2016 08:43:20 -0500 Subject: [PATCH 5/6] Verify proper header --- src/Handshake/ResponseVerifier.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Handshake/ResponseVerifier.php b/src/Handshake/ResponseVerifier.php index 63909db..f809ff3 100644 --- a/src/Handshake/ResponseVerifier.php +++ b/src/Handshake/ResponseVerifier.php @@ -12,7 +12,7 @@ class ResponseVerifier { $passes += (int)$this->verifyConnection($response->getHeader('Connection')); $passes += (int)$this->verifySecWebSocketAccept( $response->getHeader('Sec-WebSocket-Accept') - , $request->getHeader('Sec-WebSocket-Accept') + , $request->getHeader('Sec-WebSocket-Key') ); return (4 === $passes); From 9f405beccb0c0143c4d0c09bcec8635aa00e4034 Mon Sep 17 00:00:00 2001 From: Chris Boden Date: Mon, 8 Feb 2016 21:43:17 -0500 Subject: [PATCH 6/6] Added subprotocol check for client, test fixes --- src/Handshake/ResponseVerifier.php | 10 +++++- tests/unit/Handshake/RequestVerifierTest.php | 4 +-- tests/unit/Handshake/ResponseVerifierTest.php | 34 +++++++++++++++++++ tests/unit/Messaging/Protocol/FrameTest.php | 3 +- tests/unit/Messaging/Protocol/MessageTest.php | 4 +-- 5 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 tests/unit/Handshake/ResponseVerifierTest.php diff --git a/src/Handshake/ResponseVerifier.php b/src/Handshake/ResponseVerifier.php index f809ff3..de03f53 100644 --- a/src/Handshake/ResponseVerifier.php +++ b/src/Handshake/ResponseVerifier.php @@ -14,8 +14,12 @@ class ResponseVerifier { $response->getHeader('Sec-WebSocket-Accept') , $request->getHeader('Sec-WebSocket-Key') ); + $passes += (int)$this->verifySubProtocol( + $request->getHeader('Sec-WebSocket-Protocol') + , $response->getHeader('Sec-WebSocket-Protocol') + ); - return (4 === $passes); + return (5 === $passes); } public function verifyStatus($status) { @@ -41,4 +45,8 @@ class ResponseVerifier { public function sign($key) { return base64_encode(sha1($key . NegotiatorInterface::GUID, true)); } + + public function verifySubProtocol(array $requestHeader, array $responseHeader) { + return 0 === count($responseHeader) || count(array_intersect($responseHeader, $requestHeader)) > 0; + } } \ No newline at end of file diff --git a/tests/unit/Handshake/RequestVerifierTest.php b/tests/unit/Handshake/RequestVerifierTest.php index a7277ff..e0569fd 100644 --- a/tests/unit/Handshake/RequestVerifierTest.php +++ b/tests/unit/Handshake/RequestVerifierTest.php @@ -1,11 +1,9 @@ _v = new ResponseVerifier; + } + + public static function subProtocolsProvider() { + return [ + [true, ['a'], ['a']] + , [true, ['b', 'a'], ['c', 'd', 'a']] + , [false, ['a', 'b', 'c'], ['d']] + , [true, [], []] + , [true, ['a', 'b'], []] + ]; + } + + /** + * @dataProvider subProtocolsProvider + */ + public function testVerifySubProtocol($expected, $response, $request) { + $this->assertEquals($expected, $this->_v->verifySubProtocol($response, $request)); + } +} \ No newline at end of file diff --git a/tests/unit/Messaging/Protocol/FrameTest.php b/tests/unit/Messaging/Protocol/FrameTest.php index 7622599..e0a4f61 100644 --- a/tests/unit/Messaging/Protocol/FrameTest.php +++ b/tests/unit/Messaging/Protocol/FrameTest.php @@ -1,10 +1,9 @@