From 2ea8665c213bb5382cd9a04c11b2d8cffd522212 Mon Sep 17 00:00:00 2001 From: thomas-lb Date: Wed, 4 Sep 2019 18:36:35 +0200 Subject: [PATCH 01/31] Minimum PHP 7.1 --- .travis.yml | 3 --- composer.json | 11 +++++------ phpunit.xml.dist | 1 - 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/.travis.yml b/.travis.yml index 2d1c97e..9d095f2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,9 +1,6 @@ language: php php: - - 5.5 - - 5.6 - - 7.0 - 7.1 - 7.2 - 7.3 diff --git a/composer.json b/composer.json index e2a74b1..7900536 100644 --- a/composer.json +++ b/composer.json @@ -1,7 +1,7 @@ { "name": "slim/csrf", "type": "library", - "description": "Slim Framework 3 CSRF protection middleware", + "description": "Slim Framework 4 CSRF protection middleware PSR-15", "keywords": ["slim","framework","middleware","csrf"], "homepage": "http://slimframework.com", "license": "MIT", @@ -13,13 +13,12 @@ } ], "require": { - "php": ">=5.5.0", - "psr/http-message": "^1.0", - "paragonie/random_compat": "^1.1|^2.0|^9.99" + "php": "^7.1", + "psr/http-message": "^1.0" }, "require-dev": { - "slim/slim": "~3.0", - "phpunit/phpunit": "^4.0" + "phpunit/phpunit": "^7.5", + "slim/psr7": "^0.5.0" }, "autoload": { "psr-4": { diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 8259dc4..f43c285 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -8,7 +8,6 @@ convertWarningsToExceptions="true" processIsolation="false" stopOnFailure="false" - syntaxCheck="false" bootstrap="tests/bootstrap.php" > From b1c32cbe87eec55f2dc7d5b9210867d37d5cb703 Mon Sep 17 00:00:00 2001 From: thomas-lb Date: Thu, 5 Sep 2019 01:34:01 +0200 Subject: [PATCH 02/31] Slim 4 PSR-15 supported --- composer.json | 4 +- src/Guard.php | 103 +++++++------ tests/CsrfTest.php | 352 +++++++++++++++++++++++++++----------------- tests/bootstrap.php | 1 + 4 files changed, 276 insertions(+), 184 deletions(-) diff --git a/composer.json b/composer.json index 7900536..35ccaeb 100644 --- a/composer.json +++ b/composer.json @@ -14,7 +14,9 @@ ], "require": { "php": "^7.1", - "psr/http-message": "^1.0" + "psr/http-message": "^1.0", + "psr/http-server-middleware": "^1.0", + "slim/slim": "^4.2" }, "require-dev": { "phpunit/phpunit": "^7.5", diff --git a/src/Guard.php b/src/Guard.php index 46fbfd3..73ad2d3 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -1,8 +1,14 @@ prefix = rtrim($prefix, '_'); if ($strength < 16) { @@ -114,7 +120,7 @@ public function __construct( * * @return string */ - public function getTokenNameKey() + public function getTokenNameKey(): string { return $this->prefix . '_name'; } @@ -124,7 +130,7 @@ public function getTokenNameKey() * * @return string */ - public function getTokenValueKey() + public function getTokenValueKey(): string { return $this->prefix . '_value'; } @@ -132,13 +138,12 @@ public function getTokenValueKey() /** * Invoke middleware * - * @param ServerRequestInterface $request PSR7 request object - * @param ResponseInterface $response PSR7 response object - * @param callable $next Next middleware callable + * @param ServerRequestInterface $request PSR7 request object + * @param RequestHandlerInterface $handler * * @return ResponseInterface PSR7 response object */ - public function __invoke(ServerRequestInterface $request, ResponseInterface $response, callable $next) + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $this->validateStorage(); @@ -146,14 +151,14 @@ public function __invoke(ServerRequestInterface $request, ResponseInterface $res if (in_array($request->getMethod(), ['POST', 'PUT', 'DELETE', 'PATCH'])) { $body = $request->getParsedBody(); $body = $body ? (array)$body : []; - $name = isset($body[$this->prefix . '_name']) ? $body[$this->prefix . '_name'] : false; - $value = isset($body[$this->prefix . '_value']) ? $body[$this->prefix . '_value'] : false; + $name = $body[$this->prefix . '_name'] ?? false; + $value = $body[$this->prefix . '_value'] ?? false; if (!$name || !$value || !$this->validateToken($name, $value)) { // Need to regenerate a new token, as the validateToken removed the current one. $request = $this->generateNewToken($request); $failureCallable = $this->getFailureCallable(); - return $failureCallable($request, $response, $next); + return $failureCallable($request, $handler); } } @@ -168,12 +173,10 @@ public function __invoke(ServerRequestInterface $request, ResponseInterface $res // Enforce the storage limit $this->enforceStorageLimit(); - return $next($request, $response); + return $handler->handle($request); } /** - * @param $prefix - * @param $storage * @return mixed */ public function validateStorage() @@ -201,7 +204,7 @@ public function validateStorage() * * @return array */ - public function generateToken() + public function generateToken(): array { // Generate new CSRF token $name = uniqid($this->prefix); @@ -218,14 +221,13 @@ public function generateToken() /** * Generates a new CSRF token and attaches it to the Request Object - * + * * @param ServerRequestInterface $request PSR7 response object. - * + * * @return ServerRequestInterface PSR7 response object. */ - public function generateNewToken(ServerRequestInterface $request) + public function generateNewToken(ServerRequestInterface $request): ServerRequestInterface { - $pair = $this->generateToken(); $request = $this->attachRequestAttributes($request, $pair); @@ -242,7 +244,7 @@ public function generateNewToken(ServerRequestInterface $request) * * @return bool */ - public function validateToken($name, $value) + public function validateToken(string $name, string $value): bool { $token = $this->getFromStorage($name); if (function_exists('hash_equals')) { @@ -264,7 +266,7 @@ public function validateToken($name, $value) * * @return string */ - protected function createToken() + protected function createToken(): string { return bin2hex(random_bytes($this->strength)); } @@ -275,7 +277,7 @@ protected function createToken() * @param string $name CSRF token name * @param string $value CSRF token value */ - protected function saveToStorage($name, $value) + protected function saveToStorage(string $name, string $value): void { $this->storage[$name] = $value; } @@ -287,17 +289,17 @@ protected function saveToStorage($name, $value) * * @return string|bool CSRF token value or `false` if not present */ - protected function getFromStorage($name) + protected function getFromStorage(string $name) { - return isset($this->storage[$name]) ? $this->storage[$name] : false; + return $this->storage[$name] ?? false; } /** * Get the most recent key pair from storage. * * @return string[]|null Array containing name and value if found, null otherwise - */ - protected function getLastKeyPair() + */ + protected function getLastKeyPair(): ?array { // Use count, since empty ArrayAccess objects can still return false for `empty` if (count($this->storage) < 1) { @@ -321,7 +323,7 @@ protected function getLastKeyPair() * * @return bool `true` if there was a key pair to load in storage, false otherwise. */ - protected function loadLastKeyPair() + protected function loadLastKeyPair(): bool { $this->keyPair = $this->getLastKeyPair(); @@ -337,7 +339,7 @@ protected function loadLastKeyPair() * * @param string $name CSRF token name */ - protected function removeFromStorage($name) + protected function removeFromStorage(string $name): void { $this->storage[$name] = ' '; unset($this->storage[$name]); @@ -350,7 +352,7 @@ protected function removeFromStorage($name) * This is required as a token is generated every request and so * most will never be used. */ - protected function enforceStorageLimit() + protected function enforceStorageLimit(): void { if ($this->storageLimit < 1) { return; @@ -371,7 +373,7 @@ protected function enforceStorageLimit() // array_shift() doesn't work for ArrayAccess, so we need an iterator in order to use rewind() // and key(), so that we can then unset $iterator = $this->storage; - if ($this->storage instanceof \IteratorAggregate) { + if ($this->storage instanceof IteratorAggregate) { $iterator = $this->storage->getIterator(); } while (count($this->storage) > $this->storageLimit) { @@ -384,9 +386,9 @@ protected function enforceStorageLimit() /** * @param ServerRequestInterface $request * @param $pair - * @return static + * @return ServerRequestInterface */ - protected function attachRequestAttributes(ServerRequestInterface $request, $pair) + protected function attachRequestAttributes(ServerRequestInterface $request, array $pair): ServerRequestInterface { return $request->withAttribute($this->prefix . '_name', $pair[$this->prefix . '_name']) ->withAttribute($this->prefix . '_value', $pair[$this->prefix . '_value']); @@ -400,8 +402,13 @@ protected function attachRequestAttributes(ServerRequestInterface $request, $pai public function getFailureCallable() { if (is_null($this->failureCallable)) { - $this->failureCallable = function (ServerRequestInterface $request, ResponseInterface $response, $next) { - $body = new \Slim\Http\Body(fopen('php://temp', 'r+')); + $this->failureCallable = function ( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ): ResponseInterface { + $responseFactory = AppFactory::determineResponseFactory(); + $response = $responseFactory->createResponse(); + $body = $response->getBody(); $body->write('Failed CSRF check!'); return $response->withStatus(400)->withHeader('Content-type', 'text/plain')->withBody($body); }; @@ -415,7 +422,7 @@ public function getFailureCallable() * @param mixed $failureCallable Value to set * @return $this */ - public function setFailureCallable($failureCallable) + public function setFailureCallable($failureCallable): self { $this->failureCallable = $failureCallable; return $this; @@ -424,11 +431,11 @@ public function setFailureCallable($failureCallable) /** * Setter for persistentTokenMode * - * @param bool $persistentTokenMode True to use the same token throughout the session (unless there is a validation error), - * false to get a new token with each request. + * @param bool $persistentTokenMode True to use the same token throughout the session + * (unless there is a validation error), false to get a new token with each request. * @return $this */ - public function setPersistentTokenMode($persistentTokenMode) + public function setPersistentTokenMode(bool $persistentTokenMode): self { $this->persistentTokenMode = $persistentTokenMode; return $this; @@ -440,7 +447,7 @@ public function setPersistentTokenMode($persistentTokenMode) * @param integer $storageLimit Value to set * @return $this */ - public function setStorageLimit($storageLimit) + public function setStorageLimit(int $storageLimit): self { $this->storageLimit = (int)$storageLimit; return $this; @@ -451,7 +458,7 @@ public function setStorageLimit($storageLimit) * * @return bool */ - public function getPersistentTokenMode() + public function getPersistentTokenMode(): bool { return $this->persistentTokenMode; } @@ -459,16 +466,16 @@ public function getPersistentTokenMode() /** * @return string */ - public function getTokenName() + public function getTokenName(): ?string { - return isset($this->keyPair[$this->getTokenNameKey()]) ? $this->keyPair[$this->getTokenNameKey()] : null; + return $this->keyPair[$this->getTokenNameKey()] ?? null; } /** * @return string */ - public function getTokenValue() + public function getTokenValue(): ?string { - return isset($this->keyPair[$this->getTokenValueKey()]) ? $this->keyPair[$this->getTokenValueKey()] : null; + return $this->keyPair[$this->getTokenValueKey()] ?? null; } } diff --git a/tests/CsrfTest.php b/tests/CsrfTest.php index 25366c5..b0df4f4 100644 --- a/tests/CsrfTest.php +++ b/tests/CsrfTest.php @@ -1,46 +1,66 @@ createUri('https://example.com:443/foo/bar?abc=123'); $headers = new Headers(); $cookies = []; - $env = Environment::mock(); - $serverParams = $env->all(); - $body = new Body(fopen('php://temp', 'r+')); + $serverParams = Environment::mock(); + $body = new Stream(fopen('php://temp', 'r+')); $this->request = new Request('GET', $uri, $headers, $cookies, $serverParams, $body); - $this->response = new Response; + + $this->response = new Response(); + + $this->responseFactory = new ResponseFactory(); + + $this->middlewareDispatcher = new MiddlewareDispatcher($this->createMock(RequestHandlerInterface::class)); } public function testTokenKeys() @@ -55,26 +75,61 @@ public function testTokenGeneration() { $storage = []; $request = $this->request; - $response = $this->response; + $responseFactory = $this->responseFactory; $mw = new Guard('csrf', $storage); - $next = function ($req, $res) use ($mw) { - return $res - ->withHeader('X-CSRF-NAME', $req->getAttribute($mw->getTokenNameKey())) - ->withHeader('X-CSRF-VALUE', $req->getAttribute($mw->getTokenValueKey())); + $mw2 = function ( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ) use ( + $mw, + $responseFactory + ): ResponseInterface { + return $responseFactory->createResponse() + ->withHeader('X-CSRF-NAME', $request->getAttribute($mw->getTokenNameKey())) + ->withHeader('X-CSRF-VALUE', $request->getAttribute($mw->getTokenValueKey())); }; - $response1 = $mw($request, $response, $next); - $response2 = $mw($request, $response, $next); - - $this->assertStringStartsWith('csrf', $response1->getHeaderLine('X-CSRF-NAME'), 'Name key should start with csrf prefix'); - $this->assertStringStartsWith('csrf', $response2->getHeaderLine('X-CSRF-NAME'), 'Name key should start with csrf prefix'); - - $this->assertNotEquals($response1->getHeaderLine('X-CSRF-NAME'), $response2->getHeaderLine('X-CSRF-NAME'), 'Generated token names must be unique'); - - $this->assertEquals(32, strlen($response1->getHeaderLine('X-CSRF-VALUE')), 'Length of the generated token value should be double the strength'); - $this->assertEquals(32, strlen($response2->getHeaderLine('X-CSRF-VALUE')), 'Length of the generated token value should be double the strength'); - - $this->assertTrue(ctype_xdigit($response1->getHeaderLine('X-CSRF-VALUE')), 'Generated token value is not hexadecimal'); - $this->assertTrue(ctype_xdigit($response2->getHeaderLine('X-CSRF-VALUE')), 'Generated token value is not hexadecimal'); + + $this->middlewareDispatcher->addCallable($mw2); + $this->middlewareDispatcher->addMiddleware($mw); + $response1 = $this->middlewareDispatcher->handle($request); + $response2 = $this->middlewareDispatcher->handle($request); + + $this->assertStringStartsWith( + 'csrf', + $response1->getHeaderLine('X-CSRF-NAME'), + 'Name key should start with csrf prefix' + ); + $this->assertStringStartsWith( + 'csrf', + $response2->getHeaderLine('X-CSRF-NAME'), + 'Name key should start with csrf prefix' + ); + + $this->assertNotEquals( + $response1->getHeaderLine('X-CSRF-NAME'), + $response2->getHeaderLine('X-CSRF-NAME'), + 'Generated token names must be unique' + ); + + $this->assertEquals( + 32, + strlen($response1->getHeaderLine('X-CSRF-VALUE')), + 'Length of the generated token value should be double the strength' + ); + $this->assertEquals( + 32, + strlen($response2->getHeaderLine('X-CSRF-VALUE')), + 'Length of the generated token value should be double the strength' + ); + + $this->assertTrue( + ctype_xdigit($response1->getHeaderLine('X-CSRF-VALUE')), + 'Generated token value is not hexadecimal' + ); + $this->assertTrue( + ctype_xdigit($response2->getHeaderLine('X-CSRF-VALUE')), + 'Generated token value is not hexadecimal' + ); } public function testValidToken() @@ -86,12 +141,21 @@ public function testValidToken() 'csrf_name' => 'csrf_123', 'csrf_value' => 'xyz' ]); - $response = $this->response; - $next = function ($req, $res) { - return $res; - }; $mw = new Guard('csrf', $storage); - $newResponse = $mw($request, $response, $next); + $responseFactory = $this->responseFactory; + $mw2 = function ( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ) use ( + $mw, + $responseFactory + ): ResponseInterface { + return $responseFactory->createResponse(); + }; + + $this->middlewareDispatcher->addMiddleware($mw); + $this->middlewareDispatcher->addCallable($mw2); + $newResponse = $this->middlewareDispatcher->handle($request); $this->assertEquals(200, $newResponse->getStatusCode()); } @@ -105,12 +169,9 @@ public function testInvalidToken() 'csrf_name' => 'csrf_123', 'csrf_value' => 'xyz' ]); - $response = $this->response; - $next = function ($req, $res) { - return $res; - }; $mw = new Guard('csrf', $storage); - $newResponse = $mw($request, $response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $newResponse = $this->middlewareDispatcher->handle($request); $this->assertEquals(400, $newResponse->getStatusCode()); } @@ -124,12 +185,9 @@ public function testMissingToken() 'csrf_name' => 'csrf_123', 'csrf_value' => 'xyz' ]); - $response = $this->response; - $next = function ($req, $res) { - return $res; - }; $mw = new Guard('csrf', $storage); - $newResponse = $mw($request, $response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $newResponse = $this->middlewareDispatcher->handle($request); $this->assertEquals(400, $newResponse->getStatusCode()); } @@ -137,42 +195,36 @@ public function testMissingToken() public function testExternalStorageOfAnArrayAccessPersists() { $storage = new \ArrayObject(); - + $request = $this->request ->withMethod('POST') ->withParsedBody([ 'csrf_name' => 'csrf_123', 'csrf_value' => 'xyz' ]); - $response = $this->response; - $next = function ($req, $res) { - return $res; - }; $mw = new Guard('csrf', $storage); $this->assertEquals(0, count($storage)); - $newResponse = $mw($request, $response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $newResponse = $this->middlewareDispatcher->handle($request); $this->assertEquals(1, count($storage)); } public function testExternalStorageOfAnArrayPersists() { $storage = []; - + $request = $this->request ->withMethod('POST') ->withParsedBody([ 'csrf_name' => 'csrf_123', 'csrf_value' => 'xyz' ]); - $response = $this->response; - $next = function ($req, $res) { - return $res; - }; $mw = new Guard('csrf', $storage); $this->assertEquals(0, count($storage)); - $newResponse = $mw($request, $response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $newResponse = $this->middlewareDispatcher->handle($request); $this->assertEquals(1, count($storage)); } @@ -182,22 +234,30 @@ public function testPersistenceModeTrueBetweenRequestsArray() $mw = new Guard('csrf', $storage, null, 200, 16, true); - $next = function ($req, $res) use ($mw) { + $responseFactory = $this->responseFactory; + $mw2 = function ( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ) use ( + $mw, + $responseFactory + ): ResponseInterface { // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $req->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $req->getAttribute('csrf_value')); - return $res; + $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); + $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); + return $responseFactory->createResponse(); }; // Token name and value should be null if the storage is empty and middleware has not yet been invoked $this->assertNull($mw->getTokenName()); - $this->assertNull($mw->getTokenValue()); - - $response = $mw($this->request, $this->response, $next); + $this->assertNull($mw->getTokenValue()); + + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($this->request); // Persistent token name and value have now been generated $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); + $value = $mw->getTokenValue(); // Subsequent request will attempt to validate the token $request = $this->request @@ -206,35 +266,44 @@ public function testPersistenceModeTrueBetweenRequestsArray() 'csrf_name' => $name, 'csrf_value' => $value ]); - $response = $mw($request, $this->response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($request); // Token name and value should be the same after subsequent request $this->assertEquals($name, $mw->getTokenName()); $this->assertEquals($value, $mw->getTokenValue()); } - + public function testPersistenceModeTrueBetweenRequestsArrayAccess() { $storage = new \ArrayObject(); $mw = new Guard('csrf', $storage, null, 200, 16, true); - $next = function ($req, $res) use ($mw) { + $responseFactory = $this->responseFactory; + $mw2 = function ( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ) use ( + $mw, + $responseFactory + ): ResponseInterface { // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $req->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $req->getAttribute('csrf_value')); - return $res; + $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); + $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); + return $responseFactory->createResponse(); }; // Token name and value should be null if the storage is empty and middleware has not yet been invoked $this->assertNull($mw->getTokenName()); - $this->assertNull($mw->getTokenValue()); - - $response = $mw($this->request, $this->response, $next); + $this->assertNull($mw->getTokenValue()); + + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($this->request); // Persistent token name and value have now been generated $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); + $value = $mw->getTokenValue(); // Subsequent request will attempt to validate the token $request = $this->request @@ -243,35 +312,44 @@ public function testPersistenceModeTrueBetweenRequestsArrayAccess() 'csrf_name' => $name, 'csrf_value' => $value ]); - $response = $mw($request, $this->response, $next); - + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($request); + // Token name and value should be the same after subsequent request $this->assertEquals($name, $mw->getTokenName()); $this->assertEquals($value, $mw->getTokenValue()); - } - + } + public function testPersistenceModeFalseBetweenRequestsArray() { $storage = []; $mw = new Guard('csrf', $storage); - $next = function ($req, $res) use ($mw) { + $responseFactory = $this->responseFactory; + $mw2 = function ( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ) use ( + $mw, + $responseFactory + ): ResponseInterface { // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $req->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $req->getAttribute('csrf_value')); - return $res; + $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); + $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); + return $responseFactory->createResponse(); }; // Token name and value should be null if the storage is empty and middleware has not yet been invoked $this->assertNull($mw->getTokenName()); - $this->assertNull($mw->getTokenValue()); - - $response = $mw($this->request, $this->response, $next); + $this->assertNull($mw->getTokenValue()); + + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($this->request); // First token name and value have now been generated $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); + $value = $mw->getTokenValue(); // Subsequent request will attempt to validate the token $request = $this->request @@ -280,35 +358,44 @@ public function testPersistenceModeFalseBetweenRequestsArray() 'csrf_name' => $name, 'csrf_value' => $value ]); - $response = $mw($request, $this->response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($request); // Token name and value should NOT be the same after subsequent request $this->assertNotEquals($name, $mw->getTokenName()); $this->assertNotEquals($value, $mw->getTokenValue()); } - + public function testPersistenceModeFalseBetweenRequestsArrayAccess() { $storage = new \ArrayObject(); $mw = new Guard('csrf', $storage); - $next = function ($req, $res) use ($mw) { + $responseFactory = $this->responseFactory; + $mw2 = function ( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ) use ( + $mw, + $responseFactory + ): ResponseInterface { // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $req->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $req->getAttribute('csrf_value')); - return $res; + $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); + $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); + return $responseFactory->createResponse(); }; // Token name and value should be null if the storage is empty and middleware has not yet been invoked $this->assertNull($mw->getTokenName()); - $this->assertNull($mw->getTokenValue()); - - $response = $mw($this->request, $this->response, $next); + $this->assertNull($mw->getTokenValue()); + + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($this->request); // First token name and value have now been generated $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); + $value = $mw->getTokenValue(); // Subsequent request will attempt to validate the token $request = $this->request @@ -317,24 +404,22 @@ public function testPersistenceModeFalseBetweenRequestsArrayAccess() 'csrf_name' => $name, 'csrf_value' => $value ]); - $response = $mw($request, $this->response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($request); // Token name and value should NOT be the same after subsequent request $this->assertNotEquals($name, $mw->getTokenName()); $this->assertNotEquals($value, $mw->getTokenValue()); } - + public function testUpdateAfterInvalidTokenWithPersistenceModeTrue() { $storage = []; $mw = new Guard('csrf', $storage, null, 200, 16, true); - $next = function ($req, $res) { - return $res; - }; - - $response = $mw($this->request, $this->response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($this->request); // Persistent token name and value have now been generated $name = $mw->getTokenName(); @@ -346,60 +431,57 @@ public function testUpdateAfterInvalidTokenWithPersistenceModeTrue() ->withParsedBody([ 'csrf_name' => 'csrf_123', 'csrf_value' => 'xyz' - ]); - $response = $mw($request, $this->response, $next); + ]); + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($request); // Token name and value should NOT be the same after subsequent request $this->assertNotEquals($name, $mw->getTokenName()); $this->assertNotEquals($value, $mw->getTokenValue()); - } - + } + public function testStorageLimitIsEnforcedForObjects() { $storage = new \ArrayObject(); - + $request = $this->request; - $response = $this->response; - $next = function ($req, $res) { - return $res; - }; + $mw = new Guard('csrf', $storage); $mw->setStorageLimit(2); $this->assertEquals(0, count($storage)); - $response = $mw($request, $response, $next); - $response = $mw($request, $response, $next); - $response = $mw($request, $response, $next); + + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($request); + $response = $this->middlewareDispatcher->handle($request); + $response = $this->middlewareDispatcher->handle($request); $this->assertEquals(2, count($storage)); } public function testStorageLimitIsEnforcedForArrays() { $storage = []; - + $request = $this->request; - $response = $this->response; - $next = function ($req, $res) { - return $res; - }; + $mw = new Guard('csrf', $storage); $mw->setStorageLimit(2); $this->assertEquals(0, count($storage)); - $response = $mw($request, $response, $next); - $response = $mw($request, $response, $next); - $response = $mw($request, $response, $next); + + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($request); + $response = $this->middlewareDispatcher->handle($request); + $response = $this->middlewareDispatcher->handle($request); $this->assertEquals(2, count($storage)); } - public function testKeyPair() { + public function testKeyPair() + { $mw = new Guard(); - $next = function ($req, $res) { - return $res; - }; - - $response = $mw($this->request, $this->response, $next); + $this->middlewareDispatcher->addMiddleware($mw); + $response = $this->middlewareDispatcher->handle($this->request); $this->assertNotNull($mw->getTokenName()); diff --git a/tests/bootstrap.php b/tests/bootstrap.php index e44d01b..14c26e7 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -1,4 +1,5 @@ Date: Thu, 5 Sep 2019 01:54:19 +0200 Subject: [PATCH 03/31] Corrected namespace --- tests/CsrfTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/CsrfTest.php b/tests/CsrfTest.php index b0df4f4..eb2dfa5 100644 --- a/tests/CsrfTest.php +++ b/tests/CsrfTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Slim\HttpCache\Tests; +namespace Slim\Tests\Csrf; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ResponseInterface; From fe8053cff4d99d4723b64069d79195ae643efd3b Mon Sep 17 00:00:00 2001 From: thomas-lb Date: Thu, 5 Sep 2019 02:28:38 +0200 Subject: [PATCH 04/31] Fix test PHPUnit --- .travis.yml | 2 +- tests/CsrfTest.php | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 9d095f2..1c19bbc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,4 +12,4 @@ matrix: before_script: composer install -script: phpunit --coverage-text +script: vendor/bin/phpunit --coverage-text diff --git a/tests/CsrfTest.php b/tests/CsrfTest.php index eb2dfa5..9c839fa 100644 --- a/tests/CsrfTest.php +++ b/tests/CsrfTest.php @@ -46,8 +46,10 @@ class CsrfTest extends TestCase /** * Run before each test + * + * @return void */ - public function setUp() + public function setUp(): void { $uri = (new UriFactory())->createUri('https://example.com:443/foo/bar?abc=123'); $headers = new Headers(); From ceb3523952d801756ef7e946dfb8d46c815a44b2 Mon Sep 17 00:00:00 2001 From: thomas-lb Date: Thu, 5 Sep 2019 14:08:03 +0200 Subject: [PATCH 05/31] Update README.md --- README.md | 44 ++++++++++++++++++++++++-------------------- composer.json | 2 +- 2 files changed, 25 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index c73c42a..54bb6a8 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![Build Status](https://travis-ci.org/slimphp/Slim-Csrf.svg?branch=master)](https://travis-ci.org/slimphp/Slim-Csrf) -This repository contains a Slim Framework CSRF protection middleware. CSRF protection applies to all unsafe HTTP requests (POST, PUT, DELETE, PATCH). +This repository contains a Slim Framework CSRF protection PSR-15 middleware. CSRF protection applies to all unsafe HTTP requests (POST, PUT, DELETE, PATCH). You can fetch the latest CSRF token's name and value from the Request object with its `getAttribute()` method. By default, the CSRF token's name is stored in the `csrf_name` attribute, and the CSRF token's value is stored in the `csrf_value` attribute. @@ -14,7 +14,7 @@ Via Composer $ composer require slim/csrf ``` -Requires Slim 3.0.0 or newer. +Requires Slim 4.0.0 or newer. ## Usage @@ -28,13 +28,17 @@ as it is middleware, you can also register it for a subset of routes. // Start PHP session session_start(); -$app = new \Slim\App(); +// Create Container +$container = new Container(); +AppFactory::setContainer($container); + +$app = AppFactory::create(); // Register with container $container = $app->getContainer(); -$container['csrf'] = function ($c) { +$container->set('csrf', function (ContainerInterface $container) { return new \Slim\Csrf\Guard; -}; +}); // Register middleware for all routes // If you are implementing per-route checks you must not add this @@ -67,13 +71,19 @@ $app->run(); // Start PHP session session_start(); -$app = new \Slim\App(); +// Create Container +$container = new Container(); +AppFactory::setContainer($container); + +$app = AppFactory::create(); // Register with container $container = $app->getContainer(); -$container['csrf'] = function ($c) { +$container->set('csrf', function (ContainerInterface $container) { return new \Slim\Csrf\Guard; -}; +}); + +$app = new \Slim\App(); $app->get('/api/myEndPoint',function ($request, $response, $args) { $nameKey = $this->csrf->getTokenNameKey(); @@ -125,12 +135,11 @@ To use persistent tokens, set the sixth parameter of the constructor to `true`. ### Accessing the token pair in templates (Twig, etc) -In many situations, you will want to access the token pair without needing to go through the request object. In these cases, you can use `getTokenName()` and `getTokenValue()` directly on the `Guard` middleware instance. This can be useful, for example in a [Twig extension](http://twig.sensiolabs.org/doc/advanced.html#creating-an-extension): +In many situations, you will want to access the token pair without needing to go through the request object. In these cases, you can use `getTokenName()` and `getTokenValue()` directly on the `Guard` middleware instance. This can be useful, for example in a [Twig extension](https://twig.symfony.com/doc/2.x/advanced.html#creating-an-extension): ```php -class CsrfExtension extends \Twig_Extension implements Twig_Extension_GlobalsInterface +class CsrfExtension extends \Twig\Extension\AbstractExtension implements \Twig\Extension\GlobalsInterface { - /** * @var \Slim\Csrf\Guard */ @@ -160,11 +169,6 @@ class CsrfExtension extends \Twig_Extension implements Twig_Extension_GlobalsInt ] ]; } - - public function getName() - { - return 'slim/csrf'; - } } ``` @@ -183,16 +187,16 @@ a simple plain text error message. To override this, provide a callable as the third parameter to the constructor or via `setFailureCallable()`. This callable has the same signature as -middleware: `function($request, $response, $next)` and must return a Response. +middleware: `function($request, $handler)` and must return a Response. For example: ```php -$container['csrf'] = function ($c) { +$container->set('csrf', function (ContainerInterface $container) { $guard = new \Slim\Csrf\Guard(); - $guard->setFailureCallable(function ($request, $response, $next) { + $guard->setFailureCallable(function (ServerRequestInterface $request, RequestHandlerInterface $handler) { $request = $request->withAttribute("csrf_status", false); - return $next($request, $response); + return $handler->handle($request); }); return $guard; }; diff --git a/composer.json b/composer.json index 35ccaeb..07f70d5 100644 --- a/composer.json +++ b/composer.json @@ -16,7 +16,7 @@ "php": "^7.1", "psr/http-message": "^1.0", "psr/http-server-middleware": "^1.0", - "slim/slim": "^4.2" + "slim/slim": "^4.0" }, "require-dev": { "phpunit/phpunit": "^7.5", From afa062b21ec32b93e5fe5ca938e0edcb70fcf9ff Mon Sep 17 00:00:00 2001 From: thomas-lb Date: Thu, 5 Sep 2019 14:29:07 +0200 Subject: [PATCH 06/31] Remove HHVM Travis --- .travis.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1c19bbc..993959f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,11 +4,6 @@ php: - 7.1 - 7.2 - 7.3 - - hhvm - -matrix: - allow_failures: - - php: hhvm before_script: composer install From 74c82d80d713c56d4f5af5917d028671e4d34091 Mon Sep 17 00:00:00 2001 From: thomas-lb Date: Fri, 6 Sep 2019 14:21:16 +0200 Subject: [PATCH 07/31] README.md using PHP-DI --- README.md | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 54bb6a8..b26cf09 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,12 @@ as it is middleware, you can also register it for a subset of routes. ### Register for all routes ```php +use DI\Container; +use Slim\Csrf\Guard; +use Slim\Factory\AppFactory; + +require __DIR__ . '/vendor/autoload.php'; + // Start PHP session session_start(); @@ -36,8 +42,8 @@ $app = AppFactory::create(); // Register with container $container = $app->getContainer(); -$container->set('csrf', function (ContainerInterface $container) { - return new \Slim\Csrf\Guard; +$container->set('csrf', function (Container $c) { + return new Guard(); }); // Register middleware for all routes @@ -68,6 +74,12 @@ $app->run(); ### Register per route ```php +use DI\Container; +use Slim\Csrf\Guard; +use Slim\Factory\AppFactory; + +require __DIR__ . '/vendor/autoload.php'; + // Start PHP session session_start(); @@ -79,8 +91,8 @@ $app = AppFactory::create(); // Register with container $container = $app->getContainer(); -$container->set('csrf', function (ContainerInterface $container) { - return new \Slim\Csrf\Guard; +$container->set('csrf', function (Container $c) { + return new Guard(); }); $app = new \Slim\App(); @@ -114,7 +126,7 @@ If you are willing to use `Slim\Csrf\Guard` outside a `Slim\App` or not as a mid // Start PHP session session_start(); -$slimGuard = new \Slim\Csrf\Guard; +$slimGuard = new \Slim\Csrf\Guard(); $slimGuard->validateStorage(); // Generate new tokens From bdf36268101fa2f421c32ea8443b42d85c3a3c56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20B=C3=A9rub=C3=A9?= Date: Sun, 8 Sep 2019 22:53:09 -0600 Subject: [PATCH 08/31] update CI configs and dependencies --- .coveralls.yml | 1 + .editorconfig | 8 ++++++++ .gitattributes | 26 +++++++++++++++++++------- .gitignore | 4 ++-- .travis.yml | 27 +++++++++++++++++++++------ composer.json | 8 ++++---- 6 files changed, 55 insertions(+), 19 deletions(-) create mode 100644 .coveralls.yml create mode 100644 .editorconfig diff --git a/.coveralls.yml b/.coveralls.yml new file mode 100644 index 0000000..a8172fe --- /dev/null +++ b/.coveralls.yml @@ -0,0 +1 @@ +json_path: coveralls-upload.json diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..6342cfc --- /dev/null +++ b/.editorconfig @@ -0,0 +1,8 @@ +# EditorConfig is awesome: http://EditorConfig.org +root = true + +[*] +end_of_line = lf +indent_style = space +indent_size = 4 +trim_trailing_whitespace = true diff --git a/.gitattributes b/.gitattributes index c49fd82..69304c6 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,7 +1,19 @@ -/tests/ export-ignore -/.gitattributes export-ignore -/.gitignore export-ignore -/.travis.yml export-ignore -/CONTRIBUTING.md export-ignore -/README.md export-ignore -/phpunit.xml.dist export-ignore +# Enforce Unix newlines +* text=lf + +# Exclude unused files +# see: https://redd.it/2jzp6k +/.coveralls.yml export-ignore +/.editorconfig export-ignore +/.gitattributes export-ignore +/.github export-ignore +/.gitignore export-ignore +/.travis.yml export-ignore +/CODE_OF_CONDUCT.md export-ignore +/CONTRIBUTING.md export-ignore +/README.md export-ignore +/UPGRADING.md export-ignore +/phpcs.xml export-ignore +/phpstan.neon.dist export-ignore +/phpunit.xml.dist export-ignore +/tests export-ignore diff --git a/.gitignore b/.gitignore index 86d9b11..b644c64 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,4 @@ +.idea composer.lock -phpunit.xml vendor -.idea \ No newline at end of file +coverage diff --git a/.travis.yml b/.travis.yml index 993959f..ed5c58f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,10 +1,25 @@ language: php -php: - - 7.1 - - 7.2 - - 7.3 +dist: trusty -before_script: composer install +matrix: + include: + - php: 7.1 + - php: 7.2 + - php: 7.3 + env: ANALYSIS='true' + - php: nightly -script: vendor/bin/phpunit --coverage-text + allow_failures: + - php: nightly + +before_script: + - if [[ "$ANALYSIS" == 'true' ]]; then composer require php-coveralls/php-coveralls:^2.1.0 ; fi + - composer install -n + +script: + - if [[ "$ANALYSIS" != 'true' ]]; then vendor/bin/phpunit ; fi + - if [[ "$ANALYSIS" == 'true' ]]; then vendor/bin/phpunit --coverage-clover clover.xml ; fi + +after_success: + - if [[ "$ANALYSIS" == 'true' ]]; then vendor/bin/php-coveralls --coverage_clover=clover.xml -v ; fi diff --git a/composer.json b/composer.json index 07f70d5..5ddece5 100644 --- a/composer.json +++ b/composer.json @@ -1,7 +1,7 @@ { "name": "slim/csrf", "type": "library", - "description": "Slim Framework 4 CSRF protection middleware PSR-15", + "description": "Slim Framework 4 CSRF protection PSR-15 middleware", "keywords": ["slim","framework","middleware","csrf"], "homepage": "http://slimframework.com", "license": "MIT", @@ -15,12 +15,12 @@ "require": { "php": "^7.1", "psr/http-message": "^1.0", - "psr/http-server-middleware": "^1.0", - "slim/slim": "^4.0" + "psr/http-server-handler": "^1.0", + "psr/http-server-middleware": "^1.0" }, "require-dev": { "phpunit/phpunit": "^7.5", - "slim/psr7": "^0.5.0" + "phpspec/prophecy": "^1.8" }, "autoload": { "psr-4": { From 2c308de67b304283a4f768a2d5dc12cacf1efae7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20B=C3=83=C2=A9rub=C3=83=C2=A9?= Date: Sun, 8 Sep 2019 22:53:26 -0600 Subject: [PATCH 09/31] update bootstrapping and rename test class --- phpunit.xml.dist | 23 ++++++++--- tests/{CsrfTest.php => GuardTest.php} | 56 +-------------------------- tests/bootstrap.php | 9 ++++- 3 files changed, 27 insertions(+), 61 deletions(-) rename tests/{CsrfTest.php => GuardTest.php} (92%) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index f43c285..1b218ab 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -1,7 +1,11 @@ - - - tests/ + ./tests/ - - - src/ + + ./src/ + + + diff --git a/tests/CsrfTest.php b/tests/GuardTest.php similarity index 92% rename from tests/CsrfTest.php rename to tests/GuardTest.php index 9c839fa..8e34329 100644 --- a/tests/CsrfTest.php +++ b/tests/GuardTest.php @@ -10,61 +10,9 @@ use Psr\Http\Server\RequestHandlerInterface; use Slim\Csrf\Guard; use Slim\MiddlewareDispatcher; -use Slim\Psr7\Environment; -use Slim\Psr7\Factory\ResponseFactory; -use Slim\Psr7\Factory\UriFactory; -use Slim\Psr7\Headers; -use Slim\Psr7\Request; -use Slim\Psr7\Response; -use Slim\Psr7\Stream; - -class CsrfTest extends TestCase -{ - /** - * PSR7 request object - * - * @var \Psr\Http\Message\RequestInterface - */ - protected $request; - - /** - * PSR7 response object - * - * @var \Psr\Http\Message\ResponseInterface - */ - protected $response; - - /** - * @var ResponseFactory - */ - protected $responseFactory; - - /** - * @var MiddlewareDispatcher - */ - protected $middlewareDispatcher; - - /** - * Run before each test - * - * @return void - */ - public function setUp(): void - { - $uri = (new UriFactory())->createUri('https://example.com:443/foo/bar?abc=123'); - $headers = new Headers(); - $cookies = []; - $serverParams = Environment::mock(); - $body = new Stream(fopen('php://temp', 'r+')); - $this->request = new Request('GET', $uri, $headers, $cookies, $serverParams, $body); - - $this->response = new Response(); - - $this->responseFactory = new ResponseFactory(); - - $this->middlewareDispatcher = new MiddlewareDispatcher($this->createMock(RequestHandlerInterface::class)); - } +class GuardTest extends TestCase +{ public function testTokenKeys() { $mw = new Guard('test'); diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 14c26e7..7eae441 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -1,5 +1,12 @@ Date: Sun, 8 Sep 2019 22:53:26 -0600 Subject: [PATCH 10/31] add initial draft for PSR-15 support --- src/Guard.php | 453 ++++++++++++++++++++++++-------------------------- 1 file changed, 217 insertions(+), 236 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index 73ad2d3..4cc1b67 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -1,4 +1,9 @@ responseFactory = $responseFactory; $this->prefix = rtrim($prefix, '_'); + if ($strength < 16) { throw new RuntimeException('CSRF middleware failed. Minimum strength is 16.'); } $this->strength = $strength; - $this->storage = &$storage; - $this->setFailureCallable($failureCallable); + $this->setStorage($storage); + $this->setFailureHandler($failureHandler); $this->setStorageLimit($storageLimit); - $this->setPersistentTokenMode($persistentTokenMode); $this->keyPair = null; } /** - * Retrieve token name key - * - * @return string + * @param null|array|ArrayAccess + * @throws RuntimeException */ - public function getTokenNameKey(): string + public function setStorage(&$storage = null): void { - return $this->prefix . '_name'; + if (is_array($this->storage) || ($this->storage instanceof ArrayAccess)) { + $this->storage = &$storage; + return; + } + + if (!isset($_SESSION)) { + throw new RuntimeException('Invalid CSRF storage. Use session_start()'); + } + + if (!array_key_exists($this->prefix, $_SESSION) || !is_array($_SESSION[$this->prefix])) { + $_SESSION[$this->prefix] = []; + } + + $this->storage = &$_SESSION[$this->prefix]; } /** - * Retrieve token value key + * Setter for failureCallable * - * @return string + * @param mixed $failureHandler Value to set + * @return $this */ - public function getTokenValueKey(): string + public function setFailureHandler($failureHandler): self { - return $this->prefix . '_value'; + $this->failureHandler = $failureHandler; + return $this; } /** - * Invoke middleware - * - * @param ServerRequestInterface $request PSR7 request object - * @param RequestHandlerInterface $handler + * Setter for persistentTokenMode * - * @return ResponseInterface PSR7 response object + * @param bool $persistentTokenMode True to use the same token throughout the session + * (unless there is a validation error), false to get a new token with each request. + * @return $this */ - public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + public function setPersistentTokenMode(bool $persistentTokenMode): self { - $this->validateStorage(); - - // Validate POST, PUT, DELETE, PATCH requests - if (in_array($request->getMethod(), ['POST', 'PUT', 'DELETE', 'PATCH'])) { - $body = $request->getParsedBody(); - $body = $body ? (array)$body : []; - $name = $body[$this->prefix . '_name'] ?? false; - $value = $body[$this->prefix . '_value'] ?? false; - if (!$name || !$value || !$this->validateToken($name, $value)) { - // Need to regenerate a new token, as the validateToken removed the current one. - $request = $this->generateNewToken($request); - - $failureCallable = $this->getFailureCallable(); - return $failureCallable($request, $handler); - } - } - - // Generate new CSRF token if persistentTokenMode is false, or if a valid keyPair has not yet been stored - if (!$this->persistentTokenMode || !$this->loadLastKeyPair()) { - $request = $this->generateNewToken($request); - } elseif ($this->persistentTokenMode) { - $pair = $this->loadLastKeyPair() ? $this->keyPair : $this->generateToken(); - $request = $this->attachRequestAttributes($request, $pair); - } - - // Enforce the storage limit - $this->enforceStorageLimit(); - - return $handler->handle($request); + $this->persistentTokenMode = $persistentTokenMode; + return $this; } /** - * @return mixed + * Setter for storageLimit + * + * @param integer $storageLimit Value to set + * @return $this */ - public function validateStorage() + public function setStorageLimit(int $storageLimit): self { - if (is_array($this->storage)) { - return $this->storage; - } - - if ($this->storage instanceof ArrayAccess) { - return $this->storage; - } - - if (!isset($_SESSION)) { - throw new RuntimeException('CSRF middleware failed. Session not found.'); - } - if (!array_key_exists($this->prefix, $_SESSION) || !\is_array($_SESSION[$this->prefix])) { - $_SESSION[$this->prefix] = []; - } - $this->storage = &$_SESSION[$this->prefix]; - return $this->storage; + $this->storageLimit = (int)$storageLimit; + return $this; } /** - * Generates a new CSRF token + * Generate CSRF Token * + * @return string + * @throws Exception + */ + protected function createToken(): string + { + return bin2hex(random_bytes($this->strength)); + } + + /** * @return array + * + * @throws Exception */ public function generateToken(): array { @@ -218,80 +210,77 @@ public function generateToken(): array return $this->keyPair; } - + /** - * Generates a new CSRF token and attaches it to the Request Object + * Validate CSRF token from current request + * against token value stored in $_SESSION * - * @param ServerRequestInterface $request PSR7 response object. + * @param string|null $name CSRF name + * @param string|null $value CSRF token value * - * @return ServerRequestInterface PSR7 response object. + * @return bool */ - public function generateNewToken(ServerRequestInterface $request): ServerRequestInterface + public function validateToken(?string $name, ?string $value): bool { - $pair = $this->generateToken(); + $valid = false; + + if ($name !== null && isset($this->storage[$name])) { + $token = $this->storage[$name]; - $request = $this->attachRequestAttributes($request, $pair); + if (function_exists('hash_equals')) { + $valid = hash_equals($token, $value); + } else { + $valid = $token === $value; + } + } - return $request; + return $valid; } /** - * Validate CSRF token from current request - * against token value stored in $_SESSION - * - * @param string $name CSRF name - * @param string $value CSRF token value - * - * @return bool + * @return string */ - public function validateToken(string $name, string $value): bool + public function getTokenName(): ?string { - $token = $this->getFromStorage($name); - if (function_exists('hash_equals')) { - $result = ($token !== false && hash_equals($token, $value)); - } else { - $result = ($token !== false && $token === $value); - } - - // If we're not in persistent token mode, delete the token. - if (!$this->persistentTokenMode) { - $this->removeFromStorage($name); - } + return $this->keyPair[$this->getTokenNameKey()] ?? null; + } - return $result; + /** + * @return string + */ + public function getTokenValue(): ?string + { + return $this->keyPair[$this->getTokenValueKey()] ?? null; } /** - * Create CSRF token value + * Retrieve token name key * * @return string */ - protected function createToken(): string + public function getTokenNameKey(): string { - return bin2hex(random_bytes($this->strength)); + return $this->prefix . '_name'; } /** - * Save token to storage + * Retrieve token value key * - * @param string $name CSRF token name - * @param string $value CSRF token value + * @return string */ - protected function saveToStorage(string $name, string $value): void + public function getTokenValueKey(): string { - $this->storage[$name] = $value; + return $this->prefix . '_value'; } /** - * Get token from storage - * - * @param string $name CSRF token name + * Getter for persistentTokenMode * - * @return string|bool CSRF token value or `false` if not present + * @return bool */ - protected function getFromStorage(string $name) + public function getPersistentTokenMode(): bool { - return $this->storage[$name] ?? false; + return $this->persistentTokenMode; } /** @@ -306,18 +295,21 @@ protected function getLastKeyPair(): ?array return null; } - foreach ($this->storage as $name => $value) { - continue; + $name = null; + $value = null; + foreach ($this->storage as $k => $v) { + $name = $k; + $value = $v; } - $keyPair = [ - $this->prefix . '_name' => $name, - $this->prefix . '_value' => $value - ]; - - return $keyPair; + return $name !== null && $value !== null + ? [ + $this->prefix . '_name' => $name, + $this->prefix . '_value' => $value + ] + : null; } - + /** * Load the most recent key pair in storage. * @@ -325,15 +317,21 @@ protected function getLastKeyPair(): ?array */ protected function loadLastKeyPair(): bool { - $this->keyPair = $this->getLastKeyPair(); - - if ($this->keyPair) { - return true; - } + return !!($this->keyPair = $this->getLastKeyPair()); + } - return false; + /** + * Save token to storage + * + * @param string $name CSRF token name + * @param string $value CSRF token value + * @return void + */ + protected function saveToStorage(string $name, string $value): void + { + $this->storage[$name] = $value; } - + /** * Remove token from storage * @@ -354,13 +352,11 @@ protected function removeFromStorage(string $name): void */ protected function enforceStorageLimit(): void { - if ($this->storageLimit < 1) { - return; - } - - // $storage must be an array or implement Countable and Traversable - if (!is_array($this->storage) - && !($this->storage instanceof Countable && $this->storage instanceof Traversable) + if ($this->storageLimit < 1 + || ( + !is_array($this->storage) + && !($this->storage instanceof Countable && $this->storage instanceof Traversable) + ) ) { return; } @@ -369,113 +365,98 @@ protected function enforceStorageLimit(): void while (count($this->storage) > $this->storageLimit) { array_shift($this->storage); } - } else { - // array_shift() doesn't work for ArrayAccess, so we need an iterator in order to use rewind() - // and key(), so that we can then unset - $iterator = $this->storage; - if ($this->storage instanceof IteratorAggregate) { - $iterator = $this->storage->getIterator(); - } + } else if ($this->storage instanceof Iterator) { while (count($this->storage) > $this->storageLimit) { - $iterator->rewind(); - unset($this->storage[$iterator->key()]); + $this->storage->rewind(); + unset($this->storage[$this->storage->key()]); } } } /** * @param ServerRequestInterface $request - * @param $pair * @return ServerRequestInterface - */ - protected function attachRequestAttributes(ServerRequestInterface $request, array $pair): ServerRequestInterface - { - return $request->withAttribute($this->prefix . '_name', $pair[$this->prefix . '_name']) - ->withAttribute($this->prefix . '_value', $pair[$this->prefix . '_value']); - } - - /** - * Getter for failureCallable - * - * @return callable|\Closure - */ - public function getFailureCallable() - { - if (is_null($this->failureCallable)) { - $this->failureCallable = function ( - ServerRequestInterface $request, - RequestHandlerInterface $handler - ): ResponseInterface { - $responseFactory = AppFactory::determineResponseFactory(); - $response = $responseFactory->createResponse(); - $body = $response->getBody(); - $body->write('Failed CSRF check!'); - return $response->withStatus(400)->withHeader('Content-type', 'text/plain')->withBody($body); - }; - } - return $this->failureCallable; - } - - /** - * Setter for failureCallable * - * @param mixed $failureCallable Value to set - * @return $this + * @throws Exception */ - public function setFailureCallable($failureCallable): self + public function appendNewTokenToRequest(ServerRequestInterface $request): ServerRequestInterface { - $this->failureCallable = $failureCallable; - return $this; + $token = $this->generateToken(); + return $this->appendTokenToRequest($request, $token); } /** - * Setter for persistentTokenMode - * - * @param bool $persistentTokenMode True to use the same token throughout the session - * (unless there is a validation error), false to get a new token with each request. - * @return $this + * @param ServerRequestInterface $request + * @param $pair + * @return ServerRequestInterface */ - public function setPersistentTokenMode(bool $persistentTokenMode): self + protected function appendTokenToRequest(ServerRequestInterface $request, array $pair): ServerRequestInterface { - $this->persistentTokenMode = $persistentTokenMode; - return $this; + $name = $this->prefix . '_name'; + $value = $this->prefix . '_value'; + return $request + ->withAttribute($name, $pair[$name]) + ->withAttribute($value, $pair[$value]); } /** - * Setter for storageLimit - * - * @param integer $storageLimit Value to set - * @return $this + * @param ServerRequestInterface $request + * @param RequestHandlerInterface $handler + * @return ResponseInterface + * @throws Exception */ - public function setStorageLimit(int $storageLimit): self + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - $this->storageLimit = (int)$storageLimit; - return $this; - } + if (in_array($request->getMethod(), ['POST', 'PUT', 'DELETE', 'PATCH'])) { + $body = $request->getParsedBody(); + $name = null; + $value = null; - /** - * Getter for persistentTokenMode - * - * @return bool - */ - public function getPersistentTokenMode(): bool - { - return $this->persistentTokenMode; - } + if (is_array($body)) { + $name = $body[$this->prefix . '_name'] ?? null; + $value = $body[$this->prefix . '_value'] ?? null; + } - /** - * @return string - */ - public function getTokenName(): ?string - { - return $this->keyPair[$this->getTokenNameKey()] ?? null; + if (!$this->validateToken($name, $value)) { + if (!$this->persistentTokenMode && is_string($name)) { + $this->removeFromStorage($name); + } + + $request = $this->appendNewTokenToRequest($request); + return $this->handleFailure($request, $handler); + } + } + + // Generate new CSRF token if persistentTokenMode is false, or if a valid keyPair has not yet been stored + if (!$this->persistentTokenMode || !$this->loadLastKeyPair()) { + $request = $this->appendNewTokenToRequest($request); + } elseif ($this->persistentTokenMode) { + $pair = $this->loadLastKeyPair() ? $this->keyPair : $this->generateToken(); + $request = $this->appendTokenToRequest($request, $pair); + } + + $this->enforceStorageLimit(); + + return $handler->handle($request); } /** - * @return string + * @param ServerRequestInterface $request + * @param RequestHandlerInterface $handler + * @return ResponseInterface */ - public function getTokenValue(): ?string + public function handleFailure(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - return $this->keyPair[$this->getTokenValueKey()] ?? null; + if (!is_callable($this->failureHandler)) { + $response = $this->responseFactory->createResponse(); + $body = $response->getBody(); + $body->write('Failed CSRF check!'); + return $response + ->withStatus(400) + ->withHeader('Content-type', 'text/plain') + ->withBody($body); + } + + return call_user_func($this->failureHandler, $request, $handler); } } From d5b9320ab190bafc888ebc7643c46768cb10d655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20B=C3=83=C2=A9rub=C3=83=C2=A9?= Date: Sun, 8 Sep 2019 22:53:26 -0600 Subject: [PATCH 11/31] update examples --- README.md | 79 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index b26cf09..489224a 100644 --- a/README.md +++ b/README.md @@ -18,9 +18,7 @@ Requires Slim 4.0.0 or newer. ## Usage -In most cases you want to register Slim\Csrf for all routes, however, -as it is middleware, you can also register it for a subset of routes. - +In most cases you want to register Slim\Csrf for all routes, however, as it is middleware, you can also register it for a subset of routes. ### Register for all routes @@ -36,19 +34,18 @@ session_start(); // Create Container $container = new Container(); -AppFactory::setContainer($container); +// Create App $app = AppFactory::create(); +$responseFactory = $app->getResponseFactory(); -// Register with container -$container = $app->getContainer(); -$container->set('csrf', function (Container $c) { - return new Guard(); +// Register Middleware On Container +$container->set('csrf', function () use ($responseFactory) { + return new Guard($responseFactory); }); -// Register middleware for all routes -// If you are implementing per-route checks you must not add this -$app->add($container->get('csrf')); +// Register Middleware To Be Executed On All Routes +$app->add('csrf'); $app->get('/foo', function ($request, $response, $args) { // CSRF token name and value @@ -85,19 +82,17 @@ session_start(); // Create Container $container = new Container(); -AppFactory::setContainer($container); +// Create App $app = AppFactory::create(); +$responseFactory = $app->getResponseFactory(); -// Register with container -$container = $app->getContainer(); -$container->set('csrf', function (Container $c) { - return new Guard(); +// Register Middleware On Container +$container->set('csrf', function () use ($responseFactory) { + return new Guard($responseFactory); }); -$app = new \Slim\App(); - -$app->get('/api/myEndPoint',function ($request, $response, $args) { +$app->get('/api/route',function ($request, $response, $args) { $nameKey = $this->csrf->getTokenNameKey(); $valueKey = $this->csrf->getTokenValueKey(); $name = $request->getAttribute($nameKey); @@ -109,11 +104,11 @@ $app->get('/api/myEndPoint',function ($request, $response, $args) { ]; return $response->write(json_encode($tokenArray)); -})->add($container->get('csrf')); +})->add('csrf'); $app->post('/api/myEndPoint',function ($request, $response, $args) { //Do my Things Securely! -})->add($container->get('csrf')); +})->add('csrf'); $app->run(); ``` @@ -123,19 +118,23 @@ $app->run(); If you are willing to use `Slim\Csrf\Guard` outside a `Slim\App` or not as a middleware, be careful to validate the storage: ```php +use Slim\Csrf\Guard; +use Slim\Psr7\Factory\ResponseFactory; + // Start PHP session session_start(); -$slimGuard = new \Slim\Csrf\Guard(); -$slimGuard->validateStorage(); +// Create Middleware +$responseFactory = new ResponseFactory(); // Note that you will need to import +$guard = new Guard($responseFactory); // Generate new tokens -$csrfNameKey = $slimGuard->getTokenNameKey(); -$csrfValueKey = $slimGuard->getTokenValueKey(); -$keyPair = $slimGuard->generateToken(); +$csrfNameKey = $guard->getTokenNameKey(); +$csrfValueKey = $guard->getTokenValueKey(); +$keyPair = $guard->generateToken(); // Validate retrieved tokens -$slimGuard->validateToken($_POST[$csrfNameKey], $_POST[$csrfValueKey]); +$guard->validateToken($_POST[$csrfNameKey], $_POST[$csrfValueKey]); ``` ## Token persistence @@ -144,20 +143,21 @@ By default, `Slim\Csrf\Guard` will generate a fresh name/value pair after each r To use persistent tokens, set the sixth parameter of the constructor to `true`. No matter what, the token will be regenerated after a failed CSRF check. In this case, you will probably want to detect this condition and instruct your users to reload the page in their legitimate browser tab (or automatically reload on the next failed request). - ### Accessing the token pair in templates (Twig, etc) In many situations, you will want to access the token pair without needing to go through the request object. In these cases, you can use `getTokenName()` and `getTokenValue()` directly on the `Guard` middleware instance. This can be useful, for example in a [Twig extension](https://twig.symfony.com/doc/2.x/advanced.html#creating-an-extension): ```php +use Slim\Csrf\Guard; + class CsrfExtension extends \Twig\Extension\AbstractExtension implements \Twig\Extension\GlobalsInterface { /** - * @var \Slim\Csrf\Guard + * @var Guard */ protected $csrf; - public function __construct(\Slim\Csrf\Guard $csrf) + public function __construct(Guard $csrf) { $this->csrf = $csrf; } @@ -198,20 +198,21 @@ By default, `Slim\Csrf\Guard` will return a Response with a 400 status code and a simple plain text error message. To override this, provide a callable as the third parameter to the constructor -or via `setFailureCallable()`. This callable has the same signature as +or via `setFailureHandler()`. This callable has the same signature as middleware: `function($request, $handler)` and must return a Response. For example: ```php -$container->set('csrf', function (ContainerInterface $container) { - $guard = new \Slim\Csrf\Guard(); - $guard->setFailureCallable(function (ServerRequestInterface $request, RequestHandlerInterface $handler) { - $request = $request->withAttribute("csrf_status", false); - return $handler->handle($request); - }); - return $guard; -}; +use Slim\Csrf\Guard; +use Slim\Psr7\Factory\ResponseFactory; + +$responseFactory = new ResponseFactory(); +$guard = new Guard($responseFactory); +$guard->setFailureHandler(function (ServerRequestInterface $request, RequestHandlerInterface $handler) { + $request = $request->withAttribute("csrf_status", false); + return $handler->handle($request); +}); ``` In this example, an attribute is set on the request object that can then be From 18ddba43c2c72fa73361e831feb77850659db6d7 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Mon, 9 Sep 2019 13:43:45 -0600 Subject: [PATCH 12/31] add php codesniffer --- composer.json | 4 +++- phpcs.xml | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 phpcs.xml diff --git a/composer.json b/composer.json index 5ddece5..9bb6d87 100644 --- a/composer.json +++ b/composer.json @@ -14,13 +14,15 @@ ], "require": { "php": "^7.1", + "psr/http-factory": "^1.0", "psr/http-message": "^1.0", "psr/http-server-handler": "^1.0", "psr/http-server-middleware": "^1.0" }, "require-dev": { "phpunit/phpunit": "^7.5", - "phpspec/prophecy": "^1.8" + "phpspec/prophecy": "^1.8", + "squizlabs/php_codesniffer": "^3.4.2" }, "autoload": { "psr-4": { diff --git a/phpcs.xml b/phpcs.xml new file mode 100644 index 0000000..94458ea --- /dev/null +++ b/phpcs.xml @@ -0,0 +1,17 @@ + + + Slim coding standard + + + + + + + + + + + + src + tests + From 193142761f8a68e9bec625b1520bbf814f7bb31d Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Mon, 9 Sep 2019 13:43:59 -0600 Subject: [PATCH 13/31] refactor everything --- src/Guard.php | 94 ++++++++++++++++++++++----------------------------- 1 file changed, 40 insertions(+), 54 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index 4cc1b67..1241145 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -2,7 +2,7 @@ /** * Slim Framework (https://slimframework.com) * - * @license https://github.com/slimphp/Slim/blob/4.x/LICENSE.md (MIT License) + * @license https://github.com/slimphp/Slim-Csrf/blob/master/LICENSE.md (MIT License) */ declare(strict_types=1); @@ -39,8 +39,8 @@ class Guard implements MiddlewareInterface * CSRF storage * * Should be either an array or an object. If an object is used, then it must - * implement ArrayAccess and should implement Countable and Iterator (or - * IteratorAggregate) if storage limit enforcement is required. + * implement ArrayAccess and should implement Countable and Iterator + * if storage limit enforcement is required. * * @var array|ArrayAccess */ @@ -49,8 +49,6 @@ class Guard implements MiddlewareInterface /** * Number of elements to store in the storage array * - * Default is 200, set via constructor - * * @var int */ protected $storageLimit; @@ -109,7 +107,7 @@ public function __construct( $this->prefix = rtrim($prefix, '_'); if ($strength < 16) { - throw new RuntimeException('CSRF middleware failed. Minimum strength is 16.'); + throw new RuntimeException('CSRF middleware instantiation failed. Minimum strength is 16.'); } $this->strength = $strength; @@ -127,13 +125,16 @@ public function __construct( */ public function setStorage(&$storage = null): void { - if (is_array($this->storage) || ($this->storage instanceof ArrayAccess)) { + if (is_array($storage) || ($storage instanceof ArrayAccess)) { $this->storage = &$storage; return; } if (!isset($_SESSION)) { - throw new RuntimeException('Invalid CSRF storage. Use session_start()'); + throw new RuntimeException( + 'Invalid CSRF storage. ' . + 'Use session_start() before instantiating the Guard middleware or provide array storage.' + ); } if (!array_key_exists($this->prefix, $_SESSION) || !is_array($_SESSION[$this->prefix])) { @@ -144,10 +145,8 @@ public function setStorage(&$storage = null): void } /** - * Setter for failureCallable - * * @param mixed $failureHandler Value to set - * @return $this + * @return self */ public function setFailureHandler($failureHandler): self { @@ -156,11 +155,9 @@ public function setFailureHandler($failureHandler): self } /** - * Setter for persistentTokenMode - * * @param bool $persistentTokenMode True to use the same token throughout the session * (unless there is a validation error), false to get a new token with each request. - * @return $this + * @return self */ public function setPersistentTokenMode(bool $persistentTokenMode): self { @@ -169,21 +166,19 @@ public function setPersistentTokenMode(bool $persistentTokenMode): self } /** - * Setter for storageLimit - * * @param integer $storageLimit Value to set + * * @return $this */ public function setStorageLimit(int $storageLimit): self { - $this->storageLimit = (int)$storageLimit; + $this->storageLimit = $storageLimit; return $this; } /** - * Generate CSRF Token - * * @return string + * * @throws Exception */ protected function createToken(): string @@ -201,7 +196,7 @@ public function generateToken(): array // Generate new CSRF token $name = uniqid($this->prefix); $value = $this->createToken(); - $this->saveToStorage($name, $value); + $this->saveTokenToStorage($name, $value); $this->keyPair = [ $this->prefix . '_name' => $name, @@ -212,8 +207,7 @@ public function generateToken(): array } /** - * Validate CSRF token from current request - * against token value stored in $_SESSION + * Validate CSRF token from current request against token value stored in $_SESSION * * @param string|null $name CSRF name * @param string|null $value CSRF token value @@ -254,8 +248,6 @@ public function getTokenValue(): ?string } /** - * Retrieve token name key - * * @return string */ public function getTokenNameKey(): string @@ -264,8 +256,6 @@ public function getTokenNameKey(): string } /** - * Retrieve token value key - * * @return string */ public function getTokenValueKey(): string @@ -274,8 +264,6 @@ public function getTokenValueKey(): string } /** - * Getter for persistentTokenMode - * * @return bool */ public function getPersistentTokenMode(): bool @@ -284,9 +272,7 @@ public function getPersistentTokenMode(): bool } /** - * Get the most recent key pair from storage. - * - * @return string[]|null Array containing name and value if found, null otherwise + * @return string[]|null */ protected function getLastKeyPair(): ?array { @@ -321,13 +307,12 @@ protected function loadLastKeyPair(): bool } /** - * Save token to storage - * * @param string $name CSRF token name * @param string $value CSRF token value + * * @return void */ - protected function saveToStorage(string $name, string $value): void + protected function saveTokenToStorage(string $name, string $value): void { $this->storage[$name] = $value; } @@ -337,9 +322,9 @@ protected function saveToStorage(string $name, string $value): void * * @param string $name CSRF token name */ - protected function removeFromStorage(string $name): void + protected function removeTokenFromStorage(string $name): void { - $this->storage[$name] = ' '; + $this->storage[$name] = ''; unset($this->storage[$name]); } @@ -352,29 +337,27 @@ protected function removeFromStorage(string $name): void */ protected function enforceStorageLimit(): void { - if ($this->storageLimit < 1 - || ( - !is_array($this->storage) - && !($this->storage instanceof Countable && $this->storage instanceof Traversable) + if ($this->storageLimit > 0 + && (is_array($this->storage) + || ($this->storage instanceof Countable && $this->storage instanceof Iterator) ) ) { - return; - } - - if (is_array($this->storage)) { - while (count($this->storage) > $this->storageLimit) { - array_shift($this->storage); - } - } else if ($this->storage instanceof Iterator) { - while (count($this->storage) > $this->storageLimit) { - $this->storage->rewind(); - unset($this->storage[$this->storage->key()]); + if (is_array($this->storage)) { + while (count($this->storage) > $this->storageLimit) { + array_shift($this->storage); + } + } elseif ($this->storage instanceof Iterator) { + while (count($this->storage) > $this->storageLimit) { + $this->storage->rewind(); + unset($this->storage[$this->storage->key()]); + } } } } /** * @param ServerRequestInterface $request + * * @return ServerRequestInterface * * @throws Exception @@ -388,6 +371,7 @@ public function appendNewTokenToRequest(ServerRequestInterface $request): Server /** * @param ServerRequestInterface $request * @param $pair + * * @return ServerRequestInterface */ protected function appendTokenToRequest(ServerRequestInterface $request, array $pair): ServerRequestInterface @@ -402,7 +386,9 @@ protected function appendTokenToRequest(ServerRequestInterface $request, array $ /** * @param ServerRequestInterface $request * @param RequestHandlerInterface $handler + * * @return ResponseInterface + * * @throws Exception */ public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface @@ -419,7 +405,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface if (!$this->validateToken($name, $value)) { if (!$this->persistentTokenMode && is_string($name)) { - $this->removeFromStorage($name); + $this->removeTokenFromStorage($name); } $request = $this->appendNewTokenToRequest($request); @@ -427,7 +413,6 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface } } - // Generate new CSRF token if persistentTokenMode is false, or if a valid keyPair has not yet been stored if (!$this->persistentTokenMode || !$this->loadLastKeyPair()) { $request = $this->appendNewTokenToRequest($request); } elseif ($this->persistentTokenMode) { @@ -443,6 +428,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface /** * @param ServerRequestInterface $request * @param RequestHandlerInterface $handler + * * @return ResponseInterface */ public function handleFailure(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface @@ -453,7 +439,7 @@ public function handleFailure(ServerRequestInterface $request, RequestHandlerInt $body->write('Failed CSRF check!'); return $response ->withStatus(400) - ->withHeader('Content-type', 'text/plain') + ->withHeader('Content-Type', 'text/plain') ->withBody($body); } From 732fe655908a1e278f2acf660c90e0e79e43ec23 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Mon, 9 Sep 2019 13:44:08 -0600 Subject: [PATCH 14/31] rewrite all tests --- tests/Assets/PhpFunctionOverrides.php | 26 + tests/GuardTest.php | 654 +++++++++++--------------- tests/bootstrap.php | 4 +- 3 files changed, 312 insertions(+), 372 deletions(-) create mode 100644 tests/Assets/PhpFunctionOverrides.php diff --git a/tests/Assets/PhpFunctionOverrides.php b/tests/Assets/PhpFunctionOverrides.php new file mode 100644 index 0000000..3576fc0 --- /dev/null +++ b/tests/Assets/PhpFunctionOverrides.php @@ -0,0 +1,26 @@ +prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage, null, 200, 15); + } - $this->assertEquals('test_name', $mw->getTokenNameKey()); - $this->assertEquals('test_value', $mw->getTokenValueKey()); + /** + * @expectedException RuntimeException + * @expectedExceptionMessage Invalid CSRF storage. + * Use session_start() before instantiating the Guard middleware or provide array storage. + */ + public function testSetStorageThrowsExceptionWhenFallingBackOnSessionThatHasNotBeenStarted() + { + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test'); } - public function testTokenGeneration() + /** + * @runInSeparateProcess + */ + public function testSetStorageSetsKeysOnSessionObjectWhenNotExist() { - $storage = []; - $request = $this->request; - $responseFactory = $this->responseFactory; - $mw = new Guard('csrf', $storage); - $mw2 = function ( - ServerRequestInterface $request, - RequestHandlerInterface $handler - ) use ( - $mw, - $responseFactory - ): ResponseInterface { - return $responseFactory->createResponse() - ->withHeader('X-CSRF-NAME', $request->getAttribute($mw->getTokenNameKey())) - ->withHeader('X-CSRF-VALUE', $request->getAttribute($mw->getTokenValueKey())); - }; + session_start(); + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test'); - $this->middlewareDispatcher->addCallable($mw2); - $this->middlewareDispatcher->addMiddleware($mw); - $response1 = $this->middlewareDispatcher->handle($request); - $response2 = $this->middlewareDispatcher->handle($request); - - $this->assertStringStartsWith( - 'csrf', - $response1->getHeaderLine('X-CSRF-NAME'), - 'Name key should start with csrf prefix' - ); - $this->assertStringStartsWith( - 'csrf', - $response2->getHeaderLine('X-CSRF-NAME'), - 'Name key should start with csrf prefix' - ); - - $this->assertNotEquals( - $response1->getHeaderLine('X-CSRF-NAME'), - $response2->getHeaderLine('X-CSRF-NAME'), - 'Generated token names must be unique' - ); - - $this->assertEquals( - 32, - strlen($response1->getHeaderLine('X-CSRF-VALUE')), - 'Length of the generated token value should be double the strength' - ); - $this->assertEquals( - 32, - strlen($response2->getHeaderLine('X-CSRF-VALUE')), - 'Length of the generated token value should be double the strength' - ); - - $this->assertTrue( - ctype_xdigit($response1->getHeaderLine('X-CSRF-VALUE')), - 'Generated token value is not hexadecimal' - ); - $this->assertTrue( - ctype_xdigit($response2->getHeaderLine('X-CSRF-VALUE')), - 'Generated token value is not hexadecimal' - ); + $this->assertArrayHasKey('test', $_SESSION); } - public function testValidToken() + public function testSetFailureHandler() { - $storage = ['csrf_123' => 'xyz']; - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => 'csrf_123', - 'csrf_value' => 'xyz' - ]); - $mw = new Guard('csrf', $storage); - $responseFactory = $this->responseFactory; - $mw2 = function ( - ServerRequestInterface $request, - RequestHandlerInterface $handler - ) use ( - $mw, - $responseFactory - ): ResponseInterface { - return $responseFactory->createResponse(); + $self = $this; + + $storage = []; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); + + $called = 0; + $handler = function () use ($self, &$called) { + $called++; + $responseProphecy = $self->prophesize(ResponseInterface::class); + return $responseProphecy->reveal(); }; + $mw->setFailureHandler($handler); - $this->middlewareDispatcher->addMiddleware($mw); - $this->middlewareDispatcher->addCallable($mw2); - $newResponse = $this->middlewareDispatcher->handle($request); + $requestProphecy = $this->prophesize(ServerRequestInterface::class); + $requestProphecy + ->getMethod() + ->willReturn('POST') + ->shouldBeCalledOnce(); - $this->assertEquals(200, $newResponse->getStatusCode()); - } + $requestProphecy + ->withAttribute(Argument::type('string'), Argument::type('string')) + ->willReturn($requestProphecy->reveal()) + ->shouldBeCalledTimes(2); - public function testInvalidToken() - { - $storage = ['csrf_123' => 'abc']; // <-- Invalid token value - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => 'csrf_123', - 'csrf_value' => 'xyz' - ]); - $mw = new Guard('csrf', $storage); - $this->middlewareDispatcher->addMiddleware($mw); - $newResponse = $this->middlewareDispatcher->handle($request); - - $this->assertEquals(400, $newResponse->getStatusCode()); - } + $requestProphecy + ->getParsedBody() + ->willReturn([]) + ->shouldBeCalledOnce(); - public function testMissingToken() - { - $storage = []; // <-- Missing token name and value - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => 'csrf_123', - 'csrf_value' => 'xyz' - ]); - $mw = new Guard('csrf', $storage); - $this->middlewareDispatcher->addMiddleware($mw); - $newResponse = $this->middlewareDispatcher->handle($request); - - $this->assertEquals(400, $newResponse->getStatusCode()); - } + $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); - public function testExternalStorageOfAnArrayAccessPersists() - { - $storage = new \ArrayObject(); - - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => 'csrf_123', - 'csrf_value' => 'xyz' - ]); - $mw = new Guard('csrf', $storage); - - $this->assertEquals(0, count($storage)); - $this->middlewareDispatcher->addMiddleware($mw); - $newResponse = $this->middlewareDispatcher->handle($request); - $this->assertEquals(1, count($storage)); + $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal()); + $this->assertEquals(1, $called); } - public function testExternalStorageOfAnArrayPersists() + public function testDefaultFailureHandler() { + $streamProphecy = $this->prophesize(StreamInterface::class); + $streamProphecy + ->write('Failed CSRF check!') + ->shouldBeCalledOnce(); + + $responseProphecy = $this->prophesize(ResponseInterface::class); + + $responseProphecy + ->getBody() + ->willReturn($streamProphecy->reveal()) + ->shouldBeCalledOnce(); + + $responseProphecy + ->withStatus(400) + ->willReturn($responseProphecy->reveal()) + ->shouldBeCalledOnce(); + + $responseProphecy + ->withHeader('Content-Type', 'text/plain') + ->willReturn($responseProphecy->reveal()) + ->shouldBeCalledOnce(); + + $responseProphecy + ->withBody($streamProphecy->reveal()) + ->willReturn($responseProphecy->reveal()) + ->shouldBeCalledOnce(); + + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $responseFactoryProphecy + ->createResponse() + ->willReturn($responseProphecy->reveal()); + $storage = []; + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => 'csrf_123', - 'csrf_value' => 'xyz' - ]); - $mw = new Guard('csrf', $storage); - - $this->assertEquals(0, count($storage)); - $this->middlewareDispatcher->addMiddleware($mw); - $newResponse = $this->middlewareDispatcher->handle($request); - $this->assertEquals(1, count($storage)); - } + $requestProphecy = $this->prophesize(ServerRequestInterface::class); + $requestProphecy + ->getMethod() + ->willReturn('POST') + ->shouldBeCalledOnce(); - public function testPersistenceModeTrueBetweenRequestsArray() - { - $storage = []; + $requestProphecy + ->withAttribute(Argument::type('string'), Argument::type('string')) + ->willReturn($requestProphecy->reveal()) + ->shouldBeCalledTimes(2); - $mw = new Guard('csrf', $storage, null, 200, 16, true); - - $responseFactory = $this->responseFactory; - $mw2 = function ( - ServerRequestInterface $request, - RequestHandlerInterface $handler - ) use ( - $mw, - $responseFactory - ): ResponseInterface { - // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); - return $responseFactory->createResponse(); - }; + $requestProphecy + ->getParsedBody() + ->willReturn([]) + ->shouldBeCalledOnce(); - // Token name and value should be null if the storage is empty and middleware has not yet been invoked - $this->assertNull($mw->getTokenName()); - $this->assertNull($mw->getTokenValue()); + $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($this->request); - - // Persistent token name and value have now been generated - $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); - - // Subsequent request will attempt to validate the token - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => $name, - 'csrf_value' => $value - ]); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($request); - - // Token name and value should be the same after subsequent request - $this->assertEquals($name, $mw->getTokenName()); - $this->assertEquals($value, $mw->getTokenValue()); + $response = $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal()); + $this->assertSame($response, $responseProphecy->reveal()); } - public function testPersistenceModeTrueBetweenRequestsArrayAccess() + public function testValidateToken() { - $storage = new \ArrayObject(); - - $mw = new Guard('csrf', $storage, null, 200, 16, true); - - $responseFactory = $this->responseFactory; - $mw2 = function ( - ServerRequestInterface $request, - RequestHandlerInterface $handler - ) use ( - $mw, - $responseFactory - ): ResponseInterface { - // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); - return $responseFactory->createResponse(); - }; + $storage = [ + 'test_name' => 'value' + ]; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); - // Token name and value should be null if the storage is empty and middleware has not yet been invoked - $this->assertNull($mw->getTokenName()); - $this->assertNull($mw->getTokenValue()); + $this->assertTrue($mw->validateToken('test_name', 'value')); + + $GLOBALS['function_exists_return'] = false; - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($this->request); - - // Persistent token name and value have now been generated - $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); - - // Subsequent request will attempt to validate the token - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => $name, - 'csrf_value' => $value - ]); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($request); - - // Token name and value should be the same after subsequent request - $this->assertEquals($name, $mw->getTokenName()); - $this->assertEquals($value, $mw->getTokenValue()); + $this->assertTrue($mw->validateToken('test_name', 'value')); } - public function testPersistenceModeFalseBetweenRequestsArray() + public function testGetTokenNameAndValue() { $storage = []; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); - $mw = new Guard('csrf', $storage); - - $responseFactory = $this->responseFactory; - $mw2 = function ( - ServerRequestInterface $request, - RequestHandlerInterface $handler - ) use ( - $mw, - $responseFactory - ): ResponseInterface { - // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); - return $responseFactory->createResponse(); - }; - - // Token name and value should be null if the storage is empty and middleware has not yet been invoked $this->assertNull($mw->getTokenName()); $this->assertNull($mw->getTokenValue()); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($this->request); - - // First token name and value have now been generated - $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); - - // Subsequent request will attempt to validate the token - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => $name, - 'csrf_value' => $value - ]); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($request); - - // Token name and value should NOT be the same after subsequent request - $this->assertNotEquals($name, $mw->getTokenName()); - $this->assertNotEquals($value, $mw->getTokenValue()); + $loadLastKeyPairMethod = new ReflectionMethod($mw, 'loadLastKeyPair'); + $loadLastKeyPairMethod->setAccessible(true); + $loadLastKeyPairMethod->invoke($mw); + + $storage = [ + 'test_name' => 'value', + ]; + $mw->setStorage($storage); + $loadLastKeyPairMethod->invoke($mw); + + $this->assertEquals('test_name', $mw->getTokenName()); + $this->assertEquals('value', $mw->getTokenValue()); } - public function testPersistenceModeFalseBetweenRequestsArrayAccess() + public function testGetPersistentTokenMode() { - $storage = new \ArrayObject(); - - $mw = new Guard('csrf', $storage); - - $responseFactory = $this->responseFactory; - $mw2 = function ( - ServerRequestInterface $request, - RequestHandlerInterface $handler - ) use ( - $mw, - $responseFactory - ): ResponseInterface { - // Token name and value should be accessible in the middleware as request attributes - $this->assertEquals($mw->getTokenName(), $request->getAttribute('csrf_name')); - $this->assertEquals($mw->getTokenValue(), $request->getAttribute('csrf_value')); - return $responseFactory->createResponse(); - }; - - // Token name and value should be null if the storage is empty and middleware has not yet been invoked - $this->assertNull($mw->getTokenName()); - $this->assertNull($mw->getTokenValue()); + $storage = []; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage, null, 200, 16, true); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($this->request); - - // First token name and value have now been generated - $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); - - // Subsequent request will attempt to validate the token - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => $name, - 'csrf_value' => $value - ]); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($request); - - // Token name and value should NOT be the same after subsequent request - $this->assertNotEquals($name, $mw->getTokenName()); - $this->assertNotEquals($value, $mw->getTokenValue()); + $this->assertTrue($mw->getPersistentTokenMode()); } - public function testUpdateAfterInvalidTokenWithPersistenceModeTrue() + public function testGetTokenNameKeyAndValue() { $storage = []; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); - $mw = new Guard('csrf', $storage, null, 200, 16, true); - - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($this->request); + $this->assertEquals('test_name', $mw->getTokenNameKey()); + $this->assertEquals('test_value', $mw->getTokenValueKey()); + } - // Persistent token name and value have now been generated - $name = $mw->getTokenName(); - $value = $mw->getTokenValue(); + public function testRemoveTokenFromStorage() + { + $storage = [ + 'test_name' => 'value', + ]; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); - // Bad request, token should get updated - $request = $this->request - ->withMethod('POST') - ->withParsedBody([ - 'csrf_name' => 'csrf_123', - 'csrf_value' => 'xyz' - ]); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($request); + $removeTokenFromStorageMethod = new ReflectionMethod($mw, 'removeTokenFromStorage'); + $removeTokenFromStorageMethod->setAccessible(true); + $removeTokenFromStorageMethod->invoke($mw, 'test_name'); - // Token name and value should NOT be the same after subsequent request - $this->assertNotEquals($name, $mw->getTokenName()); - $this->assertNotEquals($value, $mw->getTokenValue()); + $this->assertArrayNotHasKey('test_name', $storage); } - public function testStorageLimitIsEnforcedForObjects() + public function testEnforceStorageLimitWithArray() { - $storage = new \ArrayObject(); - - $request = $this->request; - - $mw = new Guard('csrf', $storage); - $mw->setStorageLimit(2); + $storage = [ + 'test_name' => 'value', + 'test_name2' => 'value2', + ]; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage, null, 1); + + $enforceStorageLimitMethod = new ReflectionMethod($mw, 'enforceStorageLimit'); + $enforceStorageLimitMethod->setAccessible(true); + $enforceStorageLimitMethod->invoke($mw); + + $this->assertArrayNotHasKey('test_name', $storage); + $this->assertArrayHasKey('test_name2', $storage); + } - $this->assertEquals(0, count($storage)); + public function testEnforceStorageLimitWithIterator() + { + $storage = new ArrayIterator([ + 'test_name' => 'value', + 'test_name2' => 'value', + ]); + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage, null, 1); + + $enforceStorageLimitMethod = new ReflectionMethod($mw, 'enforceStorageLimit'); + $enforceStorageLimitMethod->setAccessible(true); + $enforceStorageLimitMethod->invoke($mw); + + $this->assertArrayNotHasKey('test_name', $storage); + $this->assertArrayHasKey('test_name2', $storage); + } - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($request); - $response = $this->middlewareDispatcher->handle($request); - $response = $this->middlewareDispatcher->handle($request); - $this->assertEquals(2, count($storage)); + 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(); + + $requestProphecy + ->withAttribute(Argument::type('string'), Argument::type('string')) + ->willReturn($requestProphecy->reveal()) + ->shouldBeCalledTimes(2); + + $requestProphecy + ->getParsedBody() + ->willReturn([ + 'test_name' => 'test_name123', + 'test_value' => 'invalid_value', + ]) + ->shouldBeCalledOnce(); + + $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); + + $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal()); + $this->assertArrayNotHasKey('test_name123', $storage); } - public function testStorageLimitIsEnforcedForArrays() + public function testProcessAppendsNewTokensWhenPersistentTokenModeIsOff() { $storage = []; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage); - $request = $this->request; + $responseProphecy = $this->prophesize(ResponseInterface::class); - $mw = new Guard('csrf', $storage); - $mw->setStorageLimit(2); + $requestProphecy = $this->prophesize(ServerRequestInterface::class); + $requestProphecy + ->getMethod() + ->willReturn('GET') + ->shouldBeCalledOnce(); - $this->assertEquals(0, count($storage)); + $requestProphecy + ->withAttribute(Argument::type('string'), Argument::type('string')) + ->willReturn($requestProphecy->reveal()) + ->shouldBeCalledTimes(2); - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($request); - $response = $this->middlewareDispatcher->handle($request); - $response = $this->middlewareDispatcher->handle($request); - $this->assertEquals(2, count($storage)); - } + $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); - public function testKeyPair() - { - $mw = new Guard(); - - $this->middlewareDispatcher->addMiddleware($mw); - $response = $this->middlewareDispatcher->handle($this->request); - - $this->assertNotNull($mw->getTokenName()); + $requestHandlerProphecy + ->handle($requestProphecy) + ->willReturn($responseProphecy->reveal()) + ->shouldBeCalledOnce(); - $this->assertNotNull($mw->getTokenValue()); + $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal()); } - public function testDefaultStorageIsSession() + public function testProcessAppendsNewTokensWhenPersistentTokenModeIsOn() { - $sessionBackup = $_SESSION; - $_SESSION = array(); - - $mw = new Guard('csrf'); - $mw->validateStorage(); - - $this->assertNotEmpty($_SESSION); - - $_SESSION = $sessionBackup; + $storage = [ + 'test_name123' => 'test_value123', + ]; + $responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class); + $mw = new Guard($responseFactoryProphecy->reveal(), 'test', $storage, null, 200, 16, true); + + $responseProphecy = $this->prophesize(ResponseInterface::class); + + $requestProphecy = $this->prophesize(ServerRequestInterface::class); + $requestProphecy + ->getMethod() + ->willReturn('GET') + ->shouldBeCalledOnce(); + + $requestProphecy + ->withAttribute('test_name', 'test_name123') + ->willReturn($requestProphecy->reveal()) + ->shouldBeCalledOnce(); + + $requestProphecy + ->withAttribute('test_value', 'test_value123') + ->willReturn($requestProphecy->reveal()) + ->shouldBeCalledOnce(); + + $requestHandlerProphecy = $this->prophesize(RequestHandlerInterface::class); + + $requestHandlerProphecy + ->handle($requestProphecy) + ->willReturn($responseProphecy->reveal()) + ->shouldBeCalledOnce(); + + $mw->process($requestProphecy->reveal(), $requestHandlerProphecy->reveal()); } } diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 7eae441..1661533 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -2,11 +2,11 @@ /** * Slim Framework (https://slimframework.com) * - * @license https://github.com/slimphp/Slim/blob/4.x/LICENSE.md (MIT License) + * @license https://github.com/slimphp/Slim-Csrf/blob/master/LICENSE.md (MIT License) */ declare(strict_types=1); require __DIR__ . '/../vendor/autoload.php'; -session_start(); +require __DIR__ . '/Assets/PhpFunctionOverrides.php'; From bad6da0c87f2fba0d82f18b1ac0ff0e10ab10ea9 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Mon, 9 Sep 2019 13:48:16 -0600 Subject: [PATCH 15/31] add coverage badge --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 489224a..ef8ac89 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,7 @@ # Slim Framework CSRF Protection [![Build Status](https://travis-ci.org/slimphp/Slim-Csrf.svg?branch=master)](https://travis-ci.org/slimphp/Slim-Csrf) +[![Coverage Status](https://coveralls.io/repos/github/slimphp/Slim-Csrf/badge.svg?branch=master)](https://coveralls.io/github/slimphp/Slim-Csrf?branch=master) This repository contains a Slim Framework CSRF protection PSR-15 middleware. CSRF protection applies to all unsafe HTTP requests (POST, PUT, DELETE, PATCH). From dee100bb4ddb33885eaa704dbdeff65785c2ca5e Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:38:37 -0600 Subject: [PATCH 16/31] add nullable type hint to failure handler --- src/Guard.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index 1241145..294eae7 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -98,7 +98,7 @@ public function __construct( ResponseFactoryInterface $responseFactory, string $prefix = 'csrf', &$storage = null, - callable $failureHandler = null, + ?callable $failureHandler = null, int $storageLimit = 200, int $strength = 16, bool $persistentTokenMode = false @@ -145,10 +145,10 @@ public function setStorage(&$storage = null): void } /** - * @param mixed $failureHandler Value to set + * @param callable|null $failureHandler Value to set * @return self */ - public function setFailureHandler($failureHandler): self + public function setFailureHandler(?callable $failureHandler): self { $this->failureHandler = $failureHandler; return $this; From 3e4a28a939ec6c0112971b5d4535034ee1a8eb85 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:20 -0600 Subject: [PATCH 17/31] add return self to setStorage method --- src/Guard.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index 294eae7..60911e4 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -121,9 +121,12 @@ public function __construct( /** * @param null|array|ArrayAccess + * + * @return self + * * @throws RuntimeException */ - public function setStorage(&$storage = null): void + public function setStorage(&$storage = null): self { if (is_array($storage) || ($storage instanceof ArrayAccess)) { $this->storage = &$storage; @@ -142,6 +145,7 @@ public function setStorage(&$storage = null): void } $this->storage = &$_SESSION[$this->prefix]; + return $this; } /** From 5a5dc7eb0352ec3bacab4c7235bcf9e661b22cd3 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 18/31] remove unused import --- src/Guard.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index 60911e4..597e223 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -19,7 +19,6 @@ use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use RuntimeException; -use Traversable; class Guard implements MiddlewareInterface { From 9564be3a38f438bba11635094476087129555f24 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 19/31] replace token keys and values to use helper for one source of truth --- src/Guard.php | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index 597e223..5427999 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -129,7 +129,7 @@ public function setStorage(&$storage = null): self { if (is_array($storage) || ($storage instanceof ArrayAccess)) { $this->storage = &$storage; - return; + return $this; } if (!isset($_SESSION)) { @@ -201,9 +201,10 @@ public function generateToken(): array $value = $this->createToken(); $this->saveTokenToStorage($name, $value); + $this->keyPair = [ - $this->prefix . '_name' => $name, - $this->prefix . '_value' => $value + $this->getTokenNameKey() => $name, + $this->getTokenValueKey() => $value ]; return $this->keyPair; @@ -293,8 +294,8 @@ protected function getLastKeyPair(): ?array return $name !== null && $value !== null ? [ - $this->prefix . '_name' => $name, - $this->prefix . '_value' => $value + $this->getTokenNameKey() => $name, + $this->getTokenValueKey() => $value ] : null; } @@ -379,8 +380,8 @@ public function appendNewTokenToRequest(ServerRequestInterface $request): Server */ protected function appendTokenToRequest(ServerRequestInterface $request, array $pair): ServerRequestInterface { - $name = $this->prefix . '_name'; - $value = $this->prefix . '_value'; + $name = $this->getTokenNameKey(); + $value = $this->getTokenValueKey(); return $request ->withAttribute($name, $pair[$name]) ->withAttribute($value, $pair[$value]); @@ -402,8 +403,8 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $value = null; if (is_array($body)) { - $name = $body[$this->prefix . '_name'] ?? null; - $value = $body[$this->prefix . '_value'] ?? null; + $name = $body[$this->getTokenNameKey()] ?? null; + $value = $body[$this->getTokenValueKey()] ?? null; } if (!$this->validateToken($name, $value)) { From 0b4af3b3718a38b3e5ecbf9a47abd3e261bf2acd Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 20/31] add AppFactory::setContainer($container) to examples --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index ef8ac89..77f3afe 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,7 @@ session_start(); // Create Container $container = new Container(); +AppFactory::setContainer($container); // Create App $app = AppFactory::create(); @@ -83,6 +84,7 @@ session_start(); // Create Container $container = new Container(); +AppFactory::setContainer($container); // Create App $app = AppFactory::create(); From f865fce50ad93453ff2b1ef7ef14bd0eb172d0aa Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 21/31] fix docblock for setStorage method --- src/Guard.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index 5427999..d19e62a 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -119,7 +119,7 @@ public function __construct( } /** - * @param null|array|ArrayAccess + * @param null|array|ArrayAccess $storage * * @return self * From cbf1a318248e38a18d21f83c6345bdb94ee4572f Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 22/31] fix appendTokenToRequest() docbloc $pair variable type hinting --- src/Guard.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index d19e62a..62a27a5 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -374,7 +374,7 @@ public function appendNewTokenToRequest(ServerRequestInterface $request): Server /** * @param ServerRequestInterface $request - * @param $pair + * @param array $pair * * @return ServerRequestInterface */ From fee65e2716e2a7fe1dc62161bd6a56d308a4c964 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 23/31] fix comment in validateToken() method docblock --- src/Guard.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index 62a27a5..e59bb51 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -211,7 +211,8 @@ public function generateToken(): array } /** - * Validate CSRF token from current request against token value stored in $_SESSION + * Validate CSRF token from current request against token value + * stored in $_SESSION or user provided storage * * @param string|null $name CSRF name * @param string|null $value CSRF token value From 211badec1b7b1948aceabc15c660768cc41e5248 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 24/31] typehint validateToken to require parameters --- src/Guard.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index e59bb51..54ae52a 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -214,16 +214,16 @@ public function generateToken(): array * Validate CSRF token from current request against token value * stored in $_SESSION or user provided storage * - * @param string|null $name CSRF name - * @param string|null $value CSRF token value + * @param string $name CSRF name + * @param string $value CSRF token value * * @return bool */ - public function validateToken(?string $name, ?string $value): bool + public function validateToken(string $name, string $value): bool { $valid = false; - if ($name !== null && isset($this->storage[$name])) { + if (isset($this->storage[$name])) { $token = $this->storage[$name]; if (function_exists('hash_equals')) { @@ -408,7 +408,10 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $value = $body[$this->getTokenValueKey()] ?? null; } - if (!$this->validateToken($name, $value)) { + if ($name === null + || $value === null + || !$this->validateToken((string) $name, (string) $value) + ) { if (!$this->persistentTokenMode && is_string($name)) { $this->removeTokenFromStorage($name); } From 1d21df1490749a2cec0889500e3a3a65b9e592ea Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 25/31] optimize getLastKeyPair --- src/Guard.php | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index 54ae52a..b66b2b0 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -285,13 +285,11 @@ protected function getLastKeyPair(): ?array if (count($this->storage) < 1) { return null; } - - $name = null; - $value = null; - foreach ($this->storage as $k => $v) { - $name = $k; - $value = $v; - } + + end($this->storage); + $name = key($this->storage); + $value = $this->storage[$name]; + reset($this->storage); return $name !== null && $value !== null ? [ From e12acc679c94c948036cdce589501345a0a80a8f Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 10:39:32 -0600 Subject: [PATCH 26/31] optimize loadLastKeyPair() --- src/Guard.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index b66b2b0..7761329 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -285,7 +285,7 @@ protected function getLastKeyPair(): ?array if (count($this->storage) < 1) { return null; } - + end($this->storage); $name = key($this->storage); $value = $this->storage[$name]; @@ -306,7 +306,7 @@ protected function getLastKeyPair(): ?array */ protected function loadLastKeyPair(): bool { - return !!($this->keyPair = $this->getLastKeyPair()); + return (bool) $this->keyPair = $this->getLastKeyPair(); } /** From 6facb50d3c8aaca02a6761c31c67d7e3bf53c9d2 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 12:24:25 -0600 Subject: [PATCH 27/31] re-order imports --- src/Guard.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index 7761329..65b03e0 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -14,8 +14,8 @@ use Exception; use Iterator; use Psr\Http\Message\ResponseFactoryInterface; -use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use RuntimeException; From 2feed8e3fb579f91980eaf7194c791832fea06c5 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 12:25:09 -0600 Subject: [PATCH 28/31] fix $failureHandler type hint --- src/Guard.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index 65b03e0..cf05fe1 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -63,7 +63,7 @@ class Guard implements MiddlewareInterface * Callable to be executed if the CSRF validation fails * It must return a ResponseInterface * - * @var callable + * @var callable|null */ protected $failureHandler; From 7ee477cca6cb679952f99098f42f60e05d8928d9 Mon Sep 17 00:00:00 2001 From: l0gicgate Date: Tue, 10 Sep 2019 12:27:34 -0600 Subject: [PATCH 29/31] fix always true elseif statement --- src/Guard.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Guard.php b/src/Guard.php index cf05fe1..82bb79e 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -421,7 +421,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface if (!$this->persistentTokenMode || !$this->loadLastKeyPair()) { $request = $this->appendNewTokenToRequest($request); - } elseif ($this->persistentTokenMode) { + } else { $pair = $this->loadLastKeyPair() ? $this->keyPair : $this->generateToken(); $request = $this->appendTokenToRequest($request, $pair); } From 3e48e7994dd5dc0fa5a3d4a07f04679662ca2838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20B=C3=A9rub=C3=A9?= Date: Thu, 12 Sep 2019 22:53:36 -0600 Subject: [PATCH 30/31] change setStorage logic to use session_status() --- src/Guard.php | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/Guard.php b/src/Guard.php index 82bb79e..f62c36f 100644 --- a/src/Guard.php +++ b/src/Guard.php @@ -127,12 +127,17 @@ public function __construct( */ public function setStorage(&$storage = null): self { - if (is_array($storage) || ($storage instanceof ArrayAccess)) { + if (is_array($storage) + || ($storage instanceof ArrayAccess + && $storage instanceof Countable + && $storage instanceof Iterator + ) + ) { $this->storage = &$storage; return $this; } - if (!isset($_SESSION)) { + if (session_status() !== PHP_SESSION_ACTIVE) { throw new RuntimeException( 'Invalid CSRF storage. ' . 'Use session_start() before instantiating the Guard middleware or provide array storage.' @@ -281,8 +286,10 @@ public function getPersistentTokenMode(): bool */ protected function getLastKeyPair(): ?array { - // Use count, since empty ArrayAccess objects can still return false for `empty` - if (count($this->storage) < 1) { + if ( + (is_array($this->storage) && empty($this->storage)) + || ($this->storage instanceof Countable && count($this->storage) < 1) + ) { return null; } From 7afb074aa9b1ce59f668622fc4530a848389285c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pierre=20B=C3=A9rub=C3=A9?= Date: Thu, 12 Sep 2019 22:59:13 -0600 Subject: [PATCH 31/31] fix readme to use container interface `get()` method instead of pimple synthax --- README.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 77f3afe..036eca4 100644 --- a/README.md +++ b/README.md @@ -51,8 +51,9 @@ $app->add('csrf'); $app->get('/foo', function ($request, $response, $args) { // CSRF token name and value - $nameKey = $this->csrf->getTokenNameKey(); - $valueKey = $this->csrf->getTokenValueKey(); + $csrf = $this->get('csrf'); + $nameKey = $csrf->getTokenNameKey(); + $valueKey = $csrf->getTokenValueKey(); $name = $request->getAttribute($nameKey); $value = $request->getAttribute($valueKey); @@ -96,8 +97,9 @@ $container->set('csrf', function () use ($responseFactory) { }); $app->get('/api/route',function ($request, $response, $args) { - $nameKey = $this->csrf->getTokenNameKey(); - $valueKey = $this->csrf->getTokenValueKey(); + $csrf = $this->get('csrf'); + $nameKey = $csrf->getTokenNameKey(); + $valueKey = $csrf->getTokenValueKey(); $name = $request->getAttribute($nameKey); $value = $request->getAttribute($valueKey);