From 0c9fb12265e2fe47616dbb0bcfac1ffeb008694e Mon Sep 17 00:00:00 2001
From: Ori Livneh <ori@wikimedia.org>
Date: Fri, 23 Oct 2015 15:12:51 -0700
Subject: [PATCH] Escape colons in BagOStuff key segments

For the sake of safety and correctness, the following BagOStuff::makeKey()
invocations should return distinct keys:

   $cache->makeKey( 'ab:', 'cd' );
   $cache->makeKey( 'ab', ':cd' );

That is not currently the case, because while we use ':' as a key path
separator, we don't escape ':' in the input supplied to makeKey(). So, make
BagOStuff::makeKeyInternal() URL-encode colons.

To prevent this from messing up the max. key length calculations, reproduce
this logic in MemcachedBagOStuff::makeKeyInternal(), in lieu of having the
method call its parent.

Change-Id: I83ea7e7336a1c9e64aa42284c2517089a736efe5
---
 includes/libs/objectcache/BagOStuff.php                  | 6 +++++-
 includes/objectcache/MemcachedBagOStuff.php              | 9 +++++----
 tests/phpunit/includes/objectcache/BagOStuffTest.php     | 5 +++++
 .../includes/objectcache/MemcachedBagOStuffTest.php      | 4 ++--
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index c3c357ffa5..ecc5e372f3 100644
--- a/includes/libs/objectcache/BagOStuff.php
+++ b/includes/libs/objectcache/BagOStuff.php
@@ -627,7 +627,11 @@ abstract class BagOStuff implements LoggerAwareInterface {
 	 * @return string
 	 */
 	public function makeKeyInternal( $keyspace, $args ) {
-		$key = $keyspace . ':' . implode( ':', $args );
+		$key = $keyspace;
+		foreach ( $args as $arg ) {
+			$arg = str_replace( ':', '%3A', $arg );
+			$key = $key . ':' . $arg;
+		}
 		return strtr( $key, ' ', '_' );
 	}
 
diff --git a/includes/objectcache/MemcachedBagOStuff.php b/includes/objectcache/MemcachedBagOStuff.php
index 048953140c..8bf6e3a1ff 100644
--- a/includes/objectcache/MemcachedBagOStuff.php
+++ b/includes/objectcache/MemcachedBagOStuff.php
@@ -118,9 +118,10 @@ class MemcachedBagOStuff extends BagOStuff {
 		// the separator character needed for each argument.
 		$charsLeft = 255 - strlen( $keyspace ) - count( $args );
 
-		$that = $this;
 		$args = array_map(
-			function ( $arg ) use ( $that, &$charsLeft ) {
+			function ( $arg ) use ( &$charsLeft ) {
+				$arg = strtr( $arg, ' ', '_' );
+
 				// Make sure %, #, and non-ASCII chars are escaped
 				$arg = preg_replace_callback(
 					'/[^\x21-\x22\x24\x26-\x7e]+/',
@@ -142,10 +143,10 @@ class MemcachedBagOStuff extends BagOStuff {
 		);
 
 		if ( $charsLeft < 0 ) {
-			$args = array( '##' . md5( implode( ':', $args ) ) );
+			return $keyspace . ':##' . md5( implode( ':', $args ) );
 		}
 
-		return parent::makeKeyInternal( $keyspace, $args );
+		return $keyspace . ':' . implode( ':', $args );
 	}
 
 	/**
diff --git a/tests/phpunit/includes/objectcache/BagOStuffTest.php b/tests/phpunit/includes/objectcache/BagOStuffTest.php
index 466b9f58f8..94b74cb651 100644
--- a/tests/phpunit/includes/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/BagOStuffTest.php
@@ -50,6 +50,11 @@ class BagOStuffTest extends MediaWikiTestCase {
 			$globalKey,
 			'Local key and global key with same parameters should not be equal'
 		);
+
+		$this->assertNotEquals(
+			$cache->makeKeyInternal( 'prefix', array( 'a', 'bc:', 'de' ) ),
+			$cache->makeKeyInternal( 'prefix', array( 'a', 'bc', ':de' ) )
+		);
 	}
 
 	/**
diff --git a/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php b/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
index 938f3b9ddf..a946c3cdf9 100644
--- a/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
@@ -32,7 +32,7 @@ class MemcachedBagOStuffTest extends MediaWikiTestCase {
 
 		$this->assertEquals(
 			$this->cache->makeKey( 'but spaces', 'hashes#', "and\nnewlines", 'are_not' ),
-			'test:but%20spaces:hashes%23:and%0Anewlines:are_not'
+			'test:but_spaces:hashes%23:and%0Anewlines:are_not'
 		);
 
 		$this->assertEquals(
@@ -43,7 +43,7 @@ class MemcachedBagOStuffTest extends MediaWikiTestCase {
 
 		$this->assertEquals(
 			$this->cache->makeKey( 'this', 'key', 'contains', '𝕥𝕠𝕠 𝕞𝕒𝕟𝕪 𝕞𝕦𝕝𝕥𝕚𝕓𝕪𝕥𝕖 𝕔𝕙𝕒𝕣𝕒𝕔𝕥𝕖𝕣𝕤' ),
-			'test:this:key:contains:#60190c8f5a63ba5438b124f5c10b91d0'
+			'test:this:key:contains:#c118f92685a635cb843039de50014c9c'
 		);
 
 		$this->assertEquals(
-- 
2.20.1