PHP > 7.1 - MySQL Session Handler Class with some built-in time-management/security The...

Prepend last line of stdin to entire stdin

Define command that accepts \ in arguments

Is there a reasonable and studied concept of reduction between regular lagnagues?

New carbon wheel brake pads after use on aluminum wheel?

Is a distribution that is normal, but highly skewed, considered Gaussian?

Can Plant Growth be repeatedly cast on the same area to exponentially increase the yield of harvests there (more than twice)?

How to avoid supervisors with prejudiced views?

Aggressive Under-Indexing and no data for missing index

Is there such a thing as never melting snow?

Can I add a classname to CSS variable?

Does Germany produce more waste than the US?

What flight has the highest ratio of timezone difference to flight time?

My ex-girlfriend uses my Apple ID to login to her iPad, do I have to give her my Apple ID password to reset it?

Decide between Polyglossia and Babel for LuaLaTeX in 2019

If Nick Fury and Coulson already knew about aliens (Kree and Skrull) why did they wait until Thor's appearance to start making weapons?

How to penalize for empty fields in a DataFrame?

Sulfuric acid symmetry point group

(How) Could a medieval fantasy world survive a magic-induced "nuclear winter"?

From jafe to El-Guest

Would a completely good Muggle be able to use a wand?

Yu-Gi-Oh cards in Python 3

What is the difference between Statistical Mechanics and Quantum Mechanics

How many extra stops do monopods offer for tele photographs?

Inexact numbers as keys in Association?



PHP > 7.1 - MySQL Session Handler Class with some built-in time-management/security



The Next CEO of Stack OverflowCustom session handler class - is it robust enough?Php session wrapper classBasic PHP Factory PatternPHP Session handling classPHP Session Wrapper ClassCSRF Token implementationSimple PHP session handler class (using MySQL for session data storage)Database Session ClassValidating user credentials and logging into a Symfony siteTime-based session management (module)












2












$begingroup$


I've created a class that implements the native SessionHandlerInterface (with added PHP v7+ functionality) while adding some minor security features.
My main goal was to maintain native methods for session handling while transparently adding the following features:





  1. Minor protection from session-hijacking by using client user agent as a "request signature." I decided not to use client IP or remote IP as they are highly unreliable and somewhat easy to spoof. I decided to hash the result to save space in the db and because I have no reliable way to know how long a particular user-agent data string will be (new or updated browser data may be much longer than the present).


  2. Built-in protection from session-fixation by forcing sessions to regenerate sid every x number of seconds.


  3. Built-in session expiration without relying on GC method (which does not always happen and is very unpredictable). The native GC was meant to clean up resources rather than manage session time limits. Most devs include timestamps inside of the $_SESSION global itself. Personally, I think this is poor design as they can accidentally be overridden and handling will need to explicitly be called at every request.


I avoided using prepared statements as they take an extra call to the db. I've always advocated against prepared statements for single queries - they are a lazy way of ensuring data is properly escaped and they take almost twice as long to get a result.



I decided against using INSERT ON DUPLICATE KEY UPDATE to ensure that ONLY new sessions would be added to the db (in the case that two duplicate sids are created before either is written - although the chance of this happening is just about non-existent). Also, a single insert or a single update is a tad quicker.



I have been testing this class and it's been working well... except I can see this being too slow for a high traffic site. The main bottleneck is connection and queries to the database (can't do much about that). I'd like to expand on the request signature but I can't think of a reliable way to uniquely identify a client. I'd also appreciate any feedback/criticism about this class as I'd like to expand on it. Thank you!



Here is the code:



<?php
class SessionAuthException extends RuntimeException{
public function __construct($message = 'Unknown reason', $code = 0, Exception $previous = null){
parent::__construct('Session could not be authenticated: ' . $message, $code, $previous);
}
}

class MySQLSessionHandler implements SessionHandlerInterface{
private $link;
private $cOptions = [
'regen_interval' => 600,
'idle_timeout' => 3600];

private $curReqTime;
private $reqSignature;

private $sesInitTime;
private $lastReqTime;
private $sesExpireTime;
private $numWrites;

private $isNew = false;
private $doExpire = false;

public function __construct(mysqli $link, bool $autoInit = true, array $cOptions = []){
$this->link = $link;
$this->curReqTime = $_SERVER['REQUEST_TIME'];
$this->reqSignature = md5($_SERVER['HTTP_USER_AGENT']);

if( $autoInit ){
session_set_save_handler($this);
}

/* TODO: cOptions from args */

return;
}

public function getInitTime(){
return $this->sesInitTime;
}

public function getCurReqTime(){
return $this->curReqTime;
}

public function getLastRequestTime(){
return $this->lastReqTime;
}

public function getExpireTime(){
return $this->sesExpireTime;
}

public function getNumWrites(){
return $this->numWrites;
}

public function setExpire(bool $doExpire = true){
$this->doExpire = $doExpire;
return;
}

public function isMarkedExpire(){
return $this->doExpire;
}

public function start(array $options = []){
try{
@session_start($options);
if( isset($this->cOptions['regen_interval']) && $this->sesInitTime < $this->curReqTime - $this->cOptions['regen_interval'] ){
$this->doExpire = true;
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Unable to authenticate session - setting sid to null will create a new session without destroying the old (possibly hijacked) */
session_id(null);
session_start($options);
}
}

public function open($savePath, $sessionName) : bool{
/* mysqli->ping() returns null if connection has been closed */
return @$this->link->ping() ?: false;
}

public function create_sid() : string{
$checkCollision = session_status() == PHP_SESSION_ACTIVE;

$sid_len = ini_get('session.sid_length');
$sid_bpc = ini_get('session.sid_bits_per_character');
$bytes_needed = ceil($sid_len * $sid_bpc / 8);
$mask = (1 << $sid_bpc) - 1;
$out = '';

$hexconvtab = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-';

$attempts = 0;
$maxAttempts = 5;
do{
if( $attempts >= $maxAttempts ){
throw new Exception('Could not generate non-colliding sid after ' . $maxAttempts . ' attempts');
}
$random_input_bytes = random_bytes($bytes_needed);

$p = 0;
$q = strlen($random_input_bytes);
$w = 0;
$have = 0;

$chars_remaining = $sid_len;
while( $chars_remaining-- ){
if( $have < $sid_bpc ){
if( $p < $q ) {
$byte = ord($random_input_bytes[$p++]);
$w |= ($byte << $have);
$have += 8;
}else{
break;
}
}
$out .= $hexconvtab[$w & $mask];
$w >>= $sid_bpc;
$have -= $sid_bpc;
}
$attempts++;
}while( $checkCollision && $this->sessionExists($out) );

$this->isNew = true;

return $out;
}

public function validateId(string $sid) : bool{
/* Validate ID is called after create_sid, create_sid already checks for collision */
return $this->isNew ?: $this->sessionExists($sid);
}

private function sessionExists(string $sid) : bool{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT 1 FROM `sessions` WHERE `session_id` = '' . $sid . '';');

if( !$result ){
throw new Exception('Could not determine if session exists: query failed');
}

return $result->num_rows;
}

public function read($sid) : string{
if( $this->isNew ){
/* New session created from self */
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}elseif( ($result = $this->querySession($sid)) ){
/* Existing session - validate now */
if( $result['request_signature'] && $this->reqSignature !== $result['request_signature'] ){
throw new SessionAuthException('Client request signature mismatch');
}elseif( $result['expire_unixtime'] && $result['expire_unixtime'] < $this->curReqTime ){
throw new SessionAuthException('Session is expired');
}
/* Valid session did not throw */
$this->sesInitTime = $result['init_unixtime'];
$this->lastReqTime = $result['last_request_unixtime'];
$this->sesExpireTime = $result['expire_unixtime'];
$this->numWrites = $result['writes'];
$out = $result['data'];
}else{
/* New session initialized elsewhere - potentially unsafe, but still no collision */
trigger_error('Potentially unsafe read from uninitialized session: see "session.use_strict_mode"', E_USER_WARNING);
$this->isNew = true;
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}

return $out;
}

private function querySession(string $sid) : ?array{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT * FROM sessions WHERE session_id = '' . $sid . '';');

if( !$result ){
throw new Exception('Failed to import session: query failed');
}

return $result->num_rows ? $result->fetch_assoc() : null;
}

public function write($sid, $data) : bool{
/* Determine expire unixtime */
if( $this->doExpire ){
$expireTime = 0;
}elseif( is_int($this->cOptions['idle_timeout']) ){
$expireTime = $this->curReqTime + $this->cOptions['idle_timeout'];
}else{
$expireTime = 'null';
}

$sid = $this->link->escape_string($sid);
$reqSignature = $this->link->escape_string($this->reqSignature);
$data = $this->link->escape_string($data);

if( $this->isNew ){
$this->link->query(
'INSERT INTO sessions (session_id, init_unixtime, last_request_unixtime, expire_unixtime, request_signature, writes, data) '.
'VALUES('' . $sid . '', ' . $this->curReqTime . ', init_unixtime, ' . $expireTime . ', '' . $reqSignature . '', 1, '' . $data . '');');
}else{
$this->link->query(
'UPDATE sessions '.
'SET last_request_unixtime = ' . $this->curReqTime . ', expire_unixtime = ' . $expireTime . ', request_signature = '' . $reqSignature . '', writes = writes + 1, data = '' . $data . '' '.
'WHERE session_id = '' . $sid . '';');
}

return $this->link->affected_rows > 0;
}

public function gc($maxLifetime) : bool{
return $this->link->query('DELETE FROM sessions WHERE expire_unixtime <= ' . $this->curReqTime . ';');
}

public function close() : bool{
$sesInitTime = null;
$lastReqTime = null;
$sesExpireTime = null;
$numWrites = null;
$this->isNew = false;
$this->doExpire = false;
/* Keep connection open for use - in case new session_start() or in the case of session_regenerate_id() */
return true;
}

public function destroy($sid) : bool{
$sid = $this->link->escape_string($sid);

$this->link->query('DELETE FROM sessions WHERE session_id = '' . $sid . '';');

return $this->link->affected_rows > 0;
}

public function __destruct(){
/* This will not be called in the case of Exception - resource handle will persist until PHP GC happens */
@$this->link->close();
}
}

?>


And here is how to use with built-in exception handling:



$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Use start method for automatic error handling when authentication errors occur */
$sesHandler->start();
$_SESSION['test'] = 'hello world';
session_write_close();


And here is how to use with manual exception handling:



const REGEN_INTERVAL_SEC = 600;

$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Example using native session_start with manual controls - NOTE: this example does exactly what MySQLSessionHandler::start() does */
try{
session_start();
/* Regenerate stagnant session id */
if( $sesHandler->getInitTime < ($sesHandler->getCurReqTime() - REGEN_INTERVAL_SEC) ){
/* Preserve session date while marking session expired */
$sesHandler->setExpire(true);
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Client/request could not be authenticated - start a fresh session */
session_id(null);
session_start();
}

$_SESSION['test'] = 'hello world';
session_write_close();


Here is the GIT!










share|improve this question











$endgroup$












  • $begingroup$
    Do you have a requirement to make this work for a high-traffic site? If not, don't worry about the performance. If yes, provide some more details.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 13:53










  • $begingroup$
    @ReneSaarsoo My current project will never see very high traffic... and this handler processed 1000 simulated requests in about 1.5s on my server (decent I think). Still, I will always be concerned with performance as a habit... and esp here since this class will be used in almost every request (even AJAX). If you have any sort of performance improvements (minus switching to a different storage method), I am all ears! Thanks
    $endgroup$
    – Phillip Weber
    May 3 '18 at 15:29
















2












$begingroup$


I've created a class that implements the native SessionHandlerInterface (with added PHP v7+ functionality) while adding some minor security features.
My main goal was to maintain native methods for session handling while transparently adding the following features:





  1. Minor protection from session-hijacking by using client user agent as a "request signature." I decided not to use client IP or remote IP as they are highly unreliable and somewhat easy to spoof. I decided to hash the result to save space in the db and because I have no reliable way to know how long a particular user-agent data string will be (new or updated browser data may be much longer than the present).


  2. Built-in protection from session-fixation by forcing sessions to regenerate sid every x number of seconds.


  3. Built-in session expiration without relying on GC method (which does not always happen and is very unpredictable). The native GC was meant to clean up resources rather than manage session time limits. Most devs include timestamps inside of the $_SESSION global itself. Personally, I think this is poor design as they can accidentally be overridden and handling will need to explicitly be called at every request.


I avoided using prepared statements as they take an extra call to the db. I've always advocated against prepared statements for single queries - they are a lazy way of ensuring data is properly escaped and they take almost twice as long to get a result.



I decided against using INSERT ON DUPLICATE KEY UPDATE to ensure that ONLY new sessions would be added to the db (in the case that two duplicate sids are created before either is written - although the chance of this happening is just about non-existent). Also, a single insert or a single update is a tad quicker.



I have been testing this class and it's been working well... except I can see this being too slow for a high traffic site. The main bottleneck is connection and queries to the database (can't do much about that). I'd like to expand on the request signature but I can't think of a reliable way to uniquely identify a client. I'd also appreciate any feedback/criticism about this class as I'd like to expand on it. Thank you!



Here is the code:



<?php
class SessionAuthException extends RuntimeException{
public function __construct($message = 'Unknown reason', $code = 0, Exception $previous = null){
parent::__construct('Session could not be authenticated: ' . $message, $code, $previous);
}
}

class MySQLSessionHandler implements SessionHandlerInterface{
private $link;
private $cOptions = [
'regen_interval' => 600,
'idle_timeout' => 3600];

private $curReqTime;
private $reqSignature;

private $sesInitTime;
private $lastReqTime;
private $sesExpireTime;
private $numWrites;

private $isNew = false;
private $doExpire = false;

public function __construct(mysqli $link, bool $autoInit = true, array $cOptions = []){
$this->link = $link;
$this->curReqTime = $_SERVER['REQUEST_TIME'];
$this->reqSignature = md5($_SERVER['HTTP_USER_AGENT']);

if( $autoInit ){
session_set_save_handler($this);
}

/* TODO: cOptions from args */

return;
}

public function getInitTime(){
return $this->sesInitTime;
}

public function getCurReqTime(){
return $this->curReqTime;
}

public function getLastRequestTime(){
return $this->lastReqTime;
}

public function getExpireTime(){
return $this->sesExpireTime;
}

public function getNumWrites(){
return $this->numWrites;
}

public function setExpire(bool $doExpire = true){
$this->doExpire = $doExpire;
return;
}

public function isMarkedExpire(){
return $this->doExpire;
}

public function start(array $options = []){
try{
@session_start($options);
if( isset($this->cOptions['regen_interval']) && $this->sesInitTime < $this->curReqTime - $this->cOptions['regen_interval'] ){
$this->doExpire = true;
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Unable to authenticate session - setting sid to null will create a new session without destroying the old (possibly hijacked) */
session_id(null);
session_start($options);
}
}

public function open($savePath, $sessionName) : bool{
/* mysqli->ping() returns null if connection has been closed */
return @$this->link->ping() ?: false;
}

public function create_sid() : string{
$checkCollision = session_status() == PHP_SESSION_ACTIVE;

$sid_len = ini_get('session.sid_length');
$sid_bpc = ini_get('session.sid_bits_per_character');
$bytes_needed = ceil($sid_len * $sid_bpc / 8);
$mask = (1 << $sid_bpc) - 1;
$out = '';

$hexconvtab = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-';

$attempts = 0;
$maxAttempts = 5;
do{
if( $attempts >= $maxAttempts ){
throw new Exception('Could not generate non-colliding sid after ' . $maxAttempts . ' attempts');
}
$random_input_bytes = random_bytes($bytes_needed);

$p = 0;
$q = strlen($random_input_bytes);
$w = 0;
$have = 0;

$chars_remaining = $sid_len;
while( $chars_remaining-- ){
if( $have < $sid_bpc ){
if( $p < $q ) {
$byte = ord($random_input_bytes[$p++]);
$w |= ($byte << $have);
$have += 8;
}else{
break;
}
}
$out .= $hexconvtab[$w & $mask];
$w >>= $sid_bpc;
$have -= $sid_bpc;
}
$attempts++;
}while( $checkCollision && $this->sessionExists($out) );

$this->isNew = true;

return $out;
}

public function validateId(string $sid) : bool{
/* Validate ID is called after create_sid, create_sid already checks for collision */
return $this->isNew ?: $this->sessionExists($sid);
}

private function sessionExists(string $sid) : bool{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT 1 FROM `sessions` WHERE `session_id` = '' . $sid . '';');

if( !$result ){
throw new Exception('Could not determine if session exists: query failed');
}

return $result->num_rows;
}

public function read($sid) : string{
if( $this->isNew ){
/* New session created from self */
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}elseif( ($result = $this->querySession($sid)) ){
/* Existing session - validate now */
if( $result['request_signature'] && $this->reqSignature !== $result['request_signature'] ){
throw new SessionAuthException('Client request signature mismatch');
}elseif( $result['expire_unixtime'] && $result['expire_unixtime'] < $this->curReqTime ){
throw new SessionAuthException('Session is expired');
}
/* Valid session did not throw */
$this->sesInitTime = $result['init_unixtime'];
$this->lastReqTime = $result['last_request_unixtime'];
$this->sesExpireTime = $result['expire_unixtime'];
$this->numWrites = $result['writes'];
$out = $result['data'];
}else{
/* New session initialized elsewhere - potentially unsafe, but still no collision */
trigger_error('Potentially unsafe read from uninitialized session: see "session.use_strict_mode"', E_USER_WARNING);
$this->isNew = true;
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}

return $out;
}

private function querySession(string $sid) : ?array{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT * FROM sessions WHERE session_id = '' . $sid . '';');

if( !$result ){
throw new Exception('Failed to import session: query failed');
}

return $result->num_rows ? $result->fetch_assoc() : null;
}

public function write($sid, $data) : bool{
/* Determine expire unixtime */
if( $this->doExpire ){
$expireTime = 0;
}elseif( is_int($this->cOptions['idle_timeout']) ){
$expireTime = $this->curReqTime + $this->cOptions['idle_timeout'];
}else{
$expireTime = 'null';
}

$sid = $this->link->escape_string($sid);
$reqSignature = $this->link->escape_string($this->reqSignature);
$data = $this->link->escape_string($data);

if( $this->isNew ){
$this->link->query(
'INSERT INTO sessions (session_id, init_unixtime, last_request_unixtime, expire_unixtime, request_signature, writes, data) '.
'VALUES('' . $sid . '', ' . $this->curReqTime . ', init_unixtime, ' . $expireTime . ', '' . $reqSignature . '', 1, '' . $data . '');');
}else{
$this->link->query(
'UPDATE sessions '.
'SET last_request_unixtime = ' . $this->curReqTime . ', expire_unixtime = ' . $expireTime . ', request_signature = '' . $reqSignature . '', writes = writes + 1, data = '' . $data . '' '.
'WHERE session_id = '' . $sid . '';');
}

return $this->link->affected_rows > 0;
}

public function gc($maxLifetime) : bool{
return $this->link->query('DELETE FROM sessions WHERE expire_unixtime <= ' . $this->curReqTime . ';');
}

public function close() : bool{
$sesInitTime = null;
$lastReqTime = null;
$sesExpireTime = null;
$numWrites = null;
$this->isNew = false;
$this->doExpire = false;
/* Keep connection open for use - in case new session_start() or in the case of session_regenerate_id() */
return true;
}

public function destroy($sid) : bool{
$sid = $this->link->escape_string($sid);

$this->link->query('DELETE FROM sessions WHERE session_id = '' . $sid . '';');

return $this->link->affected_rows > 0;
}

public function __destruct(){
/* This will not be called in the case of Exception - resource handle will persist until PHP GC happens */
@$this->link->close();
}
}

?>


And here is how to use with built-in exception handling:



$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Use start method for automatic error handling when authentication errors occur */
$sesHandler->start();
$_SESSION['test'] = 'hello world';
session_write_close();


And here is how to use with manual exception handling:



const REGEN_INTERVAL_SEC = 600;

$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Example using native session_start with manual controls - NOTE: this example does exactly what MySQLSessionHandler::start() does */
try{
session_start();
/* Regenerate stagnant session id */
if( $sesHandler->getInitTime < ($sesHandler->getCurReqTime() - REGEN_INTERVAL_SEC) ){
/* Preserve session date while marking session expired */
$sesHandler->setExpire(true);
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Client/request could not be authenticated - start a fresh session */
session_id(null);
session_start();
}

$_SESSION['test'] = 'hello world';
session_write_close();


Here is the GIT!










share|improve this question











$endgroup$












  • $begingroup$
    Do you have a requirement to make this work for a high-traffic site? If not, don't worry about the performance. If yes, provide some more details.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 13:53










  • $begingroup$
    @ReneSaarsoo My current project will never see very high traffic... and this handler processed 1000 simulated requests in about 1.5s on my server (decent I think). Still, I will always be concerned with performance as a habit... and esp here since this class will be used in almost every request (even AJAX). If you have any sort of performance improvements (minus switching to a different storage method), I am all ears! Thanks
    $endgroup$
    – Phillip Weber
    May 3 '18 at 15:29














2












2








2





$begingroup$


I've created a class that implements the native SessionHandlerInterface (with added PHP v7+ functionality) while adding some minor security features.
My main goal was to maintain native methods for session handling while transparently adding the following features:





  1. Minor protection from session-hijacking by using client user agent as a "request signature." I decided not to use client IP or remote IP as they are highly unreliable and somewhat easy to spoof. I decided to hash the result to save space in the db and because I have no reliable way to know how long a particular user-agent data string will be (new or updated browser data may be much longer than the present).


  2. Built-in protection from session-fixation by forcing sessions to regenerate sid every x number of seconds.


  3. Built-in session expiration without relying on GC method (which does not always happen and is very unpredictable). The native GC was meant to clean up resources rather than manage session time limits. Most devs include timestamps inside of the $_SESSION global itself. Personally, I think this is poor design as they can accidentally be overridden and handling will need to explicitly be called at every request.


I avoided using prepared statements as they take an extra call to the db. I've always advocated against prepared statements for single queries - they are a lazy way of ensuring data is properly escaped and they take almost twice as long to get a result.



I decided against using INSERT ON DUPLICATE KEY UPDATE to ensure that ONLY new sessions would be added to the db (in the case that two duplicate sids are created before either is written - although the chance of this happening is just about non-existent). Also, a single insert or a single update is a tad quicker.



I have been testing this class and it's been working well... except I can see this being too slow for a high traffic site. The main bottleneck is connection and queries to the database (can't do much about that). I'd like to expand on the request signature but I can't think of a reliable way to uniquely identify a client. I'd also appreciate any feedback/criticism about this class as I'd like to expand on it. Thank you!



Here is the code:



<?php
class SessionAuthException extends RuntimeException{
public function __construct($message = 'Unknown reason', $code = 0, Exception $previous = null){
parent::__construct('Session could not be authenticated: ' . $message, $code, $previous);
}
}

class MySQLSessionHandler implements SessionHandlerInterface{
private $link;
private $cOptions = [
'regen_interval' => 600,
'idle_timeout' => 3600];

private $curReqTime;
private $reqSignature;

private $sesInitTime;
private $lastReqTime;
private $sesExpireTime;
private $numWrites;

private $isNew = false;
private $doExpire = false;

public function __construct(mysqli $link, bool $autoInit = true, array $cOptions = []){
$this->link = $link;
$this->curReqTime = $_SERVER['REQUEST_TIME'];
$this->reqSignature = md5($_SERVER['HTTP_USER_AGENT']);

if( $autoInit ){
session_set_save_handler($this);
}

/* TODO: cOptions from args */

return;
}

public function getInitTime(){
return $this->sesInitTime;
}

public function getCurReqTime(){
return $this->curReqTime;
}

public function getLastRequestTime(){
return $this->lastReqTime;
}

public function getExpireTime(){
return $this->sesExpireTime;
}

public function getNumWrites(){
return $this->numWrites;
}

public function setExpire(bool $doExpire = true){
$this->doExpire = $doExpire;
return;
}

public function isMarkedExpire(){
return $this->doExpire;
}

public function start(array $options = []){
try{
@session_start($options);
if( isset($this->cOptions['regen_interval']) && $this->sesInitTime < $this->curReqTime - $this->cOptions['regen_interval'] ){
$this->doExpire = true;
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Unable to authenticate session - setting sid to null will create a new session without destroying the old (possibly hijacked) */
session_id(null);
session_start($options);
}
}

public function open($savePath, $sessionName) : bool{
/* mysqli->ping() returns null if connection has been closed */
return @$this->link->ping() ?: false;
}

public function create_sid() : string{
$checkCollision = session_status() == PHP_SESSION_ACTIVE;

$sid_len = ini_get('session.sid_length');
$sid_bpc = ini_get('session.sid_bits_per_character');
$bytes_needed = ceil($sid_len * $sid_bpc / 8);
$mask = (1 << $sid_bpc) - 1;
$out = '';

$hexconvtab = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-';

$attempts = 0;
$maxAttempts = 5;
do{
if( $attempts >= $maxAttempts ){
throw new Exception('Could not generate non-colliding sid after ' . $maxAttempts . ' attempts');
}
$random_input_bytes = random_bytes($bytes_needed);

$p = 0;
$q = strlen($random_input_bytes);
$w = 0;
$have = 0;

$chars_remaining = $sid_len;
while( $chars_remaining-- ){
if( $have < $sid_bpc ){
if( $p < $q ) {
$byte = ord($random_input_bytes[$p++]);
$w |= ($byte << $have);
$have += 8;
}else{
break;
}
}
$out .= $hexconvtab[$w & $mask];
$w >>= $sid_bpc;
$have -= $sid_bpc;
}
$attempts++;
}while( $checkCollision && $this->sessionExists($out) );

$this->isNew = true;

return $out;
}

public function validateId(string $sid) : bool{
/* Validate ID is called after create_sid, create_sid already checks for collision */
return $this->isNew ?: $this->sessionExists($sid);
}

private function sessionExists(string $sid) : bool{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT 1 FROM `sessions` WHERE `session_id` = '' . $sid . '';');

if( !$result ){
throw new Exception('Could not determine if session exists: query failed');
}

return $result->num_rows;
}

public function read($sid) : string{
if( $this->isNew ){
/* New session created from self */
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}elseif( ($result = $this->querySession($sid)) ){
/* Existing session - validate now */
if( $result['request_signature'] && $this->reqSignature !== $result['request_signature'] ){
throw new SessionAuthException('Client request signature mismatch');
}elseif( $result['expire_unixtime'] && $result['expire_unixtime'] < $this->curReqTime ){
throw new SessionAuthException('Session is expired');
}
/* Valid session did not throw */
$this->sesInitTime = $result['init_unixtime'];
$this->lastReqTime = $result['last_request_unixtime'];
$this->sesExpireTime = $result['expire_unixtime'];
$this->numWrites = $result['writes'];
$out = $result['data'];
}else{
/* New session initialized elsewhere - potentially unsafe, but still no collision */
trigger_error('Potentially unsafe read from uninitialized session: see "session.use_strict_mode"', E_USER_WARNING);
$this->isNew = true;
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}

return $out;
}

private function querySession(string $sid) : ?array{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT * FROM sessions WHERE session_id = '' . $sid . '';');

if( !$result ){
throw new Exception('Failed to import session: query failed');
}

return $result->num_rows ? $result->fetch_assoc() : null;
}

public function write($sid, $data) : bool{
/* Determine expire unixtime */
if( $this->doExpire ){
$expireTime = 0;
}elseif( is_int($this->cOptions['idle_timeout']) ){
$expireTime = $this->curReqTime + $this->cOptions['idle_timeout'];
}else{
$expireTime = 'null';
}

$sid = $this->link->escape_string($sid);
$reqSignature = $this->link->escape_string($this->reqSignature);
$data = $this->link->escape_string($data);

if( $this->isNew ){
$this->link->query(
'INSERT INTO sessions (session_id, init_unixtime, last_request_unixtime, expire_unixtime, request_signature, writes, data) '.
'VALUES('' . $sid . '', ' . $this->curReqTime . ', init_unixtime, ' . $expireTime . ', '' . $reqSignature . '', 1, '' . $data . '');');
}else{
$this->link->query(
'UPDATE sessions '.
'SET last_request_unixtime = ' . $this->curReqTime . ', expire_unixtime = ' . $expireTime . ', request_signature = '' . $reqSignature . '', writes = writes + 1, data = '' . $data . '' '.
'WHERE session_id = '' . $sid . '';');
}

return $this->link->affected_rows > 0;
}

public function gc($maxLifetime) : bool{
return $this->link->query('DELETE FROM sessions WHERE expire_unixtime <= ' . $this->curReqTime . ';');
}

public function close() : bool{
$sesInitTime = null;
$lastReqTime = null;
$sesExpireTime = null;
$numWrites = null;
$this->isNew = false;
$this->doExpire = false;
/* Keep connection open for use - in case new session_start() or in the case of session_regenerate_id() */
return true;
}

public function destroy($sid) : bool{
$sid = $this->link->escape_string($sid);

$this->link->query('DELETE FROM sessions WHERE session_id = '' . $sid . '';');

return $this->link->affected_rows > 0;
}

public function __destruct(){
/* This will not be called in the case of Exception - resource handle will persist until PHP GC happens */
@$this->link->close();
}
}

?>


And here is how to use with built-in exception handling:



$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Use start method for automatic error handling when authentication errors occur */
$sesHandler->start();
$_SESSION['test'] = 'hello world';
session_write_close();


And here is how to use with manual exception handling:



const REGEN_INTERVAL_SEC = 600;

$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Example using native session_start with manual controls - NOTE: this example does exactly what MySQLSessionHandler::start() does */
try{
session_start();
/* Regenerate stagnant session id */
if( $sesHandler->getInitTime < ($sesHandler->getCurReqTime() - REGEN_INTERVAL_SEC) ){
/* Preserve session date while marking session expired */
$sesHandler->setExpire(true);
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Client/request could not be authenticated - start a fresh session */
session_id(null);
session_start();
}

$_SESSION['test'] = 'hello world';
session_write_close();


Here is the GIT!










share|improve this question











$endgroup$




I've created a class that implements the native SessionHandlerInterface (with added PHP v7+ functionality) while adding some minor security features.
My main goal was to maintain native methods for session handling while transparently adding the following features:





  1. Minor protection from session-hijacking by using client user agent as a "request signature." I decided not to use client IP or remote IP as they are highly unreliable and somewhat easy to spoof. I decided to hash the result to save space in the db and because I have no reliable way to know how long a particular user-agent data string will be (new or updated browser data may be much longer than the present).


  2. Built-in protection from session-fixation by forcing sessions to regenerate sid every x number of seconds.


  3. Built-in session expiration without relying on GC method (which does not always happen and is very unpredictable). The native GC was meant to clean up resources rather than manage session time limits. Most devs include timestamps inside of the $_SESSION global itself. Personally, I think this is poor design as they can accidentally be overridden and handling will need to explicitly be called at every request.


I avoided using prepared statements as they take an extra call to the db. I've always advocated against prepared statements for single queries - they are a lazy way of ensuring data is properly escaped and they take almost twice as long to get a result.



I decided against using INSERT ON DUPLICATE KEY UPDATE to ensure that ONLY new sessions would be added to the db (in the case that two duplicate sids are created before either is written - although the chance of this happening is just about non-existent). Also, a single insert or a single update is a tad quicker.



I have been testing this class and it's been working well... except I can see this being too slow for a high traffic site. The main bottleneck is connection and queries to the database (can't do much about that). I'd like to expand on the request signature but I can't think of a reliable way to uniquely identify a client. I'd also appreciate any feedback/criticism about this class as I'd like to expand on it. Thank you!



Here is the code:



<?php
class SessionAuthException extends RuntimeException{
public function __construct($message = 'Unknown reason', $code = 0, Exception $previous = null){
parent::__construct('Session could not be authenticated: ' . $message, $code, $previous);
}
}

class MySQLSessionHandler implements SessionHandlerInterface{
private $link;
private $cOptions = [
'regen_interval' => 600,
'idle_timeout' => 3600];

private $curReqTime;
private $reqSignature;

private $sesInitTime;
private $lastReqTime;
private $sesExpireTime;
private $numWrites;

private $isNew = false;
private $doExpire = false;

public function __construct(mysqli $link, bool $autoInit = true, array $cOptions = []){
$this->link = $link;
$this->curReqTime = $_SERVER['REQUEST_TIME'];
$this->reqSignature = md5($_SERVER['HTTP_USER_AGENT']);

if( $autoInit ){
session_set_save_handler($this);
}

/* TODO: cOptions from args */

return;
}

public function getInitTime(){
return $this->sesInitTime;
}

public function getCurReqTime(){
return $this->curReqTime;
}

public function getLastRequestTime(){
return $this->lastReqTime;
}

public function getExpireTime(){
return $this->sesExpireTime;
}

public function getNumWrites(){
return $this->numWrites;
}

public function setExpire(bool $doExpire = true){
$this->doExpire = $doExpire;
return;
}

public function isMarkedExpire(){
return $this->doExpire;
}

public function start(array $options = []){
try{
@session_start($options);
if( isset($this->cOptions['regen_interval']) && $this->sesInitTime < $this->curReqTime - $this->cOptions['regen_interval'] ){
$this->doExpire = true;
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Unable to authenticate session - setting sid to null will create a new session without destroying the old (possibly hijacked) */
session_id(null);
session_start($options);
}
}

public function open($savePath, $sessionName) : bool{
/* mysqli->ping() returns null if connection has been closed */
return @$this->link->ping() ?: false;
}

public function create_sid() : string{
$checkCollision = session_status() == PHP_SESSION_ACTIVE;

$sid_len = ini_get('session.sid_length');
$sid_bpc = ini_get('session.sid_bits_per_character');
$bytes_needed = ceil($sid_len * $sid_bpc / 8);
$mask = (1 << $sid_bpc) - 1;
$out = '';

$hexconvtab = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-';

$attempts = 0;
$maxAttempts = 5;
do{
if( $attempts >= $maxAttempts ){
throw new Exception('Could not generate non-colliding sid after ' . $maxAttempts . ' attempts');
}
$random_input_bytes = random_bytes($bytes_needed);

$p = 0;
$q = strlen($random_input_bytes);
$w = 0;
$have = 0;

$chars_remaining = $sid_len;
while( $chars_remaining-- ){
if( $have < $sid_bpc ){
if( $p < $q ) {
$byte = ord($random_input_bytes[$p++]);
$w |= ($byte << $have);
$have += 8;
}else{
break;
}
}
$out .= $hexconvtab[$w & $mask];
$w >>= $sid_bpc;
$have -= $sid_bpc;
}
$attempts++;
}while( $checkCollision && $this->sessionExists($out) );

$this->isNew = true;

return $out;
}

public function validateId(string $sid) : bool{
/* Validate ID is called after create_sid, create_sid already checks for collision */
return $this->isNew ?: $this->sessionExists($sid);
}

private function sessionExists(string $sid) : bool{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT 1 FROM `sessions` WHERE `session_id` = '' . $sid . '';');

if( !$result ){
throw new Exception('Could not determine if session exists: query failed');
}

return $result->num_rows;
}

public function read($sid) : string{
if( $this->isNew ){
/* New session created from self */
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}elseif( ($result = $this->querySession($sid)) ){
/* Existing session - validate now */
if( $result['request_signature'] && $this->reqSignature !== $result['request_signature'] ){
throw new SessionAuthException('Client request signature mismatch');
}elseif( $result['expire_unixtime'] && $result['expire_unixtime'] < $this->curReqTime ){
throw new SessionAuthException('Session is expired');
}
/* Valid session did not throw */
$this->sesInitTime = $result['init_unixtime'];
$this->lastReqTime = $result['last_request_unixtime'];
$this->sesExpireTime = $result['expire_unixtime'];
$this->numWrites = $result['writes'];
$out = $result['data'];
}else{
/* New session initialized elsewhere - potentially unsafe, but still no collision */
trigger_error('Potentially unsafe read from uninitialized session: see "session.use_strict_mode"', E_USER_WARNING);
$this->isNew = true;
$this->sesInitTime = $this->curReqTime;
$this->lastReqTime = null;
$this->sesExpireTime = null;
$this->numWrites = 0;
$out = '';
}

return $out;
}

private function querySession(string $sid) : ?array{
$sid = $this->link->escape_string($sid);

$result = $this->link->query('SELECT * FROM sessions WHERE session_id = '' . $sid . '';');

if( !$result ){
throw new Exception('Failed to import session: query failed');
}

return $result->num_rows ? $result->fetch_assoc() : null;
}

public function write($sid, $data) : bool{
/* Determine expire unixtime */
if( $this->doExpire ){
$expireTime = 0;
}elseif( is_int($this->cOptions['idle_timeout']) ){
$expireTime = $this->curReqTime + $this->cOptions['idle_timeout'];
}else{
$expireTime = 'null';
}

$sid = $this->link->escape_string($sid);
$reqSignature = $this->link->escape_string($this->reqSignature);
$data = $this->link->escape_string($data);

if( $this->isNew ){
$this->link->query(
'INSERT INTO sessions (session_id, init_unixtime, last_request_unixtime, expire_unixtime, request_signature, writes, data) '.
'VALUES('' . $sid . '', ' . $this->curReqTime . ', init_unixtime, ' . $expireTime . ', '' . $reqSignature . '', 1, '' . $data . '');');
}else{
$this->link->query(
'UPDATE sessions '.
'SET last_request_unixtime = ' . $this->curReqTime . ', expire_unixtime = ' . $expireTime . ', request_signature = '' . $reqSignature . '', writes = writes + 1, data = '' . $data . '' '.
'WHERE session_id = '' . $sid . '';');
}

return $this->link->affected_rows > 0;
}

public function gc($maxLifetime) : bool{
return $this->link->query('DELETE FROM sessions WHERE expire_unixtime <= ' . $this->curReqTime . ';');
}

public function close() : bool{
$sesInitTime = null;
$lastReqTime = null;
$sesExpireTime = null;
$numWrites = null;
$this->isNew = false;
$this->doExpire = false;
/* Keep connection open for use - in case new session_start() or in the case of session_regenerate_id() */
return true;
}

public function destroy($sid) : bool{
$sid = $this->link->escape_string($sid);

$this->link->query('DELETE FROM sessions WHERE session_id = '' . $sid . '';');

return $this->link->affected_rows > 0;
}

public function __destruct(){
/* This will not be called in the case of Exception - resource handle will persist until PHP GC happens */
@$this->link->close();
}
}

?>


And here is how to use with built-in exception handling:



$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Use start method for automatic error handling when authentication errors occur */
$sesHandler->start();
$_SESSION['test'] = 'hello world';
session_write_close();


And here is how to use with manual exception handling:



const REGEN_INTERVAL_SEC = 600;

$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Example using native session_start with manual controls - NOTE: this example does exactly what MySQLSessionHandler::start() does */
try{
session_start();
/* Regenerate stagnant session id */
if( $sesHandler->getInitTime < ($sesHandler->getCurReqTime() - REGEN_INTERVAL_SEC) ){
/* Preserve session date while marking session expired */
$sesHandler->setExpire(true);
session_regenerate_id(false);
}
}catch(SessionAuthException $e){
/* Client/request could not be authenticated - start a fresh session */
session_id(null);
session_start();
}

$_SESSION['test'] = 'hello world';
session_write_close();


Here is the GIT!







php mysql security mysqli session






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited May 2 '18 at 21:56







Phillip Weber

















asked May 2 '18 at 21:50









Phillip WeberPhillip Weber

1113




1113












  • $begingroup$
    Do you have a requirement to make this work for a high-traffic site? If not, don't worry about the performance. If yes, provide some more details.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 13:53










  • $begingroup$
    @ReneSaarsoo My current project will never see very high traffic... and this handler processed 1000 simulated requests in about 1.5s on my server (decent I think). Still, I will always be concerned with performance as a habit... and esp here since this class will be used in almost every request (even AJAX). If you have any sort of performance improvements (minus switching to a different storage method), I am all ears! Thanks
    $endgroup$
    – Phillip Weber
    May 3 '18 at 15:29


















  • $begingroup$
    Do you have a requirement to make this work for a high-traffic site? If not, don't worry about the performance. If yes, provide some more details.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 13:53










  • $begingroup$
    @ReneSaarsoo My current project will never see very high traffic... and this handler processed 1000 simulated requests in about 1.5s on my server (decent I think). Still, I will always be concerned with performance as a habit... and esp here since this class will be used in almost every request (even AJAX). If you have any sort of performance improvements (minus switching to a different storage method), I am all ears! Thanks
    $endgroup$
    – Phillip Weber
    May 3 '18 at 15:29
















$begingroup$
Do you have a requirement to make this work for a high-traffic site? If not, don't worry about the performance. If yes, provide some more details.
$endgroup$
– Rene Saarsoo
May 3 '18 at 13:53




$begingroup$
Do you have a requirement to make this work for a high-traffic site? If not, don't worry about the performance. If yes, provide some more details.
$endgroup$
– Rene Saarsoo
May 3 '18 at 13:53












$begingroup$
@ReneSaarsoo My current project will never see very high traffic... and this handler processed 1000 simulated requests in about 1.5s on my server (decent I think). Still, I will always be concerned with performance as a habit... and esp here since this class will be used in almost every request (even AJAX). If you have any sort of performance improvements (minus switching to a different storage method), I am all ears! Thanks
$endgroup$
– Phillip Weber
May 3 '18 at 15:29




$begingroup$
@ReneSaarsoo My current project will never see very high traffic... and this handler processed 1000 simulated requests in about 1.5s on my server (decent I think). Still, I will always be concerned with performance as a habit... and esp here since this class will be used in almost every request (even AJAX). If you have any sort of performance improvements (minus switching to a different storage method), I am all ears! Thanks
$endgroup$
– Phillip Weber
May 3 '18 at 15:29










1 Answer
1






active

oldest

votes


















2












$begingroup$

Without looking too deeply into it, a few things immediately pop out:




  • This class is likely too large with its ~250 lines of code.

  • It has quite a bit of internal state, stored in 10 instance variables.


  • SessionHandlerInterface requires implementing 6 methods, but this class has an entire 17 public methods.


It's likely doing several things that could be extracted to other classes or functions. A good candidate for extraction is the create_sid() function. It seems to me that this has been taken from some 3rd party website or library, as its dash_separated_name doesn't fit in with other camelCasedNames in this class.



1. Replying to your comment:




For me, since everything in this class deals with sessions and how they are handled [--], doesn't it make sense to keep them together [--].




I guess it comes down to an age-old question: which criteria should you use for splitting up your code to modules/classes/functions/etc?



A good guideline to follow, is the single responsibility principle, which states:




A class should have only one reason to change.




I would expect MySQLSessionHandler to change when you need to change something in the relationship of how you're storing the session data to MySQL database.



However I would not expect changing how session ID is generated to require a change in MySQLSessionHandler. The SID does not have much to do with MySQL - you might use the same ID generator for FileSessionHandler or PostgresSessionHandler.



2. Replying to another comment:




[--] decided to add getters in the case that a user would want some sort of external control.




Follow the You ain't gonna need it principle:



Don't add methods/properties to a class just-in-case. Have a clear purpose and clear plan how you'd expect your class to be used. Unless you're really sure, it's better to leave it out. Less is more.



Design first for your own specific use case. Leave out everything you don't actually need. Then try to use it in another project - see if it works as is, or do you need to improve it somehow. Only after having found it useful in several places of your own, consider sharing it with a wider audience.






share|improve this answer











$endgroup$













  • $begingroup$
    The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:22










  • $begingroup$
    create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:30












  • $begingroup$
    I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:40










  • $begingroup$
    I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 17:41












  • $begingroup$
    PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:50












Your Answer





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














draft saved

draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f193504%2fphp-7-1-mysql-session-handler-class-with-some-built-in-time-management-secur%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes









2












$begingroup$

Without looking too deeply into it, a few things immediately pop out:




  • This class is likely too large with its ~250 lines of code.

  • It has quite a bit of internal state, stored in 10 instance variables.


  • SessionHandlerInterface requires implementing 6 methods, but this class has an entire 17 public methods.


It's likely doing several things that could be extracted to other classes or functions. A good candidate for extraction is the create_sid() function. It seems to me that this has been taken from some 3rd party website or library, as its dash_separated_name doesn't fit in with other camelCasedNames in this class.



1. Replying to your comment:




For me, since everything in this class deals with sessions and how they are handled [--], doesn't it make sense to keep them together [--].




I guess it comes down to an age-old question: which criteria should you use for splitting up your code to modules/classes/functions/etc?



A good guideline to follow, is the single responsibility principle, which states:




A class should have only one reason to change.




I would expect MySQLSessionHandler to change when you need to change something in the relationship of how you're storing the session data to MySQL database.



However I would not expect changing how session ID is generated to require a change in MySQLSessionHandler. The SID does not have much to do with MySQL - you might use the same ID generator for FileSessionHandler or PostgresSessionHandler.



2. Replying to another comment:




[--] decided to add getters in the case that a user would want some sort of external control.




Follow the You ain't gonna need it principle:



Don't add methods/properties to a class just-in-case. Have a clear purpose and clear plan how you'd expect your class to be used. Unless you're really sure, it's better to leave it out. Less is more.



Design first for your own specific use case. Leave out everything you don't actually need. Then try to use it in another project - see if it works as is, or do you need to improve it somehow. Only after having found it useful in several places of your own, consider sharing it with a wider audience.






share|improve this answer











$endgroup$













  • $begingroup$
    The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:22










  • $begingroup$
    create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:30












  • $begingroup$
    I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:40










  • $begingroup$
    I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 17:41












  • $begingroup$
    PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:50
















2












$begingroup$

Without looking too deeply into it, a few things immediately pop out:




  • This class is likely too large with its ~250 lines of code.

  • It has quite a bit of internal state, stored in 10 instance variables.


  • SessionHandlerInterface requires implementing 6 methods, but this class has an entire 17 public methods.


It's likely doing several things that could be extracted to other classes or functions. A good candidate for extraction is the create_sid() function. It seems to me that this has been taken from some 3rd party website or library, as its dash_separated_name doesn't fit in with other camelCasedNames in this class.



1. Replying to your comment:




For me, since everything in this class deals with sessions and how they are handled [--], doesn't it make sense to keep them together [--].




I guess it comes down to an age-old question: which criteria should you use for splitting up your code to modules/classes/functions/etc?



A good guideline to follow, is the single responsibility principle, which states:




A class should have only one reason to change.




I would expect MySQLSessionHandler to change when you need to change something in the relationship of how you're storing the session data to MySQL database.



However I would not expect changing how session ID is generated to require a change in MySQLSessionHandler. The SID does not have much to do with MySQL - you might use the same ID generator for FileSessionHandler or PostgresSessionHandler.



2. Replying to another comment:




[--] decided to add getters in the case that a user would want some sort of external control.




Follow the You ain't gonna need it principle:



Don't add methods/properties to a class just-in-case. Have a clear purpose and clear plan how you'd expect your class to be used. Unless you're really sure, it's better to leave it out. Less is more.



Design first for your own specific use case. Leave out everything you don't actually need. Then try to use it in another project - see if it works as is, or do you need to improve it somehow. Only after having found it useful in several places of your own, consider sharing it with a wider audience.






share|improve this answer











$endgroup$













  • $begingroup$
    The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:22










  • $begingroup$
    create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:30












  • $begingroup$
    I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:40










  • $begingroup$
    I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 17:41












  • $begingroup$
    PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:50














2












2








2





$begingroup$

Without looking too deeply into it, a few things immediately pop out:




  • This class is likely too large with its ~250 lines of code.

  • It has quite a bit of internal state, stored in 10 instance variables.


  • SessionHandlerInterface requires implementing 6 methods, but this class has an entire 17 public methods.


It's likely doing several things that could be extracted to other classes or functions. A good candidate for extraction is the create_sid() function. It seems to me that this has been taken from some 3rd party website or library, as its dash_separated_name doesn't fit in with other camelCasedNames in this class.



1. Replying to your comment:




For me, since everything in this class deals with sessions and how they are handled [--], doesn't it make sense to keep them together [--].




I guess it comes down to an age-old question: which criteria should you use for splitting up your code to modules/classes/functions/etc?



A good guideline to follow, is the single responsibility principle, which states:




A class should have only one reason to change.




I would expect MySQLSessionHandler to change when you need to change something in the relationship of how you're storing the session data to MySQL database.



However I would not expect changing how session ID is generated to require a change in MySQLSessionHandler. The SID does not have much to do with MySQL - you might use the same ID generator for FileSessionHandler or PostgresSessionHandler.



2. Replying to another comment:




[--] decided to add getters in the case that a user would want some sort of external control.




Follow the You ain't gonna need it principle:



Don't add methods/properties to a class just-in-case. Have a clear purpose and clear plan how you'd expect your class to be used. Unless you're really sure, it's better to leave it out. Less is more.



Design first for your own specific use case. Leave out everything you don't actually need. Then try to use it in another project - see if it works as is, or do you need to improve it somehow. Only after having found it useful in several places of your own, consider sharing it with a wider audience.






share|improve this answer











$endgroup$



Without looking too deeply into it, a few things immediately pop out:




  • This class is likely too large with its ~250 lines of code.

  • It has quite a bit of internal state, stored in 10 instance variables.


  • SessionHandlerInterface requires implementing 6 methods, but this class has an entire 17 public methods.


It's likely doing several things that could be extracted to other classes or functions. A good candidate for extraction is the create_sid() function. It seems to me that this has been taken from some 3rd party website or library, as its dash_separated_name doesn't fit in with other camelCasedNames in this class.



1. Replying to your comment:




For me, since everything in this class deals with sessions and how they are handled [--], doesn't it make sense to keep them together [--].




I guess it comes down to an age-old question: which criteria should you use for splitting up your code to modules/classes/functions/etc?



A good guideline to follow, is the single responsibility principle, which states:




A class should have only one reason to change.




I would expect MySQLSessionHandler to change when you need to change something in the relationship of how you're storing the session data to MySQL database.



However I would not expect changing how session ID is generated to require a change in MySQLSessionHandler. The SID does not have much to do with MySQL - you might use the same ID generator for FileSessionHandler or PostgresSessionHandler.



2. Replying to another comment:




[--] decided to add getters in the case that a user would want some sort of external control.




Follow the You ain't gonna need it principle:



Don't add methods/properties to a class just-in-case. Have a clear purpose and clear plan how you'd expect your class to be used. Unless you're really sure, it's better to leave it out. Less is more.



Design first for your own specific use case. Leave out everything you don't actually need. Then try to use it in another project - see if it works as is, or do you need to improve it somehow. Only after having found it useful in several places of your own, consider sharing it with a wider audience.







share|improve this answer














share|improve this answer



share|improve this answer








edited May 3 '18 at 18:24

























answered May 3 '18 at 17:08









Rene SaarsooRene Saarsoo

2,034814




2,034814












  • $begingroup$
    The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:22










  • $begingroup$
    create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:30












  • $begingroup$
    I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:40










  • $begingroup$
    I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 17:41












  • $begingroup$
    PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:50


















  • $begingroup$
    The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:22










  • $begingroup$
    create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:30












  • $begingroup$
    I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)?
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:40










  • $begingroup$
    I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input.
    $endgroup$
    – Rene Saarsoo
    May 3 '18 at 17:41












  • $begingroup$
    PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X
    $endgroup$
    – Phillip Weber
    May 3 '18 at 17:50
















$begingroup$
The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties?
$endgroup$
– Phillip Weber
May 3 '18 at 17:22




$begingroup$
The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties?
$endgroup$
– Phillip Weber
May 3 '18 at 17:22












$begingroup$
create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think?
$endgroup$
– Phillip Weber
May 3 '18 at 17:30






$begingroup$
create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think?
$endgroup$
– Phillip Weber
May 3 '18 at 17:30














$begingroup$
I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)?
$endgroup$
– Phillip Weber
May 3 '18 at 17:40




$begingroup$
I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)?
$endgroup$
– Phillip Weber
May 3 '18 at 17:40












$begingroup$
I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input.
$endgroup$
– Rene Saarsoo
May 3 '18 at 17:41






$begingroup$
I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input.
$endgroup$
– Rene Saarsoo
May 3 '18 at 17:41














$begingroup$
PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X
$endgroup$
– Phillip Weber
May 3 '18 at 17:50




$begingroup$
PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X
$endgroup$
– Phillip Weber
May 3 '18 at 17:50


















draft saved

draft discarded




















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f193504%2fphp-7-1-mysql-session-handler-class-with-some-built-in-time-management-secur%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

is 'sed' thread safeWhat should someone know about using Python scripts in the shell?Nexenta bash script uses...

How do i solve the “ No module named 'mlxtend' ” issue on Jupyter?

Pilgersdorf Inhaltsverzeichnis Geografie | Geschichte | Bevölkerungsentwicklung | Politik | Kultur...