From 25c1ba3afb9c271e81a107f8703bc7300b8157c1 Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Mon, 26 Oct 2026 14:09:55 +0100 Subject: [PATCH] fix(session): Prevent stale requests from replacing a regenerated session Regenerating the session id - on login, on unlocking a public share, on a remember-me login - leaves the requests that are already on their way with the previous session cookie. PHP starts a session for that id again and sends it back in a Set-Cookie header, so the id the browser had just stored is overwritten and everything the regenerating request did is lost. For a logged-in user who unlocks a password-protected public share this shows up as a 401 on PROPFIND /public.php/dav/files//. Mark the old session as superseded instead of deleting it, and move the requests that still arrive on it to an empty session. Within a grace period of 60 seconds they get no session cookie at all, so they can no longer replace the regenerated id the browser holds. reopen() does the same and only sends a cookie when the id it was given is gone for good. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: David Dreschner --- lib/OC.php | 7 +- lib/private/Session/Internal.php | 133 ++++++-- tests/lib/Session/InternalTest.php | 511 +++++++++++++++++++++++++++++ 3 files changed, 617 insertions(+), 34 deletions(-) create mode 100644 tests/lib/Session/InternalTest.php diff --git a/lib/OC.php b/lib/OC.php index 725e945219d47..40e59003401ae 100644 --- a/lib/OC.php +++ b/lib/OC.php @@ -463,15 +463,10 @@ public static function initSession(): void { $sessionName = OC_Util::getInstanceId(); try { - $logger = null; - if (Server::get(\OC\SystemConfig::class)->getValue('installed', false)) { - $logger = logger('core'); - } - // set the session name to the instance id - which is unique $session = new \OC\Session\Internal( $sessionName, - $logger, + logger('core'), ); $cryptoWrapper = Server::get(\OC\Session\CryptoWrapper::class); diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index 4d31d684adede..ffd9ee00b44af 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -18,7 +18,6 @@ use OCP\Session\Exceptions\SessionNotAvailableException; use Psr\Log\LoggerInterface; use function call_user_func_array; -use function OCP\Log\logger; /** * Class Internal @@ -31,13 +30,28 @@ class Internal extends Session { use TLogSlowOperation; + /** + * Marks a session whose id was replaced by regenerateId() + */ + public const SUPERSEDED_AT = 'superseded_at'; + + /** + * Seconds after a regeneration in which requests on the old id get no cookie, as the browser may hold the new id + */ + public const SUPERSEDED_GRACE_PERIOD = 60; + + /** + * False while the browser may hold a newer id than this request, see leaveSupersededSession() + */ + private bool $cookieAllowed = true; + /** * @param string $name * @throws \Exception */ public function __construct( string $name, - private ?LoggerInterface $logger, + private readonly LoggerInterface $logger, ) { set_error_handler([$this, 'trapError']); $this->invoke('session_name', [$name]); @@ -51,6 +65,7 @@ public function __construct( if (!isset($_SESSION)) { throw new \Exception('Failed to start session'); } + $this->leaveSupersededSession(atRequestStart: true); } /** @@ -102,8 +117,6 @@ public function clear() { $this->reopen(); $this->invoke('session_unset'); $this->regenerateId(); - $this->invoke('session_write_close'); - $this->startSession(true); $_SESSION = []; } @@ -116,32 +129,21 @@ public function close() { /** * Wrapper around session_regenerate_id * - * @param bool $deleteOldSession Whether to delete the old associated session file or not. + * @param bool $deleteOldSession Whether to delete the old id, or replace its data with the superseded marker if the client sent it * @param bool $updateToken Whether to update the associated auth token * @return void */ #[\Override] public function regenerateId(bool $deleteOldSession = true, bool $updateToken = false) { - $this->reopen(); - $oldId = null; - - if ($updateToken) { - // Get the old id to update the token - try { - $oldId = $this->getId(); - } catch (SessionNotAvailableException $e) { - // We can't update a token if there is no previous id - $updateToken = false; - } + $oldId = (string)$this->invoke('session_id', [], true); + if (!$this->switchToNewId($oldId, $deleteOldSession)) { + $this->logger->warning('Could not regenerate the session id'); + return; } + // The browser gets the new id from this request, so it needs a replacement of it as well + $this->cookieAllowed = true; - try { - @session_regenerate_id($deleteOldSession); - } catch (\Error $e) { - $this->trapError($e->getCode(), $e->getMessage()); - } - - if ($updateToken) { + if ($updateToken && $oldId !== '') { // Get the new id to update the token $newId = $this->getId(); @@ -179,8 +181,14 @@ public function getId(): string { #[\Override] public function reopen(): bool { if ($this->sessionClosed) { - $this->startSession(false, false); + $id = $this->invoke('session_id', [], true); + $this->startSession(readAndClose: false, sendCookie: false); + if ($this->cookieAllowed && $this->invoke('session_id', [], true) !== $id) { + // Strict mode replaced an id that is no longer stored, the browser needs the new one + $this->restartWithCookie(); + } $this->sessionClosed = false; + $this->leaveSupersededSession(atRequestStart: false); return true; } @@ -208,7 +216,7 @@ public function trapError(int $errorNumber, string $errorString) { private function invoke(string $functionName, array $parameters = [], bool $silence = false) { try { return $this->monitorAndLog( - $this->logger ?? logger('core'), + $this->logger, $functionName, function () use ($silence, $functionName, $parameters) { if ($silence) { @@ -223,11 +231,80 @@ function () use ($silence, $functionName, $parameters) { } } - private function startSession(bool $silence = false, bool $readAndClose = true) { - $sessionParams = ['cookie_samesite' => 'Lax']; + /** + * @param bool $sendCookie Whether PHP may send the session cookie, which can replace a newer id the browser holds + */ + private function startSession(bool $readAndClose = true, bool $sendCookie = true) { + $sessionParams = [ + 'cookie_samesite' => 'Lax', + 'use_cookies' => $sendCookie, + ]; if (\OC::hasSessionRelaxedExpiry()) { $sessionParams['read_and_close'] = $readAndClose; } - $this->invoke('session_start', [$sessionParams], $silence); + $this->invoke('session_start', [$sessionParams]); + } + + private function restartWithCookie(): void { + $this->invoke('session_write_close', [], true); + $this->startSession(readAndClose: false); + } + + private function isActive(): bool { + return session_status() === PHP_SESSION_ACTIVE; + } + + /** + * @return bool Whether the session moved to a new id. If not, it stays on $oldId with its data. + */ + private function switchToNewId(string $oldId, bool $deleteOldSession): bool { + // session_regenerate_id() needs an active session that may send cookies, reopen() starts it without. + // Also within the grace period: a login or logout on a replaced id has to reach the browser. + if (!$this->isActive() || !ini_get('session.use_cookies')) { + $this->restartWithCookie(); + } + + $data = $_SESSION; + // The restart may have read the marker of a concurrent regeneration + unset($data[self::SUPERSEDED_AT]); + // Only an id the client sent can come back in another request + $markOldId = $deleteOldSession && ($_COOKIE[session_name()] ?? null) === $oldId; + if ($markOldId) { + // The old id keeps the marker instead of the data, so session_regenerate_id() must not delete it + $_SESSION = [self::SUPERSEDED_AT => time()]; + } + + $regenerated = $this->invoke('session_regenerate_id', [$deleteOldSession && !$markOldId], true) === true; + if (!$regenerated && $oldId !== '' && !$this->isActive()) { + // Continue on the old id + $this->invoke('session_id', [$oldId], true); + $this->startSession(readAndClose: false, sendCookie: false); + } + $_SESSION = $data; + $this->sessionClosed = !$this->isActive(); + return $regenerated; + } + + /** + * Move a request on an id replaced by regenerateId() to a fresh session, so the old id can't log in again. + * It gets a cookie only if the request starts after the grace period, see SUPERSEDED_GRACE_PERIOD. + */ + private function leaveSupersededSession(bool $atRequestStart): void { + if (!isset($_SESSION[self::SUPERSEDED_AT])) { + return; + } + + $supersededAt = (int)$_SESSION[self::SUPERSEDED_AT]; + $this->logger->debug('Request uses a session id that was replaced at {supersededAt}', [ + 'supersededAt' => $supersededAt, + ]); + $this->invoke('session_abort', [], true); + $_SESSION = []; + $this->cookieAllowed = $atRequestStart && time() - $supersededAt >= self::SUPERSEDED_GRACE_PERIOD; + $freshId = $this->invoke('session_create_id', [], true); + if (is_string($freshId) && $this->invoke('session_id', [$freshId], true) !== false) { + $this->startSession(readAndClose: $atRequestStart, sendCookie: $this->cookieAllowed); + } + $this->sessionClosed = !$this->isActive(); } } diff --git a/tests/lib/Session/InternalTest.php b/tests/lib/Session/InternalTest.php new file mode 100644 index 0000000000000..8bf2a754f4c00 --- /dev/null +++ b/tests/lib/Session/InternalTest.php @@ -0,0 +1,511 @@ +logger = $this->createMock(LoggerInterface::class); + ini_set('session.save_handler', 'files'); + ini_set('session.save_path', Server::get(ITempManager::class)->getTemporaryFolder()); + ini_set('session.serialize_handler', 'php_serialize'); + ini_set('session.use_strict_mode', '1'); + } + + #[\Override] + protected function tearDown(): void { + Server::get(ITempManager::class)->clean(); + + parent::tearDown(); + } + + /** + * Start the session like a request that sends $id as session cookie + */ + private function startRequest(?string $id = null): Internal { + if ($id !== null) { + session_id($id); + $_COOKIE['oc_session_test'] = $id; + } + return new Internal('oc_session_test', $this->logger); + } + + /** + * Start a request with the id that the browser got from a previous response + */ + private function startRequestWithKnownId(): Internal { + $session = $this->startRequest(); + $session->close(); + return $this->startRequest($session->getId()); + } + + private function getStoragePath(string $id): string { + return rtrim(ini_get('session.save_path'), '/') . '/sess_' . $id; + } + + /** + * @return array The data the session handler stored for $id + */ + private function readStoredSession(string $id): array { + $path = $this->getStoragePath($id); + if (!is_file($path)) { + return []; + } + return unserialize(file_get_contents($path), ['allowed_classes' => false]); + } + + /** + * @return string The old id of a session holding 'foo' => 'bar' that regenerateId() replaced + */ + private function createSupersededId(): string { + $session = $this->startRequestWithKnownId(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + $session->regenerateId(); + $session->close(); + return $oldId; + } + + private function markSupersededLongAgo(string $id): void { + file_put_contents($this->getStoragePath($id), serialize([ + Internal::SUPERSEDED_AT => time() - Internal::SUPERSEDED_GRACE_PERIOD, + ])); + } + + public static function strictModeProvider(): array { + return [ + 'strict mode' => ['strictMode' => '1'], + 'no strict mode' => ['strictMode' => '0'], + ]; + } + + public static function staleRegenerationProvider(): array { + return [ + 'login' => ['method' => 'regenerateId'], + 'logout' => ['method' => 'clear'], + ]; + } + + public function testReopenDoesNotSendCookieAgain(): void { + $session = $this->startRequest(); + $session->close(); + + $session->set('foo', 'bar'); + + $this->assertFalse((bool)ini_get('session.use_cookies')); + } + + public function testRegenerateIdSendsCookieAfterReopen(): void { + $session = $this->startRequest(); + $session->close(); + $session->reopen(); + + $session->regenerateId(); + + $this->assertTrue((bool)ini_get('session.use_cookies')); + } + + public function testRegenerateIdSendsCookieOnClosedSession(): void { + $session = $this->startRequest(); + $session->close(); + $session->set('foo', 'bar'); + + $session->regenerateId(); + + $this->assertTrue((bool)ini_get('session.use_cookies')); + } + + public function testRegenerateIdMovesDataToNewId(): void { + $session = $this->startRequest(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + + $session->regenerateId(); + $newId = $session->getId(); + $session->close(); + + $this->assertNotSame($oldId, $newId); + $this->assertSame('bar', $this->startRequest($newId)->get('foo')); + } + + public function testRegenerateIdKeepsOnlyMarkerOnOldId(): void { + $session = $this->startRequestWithKnownId(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + + $session->regenerateId(); + $session->close(); + + $this->assertSame([Internal::SUPERSEDED_AT], array_keys($this->readStoredSession($oldId))); + } + + public function testRegenerateIdDeletesOldIdTheClientDidNotSend(): void { + $session = $this->startRequest(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + + $session->regenerateId(); + $newId = $session->getId(); + $session->close(); + + $this->assertFileDoesNotExist($this->getStoragePath($oldId)); + $this->assertSame('bar', $this->readStoredSession($newId)['foo'] ?? null); + } + + #[DataProvider('strictModeProvider')] + public function testRequestOnSupersededIdGetsFreshSessionWithoutCookie(string $strictMode): void { + ini_set('session.use_strict_mode', $strictMode); + $oldId = $this->createSupersededId(); + + $this->logger->expects($this->once()) + ->method('debug') + ->with('Request uses a session id that was replaced at {supersededAt}', $this->arrayHasKey('supersededAt')); + $staleSession = $this->startRequest($oldId); + + // The session id is also the auth token, so the old id must not be used again + $this->assertNotSame($oldId, $staleSession->getId()); + $this->assertFalse($staleSession->exists('foo')); + $this->assertFalse($staleSession->exists(Internal::SUPERSEDED_AT)); + $this->assertFalse((bool)ini_get('session.use_cookies')); + } + + public function testRequestStartingAfterGracePeriodGetsFreshSessionWithCookie(): void { + $oldId = $this->createSupersededId(); + $this->markSupersededLongAgo($oldId); + + $staleSession = $this->startRequest($oldId); + + $this->assertNotSame($oldId, $staleSession->getId()); + $this->assertFalse($staleSession->exists('foo')); + $this->assertTrue((bool)ini_get('session.use_cookies')); + } + + public function testRequestAfterGracePeriodWithReplacedIdInCookieGetsFreshSession(): void { + $oldId = bin2hex(random_bytes(16)); + $this->markSupersededLongAgo($oldId); + $_COOKIE['oc_session_test'] = $oldId; + + $this->logger->expects($this->once()) + ->method('debug') + ->with('Request uses a session id that was replaced at {supersededAt}', $this->arrayHasKey('supersededAt')); + $staleSession = $this->startRequest(); + $staleSession->set('foo', 'late'); + + $this->assertNotSame($oldId, $staleSession->getId()); + $this->assertTrue((bool)ini_get('session.use_cookies')); + $this->assertSame([Internal::SUPERSEDED_AT], array_keys($this->readStoredSession($oldId))); + } + + #[DataProvider('staleRegenerationProvider')] + public function testStaleRequestSendsCookieWhenItRegenerates(string $method): void { + $staleSession = $this->startRequest($this->createSupersededId()); + $freshId = $staleSession->getId(); + $staleSession->set('foo', 'stale'); + + $staleSession->$method(); + + $this->assertNotSame($freshId, $staleSession->getId()); + $this->assertTrue((bool)ini_get('session.use_cookies')); + } + + public function testReopenSendsCookieWhenStrictModeReplacesId(): void { + $session = $this->startRequest(); + $oldId = $session->getId(); + $session->close(); + // The session expired while the request was running + unlink($this->getStoragePath($oldId)); + + $session->set('foo', 'bar'); + + $newId = $session->getId(); + $this->assertNotSame($oldId, $newId); + $this->assertTrue((bool)ini_get('session.use_cookies')); + $this->assertSame('bar', $this->readStoredSession($newId)['foo'] ?? null); + } + + public function testReopenWithinGracePeriodDoesNotSendCookieWhenStrictModeReplacesId(): void { + $staleSession = $this->startRequest($this->createSupersededId()); + $freshId = $staleSession->getId(); + $staleSession->close(); + // Stores like Redis keep nothing for a session that was only read + unlink($this->getStoragePath($freshId)); + + $staleSession->set('foo', 'stale'); + + $this->assertNotSame($freshId, $staleSession->getId()); + $this->assertFalse((bool)ini_get('session.use_cookies')); + $this->assertSame('stale', $this->readStoredSession($staleSession->getId())['foo'] ?? null); + } + + #[DataProvider('staleRegenerationProvider')] + public function testStaleRequestSendsCookieWhenStrictModeReplacesItsRegeneratedId(string $method): void { + $staleSession = $this->startRequest($this->createSupersededId()); + $staleSession->$method(); + $newId = $staleSession->getId(); + $staleSession->close(); + // The session expired while the request was running + unlink($this->getStoragePath($newId)); + + $staleSession->set('foo', 'bar'); + + $this->assertNotSame($newId, $staleSession->getId()); + $this->assertTrue((bool)ini_get('session.use_cookies')); + $this->assertSame('bar', $this->readStoredSession($staleSession->getId())['foo'] ?? null); + } + + public function testWritesOnSupersededIdKeepOldIdEmpty(): void { + $oldId = $this->createSupersededId(); + + $staleSession = $this->startRequest($oldId); + $staleSession->set('foo', 'stale'); + $staleSession->close(); + + $this->assertSame([Internal::SUPERSEDED_AT], array_keys($this->readStoredSession($oldId))); + } + + public function testReopenAfterConcurrentRegenerationMovesToFreshSession(): void { + $session = $this->startRequest(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + $session->close(); + + $concurrentRequest = $this->startRequest($oldId); + $concurrentRequest->regenerateId(); + $concurrentRequest->close(); + $this->markSupersededLongAgo($oldId); + + // This request still runs on the old id and started before the regeneration + session_id($oldId); + $session->set('late', 'write'); + + $this->assertNotSame($oldId, $session->getId()); + $this->assertFalse((bool)ini_get('session.use_cookies')); + $this->assertSame([Internal::SUPERSEDED_AT], array_keys($this->readStoredSession($oldId))); + } + + public function testRegenerateIdAfterConcurrentRegenerationDoesNotCopyMarker(): void { + $session = $this->startRequest(); + $oldId = $session->getId(); + $session->close(); + + $concurrentRequest = $this->startRequest($oldId); + $concurrentRequest->regenerateId(); + $concurrentRequest->close(); + + // This request still runs on the old id + session_id($oldId); + $session->regenerateId(); + $newId = $session->getId(); + $session->close(); + + $this->assertArrayNotHasKey(Internal::SUPERSEDED_AT, $this->readStoredSession($newId)); + } + + public function testFailedRegenerationSkipsTokenRenewal(): void { + $handler = new class extends \SessionHandler { + public bool $failNewIds = false; + + #[\Override] + public function create_sid(): string { + return $this->failNewIds ? '' : parent::create_sid(); + } + }; + session_set_save_handler($handler, false); + $session = $this->startRequest(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + + $tokenProvider = $this->createMock(IProvider::class); + $this->overwriteService(IProvider::class, $tokenProvider); + $tokenProvider->expects($this->never()) + ->method('renewSessionToken'); + $this->logger->expects($this->once()) + ->method('warning') + ->with('Could not regenerate the session id'); + + $handler->failNewIds = true; + $session->regenerateId(true, true); + $handler->failNewIds = false; + $session->set('after', 'failed regeneration'); + $session->close(); + + $this->assertSame($oldId, $session->getId()); + $this->assertSame(['foo' => 'bar', 'after' => 'failed regeneration'], $this->readStoredSession($oldId)); + } + + public function testFailedSwitchToFreshSessionKeepsLaterWrites(): void { + $handler = new class extends \SessionHandler { + public int $opens = 0; + public int $failingOpen = 0; + + #[\Override] + public function open(string $path, string $name): bool { + return ++$this->opens !== $this->failingOpen && parent::open($path, $name); + } + }; + session_set_save_handler($handler, false); + $oldId = $this->createSupersededId(); + + // Reading the old id works, opening the fresh session fails once + $handler->failingOpen = $handler->opens + 2; + // session_start() warns about the failing storage + set_error_handler(static fn (): bool => true, E_WARNING); + try { + $staleSession = $this->startRequest($oldId); + } finally { + restore_error_handler(); + } + $staleSession->set('foo', 'stale'); + $newId = $staleSession->getId(); + $staleSession->close(); + + $this->assertNotSame($oldId, $newId); + $this->assertSame('stale', $this->readStoredSession($newId)['foo'] ?? null); + $this->assertSame([Internal::SUPERSEDED_AT], array_keys($this->readStoredSession($oldId))); + } + + public function testRegenerateIdOnSupersededIdDoesNotCopyMarker(): void { + $oldId = $this->createSupersededId(); + + $this->logger->expects($this->once()) + ->method('debug'); + $staleSession = $this->startRequest($oldId); + $staleSession->set('foo', 'stale'); + $staleSession->regenerateId(); + $newId = $staleSession->getId(); + $staleSession->close(); + + $newSession = $this->startRequest($newId); + $this->assertSame('stale', $newSession->get('foo')); + $this->assertFalse($newSession->exists(Internal::SUPERSEDED_AT)); + } + + public function testRegenerateIdRenewsSessionTokenFromOldToNewId(): void { + $session = $this->startRequest(); + $session->close(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + + $tokenProvider = $this->createMock(IProvider::class); + $userSession = $this->createMock(UserSession::class); + $this->overwriteService(IProvider::class, $tokenProvider); + $this->overwriteService(UserSession::class, $userSession); + $renewed = null; + $tokenProvider->expects($this->once()) + ->method('renewSessionToken') + ->willReturnCallback(function (string $oldSessionId, string $sessionId) use (&$renewed): IToken { + $renewed = ['oldSessionId' => $oldSessionId, 'sessionId' => $sessionId]; + return $this->createMock(IToken::class); + }); + $userSession->expects($this->once()) + ->method('renewMagicSessionId') + ->with($oldId); + + $session->regenerateId(true, true); + + $this->assertNotSame($oldId, $session->getId()); + $this->assertSame(['oldSessionId' => $oldId, 'sessionId' => $session->getId()], $renewed); + } + + public function testFailedRestartKeepsSessionWritable(): void { + $session = $this->startRequest(); + $session->set('foo', 'bar'); + $session->close(); + $savePath = ini_get('session.save_path'); + + ini_set('session.save_path', $savePath . '/missing'); + // session_start() warns about the missing save path + set_error_handler(static fn (): bool => true, E_WARNING); + try { + $session->regenerateId(); + } finally { + restore_error_handler(); + } + ini_set('session.save_path', $savePath); + $session->set('after', 'failed restart'); + $id = $session->getId(); + $session->close(); + + $this->assertSame('failed restart', $this->startRequest($id)->get('after')); + } + + public function testClearKeepsLaterWritesWithRelaxedExpiry(): void { + $config = $this->createMock(IConfig::class); + $config->method('getSystemValueBool') + ->with('session_relaxed_expiry', false) + ->willReturn(true); + $this->overwriteService(IConfig::class, $config); + $session = $this->startRequest(); + $session->close(); + $session->set('foo', 'bar'); + + $session->clear(); + $session->set('after', 'clear'); + $newId = $session->getId(); + $session->close(); + + $newSession = $this->startRequest($newId); + $this->assertFalse($newSession->exists('foo')); + $this->assertSame('clear', $newSession->get('after')); + } + + public function testRegenerateIdWithoutDeletingKeepsOldData(): void { + $session = $this->startRequest(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + $session->regenerateId(false); + $session->close(); + + $this->logger->expects($this->never()) + ->method('debug'); + $oldSession = $this->startRequest($oldId); + + $this->assertSame('bar', $oldSession->get('foo')); + $this->assertFalse($oldSession->exists(Internal::SUPERSEDED_AT)); + } + + public function testClearSupersedesOldId(): void { + $session = $this->startRequestWithKnownId(); + $session->set('foo', 'bar'); + $oldId = $session->getId(); + + $session->clear(); + $newId = $session->getId(); + $session->close(); + + $this->assertNotSame($oldId, $newId); + $this->assertArrayNotHasKey('foo', $this->readStoredSession($newId)); + $this->assertSame([Internal::SUPERSEDED_AT], array_keys($this->readStoredSession($oldId))); + } +}