From db54112f3cca012e33257c782dffd7154bf663a5 Mon Sep 17 00:00:00 2001
From: Damian Mooyman <damian@silverstripe.com>
Date: Thu, 30 Nov 2017 14:48:36 +1300
Subject: [PATCH] [ss-2017-006] Fix user agent invalidation on session startup

---
 control/Session.php           | 40 +++++++++++++++++++++++++++--------
 tests/control/SessionTest.php | 16 ++++++++++++--
 2 files changed, 45 insertions(+), 11 deletions(-)

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);
 	}
 }