patternphpMinor
Simple PHP session handler class (using MySQL for session data storage)
Viewed 0 times
simpleclassphpstoragedatamysqlusingsessionforhandler
Problem
I have tried to write a small light weighted php session handling class that use PHP's
sessionmanager.lib.php:
```
/
class sessionmanager
{
use Singleton;
/**
* [$_db PDO object holder]
* @var [object]
*/
private $_db;
/**
* [$_https Cookie secure flag holder]
* @var [boolean]
*/
private $_https;
/**
* [$_user_agent User Agent holder]
* @var [string]
*/
private $_user_agent;
/**
* [$_ip_address Client Machien IP address]
* @var [string]
*/
private $_ip_address;
/**
* [$_expiry Session LIfetime, default 2 Hours ]
* @var integer
*/
private $_expiry = 7200;
/**
* [$_session_cookie_ttl Session Cookie Lifetime , default (0:Clear the session cookies on browser close) ]
* @var integer
*/
private $_session_cookie_ttl = 0;
/**
* [$_refresh_interval Refresh Interval toi regenerate Session Id, default 10 minutes]
* @var integer
*/
private $_refresh_interval = 600;
/**
* [$_table_name Tbale name for storing Session information]
* @var string
*/
private $_table_name = "sessions";
/**
* [$_session_id Holder for session_id]
* @var [string]
*/
private $_session_id;
/**
* Sescure session Salt
* @ClassConstant
*/
const SECURE_SESSION = '--$ecure$ess10n--';
/**
* [__construct ,Pass configuration values to __setconfig, register session handlers and starts the sesssion]
* @param array $config [array of configuartion para
session_set_save_handler() function to overwrite the default session handling functionalities and usage Database to store the sessions data instead of default files system. It checks for possible session hijacking attempts and renews session periodically. I want to know how feasible my class is and improvements that can be incorporated to make it more robust and secure.sessionmanager.lib.php:
```
/
class sessionmanager
{
use Singleton;
/**
* [$_db PDO object holder]
* @var [object]
*/
private $_db;
/**
* [$_https Cookie secure flag holder]
* @var [boolean]
*/
private $_https;
/**
* [$_user_agent User Agent holder]
* @var [string]
*/
private $_user_agent;
/**
* [$_ip_address Client Machien IP address]
* @var [string]
*/
private $_ip_address;
/**
* [$_expiry Session LIfetime, default 2 Hours ]
* @var integer
*/
private $_expiry = 7200;
/**
* [$_session_cookie_ttl Session Cookie Lifetime , default (0:Clear the session cookies on browser close) ]
* @var integer
*/
private $_session_cookie_ttl = 0;
/**
* [$_refresh_interval Refresh Interval toi regenerate Session Id, default 10 minutes]
* @var integer
*/
private $_refresh_interval = 600;
/**
* [$_table_name Tbale name for storing Session information]
* @var string
*/
private $_table_name = "sessions";
/**
* [$_session_id Holder for session_id]
* @var [string]
*/
private $_session_id;
/**
* Sescure session Salt
* @ClassConstant
*/
const SECURE_SESSION = '--$ecure$ess10n--';
/**
* [__construct ,Pass configuration values to __setconfig, register session handlers and starts the sesssion]
* @param array $config [array of configuartion para
Solution
When looking over your code I find several things I would change. It should be said that I am no expert in security, so I will forward you to places where you can get better guidance than I can about the specific issue. So without further ado, let's go!
The first thing which pops into my mind when reading your code is the singleton. The singleton pattern is an anti-pattern. And it doesn't make much sense in this case either. There should only exist one instance of a session handler at any time. Since you register the handler once and then move on the singleton is obsolete. Your class provides no interface for use outside of the handler (which is good; separating concerns!), so there is not reason for a global accessor, which the singleton
Next I would make your class implement the native interface
Then I would also rename your class.
I would also urge you to avoid using destructors when dealing with critical classes such as a session handlers. There are edge cases were destructors never will be executed. An example is a fatal error. You are relying on a
Before moving on to some general things, I would like to talk about changing INI settings inside classes. You are changing almost every setting regarding sessions inside the class. As far as I know the values you are using are good, security related. But you are also hard-coding several settings. These settings will always overwrite any settings declared inside the
Now to some code changes. There are several places where you use
You are calling garbage collection (on sessions, not the PHP runtime) each time a session is closed. This may not seem like a big deal when there are a small amount of sessions stored in your database. But imagine that at some point 100 sessions are stored. Each time a request finishes your database has to loop through each of the 100 sessions to check if any of them are stale. This is unnecessary and creates a slower response time from your application as well as halt any reads from other database connections while this is done. PHP defines two INI settings which are related to garbage collection:
Instead of checking for renewal/expiration each time you read the session I would argue you should add a timestamp check to your SQL query. An example could be:
This would effectively only fetch valid sessions. If a session has exceeded its max lifetime is should be considered invalid. The
The first thing which pops into my mind when reading your code is the singleton. The singleton pattern is an anti-pattern. And it doesn't make much sense in this case either. There should only exist one instance of a session handler at any time. Since you register the handler once and then move on the singleton is obsolete. Your class provides no interface for use outside of the handler (which is good; separating concerns!), so there is not reason for a global accessor, which the singleton
getInstance() method is.Next I would make your class implement the native interface
SessionHandlerInterface. This interface was created with the idea of userland code overwriting the native session handler implementation, which is exactly what you are doing. This also means you can simplify the handler registration inside the constructor to:session_set_save_handler($this);Then I would also rename your class.
SessionManager is very broad and you may in the future make a handler implementation using a memory-based storage engine like Memcached or Redis. Therefore I would rename your class into PDOSessionHandler as this clearly shows the storage implementation used.I would also urge you to avoid using destructors when dealing with critical classes such as a session handlers. There are edge cases were destructors never will be executed. An example is a fatal error. You are relying on a
__destruct() method to register a shutdown function, which is good, but could be moved into the constructor. This will ensure that when your class was successfully instantiated the shutdown function is also registered.Before moving on to some general things, I would like to talk about changing INI settings inside classes. You are changing almost every setting regarding sessions inside the class. As far as I know the values you are using are good, security related. But you are also hard-coding several settings. These settings will always overwrite any settings declared inside the
php.ini file. This may be confusing and can lead to bugs. I think settings like session.entrophy_file, session.entrophy_length, session.hash_function, session.use_only_cookies and session.cookie_secure should only be changed from a php.ini file. The hashing algorithm should be changeable without modifying your class and the same goes for the previously mentioned setings. There are situations where the php.ini isn't under your control. If this is the case I would argue that these settings should be set in an application wide configuration file.Now to some code changes. There are several places where you use
try-catch blocks. They are good for checking for errors, but can be misused. You are catching the base Exception class. This can lead to bugs where exceptions, which are unrelated to PDO, are thrown, then caught and silently disposed. Also echoing exception messages is bad practice. Consider an exception is thrown and the message contains sensitive information such as your database type, host or username/password. Any visitor of your site when this exception occurs will see this critical information too. A malicious user would love this.You are calling garbage collection (on sessions, not the PHP runtime) each time a session is closed. This may not seem like a big deal when there are a small amount of sessions stored in your database. But imagine that at some point 100 sessions are stored. Each time a request finishes your database has to loop through each of the 100 sessions to check if any of them are stale. This is unnecessary and creates a slower response time from your application as well as halt any reads from other database connections while this is done. PHP defines two INI settings which are related to garbage collection:
session.gc_probalility and session.gc_divisor. By default the divisor is set to 100 and the probability is set to 1. This means that there is a 1% chance of invoking garbage collection. If you increase the probability to 5 there is a 5% chance and so on. You can tweak these settings to the values required for your application. These settings should also be changeable outside your class preferably in the INI file or a application wide configurations.Instead of checking for renewal/expiration each time you read the session I would argue you should add a timestamp check to your SQL query. An example could be:
SELECT session_data FROM sessions WHERE session_id = :id AND updated < :expireThis would effectively only fetch valid sessions. If a session has exceeded its max lifetime is should be considered invalid. The
read() method would then return an empty set of data and the old session would linger around in the database Code Snippets
session_set_save_handler($this);SELECT session_data FROM sessions WHERE session_id = :id AND updated < :expirepublic function read($id)
{
$sth = $this->pdo->prepare('SELECT session_data FROM session WHERE session_id = :id AND updated < :expire');
$expire = time() - (int) $this->lifetime;
$sth->bindParam(':id', $id, \PDO::PARAM_STR);
$sth->bindParam(':expire', $expire, \PDO::PARAM_INT);
if(!$sth->execute()) {
throw new \RuntimeException('Could not execute session read query.');
}
if($sth->numCount() > 0) {
$row = $sth->fetch(\PDO::FETCH_ASSOC);
return $row['session_data'];
}
return '';
}Context
StackExchange Code Review Q#100448, answer score: 5
Revisions (0)
No revisions yet.