Merge pull request #2449 from cakephp/fix-session-cyclic-error

Fixed error in CakeSession that would call start() in an infinite loop

lib/Cake/Model/Datasource/CakeSession.php

@@ -120,6 +120,13 @@ class CakeSession {
  */
 	public static $requestCountdown = 10;
 
+/**
+ * Whether or not the init function in this class was already called
+ *
+ * @var boolean
+ */
+	protected static $_initialized = false;
+
 /**
  * Pseudo constructor.
  *
@@ -128,15 +135,20 @@ class CakeSession {
  */
 	public static function init($base = null) {
 		self::$time = time();
-
 		$checkAgent = Configure::read('Session.checkAgent');
+
 		if (env('HTTP_USER_AGENT')) {
 			self::$_userAgent = md5(env('HTTP_USER_AGENT') . Configure::read('Security.salt'));
 		}
+
 		self::_setPath($base);
 		self::_setHost(env('HTTP_HOST'));
 
-		register_shutdown_function('session_write_close');
+		if (!self::$_initialized) {
+			register_shutdown_function('session_write_close');
+		}
+
+		self::$_initialized = true;
 	}
 
 /**
@@ -181,10 +193,8 @@ class CakeSession {
 		if (self::started()) {
 			return true;
 		}
-		self::init();
+
 		$id = self::id();
-		session_write_close();
-		self::_configureSession();
 		self::_startSession();
 
 		if (!$id && self::started()) {
@@ -192,6 +202,7 @@ class CakeSession {
 		}
 
 		self::$error = false;
+		self::$valid = true;
 		return self::started();
 	}
 
@@ -424,9 +435,14 @@ class CakeSession {
  * @return void
  */
 	public static function destroy() {
-		self::start();
+		if (!self::started()) {
+			self::_startSession();
+		}
+
 		session_destroy();
-		self::clear();
+
+		$_SESSION = null;
+		self::$id = null;
 	}
 
 /**
@@ -437,7 +453,6 @@ class CakeSession {
 	public static function clear() {
 		$_SESSION = null;
 		self::$id = null;
-		self::start();
 		self::renew();
 	}
 
@@ -600,6 +615,10 @@ class CakeSession {
  * @return boolean Success
  */
 	protected static function _startSession() {
+		self::init();
+		session_write_close();
+		self::_configureSession();
+
 		if (headers_sent()) {
 			if (empty($_SESSION)) {
 				$_SESSION = array();
@@ -618,14 +637,11 @@ class CakeSession {
  * @return void
  */
 	protected static function _checkValid() {
-		if (!self::start()) {
-			self::$valid = false;
-			return false;
-		}
-		if ($config = self::read('Config')) {
+		$config = self::read('Config');
+		if ($config) {
 			$sessionConfig = Configure::read('Session');
 
-			if (self::_validAgentAndTime()) {
+			if (self::valid()) {
 				self::write('Config.time', self::$sessionTime);
 				if (isset($sessionConfig['autoRegenerate']) && $sessionConfig['autoRegenerate'] === true) {
 					$check = $config['countdown'];
@@ -637,20 +653,29 @@ class CakeSession {
 						self::write('Config.countdown', self::$requestCountdown);
 					}
 				}
-				self::$valid = true;
 			} else {
+				$_SESSION = array();
 				self::destroy();
-				self::$valid = false;
 				self::_setError(1, 'Session Highjacking Attempted !!!');
+				self::_startSession();
+				self::_writeConfig();
 			}
 		} else {
-			self::write('Config.userAgent', self::$_userAgent);
-			self::write('Config.time', self::$sessionTime);
-			self::write('Config.countdown', self::$requestCountdown);
-			self::$valid = true;
+			self::_writeConfig();
 		}
 	}
 
+/**
+ * Writes configuration variables to the session
+ *
+ * @return void
+ */
+	protected static function _writeConfig() {
+		self::write('Config.userAgent', self::$_userAgent);
+		self::write('Config.time', self::$sessionTime);
+		self::write('Config.countdown', self::$requestCountdown);
+	}
+
 /**
  * Restarts this session.
  *

lib/Cake/Test/Case/Controller/Component/SessionComponentTest.php

@@ -175,6 +175,7 @@ class SessionComponentTest extends CakeTestCase {
  * @return void
  */
 	public function testSessionError() {
+		CakeSession::$lastError = null;
 		$Session = new SessionComponent($this->ComponentCollection);
 		$this->assertFalse($Session->error());
 	}

lib/Cake/Test/Case/Model/Datasource/CakeSessionTest.php

@@ -546,9 +546,13 @@ class CakeSessionTest extends CakeTestCase {
 				'engine' => 'TestAppLibSession'
 			)
 		));
-		TestCakeSession::destroy();
+
+		TestCakeSession::start();
 		$this->assertTrue(TestCakeSession::started());
 
+		TestCakeSession::destroy();
+		$this->assertFalse(TestCakeSession::started());
+
 		App::build();
 	}
 
@@ -570,9 +574,12 @@ class CakeSessionTest extends CakeTestCase {
 			)
 		));
 
-		TestCakeSession::destroy();
+		TestCakeSession::start();
 		$this->assertTrue(TestCakeSession::started());
 
+		TestCakeSession::destroy();
+		$this->assertFalse(TestCakeSession::started());
+
 		App::build();
 	}
 
@@ -750,4 +757,31 @@ class CakeSessionTest extends CakeTestCase {
 		$this->assertEquals(400, Configure::read('Session.timeout'));
 	}
 
+/**
+ * Proves that invalid sessions will be destroyed and re-created
+ * if invalid
+ *
+ * @return void
+ */
+	public function testInvalidSessionRenew() {
+		TestCakeSession::start();
+		$this->assertNotEmpty($_SESSION['Config']);
+		$data = $_SESSION;
+
+		session_write_close();
+		$_SESSION = null;
+
+		TestCakeSession::start();
+		$this->assertEquals($data, $_SESSION);
+		TestCakeSession::write('Foo', 'Bar');
+
+		session_write_close();
+		$_SESSION = null;
+
+		TestCakeSession::userAgent('bogus!');
+		TestCakeSession::start();
+		$this->assertNotEquals($data, $_SESSION);
+		$this->assertEquals('bogus!', $_SESSION['Config']['userAgent']);
+	}
+
 }