#3073 (feature/session): Fix session_regenerate_id() issues

5 files changed, 95 insertions, 27 deletions

diff --git a/system/libraries/Session/Session_driver.php b/system/libraries/Session/Session_driver.php
index fb695dade..ad64e238a 100644
--- a/system/libraries/Session/Session_driver.php
+++ b/system/libraries/Session/Session_driver.php
@@ -53,6 +53,16 @@ abstract class CI_Session_driver implements SessionHandlerInterface {
 	 */
 	protected $_lock = FALSE;
 
+	/**
+	 * Read session ID
+	 *
+	 * Used to detect session_regenerate_id() calls because PHP only calls
+	 * write() after regenerating the ID.
+	 *
+	 * @var	string
+	 */
+	protected $_session_id;
+
 	// ------------------------------------------------------------------------
 
 	/**
diff --git a/system/libraries/Session/drivers/Session_database_driver.php b/system/libraries/Session/drivers/Session_database_driver.php
index e3a3c505e..9e74605bc 100644
--- a/system/libraries/Session/drivers/Session_database_driver.php
+++ b/system/libraries/Session/drivers/Session_database_driver.php
@@ -111,6 +111,9 @@ class CI_Session_database_driver extends CI_Session_driver implements SessionHan
 	{
 		if ($this->_get_lock($session_id) !== FALSE)
 		{
+			// Needed by write() to detect session_regenerate_id() calls
+			$this->_session_id = $session_id;
+
 			$this->_db
 				->select('data')
 				->from($this->_config['save_path'])
@@ -142,6 +145,17 @@ class CI_Session_database_driver extends CI_Session_driver implements SessionHan
 		{
 			return FALSE;
 		}
+		// Was the ID regenerated?
+		elseif ($session_id !== $this->_session_id)
+		{
+			if ( ! $this->_release_lock() OR ! $this->_get_lock($session_id))
+			{
+				return FALSE;
+			}
+
+			$this->_row_exists = FALSE;
+			$this->_session_id = $session_id;
+		}
 
 		if ($this->_row_exists === FALSE)
 		{
diff --git a/system/libraries/Session/drivers/Session_files_driver.php b/system/libraries/Session/drivers/Session_files_driver.php
index ff1553f84..3d6fa6322 100644
--- a/system/libraries/Session/drivers/Session_files_driver.php
+++ b/system/libraries/Session/drivers/Session_files_driver.php
@@ -114,36 +114,37 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle
 		// which re-reads session data
 		if ($this->_file_handle === NULL)
 		{
-			$this->_file_path .= $session_id;
-
 			// Just using fopen() with 'c+b' mode would be perfect, but it is only
 			// available since PHP 5.2.6 and we have to set permissions for new files,
 			// so we'd have to hack around this ...
-			if (($this->_file_new = ! file_exists($this->_file_path)) === TRUE)
+			if (($this->_file_new = ! file_exists($this->_file_path.$session_id)) === TRUE)
 			{
-				if (($this->_file_handle = fopen($this->_file_path, 'w+b')) === FALSE)
+				if (($this->_file_handle = fopen($this->_file_path.$session_id, 'w+b')) === FALSE)
 				{
-					log_message('error', "Session: File '".$this->_file_path."' doesn't exist and cannot be created.");
+					log_message('error', "Session: File '".$this->_file_path.$session_id."' doesn't exist and cannot be created.");
 					return FALSE;
 				}
 			}
-			elseif (($this->_file_handle = fopen($this->_file_path, 'r+b')) === FALSE)
+			elseif (($this->_file_handle = fopen($this->_file_path.$session_id, 'r+b')) === FALSE)
 			{
-				log_message('error', "Session: Unable to open file '".$this->_file_path."'.");
+				log_message('error', "Session: Unable to open file '".$this->_file_path.$session_id."'.");
 				return FALSE;
 			}
 
 			if (flock($this->_file_handle, LOCK_EX) === FALSE)
 			{
-				log_message('error', "Session: Unable to obtain lock for file '".$this->_file_path."'.");
+				log_message('error', "Session: Unable to obtain lock for file '".$this->_file_path.$session_id."'.");
 				fclose($this->_file_handle);
 				$this->_file_handle = NULL;
 				return FALSE;
 			}
 
+			// Needed by write() to detect session_regenerate_id() calls
+			$this->_session_id = $session_id;
+
 			if ($this->_file_new)
 			{
-				chmod($this->_file_path, 0600);
+				chmod($this->_file_path.$session_id, 0600);
 				$this->_fingerprint = md5('');
 				return '';
 			}
@@ -154,7 +155,7 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle
 		}
 
 		$session_data = '';
-		for ($read = 0, $length = filesize($this->_file_path); $read < $length; $read += strlen($buffer))
+		for ($read = 0, $length = filesize($this->_file_path.$session_id); $read < $length; $read += strlen($buffer))
 		{
 			if (($buffer = fread($this->_file_handle, $length - $read)) === FALSE)
 			{
@@ -170,6 +171,13 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle
 
 	public function write($session_id, $session_data)
 	{
+		// If the two IDs don't match, we have a session_regenerate_id() call
+		// and we need to close the old handle and open a new one
+		if ($session_id !== $this->_session_id && ( ! $this->close() OR $this->read($session_id) === FALSE))
+		{
+			return FALSE;
+		}
+
 		if ( ! is_resource($this->_file_handle))
 		{
 			return FALSE;
@@ -178,7 +186,7 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle
 		{
 			return ($this->_file_new)
 				? TRUE
-				: touch($this->_file_path);
+				: touch($this->_file_path.$session_id);
 		}
 
 		if ( ! $this->_file_new)
@@ -218,11 +226,11 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle
 			flock($this->_file_handle, LOCK_UN);
 			fclose($this->_file_handle);
 
-			$this->_file_handle = $this->_file_new = NULL;
+			$this->_file_handle = $this->_file_new = $this->_session_id = NULL;
 			return TRUE;
 		}
 
-		return FALSE;
+		return TRUE;
 	}
 
 	// ------------------------------------------------------------------------
@@ -231,13 +239,13 @@ class CI_Session_files_driver extends CI_Session_driver implements SessionHandle
 	{
 		if ($this->close())
 		{
-			return unlink($this->_file_path) && $this->_cookie_destroy();
+			return unlink($this->_file_path.$session_id) && $this->_cookie_destroy();
 		}
 		elseif ($this->_file_path !== NULL)
 		{
 			clearstatcache();
-			return file_exists($this->_file_path)
-				? (unlink($this->_file_path) && $this->_cookie_destroy())
+			return file_exists($this->_file_path.$session_id)
+				? (unlink($this->_file_path.$session_id) && $this->_cookie_destroy())
 				: TRUE;
 		}
 
diff --git a/system/libraries/Session/drivers/Session_memcached_driver.php b/system/libraries/Session/drivers/Session_memcached_driver.php
index 318c11afa..8905e8d6f 100644
--- a/system/libraries/Session/drivers/Session_memcached_driver.php
+++ b/system/libraries/Session/drivers/Session_memcached_driver.php
@@ -133,6 +133,9 @@ class CI_Session_memcached_driver extends CI_Session_driver implements SessionHa
 	{
 		if (isset($this->_memcached) && $this->_get_lock($session_id))
 		{
+			// Needed by write() to detect session_regenerate_id() calls
+			$this->_session_id = $session_id;
+
 			$session_data = (string) $this->_memcached->get($this->_key_prefix.$session_id);
 			$this->_fingerprint = md5($session_data);
 			return $session_data;
@@ -143,7 +146,23 @@ class CI_Session_memcached_driver extends CI_Session_driver implements SessionHa
 
 	public function write($session_id, $session_data)
 	{
-		if (isset($this->_memcached, $this->_lock_key))
+		if ( ! isset($this->_memcached))
+		{
+			return FALSE;
+		}
+		// Was the ID regenerated?
+		elseif ($session_id !== $this->_session_id)
+		{
+			if ( ! $this->_release_lock() OR ! $this->_get_lock($session_id))
+			{
+				return FALSE;
+			}
+
+			$this->_fingerprint = md5('');
+			$this->_session_id = $session_id;
+		}
+
+		if (isset($this->_lock_key))
 		{
 			$this->_memcached->replace($this->_lock_key, time(), 5);
 			if ($this->_fingerprint !== ($fingerprint = md5($session_data)))
@@ -189,16 +208,17 @@ class CI_Session_memcached_driver extends CI_Session_driver implements SessionHa
 		if (isset($this->_memcached, $this->_lock_key))
 		{
 			$this->_memcached->delete($this->_key_prefix.$session_id);
-			return ($this->_cookie_destroy() && $this->close());
+			return $this->_cookie_destroy();
 		}
 
-		return $this->close();
+		return FALSE;
 	}
 
 	// ------------------------------------------------------------------------
 
 	public function gc($maxlifetime)
 	{
+		// Not necessary, Memcached takes care of that.
 		return TRUE;
 	}
 
diff --git a/system/libraries/Session/drivers/Session_redis_driver.php b/system/libraries/Session/drivers/Session_redis_driver.php
index ef18defe2..bc6150d2d 100644
--- a/system/libraries/Session/drivers/Session_redis_driver.php
+++ b/system/libraries/Session/drivers/Session_redis_driver.php
@@ -135,6 +135,9 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle
 	{
 		if (isset($this->_redis) && $this->_get_lock($session_id))
 		{
+			// Needed by write() to detect session_regenerate_id() calls
+			$this->_session_id = $session_id;
+
 			$session_data = (string) $this->_redis->get($this->_key_prefix.$session_id);
 			$this->_fingerprint = md5($session_data);
 			return $session_data;
@@ -145,7 +148,23 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle
 
 	public function write($session_id, $session_data)
 	{
-		if (isset($this->_redis, $this->_lock_key))
+		if ( ! isset($this->_redis))
+		{
+			return FALSE;
+		}
+		// Was the ID regenerated?
+		elseif ($session_id !== $this->_session_id)
+		{
+			if ( ! $this->_release_lock() OR ! $this->_get_lock($session_id))
+			{
+				return FALSE;
+			}
+
+			$this->_fingerprint = md5('');
+			$this->_session_id = $session_id;
+		}
+
+		if (isset($this->_lock_key))
 		{
 			$this->_redis->setTimeout($this->_lock_key, 5);
 			if ($this->_fingerprint !== ($fingerprint = md5($session_data)))
@@ -190,7 +209,7 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle
 			return TRUE;
 		}
 
-		return FALSE;
+		return TRUE;
 	}
 
 	// ------------------------------------------------------------------------
@@ -204,20 +223,17 @@ class CI_Session_redis_driver extends CI_Session_driver implements SessionHandle
 				log_message('debug', 'Session: Redis::delete() expected to return 1, got '.var_export($result, TRUE).' instead.');
 			}
 
-			return ($this->_cookie_destroy() && $this->close());
+			return $this->_cookie_destroy();
 		}
 
-		return $this->close();
+		return FALSE;
 	}
 
 	// ------------------------------------------------------------------------
 
 	public function gc($maxlifetime)
 	{
-		// TODO: keys()/getKeys() is said to be performance-intensive,
-		// although it supports patterns (*, [charlist] at the very least).
-		// scan() seems to be recommended, but requires redis 2.8
-		// Not sure if we need any of these though, as we set keys with expire times
+		// Not necessary, Redis takes care of that.
 		return TRUE;
 	}