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))); + } +}