2
\$\begingroup\$

I have made the decision to move the storing of session data to the database.

Any new ideas, suggestions are welcome.

Please also give security suggestions.

Like SQL injection is possible here, etc...

<?php

/**
 * MySQL/MariaDB Session Handler - Handle Sessions using Database
 * Copyright (c) 2021 Baal Krshna
 * PHP Version 5.4
 *
 * @author Puneet Gopinath (PuneetGopinath) <baalkrshna@gmail.com>
 * @copyright 2021 Baal Krshna
 */

namespace BaalKrshna\SessionHandler;

/**
 * MySQL/MariaDB Session Handler - Handle Sessions using Database
 *
 * @author Puneet Gopinath (PuneetGopinath) <baalkrshna@gmail.com>
 */
class MySQLHandler implements \SessionHandlerInterface
{
    /**
     * MySQL/MariaDB Session Handler Version number
     * Used for easier checks, like if SessionHandler is up to date or not
     *
     * @var string VERSION The Version number
     */
    const VERSION = "0.1.0";

    /**
     * Session Lifetime, default 2 hrs
     *
     * @var integer $expiry The expiry time in seconds
     */
    private $expiry = 7200;

    /**
     * Session Id
     *
     * @var string $sessionId The session id
     */
    private $sessionId = null;

    /**
     * User Agent
     *
     * @var string $userAgent The User Agent
     */
    private $userAgent = null;

    /**
     * DB connection object
     *
     * @var object $db_conn The DB connection object
     */
    private $db_conn = null;

    /**
     * DB table name for storing session info
     *
     * @var string $tablename The Table name
     */
    private $tablename = "sessions";

    /**
     * Session name
     *
     * @var string $sessionName The Session name
     */
    private $sessionName = "PHPSESSID";

    /**
     * MySQLHandler class constructor.
     *
     * @param array $config The config settings
     * @return MySQLHandler The MySQLHandler class
     */
    public function __construct($config)
    {
        /*session_set_save_handler(
            array($this, "open"),
            array($this, "close"),
            array($this, "read"),
            array($this, "write"),
            array($this, "destroy"),
            array($this, "gc")
        );*/
        session_set_save_handler($this, true);
        $this->setConfig($config);
        session_start();
    }

    /**
     * Set config
     *
     * @param array $config The config settings
     * @return bool The return value (usually TRUE on success, FALSE on failure).
     */
    private function setConfig($config)
    {
        if (empty($config["db_conn"])) {
            error_log("DB connection not set in config!!");
            return false;
        }

        $this->db_conn = $config["db_conn"];
        $this->tablename = empty($config["tablename"]) ? $this->tablename : $config["tablename"];
        $this->expiry = empty($config["expiry"]) ? $this->expiry : $config["expiry"];
        $this->userAgent = $_SERVER["HTTP_USER_AGENT"];

        ini_set("session.gc_maxlifetime", $this->expiry);
        ini_set("session.gc_probability", "0");

        if (
            $stmt = $this->db_conn->prepare(
                "CREATE TABLE IF NOT EXISTS $this->tablename (" .
                "session_id VARCHAR( 255 ) NOT NULL ," .
                "data TEXT NOT NULL ," .
                "userAgent VARCHAR( 255 ) NOT NULL ," .
                "lastModified DATETIME NOT NULL ," .
                "PRIMARY KEY ( session_id )" .
                ")"
            )
        ) {
            if (!$stmt->execute()) {
                return false;
            }
            $stmt->close();
        } else {
            return false;
        }

        return true;
    }

    /**
     * Refresh the session
     *
     * @return bool The return value (usually TRUE on success, FALSE on failure).
     */
    private function refresh()
    {
        $currentId = session_id();
        session_regenerate_id();
        $this->sessionId = session_id();
        if (
            $stmt = $this->db_conn->prepare(
                "UPDATE $this->tablename SET session_id=? WHERE session_id=?"
            )
        ) {
            $stmt->bind_param(
                "ss",
                $currentId,
                $this->sessionId
            );
            if (!$stmt->execute()) {
                return false;
            }
            $stmt->close();
        } else {
            return false;
        }
        return true;
    }

    /**
     * Open/Start session
     *
     * @param string $savePath The path where to store/retrieve the session.
     * @param string $sessionName The session name
     * @return bool The return value (usually TRUE on success, FALSE on failure).
     */
    public function open($savePath, $sessionName)
    {
        $this->sessionName = $sessionName;
        return true;
    }

    /**
     * Close session
     *
     * @return bool The return value (usually TRUE on success, FALSE on failure).
     */
    public function close()
    {
        if (!$this->gc()) {
            return false;
        }
        return true;
    }

    /**
     * Read session data
     *
     * @param string $id The session id
     * @return string The data read from database
     */
    public function read($id)
    {
        if (
            $stmt = $this->db_conn->prepare(
                "SELECT data, session_id FROM $this->tablename WHERE session_id=?"
            )
        ) {
            $stmt->bind_param("s", $id);
            $stmt->execute();
            $stmt->bind_result($data, $sessionId);
            $stmt->close();
            if (empty($sessionId)) {
                $this->refresh();
                return "";
            }
            return $data;
        } else {
            return "";
        }
    }

    /**
     * Write data to session
     *
     * @param string $id The session id
     * @param string $data The data to write
     * @return bool The return value (usually TRUE on success, FALSE on failure).
     */
    public function write($id, $data)
    {
        $date = date("Y-m-d H:i:s");
        $read = $this->read($id);
        if (empty($read) || !$read) {
            if (
                $stmt = $this->db_conn->prepare(
                    "INSERT INTO $this->tablename (
                    session_id,
                    data,
                    lastModified,
                    userAgent
                )
                VALUES
                (?, ?, ?, ?)"
                )
            ) {
                $stmt->bind_param(
                    "ssss",
                    $id,
                    $data,
                    $date,
                    $this->userAgent
                );
                if (!$stmt->execute()) {
                    return false;
                }
                $stmt->close();
            } else {
                return false;
            }
        } else {
            if (
                $stmt = $this->db_conn->prepare(
                    "UPDATE $this->tablename SET data=?, lastModified=?, userAgent=? WHERE session_id=?"
                )
            ) {
                $stmt->bind_param(
                    "ssss",
                    $data,
                    $date,
                    $this->userAgent,
                    $id
                );
                if (!$stmt->execute()) {
                    return false;
                }
                $stmt->close();
            } else {
                return false;
            }
        }
        return true;
    }

    /**
     * Destroy the session
     *
     * @param string $id The session id
     * @return bool The return value (usually TRUE on success, FALSE on failure).
     */
    public function destroy($id)
    {
        if (
            $stmt = $this->db_conn->prepare(
                "DELETE FROM $this->tablename WHERE session_id=?"
            )
        ) {
            $stmt->bind_param(
                "s",
                $id
            );
            if (!$stmt->execute()) {
                return false;
            }
            $stmt->close();
        } else {
            return false;
        }
        return true;
    }

    /**
     * Do garbage collection
     *
     * @param int $max_lifetime Sessions that have not updated for the last maxlifetime seconds will be removed.
     * @return int|bool The return value (usually TRUE on success, FALSE on failure).
     */
    public function gc($max_lifetime = null)
    {
        if (empty($max_lifetime)) {
            $max_lifetime = $this->expiry;
        }
        $sessionLife = time() - $max_lifetime;
        if (
            $stmt = $this->db_conn->prepare(
                "DELETE FROM $this->tablename WHERE lastModified < ?"
            )
        ) {
            $stmt->bind_param(
                "s",
                $sessionLife
            );
            if (!$stmt->execute()) {
                return false;
            }
            $stmt->close();
        } else {
            return false;
        }
        return true;
    }
}

The above code follows PSR12. Even though it follows PSR12, the only thing it doesn't follow is about the visibility of constant.

I have some questions:

  1. Is not closing the connection going to have problems?
  2. If question 1 is yes, then how can I get a new mysqli object in the next time I need to use mysqli
  3. I want to know whether the write function will not be used to update existing fields in dB?

Testing the above code: You have to edit the MySQL credentials in the mysqli_connect function's args.

$connection = mysqli_connect(
    "localhost", //Hostname
    "root", //Username
    "password", //Password
    "test" //DB name
);
$handler = new \BaalKrshna\SessionHandler\MySQLHandler(
    array(
        "db_conn" => $connection
    )
);
//Session handler already set in construct method
$_SESSION["foo"] = "bar";
echo $_SESSION["foo"];
session_write_close();
session_gc();
session_destroy();
\$\endgroup\$
9
  • \$\begingroup\$ The gc() metod is broken. It looks like a copy of destroy method. \$\endgroup\$ – slepic Apr 26 at 18:45
  • \$\begingroup\$ So your code is not working as intended? \$\endgroup\$ – mickmackusa Apr 26 at 22:07
  • \$\begingroup\$ Yes, my code is not working as intended \$\endgroup\$ – Puneet Gopinath Apr 27 at 4:43
  • \$\begingroup\$ @slepic Yes, I forgot to edit it, one sec \$\endgroup\$ – Puneet Gopinath Apr 27 at 4:45
  • \$\begingroup\$ Are you sure it is NOT working as intended? I ask because one of the criteria of Code Review is: "The code must not error or contain known bugs". Asking us to review code that's not working could get your question closed. \$\endgroup\$ – KIKO Software Apr 27 at 9:50
4
\$\begingroup\$
  • Use the null coalescing operator for all occurrences where you want to provide fallback values for undeclared/null variables.

    $this->tablename = $config["tablename"] ?? $this->tablename;
    
  • I agree with @Dharman's comment under the question, don't bother with manually checking for mysqli errors. See the commented links.

    private function refresh(): int
    {
        $currentId = session_id();
        session_regenerate_id();
        $this->sessionId = session_id();
        $stmt = $this->db_conn->prepare("UPDATE $this->tablename SET session_id=? WHERE session_id=?");
        $stmt->bind_param("ss", $currentId, $this->sessionId);
        $stmt->execute();
        return $this->db_conn->affected_rows;
    }
    
  • I don't personally ever bother manually closing prepared statements. PHP is going to automatically do that for you when I knows it is done using them.

  • Simplify boolean return by not manually typing the true/false. Your IDE might even be alerting you to this.

    public function close()
    {
        return (bool) $this->gc();
    }
    
  • Set up your database table(s) to have lastModified DEFAULT to the current timestamp, this way you don't have to manually write that in your sql when INSERTing a new row.

  • Inside of write(), the following is unnecessary/redundant:

    if (empty($read) || !$read) {
    

    Instead, just do a falsey check because you know that the variable will be unconditionally declared.
    if (!$read) {

  • As a general rule, when a method is performing an INSERT, I typically return the autogenerated id (whenever possible) -- even if I don't need it right now, it is possible that I may want it in the future. For UPDATE and DELETE queries, I return the affected rows as an integer -- this allows me to verify that a change had actually occurred from the executed query and knowing how many rows were affected can sometimes help with diagnostics. In both all "database writing" cases, the return value is easily compared as truthy/falsey if your method call doesn't need the excessive specificity in the outcome.

  • Again, in gc(), if (empty($max_lifetime)) { is not necessary -- the variable WILL be declared, do a falsey / function-less check here. Or even better avoid the single-use variable declarations and use the null coalescing operator again.

    public function gc($max_lifetime = null): int
    {
        $stmt = $this->db_conn->prepare("DELETE FROM $this->tablename WHERE lastModified < ?");
        $modifiedTime = time() - ($max_lifetime ?? $this->expiry);
        $stmt->bind_param("s", $modifiedTime);
        $stmt->execute();
        $this->db_conn->affected_rows;
    }
    
  • Security suggestion:
    Don't support PHP 5.3 instead support the versions of php which still receive security updates, See supported versions of php here

    PHP 5.3 has a lot of security issues.

\$\endgroup\$
14
  • \$\begingroup\$ You said "Set up your database table(s) to have lastModified DEFAULT to the current timestamp, this way you don't have to manually write that in your sql when INSERTing a new row.", how to do that \$\endgroup\$ – Puneet Gopinath Apr 29 at 6:13
  • 2
    \$\begingroup\$ PHP5.3!!!! 😱 Leave that job! \$\endgroup\$ – mickmackusa Apr 29 at 6:15
  • 1
    \$\begingroup\$ Ok i should use DEFAULT CURRENT_TIMESTAMP right? \$\endgroup\$ – Puneet Gopinath Apr 29 at 6:17
  • 2
    \$\begingroup\$ By being stuck in 5.3 you are missing out on an enormous sea of beautiful enhancements and security. There is no excuse for work in the dark ages -- convince the stakeholders that their application is not secure and funds/time must be appropriated to upgrade the site. \$\endgroup\$ – mickmackusa Apr 29 at 6:20
  • 2
    \$\begingroup\$ Don't let them decide -- force them to use non-deprecated versions of php! You may let them choose between different non-deprecated versions of php. \$\endgroup\$ – mickmackusa Apr 29 at 6:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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