From 93b0884e196fefe79ef7fdee9b9e3e6cfdf46cd4 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 18 Jul 2018 14:24:27 +1200 Subject: [PATCH 1/3] BUG Lazy session state (fixes #8267) Fixes regression from 3.x, where sessions where lazy started as required: Either because an existing session identifier was sent through with the request, or because new session data needed to be persisted as part of the request execution. Without this lazy starting, *every* request will get a session, which makes all those responses uncacheable by HTTP layers. Note that 4.x also changed the $data vs. $changedData payloads: In 3.x, they both contained key/value pairs. In 4.x, $data contains key/value, while $changedData contains key/boolean to declare isChanged. While this reduces duplication in the class, it also surfaced a bug which was latent in 3.x: When an existing session is lazily resumed via start(), $data is set back to an empty array. In 3.x, any changed data before this point was *also* retained in $changedData, ensuring it gets merged into existing $_SESSION data. In 4.x, this clears out data - hence the need for a more complex merge logic. Since isset($this->data) is no longer an accurate indicator of a started session, we introduce a separate $this->started flag. Note that I've chosen not to make lazy an opt-in (e.g. via start($request, $lazy=false)). We already have a distinction between lazy starting via init(), and force starting via start(). --- src/Control/Session.php | 77 ++++++++++++------- .../SessionAuthenticationHandler.php | 9 ++- 2 files changed, 59 insertions(+), 27 deletions(-) diff --git a/src/Control/Session.php b/src/Control/Session.php index d0e68f35e..17a2c295a 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -49,6 +49,9 @@ use SilverStripe\Dev\Deprecation; * * Once you have saved a value to the Session you can access it by using the {@link Session::get()} function. * Like the {@link Session::set()} function you can use this anywhere in your PHP files. + * Note that session data isn't persisted in PHP's own session store (via $_SESSION) + * until {@link Session::save()} is called, which happens automatically at the end of a standard request + * through {@link SilverStripe\Control\Middleware\SessionMiddleware}. * * The values in the comments are the values stored from the previous example. * @@ -84,7 +87,6 @@ use SilverStripe\Dev\Deprecation; * * * @see Cookie - * @todo This class is currently really basic and could do with a more well-thought-out implementation. */ class Session { @@ -128,6 +130,12 @@ class Session */ private static $cookie_secure = false; + /** + * @config + * @var string + */ + private static $cookie_name_secure = 'SECSESSID'; + /** * Name of session cache limiter to use. * Defaults to '' to disable cache limiter entirely. @@ -145,6 +153,11 @@ class Session */ protected $data = null; + /** + * @var bool + */ + protected $started = false; + /** * List of keys changed. This is a nested array which represents the * keys modified in $this->data. The value of each item is either "true" @@ -192,16 +205,21 @@ class Session } $this->data = $data; + $this->started = isset($data); } /** - * Init this session instance before usage + * Init this session instance before usage, + * if a session identifier is part of the passed in request. + * Otherwise, a session might be started in {@link save()} + * if session data needs to be written with a new session identifier. * * @param HTTPRequest $request */ public function init(HTTPRequest $request) { - if (!$this->isStarted()) { + + if (!$this->isStarted() && $this->requestContainsSessionId($request)) { $this->start($request); } @@ -210,6 +228,7 @@ class Session if ($this->data['HTTP_USER_AGENT'] !== $this->userAgent($request)) { $this->clearAll(); $this->destroy(); + $this->started = false; $this->start($request); } } @@ -233,11 +252,24 @@ class Session */ public function isStarted() { - return isset($this->data); + return $this->started; } /** - * Begin session + * @param HTTPRequest $request + * @return bool + */ + public function requestContainsSessionId(HTTPRequest $request) + { + $secure = Director::is_https($request) && $this->config()->get('cookie_secure'); + $name = $secure ? $this->config()->get('cookie_name_secure') : session_name(); + return (bool)Cookie::get($name); + } + + /** + * Begin session, regardless if a session identifier is present in the request, + * or whether any session data needs to be written. + * See {@link init()} if you want to "lazy start" a session. * * @param HTTPRequest $request The request for which to start a session */ @@ -281,7 +313,7 @@ class Session // If we want a secure cookie for HTTPS, use a seperate session name. This lets us have a // seperate (less secure) session for non-HTTPS requests if ($secure) { - session_name('SECSESSID'); + session_name($this->config()->get('cookie_name_secure')); } $limiter = $this->config()->get('sessionCacheLimiter'); @@ -291,7 +323,16 @@ class Session session_start(); - $this->data = isset($_SESSION) ? $_SESSION : array(); + if (isset($_SESSION)) { + // Initialise data from session store if present + $data = $_SESSION; + // Merge in existing in-memory data, taking priority over session store data + $this->recursivelyApply((array)$this->data, $data); + } else { + // Use in-memory data if the session is lazy started + $data = isset($this->data) ? $this->data : []; + } + $this->data = $data; } else { $this->data = []; } @@ -302,6 +343,8 @@ class Session Cookie::set(session_name(), session_id(), $timeout/86400, $path, $domain ? $domain : null, $secure, true); } + + $this->started = true; } /** @@ -335,9 +378,6 @@ class Session */ public function set($name, $val) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } $var = &$this->nestedValueRef($name, $this->data); // Mark changed @@ -380,10 +420,6 @@ class Session */ public function addToArray($name, $val) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } - $names = explode('.', $name); // We still want to do this even if we have strict path checking for legacy code @@ -407,9 +443,6 @@ class Session */ public function get($name) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be accessed until it's started"); - } return $this->nestedValue($name, $this->data); } @@ -421,10 +454,6 @@ class Session */ public function clear($name) { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } - // Get var by path $var = $this->nestedValue($name, $this->data); @@ -449,10 +478,6 @@ class Session */ public function clearAll() { - if (!$this->isStarted()) { - throw new BadMethodCallException("Session cannot be modified until it's started"); - } - if ($this->data && is_array($this->data)) { foreach (array_keys($this->data) as $key) { $this->clear($key); @@ -495,7 +520,7 @@ class Session $this->start($request); } - // Apply all changes recursively + // Apply all changes recursively, implicitly writing them to the actual PHP session store. $this->recursivelyApplyChanges($this->changedData, $this->data, $_SESSION); } } diff --git a/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php b/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php index a5c9ed240..1aafc020c 100644 --- a/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php +++ b/src/Security/MemberAuthenticator/SessionAuthenticationHandler.php @@ -45,9 +45,16 @@ class SessionAuthenticationHandler implements AuthenticationHandler */ public function authenticateRequest(HTTPRequest $request) { + $session = $request->getSession(); + + // Sessions are only started when a session cookie is detected + if (!$session->isStarted()) { + return null; + } + // If ID is a bad ID it will be treated as if the user is not logged in, rather than throwing a // ValidationException - $id = $request->getSession()->get($this->getSessionVariable()); + $id = $session->get($this->getSessionVariable()); if (!$id) { return null; } From e415bcb44a32e0f64f6c0a18b0afc97f0eb9edde Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 18 Jul 2018 21:15:56 +1200 Subject: [PATCH 2/3] Fix tests on unset session data Thanks Robbie! --- src/Control/Director.php | 2 +- src/Control/Session.php | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index 79622d213..6e7aef593 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -206,7 +206,7 @@ class Director implements TemplateGlobalProvider if ($session instanceof Session) { // Note: If passing $session as object, ensure that changes are written back // This is important for classes such as FunctionalTest which emulate cross-request persistence - $newVars['_SESSION'] = $sessionArray = $session->getAll(); + $newVars['_SESSION'] = $sessionArray = $session->getAll() ?: []; $finally[] = function () use ($session, $sessionArray) { if (isset($_SESSION)) { // Set new / updated keys diff --git a/src/Control/Session.php b/src/Control/Session.php index 17a2c295a..f9f95d1cf 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -262,8 +262,8 @@ class Session public function requestContainsSessionId(HTTPRequest $request) { $secure = Director::is_https($request) && $this->config()->get('cookie_secure'); - $name = $secure ? $this->config()->get('cookie_name_secure') : session_name(); - return (bool)Cookie::get($name); + $name = $secure ? $this->config()->get('cookie_name_secure') : session_name(); + return (bool)Cookie::get($name); } /** @@ -330,9 +330,9 @@ class Session $this->recursivelyApply((array)$this->data, $data); } else { // Use in-memory data if the session is lazy started - $data = isset($this->data) ? $this->data : []; + $data = $this->data; } - $this->data = $data; + $this->data = $data ?: []; } else { $this->data = []; } @@ -615,6 +615,7 @@ class Session */ protected function recursivelyApplyChanges($changes, $source, &$destination) { + $source = $source ?: []; foreach ($changes as $key => $changed) { if ($changed === true) { // Determine if replacement or removal From c541283093460faa6b3db60258b1bf5f36ae42cb Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 18 Jul 2018 21:16:06 +1200 Subject: [PATCH 3/3] Test coverage for session data change --- tests/php/Control/SessionTest.php | 142 ++++++++++++++++++ .../SessionAuthenticationHandlerTest.php | 61 ++++++++ 2 files changed, 203 insertions(+) create mode 100644 tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php diff --git a/tests/php/Control/SessionTest.php b/tests/php/Control/SessionTest.php index f1b5a5430..724e778f5 100644 --- a/tests/php/Control/SessionTest.php +++ b/tests/php/Control/SessionTest.php @@ -2,6 +2,8 @@ namespace SilverStripe\Control\Tests; +use http\Exception\BadMessageException; +use SilverStripe\Control\Cookie; use SilverStripe\Control\Session; use SilverStripe\Dev\SapphireTest; use SilverStripe\Control\HTTPRequest; @@ -22,6 +24,127 @@ class SessionTest extends SapphireTest return parent::setUp(); } + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testInitDoesNotStartSessionWithoutIdentifier() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $session->init($req); + $this->assertFalse($session->isStarted()); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testInitStartsSessionWithIdentifier() + { + $req = new HTTPRequest('GET', '/'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->init($req); + $this->assertTrue($session->isStarted()); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testInitStartsSessionWithData() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session([]); + $session->init($req); + $this->assertTrue($session->isStarted()); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartUsesDefaultCookieNameWithHttp() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('http'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->start($req); + $this->assertNotEquals(session_name(), $session->config()->get('cookie_name_secure')); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartUsesDefaultCookieNameWithHttpsAndCookieSecureOff() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('https'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->start($req); + $this->assertNotEquals(session_name(), $session->config()->get('cookie_name_secure')); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartUsesSecureCookieNameWithHttpsAndCookieSecureOn() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('https'); + Cookie::set(session_name(), '1234'); + $session = new Session(null); // unstarted session + $session->config()->update('cookie_secure', true); + $session->start($req); + $this->assertEquals(session_name(), $session->config()->get('cookie_name_secure')); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + * @expectedException BadMethodCallException + * @expectedExceptionMessage Session has already started + */ + public function testStartErrorsWhenStartingTwice() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $session->start($req); + $session->start($req); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testStartRetainsInMemoryData() + { + $this->markTestIncomplete('Test'); + // TODO Figure out how to simulate session vars without a session_start() resetting them + // $_SESSION['existing'] = true; + // $_SESSION['merge'] = 1; + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $session->set('new', true); + $session->set('merge', 2); + $session->start($req); // simulate lazy start + $this->assertEquals( + [ + // 'existing' => true, + 'new' => true, + 'merge' => 2 + ], + $session->getAll() + ); + + unset($_SESSION); + } + public function testGetSetBasics() { $this->session->set('Test', 'Test'); @@ -124,6 +247,25 @@ class SessionTest extends SapphireTest ); } + public function testRequestContainsSessionId() + { + $req = new HTTPRequest('GET', '/'); + $session = new Session(null); // unstarted session + $this->assertFalse($session->requestContainsSessionId($req)); + Cookie::set(session_name(), '1234'); + $this->assertTrue($session->requestContainsSessionId($req)); + } + + public function testRequestContainsSessionIdRespectsCookieNameSecure() + { + $req = (new HTTPRequest('GET', '/')) + ->setScheme('https'); + $session = new Session(null); // unstarted session + Cookie::set($session->config()->get('cookie_name_secure'), '1234'); + $session->config()->update('cookie_secure', true); + $this->assertTrue($session->requestContainsSessionId($req)); + } + public function testUserAgentLockout() { // Set a user agent diff --git a/tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php b/tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php new file mode 100644 index 000000000..800e49563 --- /dev/null +++ b/tests/php/Security/MemberAuthenticator/SessionAuthenticationHandlerTest.php @@ -0,0 +1,61 @@ + 'test@example.com']); + $member->write(); + + $handler = new SessionAuthenticationHandler(); + + $session = new Session(null); // unstarted, simulates lack of session cookie + $session->set($handler->getSessionVariable(), $member->ID); + + $req = new HTTPRequest('GET', '/'); + $req->setSession($session); + + $matchedMember = $handler->authenticateRequest($req); + $this->assertNull($matchedMember); + } + + /** + * @runInSeparateProcess + * @preserveGlobalState disabled + */ + public function testAuthenticateRequestStartsSessionWithSessionIdentifier() + { + $member = new Member(['Email' => 'test@example.com']); + $member->write(); + + $handler = new SessionAuthenticationHandler(); + + $session = new Session(null); // unstarted + $session->set($handler->getSessionVariable(), $member->ID); + + $req = new HTTPRequest('GET', '/'); + $req->setSession($session); + + Cookie::set(session_name(), '1234'); + $session->start($req); // simulate detection of session cookie + + $matchedMember = $handler->authenticateRequest($req); + $this->assertNotNull($matchedMember); + $this->assertEquals($matchedMember->Email, $member->Email); + } +}