rdbms: include client ID hash in ChronologyProtector cookies

Previously, if an internal service forwarded the cookies for a
user (e.g. for permissions) but not the User-Agent header or not
the IP address (e.g. XFF), ChronologyProtector could timeout
waiting for a matching writeIndex to appear for the wrong key.

The cookie now tethers the client to the key that holds the
DB positions from their last state-changing request.

Bug: T194403
Bug: T190082
Change-Id: I84f2cbea82532d911cdfed14644008894498813a

diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php
index 1642377..7978727 100644 (file)
--- a/includes/MediaWiki.php
+++ b/includes/MediaWiki.php
@@ -631,7 +631,8 @@ class MediaWiki {
 
                // Record ChronologyProtector positions for DBs affected in this request at this point
                $cpIndex = null;
-               $lbFactory->shutdown( $flags, $postCommitWork, $cpIndex );
+               $cpClientId = null;
+               $lbFactory->shutdown( $flags, $postCommitWork, $cpIndex, $cpClientId );
                wfDebug( __METHOD__ . ': LBFactory shutdown completed' );
 
                if ( $cpIndex > 0 ) {
@@ -639,7 +640,7 @@ class MediaWiki {
                                $now = time();
                                $expires = $now + ChronologyProtector::POSITION_COOKIE_TTL;
                                $options = [ 'prefix' => '' ];
-                               $value = LBFactory::makeCookieValueFromCPIndex( $cpIndex, $now ); // T190082
+                               $value = LBFactory::makeCookieValueFromCPIndex( $cpIndex, $now, $cpClientId );
                                $request->response()->setCookie( 'cpPosIndex', $value, $expires, $options );
                        }
 

diff --git a/includes/Setup.php b/includes/Setup.php
index 3cc52f8..512ecfc 100644 (file)
--- a/includes/Setup.php
+++ b/includes/Setup.php
@@ -730,20 +730,20 @@ if ( !$wgDBerrorLogTZ ) {
 // Initialize the request object in $wgRequest
 $wgRequest = RequestContext::getMain()->getRequest(); // BackCompat
 // Set user IP/agent information for agent session consistency purposes
+$cpPosInfo = LBFactory::getCPInfoFromCookieValue(
+       // The cookie has no prefix and is set by MediaWiki::preOutputCommit()
+       $wgRequest->getCookie( 'cpPosIndex', '' ),
+       // Mitigate broken client-side cookie expiration handling (T190082)
+       time() - ChronologyProtector::POSITION_COOKIE_TTL
+);
 MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->setRequestInfo( [
        'IPAddress' => $wgRequest->getIP(),
        'UserAgent' => $wgRequest->getHeader( 'User-Agent' ),
        'ChronologyProtection' => $wgRequest->getHeader( 'ChronologyProtection' ),
-       'ChronologyPositionIndex' => $wgRequest->getInt(
-               'cpPosIndex',
-               LBFactory::getCPIndexFromCookieValue(
-                       // The cookie has no prefix and is set by MediaWiki::preOutputCommit()
-                       $wgRequest->getCookie( 'cpPosIndex', '' ),
-                       // Mitigate broken client-side cookie expiration handling (T190082)
-                       time() - ChronologyProtector::POSITION_COOKIE_TTL
-               )
-       )
+       'ChronologyPositionIndex' => $wgRequest->getInt( 'cpPosIndex', $cpPosInfo['index'] ),
+       'ChronologyClientId' => $cpPosInfo['clientId']
 ] );
+unset( $cpPosInfo );
 // Make sure that object caching does not undermine the ChronologyProtector improvements
 if ( $wgRequest->getCookie( 'UseDC', '' ) === 'master' ) {
        // The user is pinned to the primary DC, meaning that they made recent changes which should

diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php
index 099f172..f5cef9e 100644 (file)
--- a/includes/libs/rdbms/ChronologyProtector.php
+++ b/includes/libs/rdbms/ChronologyProtector.php
@@ -70,13 +70,15 @@ class ChronologyProtector implements LoggerAwareInterface {
 
        /**
         * @param BagOStuff $store
-        * @param array[] $client Map of (ip: <IP>, agent: <user-agent>)
+        * @param array[] $client Map of (ip: <IP>, agent: <user-agent> [, clientId: <hash>] )
         * @param int|null $posIndex Write counter index [optional]
         * @since 1.27
         */
        public function __construct( BagOStuff $store, array $client, $posIndex = null ) {
                $this->store = $store;
-               $this->clientId = md5( $client['ip'] . "\n" . $client['agent'] );
+               $this->clientId = isset( $client['clientId'] )
+                       ? $client['clientId']
+                       : md5( $client['ip'] . "\n" . $client['agent'] );
                $this->key = $store->makeGlobalKey( __CLASS__, $this->clientId, 'v2' );
                $this->waitForPosIndex = $posIndex;
                $this->logger = new NullLogger();
@@ -86,6 +88,14 @@ class ChronologyProtector implements LoggerAwareInterface {
                $this->logger = $logger;
        }
 
+       /**
+        * @return string Client ID hash
+        * @since 1.32
+        */
+       public function getClientId() {
+               return $this->clientId;
+       }
+
        /**
         * @param bool $enabled Whether to no-op all method calls
         * @since 1.27

diff --git a/includes/libs/rdbms/lbfactory/ILBFactory.php b/includes/libs/rdbms/lbfactory/ILBFactory.php
index 16d0e31..85ab115 100644 (file)
--- a/includes/libs/rdbms/lbfactory/ILBFactory.php
+++ b/includes/libs/rdbms/lbfactory/ILBFactory.php
@@ -142,9 +142,13 @@ interface ILBFactory {
         * @param int $mode One of the class SHUTDOWN_* constants
         * @param callable|null $workCallback Work to mask ChronologyProtector writes
         * @param int|null &$cpIndex Position key write counter for ChronologyProtector
+        * @param string|null &$cpClientId Client ID hash for ChronologyProtector
         */
        public function shutdown(
-               $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null, &$cpIndex = null
+               $mode = self::SHUTDOWN_CHRONPROT_SYNC,
+               callable $workCallback = null,
+               &$cpIndex = null,
+               &$cpClientId = null
        );
 
        /**

diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php
index 2ee3419..52c2df7 100644 (file)
--- a/includes/libs/rdbms/lbfactory/LBFactory.php
+++ b/includes/libs/rdbms/lbfactory/LBFactory.php
@@ -130,9 +130,9 @@ abstract class LBFactory implements ILBFactory {
                $this->requestInfo = [
                        'IPAddress' => $_SERVER[ 'REMOTE_ADDR' ] ?? '',
                        'UserAgent' => $_SERVER['HTTP_USER_AGENT'] ?? '',
-                       'ChronologyProtection' => 'true',
-                       // phpcs:ignore MediaWiki.Usage.SuperGlobalsUsage.SuperGlobals -- library can't use $wgRequest
-                       'ChronologyPositionIndex' => $_GET['cpPosIndex'] ?? null
+                       // Headers application can inject via LBFactory::setRequestInfo()
+                       'ChronologyClientId' => null, // prior $cpClientId value from LBFactory::shutdown()
+                       'ChronologyPositionIndex' => null // prior $cpIndex value from LBFactory::shutdown()
                ];
 
                $this->cliMode = $conf['cliMode'] ?? ( PHP_SAPI === 'cli' || PHP_SAPI === 'phpdbg' );
@@ -148,7 +148,10 @@ abstract class LBFactory implements ILBFactory {
        }
 
        public function shutdown(
-               $mode = self::SHUTDOWN_CHRONPROT_SYNC, callable $workCallback = null, &$cpIndex = null
+               $mode = self::SHUTDOWN_CHRONPROT_SYNC,
+               callable $workCallback = null,
+               &$cpIndex = null,
+               &$cpClientId = null
        ) {
                $chronProt = $this->getChronologyProtector();
                if ( $mode === self::SHUTDOWN_CHRONPROT_SYNC ) {
@@ -157,6 +160,8 @@ abstract class LBFactory implements ILBFactory {
                        $this->shutdownChronologyProtector( $chronProt, null, 'async', $cpIndex );
                }
 
+               $cpClientId = $chronProt->getClientId();
+
                $this->commitMasterChanges( __METHOD__ ); // sanity
        }
 
@@ -488,6 +493,7 @@ abstract class LBFactory implements ILBFactory {
                        [
                                'ip' => $this->requestInfo['IPAddress'],
                                'agent' => $this->requestInfo['UserAgent'],
+                               'clientId' => $this->requestInfo['ChronologyClientId']
                        ],
                        $this->requestInfo['ChronologyPositionIndex']
                );
@@ -633,32 +639,38 @@ abstract class LBFactory implements ILBFactory {
 
        /**
         * @param int $index Write index
-        * @param int $time UNIX timestamp
-        * @return string Timestamp-qualified write index of the form "<index>.<timestamp>"
+        * @param int $time UNIX timestamp; can be used to detect stale cookies (T190082)
+        * @param string $clientId Agent ID hash from ILBFactory::shutdown()
+        * @return string Timestamp-qualified write index of the form "<index>@<timestamp>#<hash>"
         * @since 1.32
         */
-       public static function makeCookieValueFromCPIndex( $index, $time ) {
-               return $index . '@' . $time;
+       public static function makeCookieValueFromCPIndex( $index, $time, $clientId ) {
+               return "$index@$time#$clientId";
        }
 
        /**
-        * @param string $value String possibly of the form "<index>" or "<index>@<timestamp>"
+        * @param string $value Possible result of LBFactory::makeCookieValueFromCPIndex()
         * @param int $minTimestamp Lowest UNIX timestamp of non-expired values (if present)
-        * @return int|null Write index or null if $value is empty or expired
+        * @return array (index: int or null, clientId: string or null)
         * @since 1.32
         */
-       public static function getCPIndexFromCookieValue( $value, $minTimestamp ) {
-               if ( !preg_match( '/^(\d+)(?:@(\d+))?$/', $value, $m ) ) {
-                       return null;
+       public static function getCPInfoFromCookieValue( $value, $minTimestamp ) {
+               static $placeholder = [ 'index' => null, 'clientId' => null ];
+
+               if ( !preg_match( '/^(\d+)(?:@(\d+))?(?:#([0-9a-f]{32}))?$/', $value, $m ) ) {
+                       return $placeholder; // invalid
                }
 
                $index = (int)$m[1];
-
-               if ( isset( $m[2] ) && $m[2] !== '' && (int)$m[2] < $minTimestamp ) {
-                       return null; // expired
+               if ( $index <= 0 ) {
+                       return $placeholder; // invalid
+               } elseif ( isset( $m[2] ) && $m[2] !== '' && (int)$m[2] < $minTimestamp ) {
+                       return $placeholder; // expired
                }
 
-               return ( $index > 0 ) ? $index : null;
+               $clientId = ( isset( $m[3] ) && $m[3] !== '' ) ? $m[3] : null;
+
+               return [ 'index' => $index, 'clientId' => $clientId ];
        }
 
        public function setRequestInfo( array $info ) {

diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php
index 0395bff..fac3486 100644 (file)
--- a/tests/phpunit/includes/db/LBFactoryTest.php
+++ b/tests/phpunit/includes/db/LBFactoryTest.php
@@ -393,6 +393,8 @@ class LBFactoryTest extends MediaWikiTestCase {
                $cp->shutdown( null, 'sync', $cpIndex );
 
                $this->assertEquals( null, $cpIndex, "CP write index retained" );
+
+               $this->assertEquals( '45e93a9c215c031d38b7c42d8e4700ca', $cp->getClientId() );
        }
 
        private function newLBFactoryMulti( array $baseOverride = [], array $serverOverride = [] ) {
@@ -419,7 +421,8 @@ class LBFactoryTest extends MediaWikiTestCase {
                                'test-db1' => $wgDBserver,
                        ],
                        'loadMonitorClass' => LoadMonitorNull::class,
-                       'localDomain' => new DatabaseDomain( $wgDBname, null, $wgDBprefix )
+                       'localDomain' => new DatabaseDomain( $wgDBname, null, $wgDBprefix ),
+                       'agent' => 'MW-UNIT-TESTS'
                ] );
        }
 
@@ -609,35 +612,65 @@ class LBFactoryTest extends MediaWikiTestCase {
 
        /**
         * @covers \Wikimedia\Rdbms\LBFactory::makeCookieValueFromCPIndex()
-        * @covers \Wikimedia\Rdbms\LBFactory::getCPIndexFromCookieValue()
+        * @covers \Wikimedia\Rdbms\LBFactory::getCPInfoFromCookieValue()
         */
        public function testCPPosIndexCookieValues() {
-               $this->assertEquals( '3@542', LBFactory::makeCookieValueFromCPIndex( 3, 542 ) );
-
                $time = 1526522031;
+               $agentId = md5( 'Ramsey\'s Loyal Presa Canario' );
+
+               $lbFactory = $this->newLBFactoryMulti();
+               $this->assertEquals(
+                       '3@542#c47dcfb0566e7d7bc110a6128a45c93a',
+                       LBFactory::makeCookieValueFromCPIndex( 3, 542, $agentId )
+               );
+
+               $lbFactory = $this->newLBFactoryMulti();
+               $lbFactory->setRequestInfo( [ 'IPAddress' => '10.64.24.52', 'UserAgent' => 'meow' ] );
+               $this->assertEquals(
+                       '1@542#c47dcfb0566e7d7bc110a6128a45c93a',
+                       LBFactory::makeCookieValueFromCPIndex( 1, 542, $agentId )
+               );
                $this->assertSame(
                        5,
-                       LBFactory::getCPIndexFromCookieValue( "5", $time - 10 )
+                       LBFactory::getCPInfoFromCookieValue( "5", $time - 10 )['index'],
+                       'No time set'
                );
                $this->assertSame(
                        null,
-                       LBFactory::getCPIndexFromCookieValue( "0", $time - 10 )
+                       LBFactory::getCPInfoFromCookieValue( "0", $time - 10 )['index'],
+                       'Bad index'
                );
+
                $this->assertSame(
                        2,
-                       LBFactory::getCPIndexFromCookieValue( "2@$time", $time - 10 )
+                       LBFactory::getCPInfoFromCookieValue( "2@$time", $time - 10 )['index'],
+                       'Fresh'
                );
                $this->assertSame(
                        2,
-                       LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 9 - 10 )
+                       LBFactory::getCPInfoFromCookieValue( "2@$time", $time + 9 - 10 )['index'],
+                       'Almost stale'
+               );
+               $this->assertSame(
+                       null,
+                       LBFactory::getCPInfoFromCookieValue( "0@$time", $time + 9 - 10 )['index'],
+                       'Almost stale; bad index'
                );
                $this->assertSame(
                        null,
-                       LBFactory::getCPIndexFromCookieValue( "0@$time", $time + 9 - 10 )
+                       LBFactory::getCPInfoFromCookieValue( "2@$time", $time + 11 - 10 )['index'],
+                       'Stale'
+               );
+
+               $this->assertSame(
+                       $agentId,
+                       LBFactory::getCPInfoFromCookieValue( "5@$time#$agentId", $time - 10 )['clientId'],
+                       'Live (client ID)'
                );
                $this->assertSame(
                        null,
-                       LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 11 - 10 )
+                       LBFactory::getCPInfoFromCookieValue( "5@$time", $time + 11 - 10 )['clientId'],
+                       'Stale (client ID)'
                );
        }
 }