diff --git a/control/Session.php b/control/Session.php index e40018a4f..a4ad854f0 100644 --- a/control/Session.php +++ b/control/Session.php @@ -145,15 +145,7 @@ class Session { if($data instanceof Session) $data = $data->inst_getAll(); $this->data = $data; - - if (isset($this->data['HTTP_USER_AGENT'])) { - if ($this->data['HTTP_USER_AGENT'] != $this->userAgent()) { - // Funny business detected! - $this->inst_clearAll(); - $this->inst_destroy(); - $this->inst_start(); - } - } + $this->expireIfInvalid(); } /** @@ -392,6 +384,9 @@ class Session { $this->data = isset($_SESSION) ? $_SESSION : array(); } + // Ensure session is validated on start + $this->expireIfInvalid(); + // Modify the timeout behaviour so it's the *inactive* time before the session expires. // By default it's the total session lifetime if($timeout && !headers_sent()) { @@ -631,4 +626,31 @@ class Session { Deprecation::notice('4.0', 'Use the "Session.timeout" config setting instead'); return Config::inst()->get('Session', 'timeout'); } + + /** + * Validate the user agent against the current data, resetting the + * current session if a mismatch is detected. + * + * @deprecated 3.0..4.0 Removed in 4.0 + * @return bool If user agent has been set against this session, returns + * the valid state of this session as either true or false. If the agent + * isn't set it is assumed valid and returns true. + */ + private function expireIfInvalid() { + // If not set, indeterminable; Assume true as safe default + if (!isset($this->data['HTTP_USER_AGENT'])) { + return true; + } + + // Agents match, deterministically true + if ($this->data['HTTP_USER_AGENT'] === $this->userAgent()) { + return true; + } + + // Funny business detected! + $this->inst_clearAll(); + $this->inst_destroy(); + $this->inst_start(); + return false; + } } diff --git a/tests/control/SessionTest.php b/tests/control/SessionTest.php index 1b5c5ee45..3b54f6200 100644 --- a/tests/control/SessionTest.php +++ b/tests/control/SessionTest.php @@ -97,15 +97,27 @@ class SessionTest extends SapphireTest { $_SERVER['HTTP_USER_AGENT'] = 'Test Agent'; // Generate our session + /** @var Session $s */ $s = Injector::inst()->create('Session', array()); $s->inst_set('val', 123); $s->inst_finalize(); + $data = $s->inst_getAll(); // Change our UA $_SERVER['HTTP_USER_AGENT'] = 'Fake Agent'; - // Verify the new session reset our values - $s2 = Injector::inst()->create('Session', $s); + // Verify the new session reset our values (passed by constructor) + /** @var Session $s2 */ + $s2 = Injector::inst()->create('Session', $data); $this->assertNotEquals($s2->inst_get('val'), 123); + + // Verify a started session resets our values (initiated by $_SESSION object) + /** @var Session $s3 */ + $s3 = Injector::inst()->create('Session', []); + foreach ($data as $key => $value) { + $s3->inst_set($key, $value); + } + $s3->inst_start(); + $this->assertNotEquals($s3->inst_get('val'), 123); } }