fix for https://github.com/XOOPS/XoopsCore25/issues/1588 #1589

mambax7 · 2025-10-23T02:52:16Z

No description provided.

Copilot

Pull Request Overview

This PR refactors the XOOPS session handler to address issue #1588 by splitting the implementation into PHP version-specific files. The main session.php file now acts as a loader that includes either session74.php (for PHP < 8.0) or session80.php (for PHP >= 8.0), enabling proper type declarations and interface implementations for each PHP version.

Key changes:

Introduced version-specific session handlers with appropriate type hints for their respective PHP versions
PHP 8.0+ handler implements SessionUpdateTimestampHandlerInterface for lazy timestamp updates
Consolidated domain validation logic across both implementations

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
htdocs/kernel/session.php	Converted to a loader shim that conditionally includes the appropriate version-specific handler
htdocs/kernel/session74.php	New PHP 7.4-compatible handler without type declarations, implementing SessionHandlerInterface
htdocs/kernel/session80.php	New PHP 8.0+ handler with full type declarations and SessionUpdateTimestampHandlerInterface support

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

htdocs/kernel/session74.php

Copilot · 2025-10-23T02:53:15Z

htdocs/kernel/session80.php

+        $this->gc_force();
+        return true;
+    }
+


The return type should be string|false to match SessionHandlerInterface requirements in PHP 8.0+. However, the method returns an empty string ('') when the session is not found (line 111), and returns false on storage failure (line 106). Consider documenting this distinction in a docblock, as it deviates from the typical pattern where both cases might return false.

Suggested change

/**

* Read session data from storage.

*

* Returns the session data as a string if found and valid.

* Returns an empty string ('') if the session is not found or the IP does not match.

* Returns false on storage failure (e.g., database error).

*

* This behaviour deviates from the typical pattern where both "not found" and "error" cases return false.

*

* @param string $sessionId Session ID

* @return string|false Session data string, empty string if not found/IP mismatch, or false on storage failure

*/

htdocs/kernel/session74.php

That keeps aligned with PHP 8.4’s deprecations and cookies-only sessions.

Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

htdocs/kernel/session74.php

htdocs/include/functions.php

htdocs/kernel/session80.php

htdocs/kernel/session.php

htdocs/kernel/session80.php

Copilot · 2025-12-29T10:11:24Z

htdocs/kernel/session80.php

+
+        $result = $this->db->queryF($sql);
+        if (!$this->db->isResultSet($result)) {
+            return false; // storage failure


The read() method returns false when isResultSet() fails, indicating a database/storage failure. However, according to the SessionHandlerInterface documentation, read() should return an empty string on failure, not false. The return type in PHP 8.0 is string|false, where false specifically indicates a storage system failure. While this is technically compliant, it may be more consistent to return an empty string and log the error instead, as most session handlers do. This distinction should be documented or reconsidered for clarity.

Suggested change

return false; // storage failure

// Storage failure: log the problem and return empty string as per SessionHandlerInterface guidance

error_log(sprintf('XoopsSessionHandler::read() storage failure for session id %s', $sessionId));

return '';

Copilot · 2025-12-29T10:11:25Z

htdocs/kernel/session80.php

+<?php
+/**
+ * XOOPS session handler
+ *
+ * You may not change or alter any portion of this comment or credits
+ * of supporting developers from this source code or any supporting source code
+ * which is considered copyrighted (c) material of the original comment or credit authors.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * @copyright       (c) 2000-2025 XOOPS Project (https://xoops.org)
+ * @license             GNU GPL 2 (https://www.gnu.org/licenses/gpl-2.0.html)
+ * @package             kernel
+ * @since               2.0.0
+ * @author              Kazumi Ono (AKA onokazu) http://www.myweb.ne.jp/, http://jp.xoops.org/
+ * @author              Taiwen Jiang <[email protected]>
+ */
+
+defined('XOOPS_ROOT_PATH') || exit('Restricted access');
+
+/**
+ * XOOPS session handler (PHP 8.0+ with full types and lazy timestamp updates)
+ * @package             kernel
+ *
+ * @author              Kazumi Ono    <[email protected]>
+ * @author              Taiwen Jiang <[email protected]>
+ * @author              XOOPS Development Team
+ * @copyright       (c) 2000-2025 XOOPS Project (https://xoops.org)
+ */
+class XoopsSessionHandler implements
+    \SessionHandlerInterface,
+    \SessionUpdateTimestampHandlerInterface
+{
+    /** @var XoopsDatabase */
+    public $db;
+
+    /** @var int */
+    public $securityLevel = 3;
+
+    protected array $bitMasks = [
+        2 => ['v4' => 16, 'v6' => 64],
+        3 => ['v4' => 24, 'v6' => 56],
+        4 => ['v4' => 32, 'v6' => 128],
+    ];
+
+    public bool $enableRegenerateId = true;
+
+    public function __construct(XoopsDatabase $db)
+    {
+        global $xoopsConfig;
+        $this->db = $db;
+
+        $lifetime = ($xoopsConfig['use_mysession'] && $xoopsConfig['session_name'] != '')
+            ? $xoopsConfig['session_expire'] * 60
+            : ini_get('session.cookie_lifetime');
+
+        $secure = (XOOPS_PROT === 'https://');
+
+        $host = parse_url(XOOPS_URL, PHP_URL_HOST);
+        if (!is_string($host)) {
+            $host = '';
+        }
+        $cookieDomain = XOOPS_COOKIE_DOMAIN;
+        if (class_exists('\Xoops\RegDom\RegisteredDomain')) {
+            if (!\Xoops\RegDom\RegisteredDomain::domainMatches($host, $cookieDomain)) {
+                $cookieDomain = '';
+            }
+        }
+
+        $options = [
+            'lifetime' => $lifetime,
+            'path'     => '/',
+            'domain'   => $cookieDomain,
+            'secure'   => $secure,
+            'httponly' => true,
+            'samesite' => 'Lax',
+        ];
+        session_set_cookie_params($options);
+    }
+
+    // --- SessionHandlerInterface (typed) ---
+
+    public function open(string $savePath, string $sessionName): bool
+    {
+        return true;
+    }
+
+    public function close(): bool
+    {
+        $this->gc_force();
+        return true;
+    }
+
+    public function read(string $sessionId): string|false
+    {
+        $ip = \Xmf\IPAddress::fromRequest();
+        $sql = sprintf(
+            'SELECT sess_data, sess_ip FROM %s WHERE sess_id = %s',
+            $this->db->prefix('session'),
+            $this->db->quote($sessionId)
+        );
+
+        $result = $this->db->queryF($sql);
+        if (!$this->db->isResultSet($result)) {
+            return false; // storage failure
+        }
+
+        $row = $this->db->fetchRow($result);
+        if ($row === false) {
+            return ''; // not found → empty string
+        }
+
+        [$sess_data, $sess_ip] = $row;
+
+        if ($this->securityLevel > 1) {
+            if (false === $ip->sameSubnet(
+                    $sess_ip,
+                    $this->bitMasks[$this->securityLevel]['v4'],
+                    $this->bitMasks[$this->securityLevel]['v6']
+                )) {
+                return ''; // IP mismatch → treat as no data
+            }
+        }
+
+        return $sess_data;
+    }
+
+    public function write(string $sessionId, string $data): bool
+    {
+        $remoteAddress = \Xmf\IPAddress::fromRequest()->asReadable();
+        $sid = $this->db->quote($sessionId);
+        $now = time();
+
+        $sql = sprintf(
+            'INSERT INTO %s (sess_id, sess_updated, sess_ip, sess_data)
+             VALUES (%s, %u, %s, %s)
+             ON DUPLICATE KEY UPDATE
+             sess_updated = %u, sess_data = %s',
+            $this->db->prefix('session'),
+            $sid,
+            $now,
+            $this->db->quote($remoteAddress),
+            $this->db->quote($data),
+            $now,
+            $this->db->quote($data)
+        );
+
+        $ok = $this->db->exec($sql);
+        // update_cookie() only affects <7.3 in your code; on 8+ it is effectively a no-op
+        $this->update_cookie();
+        return (bool)$ok;
+    }
+
+    public function destroy(string $sessionId): bool
+    {
+        $sql = sprintf(
+            'DELETE FROM %s WHERE sess_id = %s',
+            $this->db->prefix('session'),
+            $this->db->quote($sessionId)
+        );
+        return (bool)$this->db->exec($sql);
+    }
+
+    public function gc(int $max_lifetime): int|false
+    {
+        if ($max_lifetime <= 0) {
+            return 0;
+        }
+        $mintime = time() - $max_lifetime;
+        $sql = sprintf(
+            'DELETE FROM %s WHERE sess_updated < %u',
+            $this->db->prefix('session'),
+            $mintime
+        );
+        if ($this->db->exec($sql)) {
+            return (int)$this->db->getAffectedRows();
+        }
+        return false;
+    }
+
+    // --- SessionUpdateTimestampHandlerInterface (8.0+) ---
+
+    public function validateId(string $id): bool
+    {
+        $sql = sprintf(
+            'SELECT 1 FROM %s WHERE sess_id = %s',
+            $this->db->prefix('session'),
+            $this->db->quote($id)
+        );
+        $res = $this->db->queryF($sql, 1, 0);
+        return $this->db->isResultSet($res) && $this->db->fetchRow($res) !== false;
+    }
+
+    public function updateTimestamp(string $id, string $data): bool
+    {
+        $sql = sprintf(
+            'UPDATE %s SET sess_updated = %u WHERE sess_id = %s',
+            $this->db->prefix('session'),
+            time(),
+            $this->db->quote($id)
+        );
+        return (bool)$this->db->exec($sql);
+    }
+
+    // --- Helpers (same behavior as your current code) ---
+
+    public function gc_force(): void
+    {
+        try {
+            if (\random_int(1, 100) < 11) {
+                $expire = (int) @ini_get('session.gc_maxlifetime');
+                if ($expire <= 0) {
+                    $expire = 900;
+                }
+                $this->gc($expire);
+            }
+        } catch (\Throwable $e) {
+            // ignore
+        }
+    }
+
+    public function regenerate_id(bool $delete_old_session = false): bool
+    {
+        $success = $this->enableRegenerateId
+            ? session_regenerate_id($delete_old_session)
+            : true;
+
+        if ($success) {
+            $this->update_cookie();
+        }
+        return $success;
+    }
+
+    public function update_cookie($sess_id = null, $expire = null): void
+    {
+        // no-op for 8.0+; retained for parity with 7.4 implementation
+    }
+}


There is significant code duplication between session74.php and session80.php. The constructor, security logic, database queries, and helper methods are nearly identical. While the separation by PHP version is necessary for type compatibility, consider extracting common logic into a shared trait or abstract base class to reduce duplication and improve maintainability. This would make future bug fixes and enhancements easier to apply consistently across both implementations.

@copilot open a new pull request to apply changes based on this feedback

htdocs/kernel/session80.php

Co-authored-by: Copilot <[email protected]>

fix for XOOPS#1588

4cbb3fd

mambax7 requested a review from Copilot October 23, 2025 02:52

Copilot AI reviewed Oct 23, 2025

View reviewed changes

mambax7 and others added 4 commits October 22, 2025 22:58

Remove trans-SID (url SID appending)

3f7b88f

That keeps aligned with PHP 8.4’s deprecations and cookies-only sessions.

Update htdocs/kernel/session74.php

974edb1

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session74.php

341a106

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session74.php

805311e

Co-authored-by: Copilot <[email protected]>

mambax7 requested a review from Copilot December 29, 2025 10:05

Copilot started reviewing on behalf of mambax7 December 29, 2025 10:05 View session

Merge branch 'master' into feature/session_for_PHP8.4

9e0780d

Copilot AI reviewed Dec 29, 2025

View reviewed changes

mambax7 and others added 7 commits December 29, 2025 05:15

Update htdocs/kernel/session74.php

94746fa

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session74.php

d1feefa

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session.php

5f811f2

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session80.php

b9a2867

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session80.php

470c048

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session80.php

dbb23d9

Co-authored-by: Copilot <[email protected]>

Update htdocs/kernel/session80.php

c62b1fd

Co-authored-by: Copilot <[email protected]>

mambax7 merged commit ecd419c into XOOPS:master Dec 29, 2025
4 checks passed

+    /**
+     * Read session data from storage.
+     *
+     * Returns the session data as a string if found and valid.
+     * Returns an empty string ('') if the session is not found or the IP does not match.
+     * Returns false on storage failure (e.g., database error).
+     *
+     * This behaviour deviates from the typical pattern where both "not found" and "error" cases return false.
+     *
+     * @param string $sessionId Session ID
+     * @return string|false Session data string, empty string if not found/IP mismatch, or false on storage failure
+     */

Uh oh!

fix for https://github.com/XOOPS/XoopsCore25/issues/1588 #1589

fix for https://github.com/XOOPS/XoopsCore25/issues/1588 #1589

Uh oh!

Conversation

mambax7 commented Oct 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

mambax7 Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant