From a7259bbd5f52c407a0690b332e90e92ad14a716e 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). --- framework/core/tests/integration/TestCase.php | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/framework/core/tests/integration/TestCase.php b/framework/core/tests/integration/TestCase.php index 9d199564e..8785f8669 100644 --- a/framework/core/tests/integration/TestCase.php +++ b/framework/core/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 53cc505037c8b47fb98524e22b62ae3cf1948329 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... --- framework/core/tests/integration/TestCase.php | 1 + 1 file changed, 1 insertion(+) diff --git a/framework/core/tests/integration/TestCase.php b/framework/core/tests/integration/TestCase.php index 8785f8669..2d295d11d 100644 --- a/framework/core/tests/integration/TestCase.php +++ b/framework/core/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 69fdd82ffcda5912e3a6b3538532d0be1fc136f8 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 framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php diff --git a/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php new file mode 100644 index 000000000..fc518a588 --- /dev/null +++ b/framework/core/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 aa43d1475e6a6b225bcadcb9b34b05351e184f79 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. --- .../core/src/Admin/AdminServiceProvider.php | 1 + framework/core/src/Api/ApiServiceProvider.php | 1 + .../core/src/Forum/ForumServiceProvider.php | 1 + .../Middleware/AuthenticateWithHeader.php | 2 + .../src/Http/Middleware/CheckCsrfToken.php | 46 +++++++++++++++++++ 5 files changed, 51 insertions(+) create mode 100644 framework/core/src/Http/Middleware/CheckCsrfToken.php diff --git a/framework/core/src/Admin/AdminServiceProvider.php b/framework/core/src/Admin/AdminServiceProvider.php index 5e61729bd..493464ba9 100644 --- a/framework/core/src/Admin/AdminServiceProvider.php +++ b/framework/core/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/framework/core/src/Api/ApiServiceProvider.php b/framework/core/src/Api/ApiServiceProvider.php index eb077259c..dd8eb6cd9 100644 --- a/framework/core/src/Api/ApiServiceProvider.php +++ b/framework/core/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/framework/core/src/Forum/ForumServiceProvider.php b/framework/core/src/Forum/ForumServiceProvider.php index cdb4e0e48..56edb7132 100644 --- a/framework/core/src/Forum/ForumServiceProvider.php +++ b/framework/core/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/framework/core/src/Http/Middleware/AuthenticateWithHeader.php b/framework/core/src/Http/Middleware/AuthenticateWithHeader.php index 787470c90..87f5bb51b 100644 --- a/framework/core/src/Http/Middleware/AuthenticateWithHeader.php +++ b/framework/core/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/framework/core/src/Http/Middleware/CheckCsrfToken.php b/framework/core/src/Http/Middleware/CheckCsrfToken.php new file mode 100644 index 000000000..d3efe18fd --- /dev/null +++ b/framework/core/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 3899cd84874974619e58a6252763652bcf7b3a04 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/framework/core/src/Http/Middleware/CheckCsrfToken.php b/framework/core/src/Http/Middleware/CheckCsrfToken.php index d3efe18fd..0d22ba340 100644 --- a/framework/core/src/Http/Middleware/CheckCsrfToken.php +++ b/framework/core/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/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index fc518a588..e0cc6755d 100644 --- a/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/framework/core/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 953cae0de1f0e1f23457c7e9e8385b77c4ef0de6 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. --- framework/core/tests/integration/TestCase.php | 70 ++++++++ .../csrf_protection/RequireCsrfTokenTest.php | 161 +++++++----------- 2 files changed, 128 insertions(+), 103 deletions(-) diff --git a/framework/core/tests/integration/TestCase.php b/framework/core/tests/integration/TestCase.php index 2d295d11d..9fa641df0 100644 --- a/framework/core/tests/integration/TestCase.php +++ b/framework/core/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/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index e0cc6755d..a3cbf4dd6 100644 --- a/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/framework/core/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 b69b24eea6fc50017d1ab410ed2bb063fa35e6bc 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 --- .../core/src/Http/Middleware/AuthenticateWithHeader.php | 3 +-- framework/core/src/Http/Middleware/CheckCsrfToken.php | 1 + framework/core/src/Http/Middleware/StartSession.php | 8 ++++---- .../api/csrf_protection/RequireCsrfTokenTest.php | 6 +++--- framework/core/tests/integration/tmp/storage/.gitkeep | 0 5 files changed, 9 insertions(+), 9 deletions(-) delete mode 100644 framework/core/tests/integration/tmp/storage/.gitkeep diff --git a/framework/core/src/Http/Middleware/AuthenticateWithHeader.php b/framework/core/src/Http/Middleware/AuthenticateWithHeader.php index 87f5bb51b..512f90101 100644 --- a/framework/core/src/Http/Middleware/AuthenticateWithHeader.php +++ b/framework/core/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/framework/core/src/Http/Middleware/CheckCsrfToken.php b/framework/core/src/Http/Middleware/CheckCsrfToken.php index 0d22ba340..d2b2d6da8 100644 --- a/framework/core/src/Http/Middleware/CheckCsrfToken.php +++ b/framework/core/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/framework/core/src/Http/Middleware/StartSession.php b/framework/core/src/Http/Middleware/StartSession.php index f5aee1573..4b4678b11 100644 --- a/framework/core/src/Http/Middleware/StartSession.php +++ b/framework/core/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/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index a3cbf4dd6..83980c6e1 100644 --- a/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/framework/core/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/framework/core/tests/integration/tmp/storage/.gitkeep b/framework/core/tests/integration/tmp/storage/.gitkeep deleted file mode 100644 index e69de29bb..000000000 From 304e36ca22811192f339a79e21fe656e0e262bee 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. --- framework/core/tests/integration/TestCase.php | 18 ++++++++++++-- .../Controller/CreateUserControllerTest.php | 3 +++ .../csrf_protection/RequireCsrfTokenTest.php | 24 +++++++++---------- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/framework/core/tests/integration/TestCase.php b/framework/core/tests/integration/TestCase.php index 9fa641df0..e705a8967 100644 --- a/framework/core/tests/integration/TestCase.php +++ b/framework/core/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/framework/core/tests/integration/api/Controller/CreateUserControllerTest.php b/framework/core/tests/integration/api/Controller/CreateUserControllerTest.php index ee9979aa8..c5f8e80b0 100644 --- a/framework/core/tests/integration/api/Controller/CreateUserControllerTest.php +++ b/framework/core/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/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php b/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php index 83980c6e1..d52ec519c 100644 --- a/framework/core/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php +++ b/framework/core/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 f49564b5482e0c90f6e718c00fa11fdd00598f3f 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 ++++ framework/core/views/error/419.blade.php | 12 ++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 framework/core/views/error/419.blade.php diff --git a/framework/core/src/Http/Exception/TokenMismatchException.php b/framework/core/src/Http/Exception/TokenMismatchException.php index 49da0abaa..87871937a 100644 --- a/framework/core/src/Http/Exception/TokenMismatchException.php +++ b/framework/core/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/framework/core/views/error/419.blade.php b/framework/core/views/error/419.blade.php new file mode 100644 index 000000000..a2282323d --- /dev/null +++ b/framework/core/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 c443aa09e37c2a7e3e7839a37d951b6e0590468e 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 --- framework/core/src/Frontend/Compiler/JsCompiler.php | 2 +- framework/core/tests/integration/tmp/storage/views/.gitkeep | 0 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 framework/core/tests/integration/tmp/storage/views/.gitkeep diff --git a/framework/core/src/Frontend/Compiler/JsCompiler.php b/framework/core/src/Frontend/Compiler/JsCompiler.php index 5ff680f29..66b3d0b94 100644 --- a/framework/core/src/Frontend/Compiler/JsCompiler.php +++ b/framework/core/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/framework/core/tests/integration/tmp/storage/views/.gitkeep b/framework/core/tests/integration/tmp/storage/views/.gitkeep new file mode 100644 index 000000000..e69de29bb