From 4ad549b9e031fb3954008232842f3e2a549c388f Mon Sep 17 00:00:00 2001 From: Matt Bonneau Date: Wed, 19 Apr 2017 23:28:04 -0400 Subject: [PATCH 1/6] Correct Responses in ServerNegotiator add tests --- src/Handshake/ServerNegotiator.php | 42 ++++- tests/unit/Handshake/ServerNegotiatorTest.php | 143 ++++++++++++++++++ 2 files changed, 177 insertions(+), 8 deletions(-) create mode 100644 tests/unit/Handshake/ServerNegotiatorTest.php diff --git a/src/Handshake/ServerNegotiator.php b/src/Handshake/ServerNegotiator.php index 6568684..14377f1 100644 --- a/src/Handshake/ServerNegotiator.php +++ b/src/Handshake/ServerNegotiator.php @@ -13,12 +13,15 @@ class ServerNegotiator implements NegotiatorInterface { */ private $verifier; - private $_supportedSubProtocols = []; + private $_supportedSubProtocols; - private $_strictSubProtocols = false; + private $_strictSubProtocols; - public function __construct(RequestVerifier $requestVerifier) { + public function __construct(RequestVerifier $requestVerifier, array $supportedSubProtocols = [], $strictSubProtocol = false) { $this->verifier = $requestVerifier; + + $this->_supportedSubProtocols = $supportedSubProtocols; + $this->_strictSubProtocols = $strictSubProtocol; } /** @@ -55,20 +58,38 @@ class ServerNegotiator implements NegotiatorInterface { return new Response(400); } + $upgradeSuggestion = [ + 'Connection' => 'Upgrade', + 'Upgrade' => 'websocket', + 'Sec-WebSocket-Version' => $this->getVersionNumber(), + 'Sec-WebSocket-Protocol' => implode(', ', $this->_supportedSubProtocols) + ]; if (true !== $this->verifier->verifyUpgradeRequest($request->getHeader('Upgrade'))) { - return new Response(400, [], '1.1', null, 'Upgrade header MUST be provided'); + return new Response( + 426, + $upgradeSuggestion, + null, + '1.1', + 'Upgrade header MUST be provided' + ); } if (true !== $this->verifier->verifyConnection($request->getHeader('Connection'))) { - return new Response(400, [], '1.1', null, 'Connection header MUST be provided'); + return new Response(400, [], null, '1.1', 'Connection Upgrade MUST be requested'); } if (true !== $this->verifier->verifyKey($request->getHeader('Sec-WebSocket-Key'))) { - return new Response(400, [], '1.1', null, 'Invalid Sec-WebSocket-Key'); + return new Response(400, [], null, '1.1', 'Invalid Sec-WebSocket-Key'); } if (true !== $this->verifier->verifyVersion($request->getHeader('Sec-WebSocket-Version'))) { - return new Response(426, ['Sec-WebSocket-Version' => $this->getVersionNumber()]); + /* + * https://tools.ietf.org/html/rfc7230#section-6.7 + * A server that sends a 426 (Upgrade Required) response MUST send an + * Upgrade header field to indicate the acceptable protocols, in order + * of descending preference + */ + return new Response(426, $upgradeSuggestion); } $headers = []; @@ -81,7 +102,7 @@ class ServerNegotiator implements NegotiatorInterface { }, null); if ($this->_strictSubProtocols && null === $match) { - return new Response(400, [], '1.1', null ,'No Sec-WebSocket-Protocols requested supported'); + return new Response(426, $upgradeSuggestion); } if (null !== $match) { @@ -107,6 +128,10 @@ class ServerNegotiator implements NegotiatorInterface { return base64_encode(sha1($key . static::GUID, true)); } + /** + * @param array $protocols + * @deprecated + */ function setSupportedSubProtocols(array $protocols) { $this->_supportedSubProtocols = array_flip($protocols); } @@ -118,6 +143,7 @@ class ServerNegotiator implements NegotiatorInterface { * @todo Consider extending this interface and moving this there. * The spec does says the server can fail for this reason, but * it is not a requirement. This is an implementation detail. + * @deprecated */ function setStrictSubProtocolCheck($enable) { $this->_strictSubProtocols = (boolean)$enable; diff --git a/tests/unit/Handshake/ServerNegotiatorTest.php b/tests/unit/Handshake/ServerNegotiatorTest.php new file mode 100644 index 0000000..f956f14 --- /dev/null +++ b/tests/unit/Handshake/ServerNegotiatorTest.php @@ -0,0 +1,143 @@ +handshake($request); + + $this->assertEquals('1.1', $response->getProtocolVersion()); + $this->assertEquals(426, $response->getStatusCode()); + $this->assertEquals('Upgrade header MUST be provided', $response->getReasonPhrase()); + $this->assertEquals('Upgrade', $response->getHeaderLine('Connection')); + $this->assertEquals('websocket', $response->getHeaderLine('Upgrade')); + $this->assertEquals('13', $response->getHeaderLine('Sec-WebSocket-Version')); + } + + public function testNoConnectionUpgradeRequested() { + $negotiator = new ServerNegotiator(new RequestVerifier()); + + $requestText = 'GET / HTTP/1.1 +Host: 127.0.0.1:6789 +Connection: keep-alive +Pragma: no-cache +Cache-Control: no-cache +Upgrade: websocket +Upgrade-Insecure-Requests: 1 +User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36 +Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 +Accept-Encoding: gzip, deflate, sdch, br +Accept-Language: en-US,en;q=0.8'; + + $request = parse_request($requestText); + + $response = $negotiator->handshake($request); + + $this->assertEquals('1.1', $response->getProtocolVersion()); + $this->assertEquals(400, $response->getStatusCode()); + $this->assertEquals('Connection Upgrade MUST be requested', $response->getReasonPhrase()); + } + + public function testInvalidSecWebsocketKey() { + $negotiator = new ServerNegotiator(new RequestVerifier()); + + $requestText = 'GET / HTTP/1.1 +Host: 127.0.0.1:6789 +Connection: Upgrade +Pragma: no-cache +Cache-Control: no-cache +Upgrade: websocket +Sec-WebSocket-Key: 12345 +Upgrade-Insecure-Requests: 1 +User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36 +Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 +Accept-Encoding: gzip, deflate, sdch, br +Accept-Language: en-US,en;q=0.8'; + + $request = parse_request($requestText); + + $response = $negotiator->handshake($request); + + $this->assertEquals('1.1', $response->getProtocolVersion()); + $this->assertEquals(400, $response->getStatusCode()); + $this->assertEquals('Invalid Sec-WebSocket-Key', $response->getReasonPhrase()); + } + + public function testInvalidSecWebsocketVersion() { + $negotiator = new ServerNegotiator(new RequestVerifier()); + + $requestText = 'GET / HTTP/1.1 +Host: 127.0.0.1:6789 +Connection: Upgrade +Pragma: no-cache +Cache-Control: no-cache +Upgrade: websocket +Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ== +Upgrade-Insecure-Requests: 1 +User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36 +Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 +Accept-Encoding: gzip, deflate, sdch, br +Accept-Language: en-US,en;q=0.8'; + + $request = parse_request($requestText); + + $response = $negotiator->handshake($request); + + $this->assertEquals('1.1', $response->getProtocolVersion()); + $this->assertEquals(426, $response->getStatusCode()); + $this->assertEquals('Upgrade Required', $response->getReasonPhrase()); + $this->assertEquals('Upgrade', $response->getHeaderLine('Connection')); + $this->assertEquals('websocket', $response->getHeaderLine('Upgrade')); + $this->assertEquals('13', $response->getHeaderLine('Sec-WebSocket-Version')); + } + + public function testBadSubprotocolResponse() { + $negotiator = new ServerNegotiator(new RequestVerifier(), [], true); + + $requestText = 'GET / HTTP/1.1 +Host: 127.0.0.1:6789 +Connection: Upgrade +Pragma: no-cache +Cache-Control: no-cache +Upgrade: websocket +Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ== +Sec-WebSocket-Version: 13 +Sec-WebSocket-Protocol: someprotocol +Upgrade-Insecure-Requests: 1 +User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36 +Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 +Accept-Encoding: gzip, deflate, sdch, br +Accept-Language: en-US,en;q=0.8'; + + $request = parse_request($requestText); + + $response = $negotiator->handshake($request); + + $this->assertEquals('1.1', $response->getProtocolVersion()); + $this->assertEquals(426, $response->getStatusCode()); + $this->assertEquals('Upgrade Required', $response->getReasonPhrase()); + $this->assertEquals('Upgrade', $response->getHeaderLine('Connection')); + $this->assertEquals('websocket', $response->getHeaderLine('Upgrade')); + $this->assertEquals('13', $response->getHeaderLine('Sec-WebSocket-Version')); + } +} \ No newline at end of file From 10c719894692259773714bdd703118a1351553b7 Mon Sep 17 00:00:00 2001 From: Matt Bonneau Date: Wed, 19 Apr 2017 23:59:18 -0400 Subject: [PATCH 2/6] Use full namespace on functions and add oh-unit dev dep --- composer.json | 3 ++- tests/unit/Handshake/ServerNegotiatorTest.php | 11 +++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/composer.json b/composer.json index 77d9fde..95d6b0f 100644 --- a/composer.json +++ b/composer.json @@ -26,6 +26,7 @@ }, "require-dev": { "react/http": "^0.4.1", - "react/socket-client": "^0.4.3" + "react/socket-client": "^0.4.3", + "phpunit/phpunit": "5.7" } } diff --git a/tests/unit/Handshake/ServerNegotiatorTest.php b/tests/unit/Handshake/ServerNegotiatorTest.php index f956f14..519a37a 100644 --- a/tests/unit/Handshake/ServerNegotiatorTest.php +++ b/tests/unit/Handshake/ServerNegotiatorTest.php @@ -2,7 +2,6 @@ namespace Ratchet\RFC6455\Test\Unit\Handshake; -use function GuzzleHttp\Psr7\parse_request; use Ratchet\RFC6455\Handshake\RequestVerifier; use Ratchet\RFC6455\Handshake\ServerNegotiator; @@ -22,7 +21,7 @@ Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0 Accept-Encoding: gzip, deflate, sdch, br Accept-Language: en-US,en;q=0.8'; - $request = parse_request($requestText); + $request = \GuzzleHttp\Psr7\parse_request($requestText); $response = $negotiator->handshake($request); @@ -49,7 +48,7 @@ Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0 Accept-Encoding: gzip, deflate, sdch, br Accept-Language: en-US,en;q=0.8'; - $request = parse_request($requestText); + $request = \GuzzleHttp\Psr7\parse_request($requestText); $response = $negotiator->handshake($request); @@ -74,7 +73,7 @@ Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0 Accept-Encoding: gzip, deflate, sdch, br Accept-Language: en-US,en;q=0.8'; - $request = parse_request($requestText); + $request = \GuzzleHttp\Psr7\parse_request($requestText); $response = $negotiator->handshake($request); @@ -99,7 +98,7 @@ Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0 Accept-Encoding: gzip, deflate, sdch, br Accept-Language: en-US,en;q=0.8'; - $request = parse_request($requestText); + $request = \GuzzleHttp\Psr7\parse_request($requestText); $response = $negotiator->handshake($request); @@ -129,7 +128,7 @@ Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0 Accept-Encoding: gzip, deflate, sdch, br Accept-Language: en-US,en;q=0.8'; - $request = parse_request($requestText); + $request = \GuzzleHttp\Psr7\parse_request($requestText); $response = $negotiator->handshake($request); From 2d10201b00f944691a243dfafdf6fd305f50e828 Mon Sep 17 00:00:00 2001 From: Matt Bonneau Date: Thu, 20 Apr 2017 19:42:30 -0400 Subject: [PATCH 3/6] Change to phpunit 4.8 to support php 5.4 and 5.5 --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 95d6b0f..224066b 100644 --- a/composer.json +++ b/composer.json @@ -27,6 +27,6 @@ "require-dev": { "react/http": "^0.4.1", "react/socket-client": "^0.4.3", - "phpunit/phpunit": "5.7" + "phpunit/phpunit": "4.8.*" } } From 78b71376277ddb751006118411f3868e68eb8a32 Mon Sep 17 00:00:00 2001 From: Matt Bonneau Date: Sun, 10 Sep 2017 15:10:54 -0400 Subject: [PATCH 4/6] Revert BC breaks to ServerNegotiator --- src/Handshake/ServerNegotiator.php | 11 +++-------- tests/unit/Handshake/ServerNegotiatorTest.php | 4 +++- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Handshake/ServerNegotiator.php b/src/Handshake/ServerNegotiator.php index 14377f1..2ebdf6b 100644 --- a/src/Handshake/ServerNegotiator.php +++ b/src/Handshake/ServerNegotiator.php @@ -13,15 +13,12 @@ class ServerNegotiator implements NegotiatorInterface { */ private $verifier; - private $_supportedSubProtocols; + private $_supportedSubProtocols = []; - private $_strictSubProtocols; + private $_strictSubProtocols = false; - public function __construct(RequestVerifier $requestVerifier, array $supportedSubProtocols = [], $strictSubProtocol = false) { + public function __construct(RequestVerifier $requestVerifier) { $this->verifier = $requestVerifier; - - $this->_supportedSubProtocols = $supportedSubProtocols; - $this->_strictSubProtocols = $strictSubProtocol; } /** @@ -130,7 +127,6 @@ class ServerNegotiator implements NegotiatorInterface { /** * @param array $protocols - * @deprecated */ function setSupportedSubProtocols(array $protocols) { $this->_supportedSubProtocols = array_flip($protocols); @@ -143,7 +139,6 @@ class ServerNegotiator implements NegotiatorInterface { * @todo Consider extending this interface and moving this there. * The spec does says the server can fail for this reason, but * it is not a requirement. This is an implementation detail. - * @deprecated */ function setStrictSubProtocolCheck($enable) { $this->_strictSubProtocols = (boolean)$enable; diff --git a/tests/unit/Handshake/ServerNegotiatorTest.php b/tests/unit/Handshake/ServerNegotiatorTest.php index 519a37a..94d9add 100644 --- a/tests/unit/Handshake/ServerNegotiatorTest.php +++ b/tests/unit/Handshake/ServerNegotiatorTest.php @@ -111,7 +111,9 @@ Accept-Language: en-US,en;q=0.8'; } public function testBadSubprotocolResponse() { - $negotiator = new ServerNegotiator(new RequestVerifier(), [], true); + $negotiator = new ServerNegotiator(new RequestVerifier()); + $negotiator->setStrictSubProtocolCheck(true); + $negotiator->setSupportedSubProtocols([]); $requestText = 'GET / HTTP/1.1 Host: 127.0.0.1:6789 From 8128af799a4577353f1502acaabc71bd298529a3 Mon Sep 17 00:00:00 2001 From: Matt Bonneau Date: Tue, 12 Sep 2017 22:33:33 -0400 Subject: [PATCH 5/6] Fix requested changes on PR --- .travis.yml | 2 +- src/Handshake/ServerNegotiator.php | 16 ++-------------- tests/unit/Handshake/ServerNegotiatorTest.php | 2 +- 3 files changed, 4 insertions(+), 16 deletions(-) diff --git a/.travis.yml b/.travis.yml index b45fd6e..11d51b4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -17,4 +17,4 @@ before_script: - sh tests/ab/run_ab_tests.sh script: - - phpunit + - vendor/bin/phpunit diff --git a/src/Handshake/ServerNegotiator.php b/src/Handshake/ServerNegotiator.php index 2ebdf6b..7e1e99d 100644 --- a/src/Handshake/ServerNegotiator.php +++ b/src/Handshake/ServerNegotiator.php @@ -62,13 +62,7 @@ class ServerNegotiator implements NegotiatorInterface { 'Sec-WebSocket-Protocol' => implode(', ', $this->_supportedSubProtocols) ]; if (true !== $this->verifier->verifyUpgradeRequest($request->getHeader('Upgrade'))) { - return new Response( - 426, - $upgradeSuggestion, - null, - '1.1', - 'Upgrade header MUST be provided' - ); + return new Response(426, $upgradeSuggestion, null, '1.1', 'Upgrade header MUST be provided'); } if (true !== $this->verifier->verifyConnection($request->getHeader('Connection'))) { @@ -80,12 +74,6 @@ class ServerNegotiator implements NegotiatorInterface { } if (true !== $this->verifier->verifyVersion($request->getHeader('Sec-WebSocket-Version'))) { - /* - * https://tools.ietf.org/html/rfc7230#section-6.7 - * A server that sends a 426 (Upgrade Required) response MUST send an - * Upgrade header field to indicate the acceptable protocols, in order - * of descending preference - */ return new Response(426, $upgradeSuggestion); } @@ -99,7 +87,7 @@ class ServerNegotiator implements NegotiatorInterface { }, null); if ($this->_strictSubProtocols && null === $match) { - return new Response(426, $upgradeSuggestion); + return new Response(426, $upgradeSuggestion, null, '1.1', 'No Sec-WebSocket-Protocols requested supported'); } if (null !== $match) { diff --git a/tests/unit/Handshake/ServerNegotiatorTest.php b/tests/unit/Handshake/ServerNegotiatorTest.php index 94d9add..4edfff3 100644 --- a/tests/unit/Handshake/ServerNegotiatorTest.php +++ b/tests/unit/Handshake/ServerNegotiatorTest.php @@ -136,7 +136,7 @@ Accept-Language: en-US,en;q=0.8'; $this->assertEquals('1.1', $response->getProtocolVersion()); $this->assertEquals(426, $response->getStatusCode()); - $this->assertEquals('Upgrade Required', $response->getReasonPhrase()); + $this->assertEquals('No Sec-WebSocket-Protocols requested supported', $response->getReasonPhrase()); $this->assertEquals('Upgrade', $response->getHeaderLine('Connection')); $this->assertEquals('websocket', $response->getHeaderLine('Upgrade')); $this->assertEquals('13', $response->getHeaderLine('Sec-WebSocket-Version')); From dd636a4058efa6c890e6b46450653052feb74f87 Mon Sep 17 00:00:00 2001 From: Matt Bonneau Date: Wed, 13 Sep 2017 09:40:02 -0400 Subject: [PATCH 6/6] When not agreeing on a subprotocol in non-strict - send no protocol header field --- src/Handshake/ServerNegotiator.php | 6 ++-- tests/unit/Handshake/ServerNegotiatorTest.php | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/Handshake/ServerNegotiator.php b/src/Handshake/ServerNegotiator.php index 7e1e99d..5a0073b 100644 --- a/src/Handshake/ServerNegotiator.php +++ b/src/Handshake/ServerNegotiator.php @@ -58,9 +58,11 @@ class ServerNegotiator implements NegotiatorInterface { $upgradeSuggestion = [ 'Connection' => 'Upgrade', 'Upgrade' => 'websocket', - 'Sec-WebSocket-Version' => $this->getVersionNumber(), - 'Sec-WebSocket-Protocol' => implode(', ', $this->_supportedSubProtocols) + 'Sec-WebSocket-Version' => $this->getVersionNumber() ]; + if (count($this->_supportedSubProtocols) > 0) { + $upgradeSuggestion['Sec-WebSocket-Protocol'] = implode(', ', $this->_supportedSubProtocols); + } if (true !== $this->verifier->verifyUpgradeRequest($request->getHeader('Upgrade'))) { return new Response(426, $upgradeSuggestion, null, '1.1', 'Upgrade header MUST be provided'); } diff --git a/tests/unit/Handshake/ServerNegotiatorTest.php b/tests/unit/Handshake/ServerNegotiatorTest.php index 4edfff3..9c9aa8d 100644 --- a/tests/unit/Handshake/ServerNegotiatorTest.php +++ b/tests/unit/Handshake/ServerNegotiatorTest.php @@ -141,4 +141,35 @@ Accept-Language: en-US,en;q=0.8'; $this->assertEquals('websocket', $response->getHeaderLine('Upgrade')); $this->assertEquals('13', $response->getHeaderLine('Sec-WebSocket-Version')); } + + public function testNonStrictSubprotocolDoesNotIncludeHeaderWhenNoneAgreedOn() { + $negotiator = new ServerNegotiator(new RequestVerifier()); + $negotiator->setStrictSubProtocolCheck(false); + $negotiator->setSupportedSubProtocols(['someproto']); + + $requestText = 'GET / HTTP/1.1 +Host: 127.0.0.1:6789 +Connection: Upgrade +Pragma: no-cache +Cache-Control: no-cache +Upgrade: websocket +Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ== +Sec-WebSocket-Version: 13 +Sec-WebSocket-Protocol: someotherproto +Upgrade-Insecure-Requests: 1 +User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36 +Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8 +Accept-Encoding: gzip, deflate, sdch, br +Accept-Language: en-US,en;q=0.8'; + + $request = \GuzzleHttp\Psr7\parse_request($requestText); + + $response = $negotiator->handshake($request); + + $this->assertEquals('1.1', $response->getProtocolVersion()); + $this->assertEquals(101, $response->getStatusCode()); + $this->assertEquals('Upgrade', $response->getHeaderLine('Connection')); + $this->assertEquals('websocket', $response->getHeaderLine('Upgrade')); + $this->assertFalse($response->hasHeader('Sec-WebSocket-Protocol')); + } } \ No newline at end of file