From 44b47b43ee0df87d01ed40ecf0899137c5bb2b68 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Mon, 11 Jun 2018 14:52:37 -0700
Subject: [PATCH] rdbms: make getCPInfoFromCookieValue() stricter about allowed
 values

All components, not just the write index, must now be present.

Bug: T194403
Change-Id: I279ba3e16d470aca09fdb74cec91d28efb5e2f95
---
 includes/libs/rdbms/lbfactory/LBFactory.php |  4 ++--
 tests/phpunit/includes/db/LBFactoryTest.php | 22 +++++++++++++--------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php
index 34436fb4a4..130a097c78 100644
--- a/includes/libs/rdbms/lbfactory/LBFactory.php
+++ b/includes/libs/rdbms/lbfactory/LBFactory.php
@@ -650,14 +650,14 @@ abstract class LBFactory implements ILBFactory {
 
 	/**
 	 * @param string $value Possible result of LBFactory::makeCookieValueFromCPIndex()
-	 * @param int $minTimestamp Lowest UNIX timestamp of non-expired values (if present)
+	 * @param int $minTimestamp Lowest UNIX timestamp that a non-expired value can have
 	 * @return array (index: int or null, clientId: string or null)
 	 * @since 1.32
 	 */
 	public static function getCPInfoFromCookieValue( $value, $minTimestamp ) {
 		static $placeholder = [ 'index' => null, 'clientId' => null ];
 
-		if ( !preg_match( '/^(\d+)(?:@(\d+))?(?:#([0-9a-f]{32}))?$/', $value, $m ) ) {
+		if ( !preg_match( '/^(\d+)@(\d+)#([0-9a-f]{32})$/', $value, $m ) ) {
 			return $placeholder; // invalid
 		}
 
diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php
index fac34867d8..82ca66a5fb 100644
--- a/tests/phpunit/includes/db/LBFactoryTest.php
+++ b/tests/phpunit/includes/db/LBFactoryTest.php
@@ -630,35 +630,41 @@ class LBFactoryTest extends MediaWikiTestCase {
 			'1@542#c47dcfb0566e7d7bc110a6128a45c93a',
 			LBFactory::makeCookieValueFromCPIndex( 1, 542, $agentId )
 		);
+
 		$this->assertSame(
-			5,
-			LBFactory::getCPInfoFromCookieValue( "5", $time - 10 )['index'],
+			null,
+			LBFactory::getCPInfoFromCookieValue( "5#$agentId", $time - 10 )['index'],
 			'No time set'
 		);
 		$this->assertSame(
 			null,
-			LBFactory::getCPInfoFromCookieValue( "0", $time - 10 )['index'],
+			LBFactory::getCPInfoFromCookieValue( "5@$time", $time - 10 )['index'],
+			'No agent set'
+		);
+		$this->assertSame(
+			null,
+			LBFactory::getCPInfoFromCookieValue( "0@$time#$agentId", $time - 10 )['index'],
 			'Bad index'
 		);
 
 		$this->assertSame(
 			2,
-			LBFactory::getCPInfoFromCookieValue( "2@$time", $time - 10 )['index'],
+			LBFactory::getCPInfoFromCookieValue( "2@$time#$agentId", $time - 10 )['index'],
 			'Fresh'
 		);
 		$this->assertSame(
 			2,
-			LBFactory::getCPInfoFromCookieValue( "2@$time", $time + 9 - 10 )['index'],
+			LBFactory::getCPInfoFromCookieValue( "2@$time#$agentId", $time + 9 - 10 )['index'],
 			'Almost stale'
 		);
 		$this->assertSame(
 			null,
-			LBFactory::getCPInfoFromCookieValue( "0@$time", $time + 9 - 10 )['index'],
+			LBFactory::getCPInfoFromCookieValue( "0@$time#$agentId", $time + 9 - 10 )['index'],
 			'Almost stale; bad index'
 		);
 		$this->assertSame(
 			null,
-			LBFactory::getCPInfoFromCookieValue( "2@$time", $time + 11 - 10 )['index'],
+			LBFactory::getCPInfoFromCookieValue( "2@$time#$agentId", $time + 11 - 10 )['index'],
 			'Stale'
 		);
 
@@ -669,7 +675,7 @@ class LBFactoryTest extends MediaWikiTestCase {
 		);
 		$this->assertSame(
 			null,
-			LBFactory::getCPInfoFromCookieValue( "5@$time", $time + 11 - 10 )['clientId'],
+			LBFactory::getCPInfoFromCookieValue( "5@$time#$agentId", $time + 11 - 10 )['clientId'],
 			'Stale (client ID)'
 		);
 	}
-- 
2.20.1