diff --git a/docs/changelogs/v4.7.7.md b/docs/changelogs/v4.7.7.md index cdf4eb2db148..a9fcfe033c9a 100644 --- a/docs/changelogs/v4.7.7.md +++ b/docs/changelogs/v4.7.7.md @@ -15,6 +15,7 @@ Release Date: Unreleased ## Bugs Fixed - **Security:** Fixed a bug where `Security::__construct()` threw a `TypeError` when the CSRF cookie was an array, causing an HTTP 500 response for affected requests. +- **Session:** Fixed a bug where `PostgreHandler::lockSession()` and `releaseLock()` interpolated session IDs and IP addresses into SQL queries instead of using query bindings. - **Worker Mode:** Fixed a bug where every request that ended with an uncaught exception, including a `404 Page Not Found` without a 404 override, called `exit()` in the exception handler. This ended the worker script, so FrankenPHP booted a new one, and the worker loop skipped its cleanup after the request. The exception handler now returns to the worker loop. See the repo's [CHANGELOG.md](https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md) for a complete list of bugs fixed. diff --git a/system/Session/Handlers/Database/PostgreHandler.php b/system/Session/Handlers/Database/PostgreHandler.php index d8c6d3c8b794..2241eafae6c5 100644 --- a/system/Session/Handlers/Database/PostgreHandler.php +++ b/system/Session/Handlers/Database/PostgreHandler.php @@ -70,9 +70,11 @@ public function gc($max_lifetime): false|int */ protected function lockSession(string $sessionID): bool { - $arg = "hashtext('{$sessionID}')" . ($this->matchIP ? ", hashtext('{$this->ipAddress}')" : ''); - if ($this->db->simpleQuery("SELECT pg_advisory_lock({$arg})") !== false) { - $this->lock = $arg; + $sql = 'SELECT pg_advisory_lock(hashtext(?)' . ($this->matchIP ? ', hashtext(?)' : '') . ')'; + $binds = $this->matchIP ? [$sessionID, $this->ipAddress] : [$sessionID]; + + if ($this->db->query($sql, $binds) !== false) { + $this->lock = $sessionID; return true; } @@ -85,11 +87,15 @@ protected function lockSession(string $sessionID): bool */ protected function releaseLock(): bool { - if (! $this->lock) { + if ($this->lock === false) { return true; } - if ($this->db->simpleQuery("SELECT pg_advisory_unlock({$this->lock})") !== false) { + $sessionID = is_string($this->lock) ? $this->lock : (string) $this->sessionID; + $sql = 'SELECT pg_advisory_unlock(hashtext(?)' . ($this->matchIP ? ', hashtext(?)' : '') . ')'; + $binds = $this->matchIP ? [$sessionID, $this->ipAddress] : [$sessionID]; + + if ($this->db->query($sql, $binds) !== false) { $this->lock = false; return true; diff --git a/tests/system/Session/Handlers/Database/PostgreHandlerMockTest.php b/tests/system/Session/Handlers/Database/PostgreHandlerMockTest.php new file mode 100644 index 000000000000..c98d8c2db4fc --- /dev/null +++ b/tests/system/Session/Handlers/Database/PostgreHandlerMockTest.php @@ -0,0 +1,102 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Session\Handlers\Database; + +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\Mock\MockConnection; +use CodeIgniter\Test\Mock\MockResult; +use Config\Session as SessionConfig; +use PHPUnit\Framework\Attributes\Group; + +/** + * @internal + */ +#[Group('Others')] +final class PostgreHandlerMockTest extends CIUnitTestCase +{ + private function createHandler(bool $matchIP, object $mockDb): PostgreHandler + { + $config = new SessionConfig(); + $config->driver = PostgreHandler::class; + $config->savePath = 'ci_sessions'; + $config->matchIP = $matchIP; + + $handler = new PostgreHandler($config, "192.168.1.1' OR '1'='1"); + $this->setPrivateProperty($handler, 'db', $mockDb); + + return $handler; + } + + private function createMockDb(): object + { + return new class (['DBDriver' => 'Postgre']) extends MockConnection { + /** + * @var list|null}> + */ + public array $executed = []; + + public function query(string $sql, $binds = null, bool $setEscapeFlags = true, string $queryClass = ''): MockResult + { + $this->executed[] = [ + 'sql' => $sql, + 'binds' => is_array($binds) ? $binds : null, + ]; + + return new MockResult($this->connID, $this->resultID); + } + }; + } + + public function testLockAndReleaseSessionWithoutMatchIP(): void + { + $db = $this->createMockDb(); + $handler = $this->createHandler(false, $db); + + $lockSession = self::getPrivateMethodInvoker($handler, 'lockSession'); + $releaseLock = self::getPrivateMethodInvoker($handler, 'releaseLock'); + + $sessionID = "sess'id; DROP TABLE users; --"; + $this->assertTrue($lockSession($sessionID)); + $this->assertTrue($releaseLock()); + + $this->assertCount(2, $db->executed); + + $this->assertSame('SELECT pg_advisory_lock(hashtext(?))', $db->executed[0]['sql']); + $this->assertSame([$sessionID], $db->executed[0]['binds']); + + $this->assertSame('SELECT pg_advisory_unlock(hashtext(?))', $db->executed[1]['sql']); + $this->assertSame([$sessionID], $db->executed[1]['binds']); + } + + public function testLockAndReleaseSessionWithMatchIP(): void + { + $db = $this->createMockDb(); + $handler = $this->createHandler(true, $db); + + $lockSession = self::getPrivateMethodInvoker($handler, 'lockSession'); + $releaseLock = self::getPrivateMethodInvoker($handler, 'releaseLock'); + + $sessionID = "sess'id; DROP TABLE users; --"; + $this->assertTrue($lockSession($sessionID)); + $this->assertTrue($releaseLock()); + + $this->assertCount(2, $db->executed); + + $this->assertSame('SELECT pg_advisory_lock(hashtext(?), hashtext(?))', $db->executed[0]['sql']); + $this->assertSame([$sessionID, "192.168.1.1' OR '1'='1"], $db->executed[0]['binds']); + + $this->assertSame('SELECT pg_advisory_unlock(hashtext(?), hashtext(?))', $db->executed[1]['sql']); + $this->assertSame([$sessionID, "192.168.1.1' OR '1'='1"], $db->executed[1]['binds']); + } +}