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/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/src/Http/Middleware/AuthenticateWithHeader.php b/src/Http/Middleware/AuthenticateWithHeader.php index 787470c90..512f90101 100644 --- a/src/Http/Middleware/AuthenticateWithHeader.php +++ b/src/Http/Middleware/AuthenticateWithHeader.php @@ -40,6 +40,7 @@ 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(); diff --git a/src/Http/Middleware/CheckCsrfToken.php b/src/Http/Middleware/CheckCsrfToken.php new file mode 100644 index 000000000..d2b2d6da8 --- /dev/null +++ b/src/Http/Middleware/CheckCsrfToken.php @@ -0,0 +1,48 @@ + + * + * 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->getParsedBody()['csrfToken'] ?? + $request->getHeaderLine('X-CSRF-Token'); + + return hash_equals($expected, $provided); + } +} 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/TestCase.php b/tests/integration/TestCase.php index 9d199564e..e705a8967 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 { @@ -24,27 +29,37 @@ 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', + 'vendor' => __DIR__.'/../../vendor', + '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; @@ -67,15 +82,94 @@ 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. $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/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 new file mode 100644 index 000000000..d52ec519c --- /dev/null +++ b/tests/integration/api/csrf_protection/RequireCsrfTokenTest.php @@ -0,0 +1,193 @@ + + * + * 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 Flarum\Tests\integration\RetrievesAuthorizedUsers; +use Flarum\Tests\integration\TestCase; + +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], + ], + 'api_keys' => [ + ['user_id' => 1, 'key' => 'superadmin'], + ], + 'settings' => [ + ['key' => 'csrf_test', 'value' => 1], + ], + ]); + } + + /** + * @test + */ + public function error_when_doing_cookie_auth_without_csrf_token() + { + $auth = $this->send( + $this->request( + 'POST', '/login', + [ + 'json' => ['identification' => 'admin', 'password' => 'password'], + ] + ) + ); + + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'cookiesFrom' => $auth, + 'json' => ['csrf_test' => 2], + ] + ) + ); + + // 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_in_header() + { + $initial = $this->send( + $this->request('GET', '/') + ); + + $token = $initial->getHeaderLine('X-CSRF-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'); + + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'cookiesFrom' => $auth, + 'json' => ['csrf_test' => 2], + ] + )->withHeader('X-CSRF-Token', $token) + ); + + // Successful response? + $this->assertEquals(204, $response->getStatusCode()); + + // Was the setting actually changed in the database? + $this->assertEquals( + 2, + $this->database()->table('settings')->where('key', 'csrf_test')->first()->value + ); + } + + /** + * @test + */ + public function cookie_auth_succeeds_with_csrf_token_in_body() + { + $initial = $this->send( + $this->request('GET', '/') + ); + + $token = $initial->getHeaderLine('X-CSRF-Token'); + + $auth = $this->send( + $this->request( + 'POST', '/login', + [ + 'cookiesFrom' => $initial, + 'json' => ['identification' => 'admin', 'password' => 'password', 'csrfToken' => $token], + ] + ) + ); + + $token = $auth->getHeaderLine('X-CSRF-Token'); + + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'cookiesFrom' => $auth, + 'json' => ['csrf_test' => 2, 'csrfToken' => $token], + ] + ) + ); + + // Successful response? + $this->assertEquals(204, $response->getStatusCode()); + + // Was the setting actually changed in the database? + $this->assertEquals( + 2, + $this->database()->table('settings')->where('key', 'csrf_test')->first()->value + ); + } + + /** + * @test + */ + public function master_api_token_does_not_need_csrf_token() + { + $response = $this->send( + $this->request( + 'POST', '/api/settings', + [ + 'json' => ['csrf_test' => 2], + ] + )->withHeader('Authorization', 'Token superadmin') + ); + + // Successful response? + $this->assertEquals(204, $response->getStatusCode()); + + // Was the setting actually changed in the database? + $this->assertEquals( + 2, + $this->database()->table('settings')->where('key', 'csrf_test')->first()->value + ); + } +} diff --git a/tests/integration/tmp/storage/.gitkeep b/tests/integration/tmp/storage/.gitkeep deleted file mode 100644 index e69de29bb..000000000 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