diff --git a/docs/changelogs/v4.7.7.md b/docs/changelogs/v4.7.7.md index ac104dc1bb40..8bd788fefd88 100644 --- a/docs/changelogs/v4.7.7.md +++ b/docs/changelogs/v4.7.7.md @@ -19,6 +19,7 @@ Release Date: Unreleased - **CORS:** Fixed a bug where `Cors::hasResponseHeaders()` matched header substrings via `str_contains()`, causing false positives when existing exposed headers shared a prefix with configured exposed headers. - **HTTP:** Fixed a bug where `UploadedFile::move()` and `UploadedFile::store()` on Windows could store a file under a different name than `getName()` reported, or write its contents to an NTFS alternate data stream. Filenames ending in a period or space or containing a colon are now rejected, including explicitly supplied names. - **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 `BaseHandler::destroyCookie()` did not send the `SameSite` attribute in the `Set-Cookie` header, which could prevent browsers from properly clearing the session cookie. - **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/BaseHandler.php b/system/Session/Handlers/BaseHandler.php index 4c6a7ddb8211..b518edde16f3 100644 --- a/system/Session/Handlers/BaseHandler.php +++ b/system/Session/Handlers/BaseHandler.php @@ -13,6 +13,7 @@ namespace CodeIgniter\Session\Handlers; +use CodeIgniter\Cookie\Cookie; use Config\Cookie as CookieConfig; use Config\Session as SessionConfig; use Psr\Log\LoggerAwareTrait; @@ -70,6 +71,13 @@ abstract class BaseHandler implements SessionHandlerInterface */ protected $cookieSecure = false; + /** + * Cookie SameSite setting. + * + * @var ''|'Lax'|'None'|'Strict' + */ + protected string $cookieSameSite = ''; + /** * Cookie name to use. * @@ -116,9 +124,10 @@ public function __construct(SessionConfig $config, string $ipAddress) $cookie = config(CookieConfig::class); // Session cookies have no prefix. - $this->cookieDomain = $cookie->domain; - $this->cookiePath = $cookie->path; - $this->cookieSecure = $cookie->secure; + $this->cookieDomain = $cookie->domain; + $this->cookiePath = $cookie->path; + $this->cookieSecure = $cookie->secure; + $this->cookieSameSite = $cookie->samesite; $this->ipAddress = $ipAddress; } @@ -129,10 +138,21 @@ public function __construct(SessionConfig $config, string $ipAddress) */ protected function destroyCookie(): bool { + $samesite = $this->cookieSameSite === '' + ? Cookie::SAMESITE_LAX + : ucfirst(strtolower($this->cookieSameSite)); + return setcookie( $this->cookieName, '', - ['expires' => 1, 'path' => $this->cookiePath, 'domain' => $this->cookieDomain, 'secure' => $this->cookieSecure, 'httponly' => true], + [ + 'expires' => 1, + 'path' => $this->cookiePath, + 'domain' => $this->cookieDomain, + 'secure' => $this->cookieSecure, + 'httponly' => true, + 'samesite' => $samesite, + ], ); } diff --git a/tests/system/Session/Handlers/BaseHandlerTest.php b/tests/system/Session/Handlers/BaseHandlerTest.php new file mode 100644 index 000000000000..96eda274b996 --- /dev/null +++ b/tests/system/Session/Handlers/BaseHandlerTest.php @@ -0,0 +1,198 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Session\Handlers; + +use CodeIgniter\Config\Factories; +use CodeIgniter\Test\CIUnitTestCase; +use Config\Cookie as CookieConfig; +use Config\Session as SessionConfig; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; + +/** + * Mock setcookie function in CodeIgniter\Session\Handlers namespace. + * + * @param array $options + */ +function setcookie(string $name, string $value = '', array $options = []): bool +{ + BaseHandlerTest::$lastSetCookieArgs = [ + 'name' => $name, + 'value' => $value, + 'options' => $options, + ]; + + return true; +} + +/** + * @internal + */ +#[Group('Others')] +final class BaseHandlerTest extends CIUnitTestCase +{ + /** + * @var array{name: string, value: string, options: array}|null + */ + public static ?array $lastSetCookieArgs = null; + + protected function setUp(): void + { + parent::setUp(); + + self::$lastSetCookieArgs = null; + Factories::reset('config'); + } + + protected function tearDown(): void + { + parent::tearDown(); + + self::$lastSetCookieArgs = null; + Factories::reset('config'); + } + + private function createHandler(?CookieConfig $cookieConfig = null): BaseHandler + { + if ($cookieConfig instanceof CookieConfig) { + Factories::injectMock('config', CookieConfig::class, $cookieConfig); + } + + $sessionConfig = new SessionConfig(); + $sessionConfig->cookieName = 'ci_session'; + + return new class ($sessionConfig, '127.0.0.1') extends BaseHandler { + public function open(string $path, string $name): bool + { + return true; + } + + public function close(): bool + { + return true; + } + + public function read(string $id): string + { + return ''; + } + + public function write(string $id, string $data): bool + { + return true; + } + + public function destroy(string $id): bool + { + return $this->destroyCookie(); + } + + public function gc(int $maxLifetime): int + { + return 1; + } + }; + } + + public function testConstructorInitializesCookieSameSiteFromConfig(): void + { + $cookieConfig = new CookieConfig(); + $cookieConfig->samesite = 'Strict'; + + $handler = $this->createHandler($cookieConfig); + + $this->assertSame('Strict', $this->getPrivateProperty($handler, 'cookieSameSite')); + } + + public function testConstructorDefaultsToCookieConfigSameSite(): void + { + $handler = $this->createHandler(); + + $this->assertSame('Lax', $this->getPrivateProperty($handler, 'cookieSameSite')); + } + + public function testDestroyCookiePassesSameSiteInOptions(): void + { + $cookieConfig = new CookieConfig(); + $cookieConfig->domain = 'example.com'; + $cookieConfig->path = '/app'; + $cookieConfig->secure = true; + $cookieConfig->samesite = 'Strict'; + + $handler = $this->createHandler($cookieConfig); + $result = $handler->destroy('ci_session'); + + $this->assertTrue($result); + $this->assertNotNull(self::$lastSetCookieArgs); + $this->assertSame('ci_session', self::$lastSetCookieArgs['name']); + $this->assertSame('', self::$lastSetCookieArgs['value']); + + $options = self::$lastSetCookieArgs['options']; + $this->assertSame(1, $options['expires']); + $this->assertSame('/app', $options['path']); + $this->assertSame('example.com', $options['domain']); + $this->assertTrue($options['secure']); + $this->assertTrue($options['httponly']); + $this->assertSame('Strict', $options['samesite']); + } + + public function testDestroyCookieNormalizesEmptySameSiteToLax(): void + { + $cookieConfig = new CookieConfig(); + $cookieConfig->samesite = ''; + + $handler = $this->createHandler($cookieConfig); + $handler->destroy('ci_session'); + + $this->assertNotNull(self::$lastSetCookieArgs); + $this->assertSame('Lax', self::$lastSetCookieArgs['options']['samesite']); + } + + public function testDestroyCookieNormalizesLowercaseSameSite(): void + { + $handler = $this->createHandler(); + $this->setPrivateProperty($handler, 'cookieSameSite', 'none'); + $handler->destroy('ci_session'); + + $this->assertNotNull(self::$lastSetCookieArgs); + $this->assertSame('None', self::$lastSetCookieArgs['options']['samesite']); + } + + #[DataProvider('provideDestroyCookieNormalizesSameSiteCase')] + public function testDestroyCookieNormalizesSameSiteCase(string $input, string $expected): void + { + $handler = $this->createHandler(); + $this->setPrivateProperty($handler, 'cookieSameSite', $input); + $handler->destroy('ci_session'); + + $this->assertNotNull(self::$lastSetCookieArgs); + $this->assertSame($expected, self::$lastSetCookieArgs['options']['samesite']); + } + + /** + * @return iterable + */ + public static function provideDestroyCookieNormalizesSameSiteCase(): iterable + { + yield 'uppercase LAX' => ['LAX', 'Lax']; + + yield 'uppercase STRICT' => ['STRICT', 'Strict']; + + yield 'uppercase NONE' => ['NONE', 'None']; + + yield 'mixed case sTrIcT' => ['sTrIcT', 'Strict']; + + yield 'empty string' => ['', 'Lax']; + } +}