From a0105eb40b490e7b4697def543b0c8663babac82 Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Sat, 10 Feb 2018 12:04:07 +1030 Subject: [PATCH 1/2] Use Illuminate Session component instead of Symfony Symfony's component relies on PHP's native session functionality, which is not ideal. It automatically sets its own cookie headers, resulting in this issue: https://github.com/flarum/core/issues/1084#issuecomment-364569953 The Illuminate component is more powerful and has a simpler API for extension with other drivers and such, and fits in nicely with other components we use (the majority of which are from Illuminate). --- framework/core/composer.json | 1 + .../Controller/AbstractOAuth2Controller.php | 2 +- .../src/Forum/Controller/LogOutController.php | 2 +- .../Controller/ResetPasswordController.php | 2 +- framework/core/src/Foundation/Application.php | 2 + framework/core/src/Foundation/Site.php | 11 ++++ framework/core/src/Frontend/FrontendView.php | 2 +- .../Middleware/AuthenticateWithSession.php | 4 +- .../Http/Middleware/RememberFromCookie.php | 19 +++++- .../core/src/Http/Middleware/StartSession.php | 61 +++++++++++++------ .../core/src/Http/SessionAuthenticator.php | 15 ++--- framework/core/src/User/User.php | 8 +-- 12 files changed, 91 insertions(+), 38 deletions(-) diff --git a/framework/core/composer.json b/framework/core/composer.json index cc9151b2d..27c91e627 100644 --- a/framework/core/composer.json +++ b/framework/core/composer.json @@ -35,6 +35,7 @@ "illuminate/filesystem": "5.5.*", "illuminate/hashing": "5.5.*", "illuminate/mail": "5.5.*", + "illuminate/session": "5.5.*", "illuminate/support": "5.5.*", "illuminate/validation": "5.5.*", "illuminate/view": "5.5.*", diff --git a/framework/core/src/Forum/Controller/AbstractOAuth2Controller.php b/framework/core/src/Forum/Controller/AbstractOAuth2Controller.php index 52f8c1e17..ed0946dda 100644 --- a/framework/core/src/Forum/Controller/AbstractOAuth2Controller.php +++ b/framework/core/src/Forum/Controller/AbstractOAuth2Controller.php @@ -62,7 +62,7 @@ abstract class AbstractOAuth2Controller implements ControllerInterface if (! $code) { $authUrl = $this->provider->getAuthorizationUrl($this->getAuthorizationUrlOptions()); - $session->set('oauth2state', $this->provider->getState()); + $session->put('oauth2state', $this->provider->getState()); return new RedirectResponse($authUrl.'&display=popup'); } elseif (! $state || $state !== $session->get('oauth2state')) { diff --git a/framework/core/src/Forum/Controller/LogOutController.php b/framework/core/src/Forum/Controller/LogOutController.php index c819fec7d..86bb45480 100644 --- a/framework/core/src/Forum/Controller/LogOutController.php +++ b/framework/core/src/Forum/Controller/LogOutController.php @@ -102,7 +102,7 @@ class LogOutController implements ControllerInterface // If a valid CSRF token hasn't been provided, show a view which will // allow the user to press a button to complete the log out process. - $csrfToken = $session->get('csrf_token'); + $csrfToken = $session->token(); if (array_get($request->getQueryParams(), 'token') !== $csrfToken) { $return = array_get($request->getQueryParams(), 'return'); diff --git a/framework/core/src/Forum/Controller/ResetPasswordController.php b/framework/core/src/Forum/Controller/ResetPasswordController.php index 3752ead21..a95dd07cc 100644 --- a/framework/core/src/Forum/Controller/ResetPasswordController.php +++ b/framework/core/src/Forum/Controller/ResetPasswordController.php @@ -50,6 +50,6 @@ class ResetPasswordController extends AbstractHtmlController return $this->view->make('flarum.forum::reset-password') ->with('passwordToken', $token->id) - ->with('csrfToken', $request->getAttribute('session')->get('csrf_token')); + ->with('csrfToken', $request->getAttribute('session')->token()); } } diff --git a/framework/core/src/Foundation/Application.php b/framework/core/src/Foundation/Application.php index 2753291b5..1c8ba7718 100644 --- a/framework/core/src/Foundation/Application.php +++ b/framework/core/src/Foundation/Application.php @@ -718,6 +718,8 @@ class Application extends Container implements ApplicationContract 'filesystem.cloud' => [\Illuminate\Contracts\Filesystem\Cloud::class], 'hash' => [\Illuminate\Contracts\Hashing\Hasher::class], 'mailer' => [\Illuminate\Mail\Mailer::class, \Illuminate\Contracts\Mail\Mailer::class, \Illuminate\Contracts\Mail\MailQueue::class], + 'session' => [\Illuminate\Session\SessionManager::class], + 'session.driver' => [\Illuminate\Contracts\Session\Session::class], 'validator' => [\Illuminate\Validation\Factory::class, \Illuminate\Contracts\Validation\Factory::class], 'view' => [\Illuminate\View\Factory::class, \Illuminate\Contracts\View\Factory::class], ]; diff --git a/framework/core/src/Foundation/Site.php b/framework/core/src/Foundation/Site.php index a86f7d6e1..f1978f217 100644 --- a/framework/core/src/Foundation/Site.php +++ b/framework/core/src/Foundation/Site.php @@ -33,6 +33,7 @@ use Illuminate\Config\Repository as ConfigRepository; use Illuminate\Filesystem\FilesystemServiceProvider; use Illuminate\Hashing\HashServiceProvider; use Illuminate\Mail\MailServiceProvider; +use Illuminate\Session\SessionServiceProvider; use Illuminate\Validation\ValidationServiceProvider; use Illuminate\View\ViewServiceProvider; use Monolog\Formatter\LineFormatter; @@ -169,6 +170,7 @@ class Site $app->register(FilesystemServiceProvider::class); $app->register(HashServiceProvider::class); $app->register(MailServiceProvider::class); + $app->register(SessionServiceProvider::class); $app->register(ViewServiceProvider::class); $app->register(ValidationServiceProvider::class); @@ -236,6 +238,15 @@ class Site 'root' => $app->publicPath().'/assets/avatars' ] ] + ], + 'session' => [ + 'driver' => 'file', + 'lifetime' => 120, + 'expire_on_close' => false, + 'encrypt' => false, + 'files' => $app->storagePath().'/sessions', + 'lottery' => [2, 100], + 'cookie' => 'session' ] ]); } diff --git a/framework/core/src/Frontend/FrontendView.php b/framework/core/src/Frontend/FrontendView.php index 45d36d12c..0063384d3 100644 --- a/framework/core/src/Frontend/FrontendView.php +++ b/framework/core/src/Frontend/FrontendView.php @@ -464,7 +464,7 @@ class FrontendView return [ 'userId' => $actor->id, - 'csrfToken' => $session->get('csrf_token') + 'csrfToken' => $session->token() ]; } diff --git a/framework/core/src/Http/Middleware/AuthenticateWithSession.php b/framework/core/src/Http/Middleware/AuthenticateWithSession.php index 6c6729f16..4c780925b 100644 --- a/framework/core/src/Http/Middleware/AuthenticateWithSession.php +++ b/framework/core/src/Http/Middleware/AuthenticateWithSession.php @@ -13,10 +13,10 @@ namespace Flarum\Http\Middleware; use Flarum\User\Guest; use Flarum\User\User; +use Illuminate\Contracts\Session\Session; use Interop\Http\ServerMiddleware\DelegateInterface; use Interop\Http\ServerMiddleware\MiddlewareInterface; use Psr\Http\Message\ServerRequestInterface as Request; -use Symfony\Component\HttpFoundation\Session\SessionInterface; class AuthenticateWithSession implements MiddlewareInterface { @@ -33,7 +33,7 @@ class AuthenticateWithSession implements MiddlewareInterface return $delegate->process($request); } - private function getActor(SessionInterface $session) + private function getActor(Session $session) { $actor = User::find($session->get('user_id')) ?: new Guest; diff --git a/framework/core/src/Http/Middleware/RememberFromCookie.php b/framework/core/src/Http/Middleware/RememberFromCookie.php index 96cf1e289..335c43198 100644 --- a/framework/core/src/Http/Middleware/RememberFromCookie.php +++ b/framework/core/src/Http/Middleware/RememberFromCookie.php @@ -12,15 +12,29 @@ namespace Flarum\Http\Middleware; use Flarum\Http\AccessToken; +use Flarum\Http\CookieFactory; use Interop\Http\ServerMiddleware\DelegateInterface; use Interop\Http\ServerMiddleware\MiddlewareInterface; use Psr\Http\Message\ServerRequestInterface as Request; class RememberFromCookie implements MiddlewareInterface { + /** + * @var CookieFactory + */ + protected $cookie; + + /** + * @param CookieFactory $cookie + */ + public function __construct(CookieFactory $cookie) + { + $this->cookie = $cookie; + } + public function process(Request $request, DelegateInterface $delegate) { - $id = array_get($request->getCookieParams(), 'flarum_remember'); + $id = array_get($request->getCookieParams(), $this->cookie->getName('remember')); if ($id) { $token = AccessToken::find($id); @@ -28,8 +42,9 @@ class RememberFromCookie implements MiddlewareInterface if ($token) { $token->touch(); + /** @var \Illuminate\Contracts\Session\Session $session */ $session = $request->getAttribute('session'); - $session->set('user_id', $token->user_id); + $session->put('user_id', $token->user_id); } } diff --git a/framework/core/src/Http/Middleware/StartSession.php b/framework/core/src/Http/Middleware/StartSession.php index 239fed9c1..701a30732 100644 --- a/framework/core/src/Http/Middleware/StartSession.php +++ b/framework/core/src/Http/Middleware/StartSession.php @@ -13,17 +13,19 @@ namespace Flarum\Http\Middleware; use Dflydev\FigCookies\FigResponseCookies; use Flarum\Http\CookieFactory; -use Illuminate\Support\Str; +use Illuminate\Contracts\Session\Session; +use Illuminate\Session\SessionManager; use Interop\Http\ServerMiddleware\DelegateInterface; use Interop\Http\ServerMiddleware\MiddlewareInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; -use Symfony\Component\HttpFoundation\Session\Session; -use Symfony\Component\HttpFoundation\Session\SessionInterface; class StartSession implements MiddlewareInterface { - const COOKIE_NAME = 'session'; + /** + * @var SessionManager + */ + protected $manager; /** * @var CookieFactory @@ -31,54 +33,75 @@ class StartSession implements MiddlewareInterface protected $cookie; /** + * @param SessionManager $manager * @param CookieFactory $cookie */ - public function __construct(CookieFactory $cookie) + public function __construct(SessionManager $manager, CookieFactory $cookie) { + $this->manager = $manager; $this->cookie = $cookie; } public function process(Request $request, DelegateInterface $delegate) { - $session = $this->startSession(); + $request = $request->withAttribute('session', + $session = $this->startSession($request) + ); - $request = $request->withAttribute('session', $session); + $this->collectGarbage($session); $response = $delegate->process($request); + $session->save(); + $response = $this->withCsrfTokenHeader($response, $session); return $this->withSessionCookie($response, $session); } - private function startSession() + private function startSession(Request $request) { - $session = new Session; + $session = $this->manager->driver(); - $session->setName($this->cookie->getName(self::COOKIE_NAME)); + $id = array_get($request->getCookieParams(), $this->cookie->getName($session->getName())); + + $session->setId($id); $session->start(); - if (! $session->has('csrf_token')) { - $session->set('csrf_token', Str::random(40)); - } - return $session; } - private function withCsrfTokenHeader(Response $response, SessionInterface $session) + private function collectGarbage(Session $session) { - if ($session->has('csrf_token')) { - $response = $response->withHeader('X-CSRF-Token', $session->get('csrf_token')); + $config = $this->manager->getSessionConfig(); + + if ($this->configHitsLottery($config)) { + $session->getHandler()->gc($this->getSessionLifetimeInSeconds()); } + } + + private function configHitsLottery(array $config) + { + return random_int(1, $config['lottery'][1]) <= $config['lottery'][0]; + } + + private function withCsrfTokenHeader(Response $response, Session $session) + { + $response = $response->withHeader('X-CSRF-Token', $session->token()); return $response; } - private function withSessionCookie(Response $response, SessionInterface $session) + private function withSessionCookie(Response $response, Session $session) { return FigResponseCookies::set( $response, - $this->cookie->make(self::COOKIE_NAME, $session->getId()) + $this->cookie->make($session->getName(), $session->getId(), $this->getSessionLifetimeInSeconds()) ); } + + private function getSessionLifetimeInSeconds() + { + return ($this->manager->getSessionConfig()['lifetime'] ?? null) * 60; + } } diff --git a/framework/core/src/Http/SessionAuthenticator.php b/framework/core/src/Http/SessionAuthenticator.php index 8ccb06524..8e23027a0 100644 --- a/framework/core/src/Http/SessionAuthenticator.php +++ b/framework/core/src/Http/SessionAuthenticator.php @@ -11,25 +11,26 @@ namespace Flarum\Http; -use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Illuminate\Contracts\Session\Session; class SessionAuthenticator { /** - * @param SessionInterface $session + * @param Session $session * @param int $userId */ - public function logIn(SessionInterface $session, $userId) + public function logIn(Session $session, $userId) { - $session->migrate(); - $session->set('user_id', $userId); + $session->regenerate(true); + $session->put('user_id', $userId); } /** - * @param SessionInterface $session + * @param Session $session */ - public function logOut(SessionInterface $session) + public function logOut(Session $session) { $session->invalidate(); + $session->regenerateToken(); } } diff --git a/framework/core/src/User/User.php b/framework/core/src/User/User.php index 3a695aa78..fa30ec01a 100644 --- a/framework/core/src/User/User.php +++ b/framework/core/src/User/User.php @@ -34,7 +34,7 @@ use Flarum\User\Event\PasswordChanged; use Flarum\User\Event\Registered; use Flarum\User\Event\Renamed; use Illuminate\Contracts\Hashing\Hasher; -use Symfony\Component\HttpFoundation\Session\SessionInterface; +use Illuminate\Contracts\Session\Session; /** * @property int $id @@ -698,7 +698,7 @@ class User extends AbstractModel } /** - * @return SessionInterface + * @return Session */ public function getSession() { @@ -706,9 +706,9 @@ class User extends AbstractModel } /** - * @param SessionInterface $session + * @param Session $session */ - public function setSession(SessionInterface $session) + public function setSession(Session $session) { $this->session = $session; } From d60cec33be2352a9639e28044bc98cae617842c1 Mon Sep 17 00:00:00 2001 From: Franz Liedke Date: Sun, 18 Mar 2018 15:58:31 +0100 Subject: [PATCH 2/2] Bind session handling to request lifecycle With this change, session objects are no longer instantiated globally, but instead created within a middleware during the request lifecycle. In addition, session garbage collection is integrated with the already existing middleware for this purpose. --- framework/core/src/Foundation/Application.php | 2 - framework/core/src/Foundation/Site.php | 4 -- .../src/Http/Middleware/CollectGarbage.php | 25 ++++++++ .../core/src/Http/Middleware/StartSession.php | 63 ++++++++----------- .../core/src/User/UserServiceProvider.php | 24 ++++++- 5 files changed, 73 insertions(+), 45 deletions(-) diff --git a/framework/core/src/Foundation/Application.php b/framework/core/src/Foundation/Application.php index 1c8ba7718..2753291b5 100644 --- a/framework/core/src/Foundation/Application.php +++ b/framework/core/src/Foundation/Application.php @@ -718,8 +718,6 @@ class Application extends Container implements ApplicationContract 'filesystem.cloud' => [\Illuminate\Contracts\Filesystem\Cloud::class], 'hash' => [\Illuminate\Contracts\Hashing\Hasher::class], 'mailer' => [\Illuminate\Mail\Mailer::class, \Illuminate\Contracts\Mail\Mailer::class, \Illuminate\Contracts\Mail\MailQueue::class], - 'session' => [\Illuminate\Session\SessionManager::class], - 'session.driver' => [\Illuminate\Contracts\Session\Session::class], 'validator' => [\Illuminate\Validation\Factory::class, \Illuminate\Contracts\Validation\Factory::class], 'view' => [\Illuminate\View\Factory::class, \Illuminate\Contracts\View\Factory::class], ]; diff --git a/framework/core/src/Foundation/Site.php b/framework/core/src/Foundation/Site.php index f1978f217..9f17b4a27 100644 --- a/framework/core/src/Foundation/Site.php +++ b/framework/core/src/Foundation/Site.php @@ -240,12 +240,8 @@ class Site ] ], 'session' => [ - 'driver' => 'file', 'lifetime' => 120, - 'expire_on_close' => false, - 'encrypt' => false, 'files' => $app->storagePath().'/sessions', - 'lottery' => [2, 100], 'cookie' => 'session' ] ]); diff --git a/framework/core/src/Http/Middleware/CollectGarbage.php b/framework/core/src/Http/Middleware/CollectGarbage.php index 4754f2300..f4882c9fb 100644 --- a/framework/core/src/Http/Middleware/CollectGarbage.php +++ b/framework/core/src/Http/Middleware/CollectGarbage.php @@ -15,12 +15,30 @@ use Flarum\Http\AccessToken; use Flarum\User\AuthToken; use Flarum\User\EmailToken; use Flarum\User\PasswordToken; +use Illuminate\Contracts\Config\Repository as ConfigRepository; use Interop\Http\ServerMiddleware\DelegateInterface; use Interop\Http\ServerMiddleware\MiddlewareInterface; use Psr\Http\Message\ServerRequestInterface as Request; +use SessionHandlerInterface; class CollectGarbage implements MiddlewareInterface { + /** + * @var SessionHandlerInterface + */ + protected $sessionHandler; + + /** + * @var array + */ + protected $sessionConfig; + + public function __construct(SessionHandlerInterface $handler, ConfigRepository $config) + { + $this->sessionHandler = $handler; + $this->sessionConfig = $config->get('session'); + } + public function process(Request $request, DelegateInterface $delegate) { $this->collectGarbageSometimes(); @@ -43,10 +61,17 @@ class CollectGarbage implements MiddlewareInterface EmailToken::where('created_at', '<=', $earliestToKeep)->delete(); PasswordToken::where('created_at', '<=', $earliestToKeep)->delete(); AuthToken::where('created_at', '<=', $earliestToKeep)->delete(); + + $this->sessionHandler->gc($this->getSessionLifetimeInSeconds()); } private function hit() { return mt_rand(1, 100) <= 2; } + + private function getSessionLifetimeInSeconds() + { + return $this->sessionConfig['lifetime'] * 60; + } } diff --git a/framework/core/src/Http/Middleware/StartSession.php b/framework/core/src/Http/Middleware/StartSession.php index 701a30732..7447aceea 100644 --- a/framework/core/src/Http/Middleware/StartSession.php +++ b/framework/core/src/Http/Middleware/StartSession.php @@ -13,19 +13,21 @@ namespace Flarum\Http\Middleware; use Dflydev\FigCookies\FigResponseCookies; use Flarum\Http\CookieFactory; +use Illuminate\Contracts\Config\Repository as ConfigRepository; use Illuminate\Contracts\Session\Session; -use Illuminate\Session\SessionManager; +use Illuminate\Session\Store; use Interop\Http\ServerMiddleware\DelegateInterface; use Interop\Http\ServerMiddleware\MiddlewareInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; +use SessionHandlerInterface; class StartSession implements MiddlewareInterface { /** - * @var SessionManager + * @var SessionHandlerInterface */ - protected $manager; + protected $handler; /** * @var CookieFactory @@ -33,25 +35,29 @@ class StartSession implements MiddlewareInterface protected $cookie; /** - * @param SessionManager $manager + * @var array + */ + protected $config; + + /** * @param CookieFactory $cookie */ - public function __construct(SessionManager $manager, CookieFactory $cookie) + public function __construct(SessionHandlerInterface $handler, CookieFactory $cookie, ConfigRepository $config) { - $this->manager = $manager; + $this->handler = $handler; $this->cookie = $cookie; + $this->config = $config->get('session'); } public function process(Request $request, DelegateInterface $delegate) { - $request = $request->withAttribute('session', - $session = $this->startSession($request) + $request = $request->withAttribute( + 'session', + $session = $this->makeSession($request) ); - $this->collectGarbage($session); - + $session->start(); $response = $delegate->process($request); - $session->save(); $response = $this->withCsrfTokenHeader($response, $session); @@ -59,37 +65,20 @@ class StartSession implements MiddlewareInterface return $this->withSessionCookie($response, $session); } - private function startSession(Request $request) + private function makeSession(Request $request) { - $session = $this->manager->driver(); + $cookieName = $this->cookie->getName($this->config['cookie']); - $id = array_get($request->getCookieParams(), $this->cookie->getName($session->getName())); - - $session->setId($id); - $session->start(); - - return $session; - } - - private function collectGarbage(Session $session) - { - $config = $this->manager->getSessionConfig(); - - if ($this->configHitsLottery($config)) { - $session->getHandler()->gc($this->getSessionLifetimeInSeconds()); - } - } - - private function configHitsLottery(array $config) - { - return random_int(1, $config['lottery'][1]) <= $config['lottery'][0]; + return new Store( + $cookieName, + $this->handler, + array_get($request->getCookieParams(), $cookieName) + ); } private function withCsrfTokenHeader(Response $response, Session $session) { - $response = $response->withHeader('X-CSRF-Token', $session->token()); - - return $response; + return $response->withHeader('X-CSRF-Token', $session->token()); } private function withSessionCookie(Response $response, Session $session) @@ -102,6 +91,6 @@ class StartSession implements MiddlewareInterface private function getSessionLifetimeInSeconds() { - return ($this->manager->getSessionConfig()['lifetime'] ?? null) * 60; + return $this->config['lifetime'] * 60; } } diff --git a/framework/core/src/User/UserServiceProvider.php b/framework/core/src/User/UserServiceProvider.php index 03f3cbf55..a52d128ad 100644 --- a/framework/core/src/User/UserServiceProvider.php +++ b/framework/core/src/User/UserServiceProvider.php @@ -15,7 +15,9 @@ use Flarum\Event\ConfigureUserPreferences; use Flarum\Event\GetPermission; use Flarum\Foundation\AbstractServiceProvider; use Illuminate\Contracts\Container\Container; +use Illuminate\Session\FileSessionHandler; use RuntimeException; +use SessionHandlerInterface; class UserServiceProvider extends AbstractServiceProvider { @@ -23,6 +25,26 @@ class UserServiceProvider extends AbstractServiceProvider * {@inheritdoc} */ public function register() + { + $this->registerSession(); + $this->registerGate(); + $this->registerAvatarsFilesystem(); + } + + protected function registerSession() + { + $this->app->singleton('session.handler', function ($app) { + return new FileSessionHandler( + $app['files'], + $app['config']['session.files'], + $app['config']['session.lifetime'] + ); + }); + + $this->app->alias('session.handler', SessionHandlerInterface::class); + } + + protected function registerGate() { $this->app->singleton('flarum.gate', function ($app) { return new Gate($app, function () { @@ -32,8 +54,6 @@ class UserServiceProvider extends AbstractServiceProvider $this->app->alias('flarum.gate', 'Illuminate\Contracts\Auth\Access\Gate'); $this->app->alias('flarum.gate', Gate::class); - - $this->registerAvatarsFilesystem(); } protected function registerAvatarsFilesystem()