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

when the session is marked as invalid
cakephp · Dec 7, 2013 · 848a0ce · 848a0ce
1 parent 06a89f1
commit 848a0ce
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 23 deletions.
diff --git a/lib/Cake/Model/Datasource/CakeSession.php b/lib/Cake/Model/Datasource/CakeSession.php
@@ -194,6 +194,7 @@ public static function start() {
 		}
 
 		self::$error = false;
+		self::$valid = true;
 		return self::started();
 	}
 
@@ -426,9 +427,14 @@ public static function write($name, $value = null) {
  * @return void
  */
 	public static function destroy() {
-		self::start();
+		if (!self::started()) {
+			self::_startSession();
+		}
+
 		session_destroy();
-		self::clear();
+
+		$_SESSION = null;
+		self::$id = null;
 	}
 
 /**
@@ -439,7 +445,6 @@ public static function destroy() {
 	public static function clear() {
 		$_SESSION = null;
 		self::$id = null;
-		self::start();
 		self::renew();
 	}
 
@@ -620,14 +625,11 @@ protected static function _startSession() {
  * @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'];
@@ -639,20 +641,29 @@ protected static function _checkValid() {
 						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.
  *

diff --git a/lib/Cake/Network/CakeResponse.php b/lib/Cake/Network/CakeResponse.php
@@ -629,12 +629,12 @@ public function statusCode($code = null) {
 /**
  * Queries & sets valid HTTP response codes & messages.
  *
- * @param integer|array $code If $code is an integer, then the corresponding code/message is 
- *        returned if it exists, null if it does not exist. If $code is an array, then the 
- *        keys are used as codes and the values as messages to add to the default HTTP 
- *        codes. The codes must be integers greater than 99 and less than 1000. Keep in 
- *        mind that the HTTP specification outlines that status codes begin with a digit 
- *        between 1 and 5, which defines the class of response the client is to expect. 
+ * @param integer|array $code If $code is an integer, then the corresponding code/message is
+ *        returned if it exists, null if it does not exist. If $code is an array, then the
+ *        keys are used as codes and the values as messages to add to the default HTTP
+ *        codes. The codes must be integers greater than 99 and less than 1000. Keep in
+ *        mind that the HTTP specification outlines that status codes begin with a digit
+ *        between 1 and 5, which defines the class of response the client is to expect.
  *        Example:
  *
  *        httpCodes(404); // returns array(404 => 'Not Found')

diff --git a/lib/Cake/Test/Case/Model/Datasource/CakeSessionTest.php b/lib/Cake/Test/Case/Model/Datasource/CakeSessionTest.php
@@ -548,9 +548,13 @@ public function testUsingAppLibsHandler() {
 				'engine' => 'TestAppLibSession'
 			)
 		));
-		TestCakeSession::destroy();
+
+		TestCakeSession::start();
 		$this->assertTrue(TestCakeSession::started());
 
+		TestCakeSession::destroy();
+		$this->assertFalse(TestCakeSession::started());
+
 		App::build();
 	}
 
@@ -572,9 +576,12 @@ public function testUsingPluginHandler() {
 			)
 		));
 
-		TestCakeSession::destroy();
+		TestCakeSession::start();
 		$this->assertTrue(TestCakeSession::started());
 
+		TestCakeSession::destroy();
+		$this->assertFalse(TestCakeSession::started());
+
 		App::build();
 	}
 
@@ -752,4 +759,31 @@ public function testCookieTimeoutFallback() {
 		$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']);
+	}
+
 }