fix: MysqlSession should validate session ID format before checking DB

CHANGELOG.md

@@ -1,5 +1,10 @@
 ### Unreleased
 
+### v2.5.1 (2025-09-17)
+
+* Fix MysqlSession to immediately return false (without database check) if session ID does not match the pattern of IDs
+  generated by the application.
+
 ### v2.5.0 (2025-06-26)
 
 * Support PHP 8.4

src/Session/MysqlSession.php

@@ -12,6 +12,7 @@
 use Ingenerator\PHPUtils\StringEncoding\StringSanitiser;
 use PDO;
 use SessionHandlerInterface;
+use UnexpectedValueException;
 
 class MysqlSession implements SessionHandlerInterface, \SessionUpdateTimestampHandlerInterface, \SessionIdInterface
 {
@@ -186,6 +187,12 @@ public function create_sid(): string
      */
     public function validateId($session_id): bool
     {
+        if ( ! (is_string($session_id) && preg_match($this->getValidSessionIdRegex(), $session_id))) {
+            // It cannot be a valid SID if it doesn't match the format that we generate
+            // It's likely a credential stuffing bot - ignore it
+            return false;
+        }
+
         $now = new \DateTimeImmutable();
         $this->getLock($session_id);
 
@@ -230,6 +237,28 @@ public function validateId($session_id): bool
         }
     }
 
+    private function getValidSessionIdRegex(): string
+    {
+        // sid_length and sid_bits_per_character can be configured with INI settings, but this is deprecated from 8.4
+        // https://wiki.php.net/rfc/deprecations_php_8_4#sessionsid_length_and_sessionsid_bits_per_character
+        // In future they will always be 32 byte hexadecimal strings.
+        // In the meantime we need to accommodate the potential that the running app has different defaults.
+        return sprintf(
+            match (ini_get('session.sid_bits_per_character')) {
+                '5' => '/^[0-9a-v]{%d}$/',
+                '6' => '/^[0-9a-zA-Z,-]{%d}$/',
+                // The default, which will also become the standard once the INI setting is removed (at which point
+                // ini_get will return false)
+                false,
+                '4' => '/^[0-9a-f]{%d}$/',
+                default => throw new UnexpectedValueException(
+                    'Unexpected ini setting for session.sid_bits_per_character',
+                ),
+            },
+            ini_get('session.sid_length') ?: 32,
+        );
+    }
+
     /**
      * {@inheritdoc}
      */

test/phpunit-bootstrap.php

@@ -6,6 +6,11 @@
 // https://github.com/sebastianbergmann/phpunit/issues/2449
 \set_error_handler(
     function ($severity, $message, $file, $line) {
-        throw new ErrorException($message, 0, $severity, $file, $line);
-    }
+        if (error_reporting() & $severity) {
+            throw new ErrorException($message, 0, $severity, $file, $line);
+        }
+
+        // This error has been silenced locally, ignore it
+        return true;
+    },
 );

test/unit/Session/MysqlSessionTest.php

@@ -7,24 +7,189 @@
 namespace test\unit\Ingenerator\PHPUtils\Session;
 
 
+use Closure;
 use Ingenerator\PHPUtils\Session\MysqlSession;
+use PDO;
+use PDOStatement;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
+use UnexpectedValueException;
 
 class MysqlSessionTest extends TestCase
 {
+    private array $old_ini_vars = [];
 
     public function test_it_is_initialisable()
     {
         $this->assertInstanceOf(MysqlSession::class, $this->newSubject());
     }
 
-    protected function newSubject()
+    public static function provider_validate_invalid_sid(): array
     {
-        return new MysqlSession(new PDOMock, 'insecure-salt');
+        return [
+            'with path injection attempt' => [
+                [],
+                '../../../../../../../../../../../../../../windows/win.ini',
+            ],
+            'with url injection attempt' => [
+                [],
+                'http://some-inexistent-website.acu/some_inexistent_file_with_long_name?.jpg',
+            ],
+            'with SQL injection attempt' => [
+                [],
+                "'; TRUNCATE sessions;",
+            ],
+            'with invalid chars (in default bits per character)' => [
+                [],
+                str_pad('v', 32, 'a'),
+            ],
+            'with SID too short (with default length configuration)' => [
+                [],
+                str_repeat('a', 31),
+            ],
+            'with SID too long (with default length configuration)' => [
+                [],
+                str_repeat('a', 33),
+            ],
+            'with invalid characters (using custom bits)' => [
+                ['session.sid_bits_per_character' => '5'],
+                str_pad(',', 32, 'a'),
+            ],
+            'too long (with custom length)' => [
+                ['session.sid_length' => '44'],
+                str_repeat('a', 45),
+            ],
+            'too short (with custom length)' => [
+                ['session.sid_length' => '44'],
+                str_repeat('a', 43),
+            ],
+        ];
     }
 
-}
+    #[DataProvider('provider_validate_invalid_sid')]
+    public function test_it_rejects_invalid_session_ids_without_checking_database(array $config, string $sid): void
+    {
+        $this->configurePhpIniVars($config);
+        $subject = $this->newSubject(pdo: $this->mockPDOExpectingNoCalls());
+        $this->assertFalse($subject->validateId($sid));
+    }
+
+    public static function provider_validate_own_sid(): array
+    {
+        return [
+            'with default config' => [
+                [],
+                '/^[0-9a-f]{32}$/',
+            ],
+            'with 5 bits per char' => [
+                ['session.sid_bits_per_character' => '5'],
+                '/^[0-9a-v]{32}$/',
+            ],
+            'with 6 bits per char' => [
+                ['session.sid_bits_per_character' => '6'],
+                '/^[0-9a-zA-Z,-]{32}$/',
+            ],
+            'with custom length' => [
+                ['session.sid_length' => '22'],
+                '/^[0-9a-f]{22}$/',
+            ],
+            'with custom length and chars' => [
+                ['session.sid_length' => '22', 'session.sid_bits_per_character' => '5'],
+                '/^[0-9a-v,-]{22}$/',
+            ],
+        ];
+    }
+
+    #[DataProvider('provider_validate_own_sid')]
+    public function test_sid_that_it_creates_is_valid(array $config, string $expected_pattern): void
+    {
+        $this->configurePhpIniVars($config);
+
+        // This mocking isn't very nice - it is coupled to the implementation details of the SQL queries and the
+        // results the class expects (and how it fetches them internally). I've tried to limit that as much as possible,
+        // it would obviously be better if this ran as an integration test against an actual mysql instance - however
+        // really here I only want to test that the validation is done against the database e.g. not short-circuited by
+        // the pattern matching.
+        $pdo = $this->mockPDOToPrepareQueries(function ($query) {
+            $result = $this->createMock(PDOStatement::class);
+
+            if (str_starts_with($query, 'INSERT INTO `sessions`')) {
+                // Result of the query is never read
+                return $result;
+            }
+
+            if (str_starts_with($query, 'SELECT GET_LOCK')) {
+                // Just needs to return 1 to show that the lock is acquired. Note that validateSid does not release the
+                // lock if it successfully loads a session (it'll be released on session_write_close or end of request)
+                $result->expects($this->once())->method('fetchColumn')->willReturn('1');
+                return $result;
+            }
+
+            if (str_starts_with($query, 'SELECT `session_data`')) {
+                // Just needs to return some data, even empty
+                $result->expects($this->once())->method('fetchAll')->willReturn([['session_data' => serialize([])]]);
+                return $result;
+            }
+
+            throw new UnexpectedValueException('Un-mocked query '.$query);
+        });
+
+        $subject = $this->newSubject(pdo: $pdo);
+
+        $sid = $subject->create_sid();
+        // Sanity check that the settings applied as expected
+        $this->assertMatchesRegularExpression($expected_pattern, $sid, 'Should generate SID in expected format');
+
+        $this->assertTrue($subject->validateId($sid), 'Should validate SID "'.$sid.'"');
+    }
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        // Force to the normal default configs
+        $this->configurePhpIniVars([
+            'session.sid_bits_per_character' => '4',
+            'session.sid_length' => '32',
+        ]);
+    }
+
+    protected function tearDown(): void
+    {
+        parent::tearDown();
+        foreach ($this->old_ini_vars as $key => $value) {
+            @ini_set($key, $value);
+        }
+    }
+
+    protected function newSubject(
+        ?PDO $pdo = null
+    )
+    {
+        return new MysqlSession(
+            $pdo ?? $this->mockPDOExpectingNoCalls(),
+            'insecure-salt',
+        );
+    }
+
+    private function mockPDOExpectingNoCalls(): PDO
+    {
+        $pdo = $this->createMock(PDO::class);
+        $pdo->expects($this->never())->method($this->anything());
+        return $pdo;
+    }
+
+    private function configurePhpIniVars(array $config): void
+    {
+        foreach ($config as $setting => $value) {
+            $this->old_ini_vars[$setting] = @ini_set($setting, $value);
+        }
+    }
+
+    private function mockPDOToPrepareQueries(Closure $callback): PDO
+    {
+        $pdo = $this->createMock(PDO::class);
+        $pdo->expects($this->any())->method('prepare')->willReturnCallback($callback);
+        return $pdo;
+    }
 
-class PDOMock extends \PDO {
-    public function __construct() {}
 }