From 4c309d2ad75a00341fe1465b3cfb7de11e7257f1 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Tue, 4 Jun 2019 23:50:38 +0200 Subject: [PATCH 01/10] Integration tests: Memoize request handler as well This is useful to send HTTP requests (or their PSR-7 equivalents) through the entire application's middleware stack (instead of talking to specific controllers, which should be considered implementation detail). --- tests/integration/TestCase.php | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/tests/integration/TestCase.php b/tests/integration/TestCase.php index 9d199564e..8785f8669 100644 --- a/tests/integration/TestCase.php +++ b/tests/integration/TestCase.php @@ -24,27 +24,36 @@ abstract class TestCase extends \PHPUnit\Framework\TestCase $this->app(); } + /** + * @var \Flarum\Foundation\InstalledApp + */ protected $app; + /** + * @var \Psr\Http\Server\RequestHandlerInterface + */ + protected $server; + /** * @return \Flarum\Foundation\InstalledApp */ protected function app() { - if (! is_null($this->app)) { - return $this->app; + if (is_null($this->app)) { + $site = new InstalledSite( + [ + 'base' => __DIR__.'/tmp', + 'public' => __DIR__.'/tmp/public', + 'storage' => __DIR__.'/tmp/storage', + ], + include __DIR__.'/tmp/config.php' + ); + + $this->app = $site->bootApp(); + $this->server = $this->app->getRequestHandler(); } - $site = new InstalledSite( - [ - 'base' => __DIR__.'/tmp', - 'public' => __DIR__.'/tmp/public', - 'storage' => __DIR__.'/tmp/storage', - ], - include __DIR__.'/tmp/config.php' - ); - - return $this->app = $site->bootApp(); + return $this->app; } protected $database; From 8061bfd74aba080cbc1ac4f92f3c094f2fba2b7a Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Wed, 12 Jun 2019 23:46:15 +0200 Subject: [PATCH 02/10] Integration tests: Configure vendor path Now that this is possible, make the easy change... --- tests/integration/TestCase.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/TestCase.php b/tests/integration/TestCase.php index 8785f8669..2d295d11d 100644 --- a/tests/integration/TestCase.php +++ b/tests/integration/TestCase.php @@ -43,6 +43,7 @@ abstract class TestCase extends \PHPUnit\Framework\TestCase $site = new InstalledSite( [ 'base' => __DIR__.'/tmp', + 'vendor' => __DIR__.'/../../vendor', 'public' => __DIR__.'/tmp/public', 'storage' => __DIR__.'/tmp/storage', ], From e927254e99b5bcdbf5621017c40de5545fe477fb Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Sun, 9 Jun 2019 00:22:22 +0200 Subject: [PATCH 03/10] Add tests for CSRF token check --- .../csrf_protection/RequireCsrfTokenTest.php | 181 ++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 tests/integration/api/csrf_protection/RequireCsrfTokenTest.php diff --git a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php new file mode 100644 index 000000000..fc518a588 --- /dev/null +++ b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php @@ -0,0 +1,181 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Flarum\Tests\integration\api\csrf_protection; + +use Dflydev\FigCookies\SetCookie; +use Flarum\Foundation\Application; +use Flarum\Tests\integration\RetrievesAuthorizedUsers; +use Flarum\Tests\integration\TestCase; +use Zend\Diactoros\CallbackStream; +use Zend\Diactoros\ServerRequest; + +class RequireCsrfTokenTest extends TestCase +{ + use RetrievesAuthorizedUsers; + + public function setUp() + { + parent::setUp(); + + $this->prepareDatabase([ + 'users' => [ + $this->adminUser(), + ], + 'groups' => [ + $this->adminGroup(), + ], + 'group_user' => [ + ['user_id' => 1, 'group_id' => 1], + ], + 'group_permission' => [ + ['permission' => 'viewUserList', 'group_id' => 3], + ], + 'access_tokens' => [ + ['user_id' => 1, 'token' => 'superadmin', 'lifetime_seconds' => 30], + ], + 'settings' => [ + ['key' => 'mail_driver', 'value' => 'smtp'], + ['key' => 'version', 'value' => Application::VERSION], + ], + ]); + } + + /** + * @test + */ + public function error_when_doing_cookie_auth_without_csrf_token() + { + $auth = $this->server->handle( + (new ServerRequest([], [], '/login', 'POST')) + ->withBody(new CallbackStream(function () { + return '{"identification": "admin", "password": "password"}'; + })) + ->withHeader('Content-Type', 'application/json') + ); + + $cookies = array_reduce( + $auth->getHeader('Set-Cookie'), + function ($memo, $setCookieString) { + $setCookie = SetCookie::fromSetCookieString($setCookieString); + $memo[$setCookie->getName()] = $setCookie->getValue(); + return $memo; + }, + [] + ); + + $response = $this->server->handle( + (new ServerRequest([], [], '/api/settings', 'POST')) + ->withBody(new CallbackStream(function () { + return '{"mail_driver": "log"}'; + })) + ->withCookieParams($cookies) + ->withHeader('Content-Type', 'application/json') + ); + + // Response should be "HTTP 400 Bad Request" + $this->assertEquals(400, $response->getStatusCode()); + + // The response body should contain proper error details + $body = (string) $response->getBody(); + $this->assertJson($body); + $this->assertEquals([ + 'errors' => [ + ['status' => '400', 'code' => 'csrf_token_mismatch'], + ], + ], json_decode($body, true)); + } + + /** + * @test + */ + public function cookie_auth_succeeds_with_csrf_token() + { + $initial = $this->server->handle( + (new ServerRequest([], [], '/', 'GET')) + ); + + $token = $initial->getHeaderLine('X-CSRF-Token'); + $cookies = array_reduce( + $initial->getHeader('Set-Cookie'), + function ($memo, $setCookieString) { + $setCookie = SetCookie::fromSetCookieString($setCookieString); + $memo[$setCookie->getName()] = $setCookie->getValue(); + return $memo; + }, + [] + ); + + $auth = $this->server->handle( + (new ServerRequest([], [], '/login', 'POST')) + ->withBody(new CallbackStream(function () { + return '{"identification": "admin", "password": "password"}'; + })) + ->withCookieParams($cookies) + ->withHeader('Content-Type', 'application/json') + ->withHeader('X-CSRF-Token', $token) + ); + + $token = $auth->getHeaderLine('X-CSRF-Token'); + $cookies = array_reduce( + $auth->getHeader('Set-Cookie'), + function ($memo, $setCookieString) { + $setCookie = SetCookie::fromSetCookieString($setCookieString); + $memo[$setCookie->getName()] = $setCookie->getValue(); + return $memo; + }, + [] + ); + + $response = $this->server->handle( + (new ServerRequest([], [], '/api/settings', 'POST')) + ->withBody(new CallbackStream(function () { + return '{"mail_driver": "log"}'; + })) + ->withCookieParams($cookies) + ->withHeader('Content-Type', 'application/json') + ->withHeader('X-CSRF-Token', $token) + ); + + // Successful response? + $this->assertEquals(204, $response->getStatusCode()); + + // Was the setting actually changed in the database? + $this->assertEquals( + 'log', + $this->database()->table('settings')->where('key', 'mail_driver')->first()->value + ); + } + + /** + * @test + */ + public function master_api_token_does_not_need_csrf_token() + { + $response = $this->server->handle( + (new ServerRequest([], [], '/api/settings', 'POST')) + ->withBody(new CallbackStream(function () { + return '{"mail_driver": "log"}'; + })) + ->withHeader('Authorization', 'Token superadmin') + ->withHeader('Content-Type', 'application/json') + ); + + // Successful response? + $this->assertEquals(204, $response->getStatusCode()); + + // Was the setting actually changed in the database? + $this->assertEquals( + 'log', + $this->database()->table('settings')->where('key', 'mail_driver')->first()->value + ); + } +} From 5d64056e89ba95f3d4fe704fdaa6450b8f152158 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Sun, 9 Jun 2019 00:25:26 +0200 Subject: [PATCH 04/10] Implement middleware for CSRF token verification This fixes a rather large oversight in Flarum's codebase, which was that we had no explicit CSRF protection using the traditional token approach. The JS frontend was actually sending these tokens, but the backend did not require them. --- src/Admin/AdminServiceProvider.php | 1 + src/Api/ApiServiceProvider.php | 1 + src/Forum/ForumServiceProvider.php | 1 + .../Middleware/AuthenticateWithHeader.php | 2 + src/Http/Middleware/CheckCsrfToken.php | 46 +++++++++++++++++++ 5 files changed, 51 insertions(+) create mode 100644 src/Http/Middleware/CheckCsrfToken.php diff --git a/src/Admin/AdminServiceProvider.php b/src/Admin/AdminServiceProvider.php index 5e61729bd..493464ba9 100644 --- a/src/Admin/AdminServiceProvider.php +++ b/src/Admin/AdminServiceProvider.php @@ -61,6 +61,7 @@ class AdminServiceProvider extends AbstractServiceProvider $pipe->pipe($app->make(HttpMiddleware\StartSession::class)); $pipe->pipe($app->make(HttpMiddleware\RememberFromCookie::class)); $pipe->pipe($app->make(HttpMiddleware\AuthenticateWithSession::class)); + $pipe->pipe($app->make(HttpMiddleware\CheckCsrfToken::class)); $pipe->pipe($app->make(HttpMiddleware\SetLocale::class)); $pipe->pipe($app->make(Middleware\RequireAdministrateAbility::class)); diff --git a/src/Api/ApiServiceProvider.php b/src/Api/ApiServiceProvider.php index eb077259c..dd8eb6cd9 100644 --- a/src/Api/ApiServiceProvider.php +++ b/src/Api/ApiServiceProvider.php @@ -57,6 +57,7 @@ class ApiServiceProvider extends AbstractServiceProvider $pipe->pipe($app->make(HttpMiddleware\RememberFromCookie::class)); $pipe->pipe($app->make(HttpMiddleware\AuthenticateWithSession::class)); $pipe->pipe($app->make(HttpMiddleware\AuthenticateWithHeader::class)); + $pipe->pipe($app->make(HttpMiddleware\CheckCsrfToken::class)); $pipe->pipe($app->make(HttpMiddleware\SetLocale::class)); event(new ConfigureMiddleware($pipe, 'api')); diff --git a/src/Forum/ForumServiceProvider.php b/src/Forum/ForumServiceProvider.php index cdb4e0e48..56edb7132 100644 --- a/src/Forum/ForumServiceProvider.php +++ b/src/Forum/ForumServiceProvider.php @@ -68,6 +68,7 @@ class ForumServiceProvider extends AbstractServiceProvider $pipe->pipe($app->make(HttpMiddleware\StartSession::class)); $pipe->pipe($app->make(HttpMiddleware\RememberFromCookie::class)); $pipe->pipe($app->make(HttpMiddleware\AuthenticateWithSession::class)); + $pipe->pipe($app->make(HttpMiddleware\CheckCsrfToken::class)); $pipe->pipe($app->make(HttpMiddleware\SetLocale::class)); $pipe->pipe($app->make(HttpMiddleware\ShareErrorsFromSession::class)); diff --git a/src/Http/Middleware/AuthenticateWithHeader.php b/src/Http/Middleware/AuthenticateWithHeader.php index 787470c90..87f5bb51b 100644 --- a/src/Http/Middleware/AuthenticateWithHeader.php +++ b/src/Http/Middleware/AuthenticateWithHeader.php @@ -44,6 +44,8 @@ class AuthenticateWithHeader implements Middleware $token->touch(); $actor = $token->user; + + $request = $request->withAttribute('bypassCsrfToken', true); } if (isset($actor)) { diff --git a/src/Http/Middleware/CheckCsrfToken.php b/src/Http/Middleware/CheckCsrfToken.php new file mode 100644 index 000000000..d3efe18fd --- /dev/null +++ b/src/Http/Middleware/CheckCsrfToken.php @@ -0,0 +1,46 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Flarum\Http\Middleware; + +use Flarum\Http\Exception\TokenMismatchException; +use Psr\Http\Message\ResponseInterface as Response; +use Psr\Http\Message\ServerRequestInterface as Request; +use Psr\Http\Server\MiddlewareInterface as Middleware; +use Psr\Http\Server\RequestHandlerInterface as Handler; + +class CheckCsrfToken implements Middleware +{ + public function process(Request $request, Handler $handler): Response + { + if (in_array($request->getMethod(), ['GET', 'HEAD', 'OPTIONS'])) { + return $handler->handle($request); + } + + if ($request->getAttribute('bypassCsrfToken', false)) { + return $handler->handle($request); + } + + if ($this->tokensMatch($request)) { + return $handler->handle($request); + } + + throw new TokenMismatchException('CSRF token did not match'); + } + + private function tokensMatch(Request $request): bool + { + $expected = (string) $request->getAttribute('session')->token(); + $provided = $request->getHeaderLine('X-CSRF-Token'); // TODO: Use form field, if provided + + return hash_equals($expected, $provided); + } +} From 7f7484e790bbd9ae032e1e79e0859cfe73a9f3ab Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Wed, 12 Jun 2019 22:14:36 +0200 Subject: [PATCH 05/10] Accept CSRF token in request body as well --- src/Http/Middleware/CheckCsrfToken.php | 3 +- .../csrf_protection/RequireCsrfTokenTest.php | 61 ++++++++++++++++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/Http/Middleware/CheckCsrfToken.php b/src/Http/Middleware/CheckCsrfToken.php index d3efe18fd..0d22ba340 100644 --- a/src/Http/Middleware/CheckCsrfToken.php +++ b/src/Http/Middleware/CheckCsrfToken.php @@ -39,7 +39,8 @@ class CheckCsrfToken implements Middleware private function tokensMatch(Request $request): bool { $expected = (string) $request->getAttribute('session')->token(); - $provided = $request->getHeaderLine('X-CSRF-Token'); // TODO: Use form field, if provided + $provided = $request->getParsedBody()['csrfToken'] ?? + $request->getHeaderLine('X-CSRF-Token'); return hash_equals($expected, $provided); } diff --git a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index fc518a588..e0cc6755d 100644 --- a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php @@ -97,7 +97,7 @@ class RequireCsrfTokenTest extends TestCase /** * @test */ - public function cookie_auth_succeeds_with_csrf_token() + public function cookie_auth_succeeds_with_csrf_token_in_header() { $initial = $this->server->handle( (new ServerRequest([], [], '/', 'GET')) @@ -155,6 +155,65 @@ class RequireCsrfTokenTest extends TestCase ); } + /** + * @test + */ + public function cookie_auth_succeeds_with_csrf_token_in_body() + { + $initial = $this->server->handle( + (new ServerRequest([], [], '/', 'GET')) + ); + + $token = $initial->getHeaderLine('X-CSRF-Token'); + $cookies = array_reduce( + $initial->getHeader('Set-Cookie'), + function ($memo, $setCookieString) { + $setCookie = SetCookie::fromSetCookieString($setCookieString); + $memo[$setCookie->getName()] = $setCookie->getValue(); + return $memo; + }, + [] + ); + + $auth = $this->server->handle( + (new ServerRequest([], [], '/login', 'POST')) + ->withBody(new CallbackStream(function () use ($token) { + return '{"identification": "admin", "password": "password", "csrfToken": "'.$token.'"}'; + })) + ->withCookieParams($cookies) + ->withHeader('Content-Type', 'application/json') + ); + + $token = $auth->getHeaderLine('X-CSRF-Token'); + $cookies = array_reduce( + $auth->getHeader('Set-Cookie'), + function ($memo, $setCookieString) { + $setCookie = SetCookie::fromSetCookieString($setCookieString); + $memo[$setCookie->getName()] = $setCookie->getValue(); + return $memo; + }, + [] + ); + + $response = $this->server->handle( + (new ServerRequest([], [], '/api/settings', 'POST')) + ->withBody(new CallbackStream(function () use ($token) { + return '{"mail_driver": "log", "csrfToken": "'.$token.'"}'; + })) + ->withCookieParams($cookies) + ->withHeader('Content-Type', 'application/json') + ); + + // Successful response? + $this->assertEquals(204, $response->getStatusCode()); + + // Was the setting actually changed in the database? + $this->assertEquals( + 'log', + $this->database()->table('settings')->where('key', 'mail_driver')->first()->value + ); + } + /** * @test */ From c126b954516f4376c4aa6aad12c3c69e9bdd27de Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Wed, 12 Jun 2019 22:57:38 +0200 Subject: [PATCH 06/10] Refactor tests to shorten HTTP requests Multiple tests now provide JSON request bodies, and others copy cookies from previous responses, so let's provide convenient helpers for these. --- tests/integration/TestCase.php | 70 ++++++++ .../csrf_protection/RequireCsrfTokenTest.php | 161 +++++++----------- 2 files changed, 128 insertions(+), 103 deletions(-) diff --git a/tests/integration/TestCase.php b/tests/integration/TestCase.php index 2d295d11d..9fa641df0 100644 --- a/tests/integration/TestCase.php +++ b/tests/integration/TestCase.php @@ -11,8 +11,13 @@ namespace Flarum\Tests\integration; +use Dflydev\FigCookies\SetCookie; use Flarum\Foundation\InstalledSite; use Illuminate\Database\ConnectionInterface; +use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\ServerRequestInterface; +use Zend\Diactoros\CallbackStream; +use Zend\Diactoros\ServerRequest; abstract class TestCase extends \PHPUnit\Framework\TestCase { @@ -88,4 +93,69 @@ abstract class TestCase extends \PHPUnit\Framework\TestCase // And finally, turn on foreign key checks again. $this->database()->getSchemaBuilder()->enableForeignKeyConstraints(); } + + /** + * Send a full HTTP request through Flarum's middleware stack. + */ + protected function send(ServerRequestInterface $request): ResponseInterface + { + return $this->server->handle($request); + } + + /** + * Build a HTTP request that can be passed through middleware. + * + * This method simplifies building HTTP request for use in our HTTP-level + * integration tests. It provides options for all features repeatedly being + * used in those tests. + * + * @param string $method + * @param string $path + * @param array $options + * An array of optional request properties. + * Currently supported: + * - "json" should point to a JSON-serializable object that will be + * serialized and used as request body. The corresponding Content-Type + * header will be set automatically. + * - "cookiesFrom" should hold a response object from a previous HTTP + * interaction. All cookies returned from the server in that response + * (via the "Set-Cookie" header) will be copied to the cookie params of + * the new request. + * @return ServerRequestInterface + */ + protected function request(string $method, string $path, array $options = []): ServerRequestInterface + { + $request = new ServerRequest([], [], $path, $method); + + // Do we want a JSON request body? + if (isset($options['json'])) { + $request = $request + ->withHeader('Content-Type', 'application/json') + ->withBody( + new CallbackStream(function () use ($options) { + return json_encode($options['json']); + }) + ); + } + + // Let's copy the cookies from a previous response + if (isset($options['cookiesFrom'])) { + /** @var ResponseInterface $previousResponse */ + $previousResponse = $options['cookiesFrom']; + + $cookies = array_reduce( + $previousResponse->getHeader('Set-Cookie'), + function ($memo, $setCookieString) { + $setCookie = SetCookie::fromSetCookieString($setCookieString); + $memo[$setCookie->getName()] = $setCookie->getValue(); + return $memo; + }, + [] + ); + + $request = $request->withCookieParams($cookies); + } + + return $request; + } } diff --git a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index e0cc6755d..a3cbf4dd6 100644 --- a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php @@ -11,12 +11,9 @@ namespace Flarum\Tests\integration\api\csrf_protection; -use Dflydev\FigCookies\SetCookie; use Flarum\Foundation\Application; use Flarum\Tests\integration\RetrievesAuthorizedUsers; use Flarum\Tests\integration\TestCase; -use Zend\Diactoros\CallbackStream; -use Zend\Diactoros\ServerRequest; class RequireCsrfTokenTest extends TestCase { @@ -54,31 +51,23 @@ class RequireCsrfTokenTest extends TestCase */ public function error_when_doing_cookie_auth_without_csrf_token() { - $auth = $this->server->handle( - (new ServerRequest([], [], '/login', 'POST')) - ->withBody(new CallbackStream(function () { - return '{"identification": "admin", "password": "password"}'; - })) - ->withHeader('Content-Type', 'application/json') + $auth = $this->send( + $this->request( + 'POST', '/login', + [ + 'json' => ['identification' => 'admin', 'password' => 'password'], + ] + ) ); - $cookies = array_reduce( - $auth->getHeader('Set-Cookie'), - function ($memo, $setCookieString) { - $setCookie = SetCookie::fromSetCookieString($setCookieString); - $memo[$setCookie->getName()] = $setCookie->getValue(); - return $memo; - }, - [] - ); - - $response = $this->server->handle( - (new ServerRequest([], [], '/api/settings', 'POST')) - ->withBody(new CallbackStream(function () { - return '{"mail_driver": "log"}'; - })) - ->withCookieParams($cookies) - ->withHeader('Content-Type', 'application/json') + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'cookiesFrom' => $auth, + 'json' => ['mail_driver' => 'log'], + ] + ) ); // Response should be "HTTP 400 Bad Request" @@ -99,50 +88,32 @@ class RequireCsrfTokenTest extends TestCase */ public function cookie_auth_succeeds_with_csrf_token_in_header() { - $initial = $this->server->handle( - (new ServerRequest([], [], '/', 'GET')) + $initial = $this->send( + $this->request('GET', '/') ); $token = $initial->getHeaderLine('X-CSRF-Token'); - $cookies = array_reduce( - $initial->getHeader('Set-Cookie'), - function ($memo, $setCookieString) { - $setCookie = SetCookie::fromSetCookieString($setCookieString); - $memo[$setCookie->getName()] = $setCookie->getValue(); - return $memo; - }, - [] - ); - $auth = $this->server->handle( - (new ServerRequest([], [], '/login', 'POST')) - ->withBody(new CallbackStream(function () { - return '{"identification": "admin", "password": "password"}'; - })) - ->withCookieParams($cookies) - ->withHeader('Content-Type', 'application/json') - ->withHeader('X-CSRF-Token', $token) + $auth = $this->send( + $this->request( + 'POST', '/login', + [ + 'cookiesFrom' => $initial, + 'json' => ['identification' => 'admin', 'password' => 'password'], + ] + )->withHeader('X-CSRF-Token', $token) ); $token = $auth->getHeaderLine('X-CSRF-Token'); - $cookies = array_reduce( - $auth->getHeader('Set-Cookie'), - function ($memo, $setCookieString) { - $setCookie = SetCookie::fromSetCookieString($setCookieString); - $memo[$setCookie->getName()] = $setCookie->getValue(); - return $memo; - }, - [] - ); - $response = $this->server->handle( - (new ServerRequest([], [], '/api/settings', 'POST')) - ->withBody(new CallbackStream(function () { - return '{"mail_driver": "log"}'; - })) - ->withCookieParams($cookies) - ->withHeader('Content-Type', 'application/json') - ->withHeader('X-CSRF-Token', $token) + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'cookiesFrom' => $auth, + 'json' => ['mail_driver' => 'log'], + ] + )->withHeader('X-CSRF-Token', $token) ); // Successful response? @@ -160,48 +131,32 @@ class RequireCsrfTokenTest extends TestCase */ public function cookie_auth_succeeds_with_csrf_token_in_body() { - $initial = $this->server->handle( - (new ServerRequest([], [], '/', 'GET')) + $initial = $this->send( + $this->request('GET', '/') ); $token = $initial->getHeaderLine('X-CSRF-Token'); - $cookies = array_reduce( - $initial->getHeader('Set-Cookie'), - function ($memo, $setCookieString) { - $setCookie = SetCookie::fromSetCookieString($setCookieString); - $memo[$setCookie->getName()] = $setCookie->getValue(); - return $memo; - }, - [] - ); - $auth = $this->server->handle( - (new ServerRequest([], [], '/login', 'POST')) - ->withBody(new CallbackStream(function () use ($token) { - return '{"identification": "admin", "password": "password", "csrfToken": "'.$token.'"}'; - })) - ->withCookieParams($cookies) - ->withHeader('Content-Type', 'application/json') + $auth = $this->send( + $this->request( + 'POST', '/login', + [ + 'cookiesFrom' => $initial, + 'json' => ['identification' => 'admin', 'password' => 'password', 'csrfToken' => $token], + ] + ) ); $token = $auth->getHeaderLine('X-CSRF-Token'); - $cookies = array_reduce( - $auth->getHeader('Set-Cookie'), - function ($memo, $setCookieString) { - $setCookie = SetCookie::fromSetCookieString($setCookieString); - $memo[$setCookie->getName()] = $setCookie->getValue(); - return $memo; - }, - [] - ); - $response = $this->server->handle( - (new ServerRequest([], [], '/api/settings', 'POST')) - ->withBody(new CallbackStream(function () use ($token) { - return '{"mail_driver": "log", "csrfToken": "'.$token.'"}'; - })) - ->withCookieParams($cookies) - ->withHeader('Content-Type', 'application/json') + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'cookiesFrom' => $auth, + 'json' => ['mail_driver' => 'log', 'csrfToken' => $token], + ] + ) ); // Successful response? @@ -219,13 +174,13 @@ class RequireCsrfTokenTest extends TestCase */ public function master_api_token_does_not_need_csrf_token() { - $response = $this->server->handle( - (new ServerRequest([], [], '/api/settings', 'POST')) - ->withBody(new CallbackStream(function () { - return '{"mail_driver": "log"}'; - })) - ->withHeader('Authorization', 'Token superadmin') - ->withHeader('Content-Type', 'application/json') + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'json' => ['mail_driver' => 'log'], + ] + )->withHeader('Authorization', 'Token superadmin') ); // Successful response? From bfd3a667dd223ea33ff37cc55721def4b2a8e9fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Klabbers?= Date: Tue, 18 Jun 2019 17:22:23 +0200 Subject: [PATCH 07/10] Fixed issue with tmp/storage/views not existing, this caused tmpname to notice. Fixed csrf test that assumed an access token allows application access, which is actually api token. Improved return type hinting in the StartSession middleware --- src/Http/Middleware/AuthenticateWithHeader.php | 3 +-- src/Http/Middleware/CheckCsrfToken.php | 1 + src/Http/Middleware/StartSession.php | 8 ++++---- .../api/csrf_protection/RequireCsrfTokenTest.php | 6 +++--- tests/integration/tmp/storage/.gitkeep | 0 5 files changed, 9 insertions(+), 9 deletions(-) delete mode 100644 tests/integration/tmp/storage/.gitkeep diff --git a/src/Http/Middleware/AuthenticateWithHeader.php b/src/Http/Middleware/AuthenticateWithHeader.php index 87f5bb51b..512f90101 100644 --- a/src/Http/Middleware/AuthenticateWithHeader.php +++ b/src/Http/Middleware/AuthenticateWithHeader.php @@ -40,12 +40,11 @@ class AuthenticateWithHeader implements Middleware $request = $request->withAttribute('apiKey', $key); $request = $request->withAttribute('bypassFloodgate', true); + $request = $request->withAttribute('bypassCsrfToken', true); } elseif ($token = AccessToken::find($id)) { $token->touch(); $actor = $token->user; - - $request = $request->withAttribute('bypassCsrfToken', true); } if (isset($actor)) { diff --git a/src/Http/Middleware/CheckCsrfToken.php b/src/Http/Middleware/CheckCsrfToken.php index 0d22ba340..d2b2d6da8 100644 --- a/src/Http/Middleware/CheckCsrfToken.php +++ b/src/Http/Middleware/CheckCsrfToken.php @@ -39,6 +39,7 @@ class CheckCsrfToken implements Middleware private function tokensMatch(Request $request): bool { $expected = (string) $request->getAttribute('session')->token(); + $provided = $request->getParsedBody()['csrfToken'] ?? $request->getHeaderLine('X-CSRF-Token'); diff --git a/src/Http/Middleware/StartSession.php b/src/Http/Middleware/StartSession.php index f5aee1573..4b4678b11 100644 --- a/src/Http/Middleware/StartSession.php +++ b/src/Http/Middleware/StartSession.php @@ -67,7 +67,7 @@ class StartSession implements Middleware return $this->withSessionCookie($response, $session); } - private function makeSession(Request $request) + private function makeSession(Request $request): Store { return new Store( $this->config['cookie'], @@ -76,12 +76,12 @@ class StartSession implements Middleware ); } - private function withCsrfTokenHeader(Response $response, Session $session) + private function withCsrfTokenHeader(Response $response, Session $session): Response { return $response->withHeader('X-CSRF-Token', $session->token()); } - private function withSessionCookie(Response $response, Session $session) + private function withSessionCookie(Response $response, Session $session): Response { return FigResponseCookies::set( $response, @@ -89,7 +89,7 @@ class StartSession implements Middleware ); } - private function getSessionLifetimeInSeconds() + private function getSessionLifetimeInSeconds(): int { return $this->config['lifetime'] * 60; } diff --git a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index a3cbf4dd6..83980c6e1 100644 --- a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php @@ -36,11 +36,11 @@ class RequireCsrfTokenTest extends TestCase 'group_permission' => [ ['permission' => 'viewUserList', 'group_id' => 3], ], - 'access_tokens' => [ - ['user_id' => 1, 'token' => 'superadmin', 'lifetime_seconds' => 30], + 'api_keys' => [ + ['user_id' => 1, 'key' => 'superadmin'], ], 'settings' => [ - ['key' => 'mail_driver', 'value' => 'smtp'], + ['key' => 'mail_driver', 'value' => 'mail'], ['key' => 'version', 'value' => Application::VERSION], ], ]); diff --git a/tests/integration/tmp/storage/.gitkeep b/tests/integration/tmp/storage/.gitkeep deleted file mode 100644 index e69de29bb..000000000 From ce42b5e035b56b201e2d0fc715fdae0784f089d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Klabbers?= Date: Tue, 18 Jun 2019 17:45:29 +0200 Subject: [PATCH 08/10] Using a different setting key now, so that it won't break tests whenever you re-run them once smtp is set. Fixed, badly, the test to create users etc caused by the prepareDatabase flushing all settings by default. --- tests/integration/TestCase.php | 18 ++++++++++++-- .../Controller/CreateUserControllerTest.php | 3 +++ .../csrf_protection/RequireCsrfTokenTest.php | 24 +++++++++---------- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/tests/integration/TestCase.php b/tests/integration/TestCase.php index 9fa641df0..e705a8967 100644 --- a/tests/integration/TestCase.php +++ b/tests/integration/TestCase.php @@ -82,12 +82,26 @@ abstract class TestCase extends \PHPUnit\Framework\TestCase // First, truncate all referenced tables so that they are empty. foreach (array_keys($tableData) as $table) { - $this->database()->table($table)->truncate(); + if ($table !== 'settings') { + $this->database()->table($table)->truncate(); + } } // Then, insert all rows required for this test case. foreach ($tableData as $table => $rows) { - $this->database()->table($table)->insert($rows); + foreach ($rows as $row) { + if ($table === 'settings') { + $this->database()->table($table)->updateOrInsert( + ['key' => $row['key']], + $row + ); + } else { + $this->database()->table($table)->updateOrInsert( + isset($row['id']) ? ['id' => $row['id']] : $row, + $row + ); + } + } } // And finally, turn on foreign key checks again. diff --git a/tests/integration/api/Controller/CreateUserControllerTest.php b/tests/integration/api/Controller/CreateUserControllerTest.php index ee9979aa8..c5f8e80b0 100644 --- a/tests/integration/api/Controller/CreateUserControllerTest.php +++ b/tests/integration/api/Controller/CreateUserControllerTest.php @@ -40,6 +40,9 @@ class CreateUserControllerTest extends ApiControllerTestCase 'group_user' => [ ['user_id' => 1, 'group_id' => 1], ], + 'settings' => [ + ['key' => 'mail_driver', 'value' => 'log'] + ] ]); } diff --git a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index 83980c6e1..d52ec519c 100644 --- a/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php @@ -11,7 +11,6 @@ namespace Flarum\Tests\integration\api\csrf_protection; -use Flarum\Foundation\Application; use Flarum\Tests\integration\RetrievesAuthorizedUsers; use Flarum\Tests\integration\TestCase; @@ -40,8 +39,7 @@ class RequireCsrfTokenTest extends TestCase ['user_id' => 1, 'key' => 'superadmin'], ], 'settings' => [ - ['key' => 'mail_driver', 'value' => 'mail'], - ['key' => 'version', 'value' => Application::VERSION], + ['key' => 'csrf_test', 'value' => 1], ], ]); } @@ -65,7 +63,7 @@ class RequireCsrfTokenTest extends TestCase 'POST', '/api/settings', [ 'cookiesFrom' => $auth, - 'json' => ['mail_driver' => 'log'], + 'json' => ['csrf_test' => 2], ] ) ); @@ -111,7 +109,7 @@ class RequireCsrfTokenTest extends TestCase 'POST', '/api/settings', [ 'cookiesFrom' => $auth, - 'json' => ['mail_driver' => 'log'], + 'json' => ['csrf_test' => 2], ] )->withHeader('X-CSRF-Token', $token) ); @@ -121,8 +119,8 @@ class RequireCsrfTokenTest extends TestCase // Was the setting actually changed in the database? $this->assertEquals( - 'log', - $this->database()->table('settings')->where('key', 'mail_driver')->first()->value + 2, + $this->database()->table('settings')->where('key', 'csrf_test')->first()->value ); } @@ -154,7 +152,7 @@ class RequireCsrfTokenTest extends TestCase 'POST', '/api/settings', [ 'cookiesFrom' => $auth, - 'json' => ['mail_driver' => 'log', 'csrfToken' => $token], + 'json' => ['csrf_test' => 2, 'csrfToken' => $token], ] ) ); @@ -164,8 +162,8 @@ class RequireCsrfTokenTest extends TestCase // Was the setting actually changed in the database? $this->assertEquals( - 'log', - $this->database()->table('settings')->where('key', 'mail_driver')->first()->value + 2, + $this->database()->table('settings')->where('key', 'csrf_test')->first()->value ); } @@ -178,7 +176,7 @@ class RequireCsrfTokenTest extends TestCase $this->request( 'POST', '/api/settings', [ - 'json' => ['mail_driver' => 'log'], + 'json' => ['csrf_test' => 2], ] )->withHeader('Authorization', 'Token superadmin') ); @@ -188,8 +186,8 @@ class RequireCsrfTokenTest extends TestCase // Was the setting actually changed in the database? $this->assertEquals( - 'log', - $this->database()->table('settings')->where('key', 'mail_driver')->first()->value + 2, + $this->database()->table('settings')->where('key', 'csrf_test')->first()->value ); } } From fd667229450606c489838b1218dbd46bb7a9bd5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Klabbers?= Date: Sat, 22 Jun 2019 19:40:20 +0200 Subject: [PATCH 09/10] added custom view, now needs translation --- src/Http/Exception/TokenMismatchException.php | 4 ++++ views/error/419.blade.php | 12 ++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 views/error/419.blade.php diff --git a/src/Http/Exception/TokenMismatchException.php b/src/Http/Exception/TokenMismatchException.php index 49da0abaa..87871937a 100644 --- a/src/Http/Exception/TokenMismatchException.php +++ b/src/Http/Exception/TokenMismatchException.php @@ -15,4 +15,8 @@ use Exception; class TokenMismatchException extends Exception { + public function __construct($message = null, $code = 419, Exception $previous = null) + { + parent::__construct($message, $code, $previous); + } } diff --git a/views/error/419.blade.php b/views/error/419.blade.php new file mode 100644 index 000000000..a2282323d --- /dev/null +++ b/views/error/419.blade.php @@ -0,0 +1,12 @@ +@extends('flarum.forum::layouts.basic') + +@section('content') +

+ {{ $message }} +

+

+ + {{ $translator->trans('core.views.error.419_return_link', ['{forum}' => $settings->get('forum_title')]) }} + +

+@endsection From 935a9682570f3b50536f96b31046125a79b6c9fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20Klabbers?= Date: Mon, 24 Jun 2019 13:00:36 +0200 Subject: [PATCH 10/10] fixed tests on master, missing views directory and suppressing notices from tempnam when storing files in tmp --- src/Frontend/Compiler/JsCompiler.php | 2 +- tests/integration/tmp/storage/views/.gitkeep | 0 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 tests/integration/tmp/storage/views/.gitkeep diff --git a/src/Frontend/Compiler/JsCompiler.php b/src/Frontend/Compiler/JsCompiler.php index 5ff680f29..66b3d0b94 100644 --- a/src/Frontend/Compiler/JsCompiler.php +++ b/src/Frontend/Compiler/JsCompiler.php @@ -58,7 +58,7 @@ class JsCompiler extends RevisionCompiler $this->assetsDir->put($file, implode("\n", $output)); - $mapTemp = tempnam(storage_path('tmp'), $mapFile); + $mapTemp = @tempnam(storage_path('tmp'), $mapFile); $map->save($mapTemp); $this->assetsDir->put($mapFile, file_get_contents($mapTemp)); @unlink($mapTemp); diff --git a/tests/integration/tmp/storage/views/.gitkeep b/tests/integration/tmp/storage/views/.gitkeep new file mode 100644 index 000000000..e69de29bb