From b198b43efdd7e5ea9a58089d2326cace7af25ab2 Mon Sep 17 00:00:00 2001 From: Lonnie Ezell Date: Wed, 30 Nov 2016 22:58:38 -0600 Subject: [PATCH] Misc session fixes, inspired by #318 --- application/Config/App.php | 14 ++-- system/Session/Handlers/BaseHandler.php | 12 +++- system/Session/Handlers/FileHandler.php | 57 +++++++++-------- system/Session/Handlers/MemcachedHandler.php | 42 ++++++------ system/Session/Handlers/RedisHandler.php | 67 ++++++++++---------- 5 files changed, 102 insertions(+), 90 deletions(-) diff --git a/application/Config/App.php b/application/Config/App.php index 65f726ee37d7..af05f0f36182 100644 --- a/application/Config/App.php +++ b/application/Config/App.php @@ -181,13 +181,13 @@ class App extends BaseConfig | except for 'cookie_prefix' and 'cookie_httponly', which are ignored here. | */ - public $sessionDriver = 'CodeIgniter\Session\Handlers\FileHandler'; - public $sessionCookieName = 'ci_session'; - public $sessionExpiration = 7200; - public $sessionSavePath = NULL; - public $sessionMatchIP = FALSE; - public $sessionTimeToUpdate = 300; - public $sessionRegenerateDestroy = FALSE; + public $sessionDriver = 'CodeIgniter\Session\Handlers\FileHandler'; + public $sessionCookieName = 'ci_session'; + public $sessionExpiration = 7200; + public $sessionSavePath = null; + public $sessionMatchIP = false; + public $sessionTimeToUpdate = 300; + public $sessionRegenerateDestroy = false; /* |-------------------------------------------------------------------------- diff --git a/system/Session/Handlers/BaseHandler.php b/system/Session/Handlers/BaseHandler.php index 671cbf208dc0..90df2ea5d020 100644 --- a/system/Session/Handlers/BaseHandler.php +++ b/system/Session/Handlers/BaseHandler.php @@ -106,6 +106,13 @@ abstract class BaseHandler implements \SessionHandlerInterface */ protected $sessionID; + /** + * The 'save path' for the session + * varies between + * @var mixed + */ + protected $savePath; + //-------------------------------------------------------------------- /** @@ -120,6 +127,7 @@ public function __construct(BaseConfig $config) $this->cookieSecure = $config->cookieSecure; $this->cookieName = $config->sessionCookieName; $this->matchIP = $config->sessionMatchIP; + $this->savePath = $config->sessionSavePath; } //-------------------------------------------------------------------- @@ -150,11 +158,11 @@ protected function destroyCookie(): bool * (databases other than PostgreSQL and MySQL) to act as if they * do acquire a lock. * - * @param string $session_id + * @param string $sessionID * * @return bool */ - protected function lockSession(string $session_id): bool + protected function lockSession(string $sessionID): bool { $this->lock = true; return true; diff --git a/system/Session/Handlers/FileHandler.php b/system/Session/Handlers/FileHandler.php index 675b68c623b5..cd893db0fb01 100644 --- a/system/Session/Handlers/FileHandler.php +++ b/system/Session/Handlers/FileHandler.php @@ -94,16 +94,17 @@ public function __construct(BaseConfig $config) //-------------------------------------------------------------------- - /** - * Open - * - * Sanitizes the save_path directory. - * - * @param string $savePath Path to session files' directory - * @param string $name Session cookie name - * - * @return bool - */ + /** + * Open + * + * Sanitizes the save_path directory. + * + * @param string $savePath Path to session files' directory + * @param string $name Session cookie name + * + * @return bool + * @throws \Exception + */ public function open($savePath, $name): bool { if ( ! is_dir($savePath)) @@ -135,27 +136,27 @@ public function open($savePath, $name): bool * * Reads session data and acquires a lock * - * @param string $session_id Session ID + * @param string $sessionID Session ID * * @return string Serialized session data */ - public function read($session_id) + public function read($sessionID) { // This might seem weird, but PHP 5.6 introduced session_reset(), // which re-reads session data if ($this->fileHandle === null) { - if (($this->fileHandle = fopen($this->filePath.$session_id, 'c+b')) === false) + if (($this->fileHandle = fopen($this->filePath.$sessionID, 'c+b')) === false) { - $this->logger->error("Session: Unable to open file '".$this->filePath.$session_id."'."); + $this->logger->error("Session: Unable to open file '".$this->filePath.$sessionID."'."); return false; } if (flock($this->fileHandle, LOCK_EX) === false) { - $this->logger->error("Session: Unable to obtain lock for file '".$this->filePath.$session_id."'."); + $this->logger->error("Session: Unable to obtain lock for file '".$this->filePath.$sessionID."'."); fclose($this->fileHandle); $this->fileHandle = null; @@ -163,11 +164,11 @@ public function read($session_id) } // Needed by write() to detect session_regenerate_id() calls - $this->sessionID = $session_id; + $this->sessionID = $sessionID; if ($this->fileNew) { - chmod($this->filePath.$session_id, 0600); + chmod($this->filePath.$sessionID, 0600); $this->fingerprint = md5(''); return ''; @@ -179,7 +180,7 @@ public function read($session_id) } $session_data = ''; - for ($read = 0, $length = filesize($this->filePath.$session_id); $read < $length; $read += strlen($buffer)) + for ($read = 0, $length = filesize($this->filePath.$sessionID); $read < $length; $read += strlen($buffer)) { if (($buffer = fread($this->fileHandle, $length - $read)) === false) { @@ -201,16 +202,16 @@ public function read($session_id) * * Writes (create / update) session data * - * @param string $session_id Session ID - * @param string $session_data Serialized session data + * @param string $sessionID Session ID + * @param string $sessionData Serialized session data * * @return bool */ - public function write($session_id, $session_data): bool + public function write($sessionID, $sessionData): bool { // 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->sessionID && ( ! $this->close() || $this->read($session_id) === false)) + if ($sessionID !== $this->sessionID && (! $this->close() || $this->read($sessionID) === false)) { return false; } @@ -219,11 +220,11 @@ public function write($session_id, $session_data): bool { return false; } - elseif ($this->fingerprint === md5($session_data)) + elseif ($this->fingerprint === md5($sessionData)) { return ($this->fileNew) ? true - : touch($this->filePath.$session_id); + : touch($this->filePath.$sessionID); } if ( ! $this->fileNew) @@ -232,11 +233,11 @@ public function write($session_id, $session_data): bool rewind($this->fileHandle); } - if (($length = strlen($session_data)) > 0) + if (($length = strlen($sessionData)) > 0) { for ($written = 0; $written < $length; $written += $result) { - if (($result = fwrite($this->fileHandle, substr($session_data, $written))) === false) + if (($result = fwrite($this->fileHandle, substr($sessionData, $written))) === false) { break; } @@ -244,14 +245,14 @@ public function write($session_id, $session_data): bool if ( ! is_int($result)) { - $this->_fingerprint = md5(substr($session_data, 0, $written)); + $this->fingerprint = md5(substr($sessionData, 0, $written)); $this->logger->error('Session: Unable to write data.'); return false; } } - $this->fingerprint = md5($session_data); + $this->fingerprint = md5($sessionData); return true; } diff --git a/system/Session/Handlers/MemcachedHandler.php b/system/Session/Handlers/MemcachedHandler.php index e4b7175767c5..b904eeed339c 100644 --- a/system/Session/Handlers/MemcachedHandler.php +++ b/system/Session/Handlers/MemcachedHandler.php @@ -75,7 +75,7 @@ class MemcachedHandler extends BaseHandler implements \SessionHandlerInterface /** * Constructor - * + * * @param BaseConfig $config * @throws \Exception */ @@ -166,18 +166,18 @@ public function open($save_path, $name) * * Reads session data and acquires a lock * - * @param string $session_id Session ID + * @param string $sessionID Session ID * * @return string Serialized session data */ - public function read($session_id) + public function read($sessionID) { - if (isset($this->memcached) && $this->lockSession($session_id)) + if (isset($this->memcached) && $this->lockSession($sessionID)) { // Needed by write() to detect session_regenerate_id() calls - $this->sessionID = $session_id; + $this->sessionID = $sessionID; - $session_data = (string)$this->memcached->get($this->keyPrefix.$session_id); + $session_data = (string)$this->memcached->get($this->keyPrefix.$sessionID); $this->fingerprint = md5($session_data); return $session_data; @@ -193,38 +193,38 @@ public function read($session_id) * * Writes (create / update) session data * - * @param string $session_id Session ID - * @param string $session_data Serialized session data + * @param string $sessionID Session ID + * @param string $sessionData Serialized session data * * @return bool */ - public function write($session_id, $session_data) + public function write($sessionID, $sessionData) { if ( ! isset($this->memcached)) { return false; } // Was the ID regenerated? - elseif ($session_id !== $this->sessionID) + elseif ($sessionID !== $this->sessionID) { - if ( ! $this->releaseLock() || ! $this->lockSession($session_id)) + if ( ! $this->releaseLock() || ! $this->lockSession($sessionID)) { return false; } $this->fingerprint = md5(''); - $this->sessionID = $session_id; + $this->sessionID = $sessionID; } if (isset($this->lockKey)) { $this->memcached->replace($this->lockKey, time(), 300); - if ($this->fingerprint !== ($fingerprint = md5($session_data))) + if ($this->fingerprint !== ($fingerprint = md5($sessionData))) { - if ($this->memcached->set($this->keyPrefix.$session_id, $session_data, $this->sessionExpiration)) + if ($this->memcached->set($this->keyPrefix.$sessionID, $sessionData, $this->sessionExpiration)) { - $this->_fingerprint = $fingerprint; + $this->fingerprint = $fingerprint; return true; } @@ -232,7 +232,7 @@ public function write($session_id, $session_data) return false; } - return $this->memcached->touch($this->keyPrefix.$session_id, $this->sessionExpiration); + return $this->memcached->touch($this->keyPrefix.$sessionID, $this->sessionExpiration); } return false; @@ -313,11 +313,11 @@ public function gc($maxlifetime) * * Acquires an (emulated) lock. * - * @param string $session_id Session ID + * @param string $sessionID Session ID * * @return bool */ - protected function lockSession(string $session_id): bool + protected function lockSession(string $sessionID): bool { if (isset($this->lockKey)) { @@ -325,7 +325,7 @@ protected function lockSession(string $session_id): bool } // 30 attempts to obtain a lock, in case another request already has it - $lock_key = $this->keyPrefix.$session_id.':lock'; + $lock_key = $this->keyPrefix.$sessionID.':lock'; $attempt = 0; do @@ -338,7 +338,7 @@ protected function lockSession(string $session_id): bool if ( ! $this->memcached->set($lock_key, time(), 300)) { - $this->logger->error('Session: Error while trying to obtain lock for '.$this->keyPrefix.$session_id); + $this->logger->error('Session: Error while trying to obtain lock for '.$this->keyPrefix.$sessionID); return false; } @@ -350,7 +350,7 @@ protected function lockSession(string $session_id): bool if ($attempt === 30) { - $this->logger->error('Session: Unable to obtain lock for '.$this->keyPrefix.$session_id.' after 30 attempts, aborting.'); + $this->logger->error('Session: Unable to obtain lock for '.$this->keyPrefix.$sessionID.' after 30 attempts, aborting.'); return false; } diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 8c33abbe481a..ed9ee4e2f35a 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -76,7 +76,7 @@ class RedisHandler extends BaseHandler implements \SessionHandlerInterface /** * Constructor - * + * * @param BaseConfig $config * @throws \Exception */ @@ -139,13 +139,13 @@ public function open($save_path, $name) { $this->logger->error('Session: Unable to connect to Redis with the configured settings.'); } - elseif (isset($this->_config['save_path']['password']) && ! $redis->auth($this->_config['save_path']['password'])) + elseif (isset($this->savePath['password']) && ! $redis->auth($this->savePath['password'])) { $this->logger->error('Session: Unable to authenticate to Redis instance.'); } - elseif (isset($this->_config['save_path']['database']) && ! $redis->select($this->_config['save_path']['database'])) + elseif (isset($this->savePath['database']) && ! $redis->select($this->savePath['database'])) { - $this->logger->error('Session: Unable to select Redis database with index '.$this->_config['save_path']['database']); + $this->logger->error('Session: Unable to select Redis database with index '.$this->savePath['database']); } else { @@ -163,18 +163,19 @@ public function open($save_path, $name) * * Reads session data and acquires a lock * - * @param string $session_id Session ID - * @return string Serialized session data + * @param string $sessionID Session ID + * + * @return string Serialized session data */ - public function read($session_id) + public function read($sessionID) { - if (isset($this->redis) && $this->lockSession($session_id)) + if (isset($this->redis) && $this->lockSession($sessionID)) { // Needed by write() to detect session_regenerate_id() calls - $this->_session_id = $session_id; + $this->sessionID = $sessionID; - $session_data = (string) $this->redis->get($this->keyPrefix.$session_id); - $this->_fingerprint = md5($session_data); + $session_data = (string) $this->redis->get($this->keyPrefix.$sessionID); + $this->fingerprint = md5($session_data); return $session_data; } @@ -189,44 +190,45 @@ public function read($session_id) * * Writes (create / update) session data * - * @param string $session_id Session ID - * @param string $session_data Serialized session data - * @return bool + * @param string $sessionID Session ID + * @param string $sessionData Serialized session data + * + * @return bool */ - public function write($session_id, $session_data) + public function write($sessionID, $sessionData) { if ( ! isset($this->redis)) { return FALSE; } // Was the ID regenerated? - elseif ($session_id !== $this->sessionID) + elseif ($sessionID !== $this->sessionID) { - if ( ! $this->releaseLock() || ! $this->lockSession($session_id)) + if ( ! $this->releaseLock() || ! $this->lockSession($sessionID)) { return FALSE; } - $this->_fingerprint = md5(''); - $this->_session_id = $session_id; + $this->fingerprint = md5(''); + $this->sessionID = $sessionID; } if (isset($this->lockKey)) { $this->redis->setTimeout($this->lockKey, 300); - if ($this->fingerprint !== ($fingerprint = md5($session_data))) + if ($this->fingerprint !== ($fingerprint = md5($sessionData))) { - if ($this->redis->set($this->keyPrefix.$session_id, $session_data, $this->sessionExpiration)) + if ($this->redis->set($this->keyPrefix.$sessionID, $sessionData, $this->sessionExpiration)) { - $this->_fingerprint = $fingerprint; + $this->fingerprint = $fingerprint; return TRUE; } return FALSE; } - return $this->redis->setTimeout($this->keyPrefix.$session_id, $this->sessionExpiration); + return $this->redis->setTimeout($this->keyPrefix.$sessionID, $this->sessionExpiration); } return FALSE; @@ -317,10 +319,11 @@ public function gc($maxlifetime) * * Acquires an (emulated) lock. * - * @param string $session_id Session ID - * @return bool + * @param string $sessionID Session ID + * + * @return bool */ - protected function lockSession(string $session_id): bool + protected function lockSession(string $sessionID): bool { if (isset($this->lockKey)) { @@ -328,7 +331,7 @@ protected function lockSession(string $session_id): bool } // 30 attempts to obtain a lock, in case another request already has it - $lock_key = $this->keyPrefix.$session_id.':lock'; + $lock_key = $this->keyPrefix.$sessionID.':lock'; $attempt = 0; do @@ -341,7 +344,7 @@ protected function lockSession(string $session_id): bool if ( ! $this->redis->setex($lock_key, 300, time())) { - $this->logger->error('Session: Error while trying to obtain lock for '.$this->keyPrefix.$session_id); + $this->logger->error('Session: Error while trying to obtain lock for '.$this->keyPrefix.$sessionID); return FALSE; } @@ -352,12 +355,12 @@ protected function lockSession(string $session_id): bool if ($attempt === 30) { - log_message('error', 'Session: Unable to obtain lock for '.$this->keyPrefix.$session_id.' after 30 attempts, aborting.'); + log_message('error', 'Session: Unable to obtain lock for '.$this->keyPrefix.$sessionID.' after 30 attempts, aborting.'); return FALSE; } elseif ($ttl === -1) { - log_message('debug', 'Session: Lock for '.$this->keyPrefix.$session_id.' had no TTL, overriding.'); + log_message('debug', 'Session: Lock for '.$this->keyPrefix.$sessionID.' had no TTL, overriding.'); } $this->lock = TRUE; @@ -389,7 +392,7 @@ protected function releaseLock(): bool return TRUE; } - + //-------------------------------------------------------------------- - + }