From 9e405abb34018b939117526c78d08b7af2a7ff20 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Mon, 4 Jan 2021 13:16:20 +0000 Subject: [PATCH 1/5] Rewrite testTokenIsRemovedFromStorageWhenPersistentModeIsOff When persistent mode is off, then the token should be removed from the storage when it has been validated. This test did not do this, but does do now. --- tests/GuardTest.php | 46 +++++++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/tests/GuardTest.php b/tests/GuardTest.php index 1b1f65b..0b3c2b8 100644 --- a/tests/GuardTest.php +++ b/tests/GuardTest.php @@ -279,43 +279,35 @@ public function testEnforceStorageLimitWithIterator() public function testTokenIsRemovedFromStorageWhenPersistentModeIsOff() { - $self = $this; - $storage = [ 'test_name' => 'test_value123', ]; - $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); - $handler = function () use ($self, &$called) { - $responseProphecy = $self->prophesize(ResponseInterface::class); - return $responseProphecy->reveal(); - }; - $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage, $handler); - $requestProphecy = $this->prophesize(ServerRequestInterface::class); - $requestProphecy - ->getMethod() - ->willReturn('POST') - ->shouldBeCalledOnce(); + $response = $this->createMock(ResponseInterface::class); + $requestHandler = $this->createMock(RequestHandlerInterface::class); + $requestHandler->method('handle')->willReturn($response); - $requestProphecy - ->withAttribute(Argument::type('string'), Argument::type('string')) - ->willReturn($requestProphecy->reveal()) - ->shouldBeCalledTimes(2); + $responseFactory = $this->createMock(ResponseFactoryInterface::class); - $requestProphecy - ->getParsedBody() - ->willReturn([ - 'test_name' => 'test_name123', - 'test_value' => 'invalid_value', - ]) - ->shouldBeCalledOnce(); + $handler = function () use ($response) { + return $response; + }; - $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); + $mw = new Guard($responseFactory, 'test', $storage, $handler); - $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal()); - $this->assertArrayNotHasKey('test_name123', $storage); + $request = $this->createMock(ServerRequestInterface::class); + $request->expects(self::once())->method('getMethod')->willReturn('POST'); + $request->expects(self::exactly(2))->method('withAttribute')->willReturn($request); + $request->expects(self::once())->method('getParsedBody')->willReturn([ + 'test_name' => 'test_name', + 'test_value' => 'test_value123', + ]); + + $mw->process($request, $requestHandler); + self::assertArrayNotHasKey('test_name', $storage); } + public function testProcessAppendsNewTokensWhenPersistentTokenModeIsOff() { $storage = []; From caaae6ff3819a1de5dc79ad05f5e6fa02a82dff4 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Mon, 4 Jan 2021 13:20:11 +0000 Subject: [PATCH 2/5] Ensure token is removed in persisent mode on validation When the token is successfully validated and we are not in persistent mode, then we need to remove the token. This is a separate operation to handling failure. The side-effect of the previous code is that is allowed a replay attack where the same token could be used multiple times even when persisent mode was disabled. Thanks to Xhelal Likaj (https://github.com/xhlika) for the report. --- src/Guard.php | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index 9e3721a..4936459 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -415,14 +415,13 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $value = $body[$this->getTokenValueKey()] ?? null; } - if ($name === null - || $value === null - || !$this->validateToken((string) $name, (string) $value) - ) { - if (!$this->persistentTokenMode && is_string($name)) { - $this->removeTokenFromStorage($name); - } + $isValid = $this->validateToken((string) $name, (string) $value); + if ($isValid && !$this->persistentTokenMode) { + // successfully validated token, so delete it if not in persistentTokenMode + $this->removeTokenFromStorage($name); + } + if ($name === null || $value === null || !$isValid) { $request = $this->appendNewTokenToRequest($request); return $this->handleFailure($request, $handler); } From 6e50dd9f035e0c0007a8a9ed275594acfc285a52 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Mon, 4 Jan 2021 13:30:48 +0000 Subject: [PATCH 3/5] Add xdebug.mode setting for xdebug 3 This is required for PHP7.3+ as our phpunit.xml.dist automatically creates coverage. --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 17531c8..beefe35 100644 --- a/.travis.yml +++ b/.travis.yml @@ -19,8 +19,8 @@ before_script: - composer install -n script: - - if [[ "$ANALYSIS" != 'true' ]]; then vendor/bin/phpunit ; fi - - if [[ "$ANALYSIS" == 'true' ]]; then vendor/bin/phpunit --coverage-clover clover.xml ; fi + - if [[ "$ANALYSIS" != 'true' ]]; then XDEBUG_MODE=coverage ./vendor/bin/phpunit; fi + - if [[ "$ANALYSIS" == 'true' ]]; then XDEBUG_MODE=coverage ./vendor/bin/phpunit --coverage-clover clover.xml ; fi after_success: - if [[ "$ANALYSIS" == 'true' ]]; then vendor/bin/php-coveralls --coverage_clover=clover.xml -v ; fi From 9fe81ac34733e6206837f3183d5177102daf8a00 Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Tue, 5 Jan 2021 10:05:04 +0000 Subject: [PATCH 4/5] Convert unit test to Prophecy --- tests/GuardTest.php | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/tests/GuardTest.php b/tests/GuardTest.php index 0b3c2b8..701ceb5 100644 --- a/tests/GuardTest.php +++ b/tests/GuardTest.php @@ -283,27 +283,37 @@ public function testTokenIsRemovedFromStorageWhenPersistentModeIsOff() 'test_name' => 'test_value123', ]; - $response = $this->createMock(ResponseInterface::class); - $requestHandler = $this->createMock(RequestHandlerInterface::class); - $requestHandler->method('handle')->willReturn($response); + $responseProphecy = $this->prophesize(ResponseInterface::class) + ->willImplement(ResponseInterface::class); - $responseFactory = $this->createMock(ResponseFactoryInterface::class); + $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); + $requestHandlerProphecy + ->handle(Argument::type(ServerRequestInterface::class)) + ->willReturn($responseProphecy->reveal()); - $handler = function () use ($response) { - return $response; - }; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); - $mw = new Guard($responseFactory, 'test', $storage, $handler); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); - $request = $this->createMock(ServerRequestInterface::class); - $request->expects(self::once())->method('getMethod')->willReturn('POST'); - $request->expects(self::exactly(2))->method('withAttribute')->willReturn($request); - $request->expects(self::once())->method('getParsedBody')->willReturn([ - 'test_name' => 'test_name', - 'test_value' => 'test_value123', - ]); + $requestProphecy = $this->prophesize(ServerRequestInterface::class); + $requestProphecy + ->getMethod() + ->willReturn('POST') + ->shouldBeCalledOnce(); + $requestProphecy + ->withAttribute(Argument::type('string'), Argument::type('string')) + ->willReturn($requestProphecy->reveal()) + ->shouldBeCalledTimes(2); + $requestProphecy + ->getParsedBody() + ->willReturn([ + 'test_name' => 'test_name', + 'test_value' => 'test_value123', + ]) + ->shouldBeCalledOnce(); - $mw->process($request, $requestHandler); + + $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal()); self::assertArrayNotHasKey('test_name', $storage); } From cb875043e007a4ea270d35666ca01d36c3cbb81e Mon Sep 17 00:00:00 2001 From: Rob Allen Date: Wed, 6 Jan 2021 09:18:54 +0000 Subject: [PATCH 5/5] Ensure that the requestHandlerProphecy's handle() is called --- tests/GuardTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/GuardTest.php b/tests/GuardTest.php index 701ceb5..bd410c6 100644 --- a/tests/GuardTest.php +++ b/tests/GuardTest.php @@ -289,7 +289,8 @@ public function testTokenIsRemovedFromStorageWhenPersistentModeIsOff() $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); $requestHandlerProphecy ->handle(Argument::type(ServerRequestInterface::class)) - ->willReturn($responseProphecy->reveal()); + ->willReturn($responseProphecy->reveal()) + ->shouldBeCalledOnce(); $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class);