2
\$\begingroup\$

At this point i am interested to see other techniques on handling session timeouts.

What are some good improvements on this script to detect when a session is no longer valid?

Important definitions:

session.gc_maxlifetime - (default 1440 seconds) defines how long an unused PHP session will be kept alive. For example: A user logs in, browses through your application or web site, for hours, for days. No problem. As long as the time between his clicks never exceed 1440 seconds. It's a timeout value.

session.cookie_lifetime - *This value (default 0, which means until the browser's next restart) defines how long (in seconds) a session cookie will live. Sounds similar to session.gc_maxlifetime, but it's a completely different approach. This value indirectly defines the "absolute" maximum lifetime of a session, whether the user is active or not. If this value is set to 1*60*60, every session ends after an hour.*

Problems to address:

  • Session timeout
  • Cookie timeout
  • Browser deleted cookies / cleared history

SESSION_handler.php

<?php
class SESSION_handler{
public static $gc_maxlifetime;
public static $cookie_lifetime;
function __construct(){
    self::$gc_maxlifetime = ini_get('session.gc_maxlifetime');
    self::$cookie_lifetime = ini_get('session.cookie_lifetime');
    if(session_status() == PHP_SESSION_NONE) {
        session_start();
    }
}
public function get($session_key){
    if(session_status() == PHP_SESSION_NONE) {
        if(!empty($_SESSION[$session_key])){
            $_SESSION['time_at_last_session'] = time();
            return $_SESSION[$session_key];
        }
    }
    return undefined;
}
public function set($session_key, $session_value){
    if(session_status() == PHP_SESSION_NONE) {
        $_SESSION[$session_key] = $session_value;
        $_SESSION['time_at_last_session'] = time();
        return $_SESSION[$session_key];
    }
    return undefined;
}
public function session_expired(){
    if(
        time() - ((!empty($_SESSION['time_at_last_session']))? $_SESSION['time_at_last_session'] : 0) >= self::$gc_maxlifetime ||
        session_status() == PHP_SESSION_NONE ||
        !isset($_COOKIE[session_name()])
        ){
        return true;
    }
    return false;
}
}
?>

is_session_expired.php

<?php
    $SESSION = new SESSION_handler();
    if($SESSION->session_expired()){
        echo "true";
    }else{
        echo "false";
    }
?>

AJAX

<script type="text/javascript">
    $(document).ready(function(){
        (function(){
            tmp_ = arguments.callee;
            window.setTimeout(function(){
                $.ajax({
                    url:'/php/is_session_expired.php',
                    success: function(session_expired){
                        if(session_expired == "true"){
                            alert("session has expired");
                        }else{
                            console.log('session has not expired');
                        }
                    }
                });
            });
        })();
    })
</script>
\$\endgroup\$
5
  • \$\begingroup\$ "If this value [in seconds] is set to 60, every session ends after an hour." ... What? is cookie_timeout in minutes or seconds now? \$\endgroup\$
    – Vogel612
    Commented Apr 21, 2016 at 19:02
  • \$\begingroup\$ After some research, i assume seconds. @Vogel612 \$\endgroup\$ Commented Apr 21, 2016 at 19:03
  • \$\begingroup\$ is $this->time_at_last_session initialized? I do not see where? \$\endgroup\$
    – Nick
    Commented Apr 25, 2016 at 20:02
  • \$\begingroup\$ @Nick it is initialized at $this->time_at_last_session = time(); in the session set method, but i now see a concern for it not being set when in the session_expired method. I have correct that mistake. \$\endgroup\$ Commented Apr 25, 2016 at 20:05
  • \$\begingroup\$ exactly. I do not see any other issue with your code. Do you need the session only for JS and not for PHP ? I could propose an alternative such Redis. \$\endgroup\$
    – Nick
    Commented Apr 25, 2016 at 20:27

1 Answer 1

2
+100
\$\begingroup\$

I don't believe the code works as expected.

One of us for sure made mistake with session_status() == PHP_SESSION_NONE. If is me, comment so I can fix the answer.

Code style

You will have better readability, if you indent everything inside the class.

You need to work on if statement in SESSION_handler::session_expired(). I can not read it at all. I personally would do it on single row, but then many people will not like it at all.

Constructor

self::$cookie_lifetime - I do not see where you need this?

You initializing the static fields every time.

self::$gc_maxlifetime = ini_get('session.gc_maxlifetime');
self::$cookie_lifetime = ini_get('session.cookie_lifetime');

I would suggest something like this:

if (self::$gc_maxlifetime = -1){
   self::$gc_maxlifetime = ini_get('session.gc_maxlifetime');
   self::$cookie_lifetime = ini_get('session.cookie_lifetime');
}

You also might decide to remove them and use ini_get directly. Or you can do static method SESSION_handler::gc_maxlifetime().

Here you create session as well:

if(session_status() == PHP_SESSION_NONE) {
    session_start();
}

Do you really need to check session_status()? My experience is you can use session_start() directly. If there is a session created, statement will do no harm.

I believe you need to initialize $_SESSION['time_at_last_session'], but only if is not set already.

Something like:

if (!isset($_SESSION['time_at_last_session']))
    $_SESSION['time_at_last_session'] = time();

SESSION_handler::get() and SESSION_handler::set()

Why you have this:

if(session_status() == PHP_SESSION_NONE) {
    // ...
}
return undefined;

You probably want to check if everything with the session is correct? to do so, use:

if(session_status() != PHP_SESSION_NONE) {
    // ...
}

You return undefined;. This is first time I see this line in PHP. Just remove the if statement. Session is already made in constructor. If is not undefined will not help at all.

Actually, in constructor, you can save the result of session_start(); into some class property and use it instead of session_status().

SESSION_handler::session_expired()

should be something like this?

public function session_expired(){
    $time = isset($_SESSION['time_at_last_session']) ?
             $_SESSION['time_at_last_session'] : 0;

    // same, but ugly:
    //$time = (int) @$_SESSION['time_at_last_session'];

    if (session_status() == PHP_SESSION_NONE)
        return true; // session problem

    if (time() - $time > self::$gc_maxlifetime)
        return true; // session expired

    // fix $_COOKIE[session_name() thing

    return false; // session is NOT expired
}

is_session_expired.php

All OK if you fix the constructor.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.