Tombstone the old session on SessionBackend::resetId()

SessionBackend::resetId() is prone to race conditions with
cookie-based session providers, where MediaWiki receives
a request with the old session and forces the client to log
out. To handle that, add a tombstone mechanism to
SessionBackend, so instead of deleting the old session from
the store on ID reset, it is marked as invalid. Tombstoned
sessions are handled as nonexistent ones, except unpersist()
is not called.

Unlike Iffd69c7f246adff40b07668328a07329440dbd6f this doesn't
prevent overwriting the session if the MediaWiki endpoint calls
persist() or unpersist(), but it is vastly simpler, and very
few endpoints persist the session.

The behavior of SessionManager::loadSessionInfoFromStore()
with a tombstoned session and SessionInfo::forceUse()===true
does not make much sense, but that's a nonsensical scenario
in the first place (it only happens when the session provider
returns true from persistsSessionId() but sets the forceUse
flag which is meant for providers which can't change the
session ID) and we are only really concerned here about
cookie-based sessions anyway.

Bug: T299193
Change-Id: I3a76b67aa51159ebf0195db15cf7c34e00a64a2e

includes/session/SessionBackend.php

@@ -32,6 +32,7 @@ use Psr\Log\LoggerInterface;
 use User;
 use WebRequest;
 use Wikimedia\AtEase\AtEase;
+use Wikimedia\LightweightObjectStore\ExpirationAwareness;
 
 /**
  * This is the actual workhorse for Session.
@@ -52,6 +53,10 @@ use Wikimedia\AtEase\AtEase;
  * @since 1.27
  */
 final class SessionBackend {
+
+	/** Expiration time of tombstone records in seconds */
+	private const TOMBSTONE_EXPIRY = ExpirationAwareness::TTL_MINUTE;
+
 	/** @var SessionId */
 	private $id;
 
@@ -122,7 +127,7 @@ final class SessionBackend {
 	/** @var array|null provider-specified metadata */
 	private $providerMetadata = null;
 
-	/** @var int */
+	/** @var int UNIX timestamp at which the session will expire */
 	private $expires = 0;
 
 	/** @var int */
@@ -296,8 +301,7 @@ final class SessionBackend {
 
 			$this->autosave();
 
-			// Delete the data for the old session ID now
-			$this->store->delete( $this->store->makeKey( 'MWSession', $oldId ) );
+			$this->tombstone( $oldId );
 		}
 
 		return $this->id;
@@ -630,6 +634,39 @@ final class SessionBackend {
 		$this->autosave();
 	}
 
+	/**
+	 * Put a tombstone in the session store for the given ID, to mark it as not used anymore.
+	 * For most purposes this is the same as deleting the object with that ID.
+	 *
+	 * @param string $id
+	 * @return bool Success.
+	 */
+	private function tombstone( $id ) {
+		// Match the data format from save(). The 'tombstoned' flag makes SessionManager
+		// abort any loading attempts, but use data for an anonymous session just in case.
+		return $this->store->set(
+			$this->store->makeKey( 'MWSession', $id ),
+			[
+				'data' => [],
+				'metadata' => [
+					'provider' => (string)$this->provider,
+					'providerMetadata' => null,
+					'userId' => 0,
+					'userName' => null,
+					'userToken' => null,
+					'remember' => false,
+					'forceHTTPS' => $this->forceHTTPS,
+					'expires' => time() + self::TOMBSTONE_EXPIRY,
+					'loggedOut' => 0,
+					'persisted' => false,
+					'tombstoned' => true,
+				],
+			],
+			self::TOMBSTONE_EXPIRY,
+			CachedBagOStuff::WRITE_SYNC
+		);
+	}
+
 	/**
 	 * Delay automatic saving while multiple updates are being made
 	 *
@@ -769,6 +806,7 @@ final class SessionBackend {
 			'expires' => time() + $this->lifetime,
 			'loggedOut' => $this->loggedOut,
 			'persisted' => $this->persist,
+			'tombstoned' => false,
 		];
 
 		$this->hookRunner->onSessionMetadata( $this, $metadata, $this->requests );

includes/session/SessionManager.php

@@ -547,8 +547,9 @@ class SessionManager implements SessionManagerInterface {
 		usort( $infos, [ SessionInfo::class, 'compare' ] );
 		$retInfos = [];
 		while ( $infos ) {
+			$tombstoned = false;
 			$info = array_pop( $infos );
-			if ( $this->loadSessionInfoFromStore( $info, $request ) ) {
+			if ( $this->loadSessionInfoFromStore( $info, $request, $tombstoned ) ) {
 				$retInfos[] = $info;
 				while ( $infos ) {
 					/** @var SessionInfo $info */
@@ -567,8 +568,10 @@ class SessionManager implements SessionManagerInterface {
 						$info->getProvider()->unpersistSession( $request );
 					}
 				}
-			} else {
-				// Session load failed, so unpersist it from this request
+			} elseif ( !$tombstoned ) {
+				// Session load failed, so unpersist it from this request; but don't unpersist
+				// tombstoned sessions to avoid unpersisting the user's newer, valid session
+				// via a race condition (T299193).
 				$this->logUnpersist( $info, $request );
 				$info->getProvider()->unpersistSession( $request );
 			}
@@ -589,9 +592,16 @@ class SessionManager implements SessionManagerInterface {
 	 *
 	 * @param SessionInfo &$info Will likely be replaced with an updated SessionInfo instance
 	 * @param WebRequest $request
+	 * @param bool &$tombstoned Output parameter telling whether the session was tombstoned.
+	 *   Tombstoned sessions should be treated as nonexistent, but care should be taken not to
+	 *   overwrite another valid session the user might have.
 	 * @return bool Whether the session info matches the stored data (if any)
 	 */
-	private function loadSessionInfoFromStore( SessionInfo &$info, WebRequest $request ) {
+	private function loadSessionInfoFromStore(
+		SessionInfo &$info,
+		WebRequest $request,
+		bool &$tombstoned = false
+	) {
 		$key = $this->store->makeKey( 'MWSession', $info->getId() );
 		$blob = $this->store->get( $key );
 
@@ -700,7 +710,10 @@ class SessionManager implements SessionManagerInterface {
 
 			// Next, load the user from metadata, or validate it against the metadata.
 			$userInfo = $info->getUserInfo();
-			if ( !$userInfo ) {
+			if ( $metadata['tombstoned'] ?? false ) {
+				$tombstoned = true;
+				return $failHandler();
+			} elseif ( !$userInfo ) {
 				// For loading, id is preferred to name.
 				try {
 					if ( $metadata['userId'] ) {

tests/phpunit/includes/auth/AuthManagerTest.php

@@ -13,6 +13,7 @@ use MediaWiki\HookContainer\HookContainer;
 use MediaWiki\HookContainer\StaticHookRegistry;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Session\SessionInfo;
+use MediaWiki\Session\SessionProvider;
 use MediaWiki\Session\UserInfo;
 use MediaWiki\User\BotPasswordStore;
 use MediaWiki\User\UserFactory;
@@ -392,17 +393,19 @@ class AuthManagerTest extends \MediaWikiIntegrationTestCase {
 	public function testSecuritySensitiveOperationStatus( $mutableSession ) {
 		$this->logger = new \Psr\Log\NullLogger();
 		$user = \User::newFromName( 'UTSysop' );
+		$sessionId = str_pad( '', 32, 'a' );
 		$provideUser = null;
 		$reauth = $mutableSession ? AuthManager::SEC_REAUTH : AuthManager::SEC_FAIL;
 
+		/** @var SessionProvider $provider */
 		list( $provider, $reset ) = $this->getMockSessionProvider(
 			$mutableSession, [ 'provideSessionInfo' ]
 		);
 		$provider->method( 'provideSessionInfo' )
-			->will( $this->returnCallback( static function () use ( $provider, &$provideUser ) {
+			->will( $this->returnCallback( static function () use ( $provider, &$sessionId, &$provideUser ) {
 				return new SessionInfo( SessionInfo::MIN_PRIORITY, [
 					'provider' => $provider,
-					'id' => \DummySessionProvider::ID,
+					'id' => $sessionId,
 					'persisted' => true,
 					'userInfo' => UserInfo::newFromUser( $provideUser, true )
 				] );
@@ -420,6 +423,8 @@ class AuthManagerTest extends \MediaWikiIntegrationTestCase {
 		$session->set( 'AuthManager:lastAuthTimestamp', time() - 5 );
 		$this->assertSame( $reauth, $this->manager->securitySensitiveOperationStatus( 'foo' ) );
 
+		// reset mock ID to avoid tombstone
+		$sessionId = str_pad( '', 32, 'b' );
 		$provideUser = $user;
 		$session = $provider->getManager()->getSessionForRequest( $this->request );
 		$this->assertSame( $user->getId(), $session->getUser()->getId() );

tests/phpunit/includes/session/SessionBackendTest.php

@@ -307,7 +307,15 @@ class SessionBackendTest extends MediaWikiIntegrationTestCase {
 		$this->assertNotEquals( self::SESSIONID, $backend->getId() );
 		$this->assertSame( $backend->getId(), $sessionId->getId() );
 		$this->assertIsArray( $this->store->getSession( $backend->getId() ) );
-		$this->assertFalse( $this->store->getSession( self::SESSIONID ) );
+		$oldSession = $this->store->getSession( self::SESSIONID );
+		$oldSessionFromBackend = $this->store->getSessionFromBackend( self::SESSIONID );
+		$this->assertIsArray( $oldSession );
+		foreach ( [ $oldSession, $oldSessionFromBackend ] as $sessionStore ) {
+			$this->assertTrue( $sessionStore['metadata']['tombstoned'] );
+			$this->assertSame( 0, $sessionStore['metadata']['userId'] );
+			$this->assertSame( null, $sessionStore['metadata']['userName'] );
+			$this->assertSame( null, $sessionStore['metadata']['userToken'] );
+		}
 		$this->assertSame( $id, session_id() );
 		$this->assertArrayNotHasKey( self::SESSIONID, $manager->allSessionBackends );
 		$this->assertArrayHasKey( $backend->getId(), $manager->allSessionBackends );