jenkins-bot has submitted this change and it was merged.
Change subject: Ensure all key transformations are applied by
BagOStuff::makeKeyInternal()
......................................................................
Ensure all key transformations are applied by BagOStuff::makeKeyInternal()
Currently we have to undo any transformations we apply to keys in getMulti() by
iterating over the response array keys reversing any changes. This is hairy and
complicated. So --
* Replace calls to MemcachedBagOStuff::encodeKey() with calls to a new method,
MemcachedBagOStuff::validateKeyEncoding(). The new method only validates that
the key is compatible with memcached. If it is not, it throws an exception.
If it is, it returns the key unmodified.
* Remove MemcachedBagOStuff::{encode,decode}Key(), since they no longer serve a
purpose, and have no callers.
Change-Id: If3e20c6a1a1b42fc1f2823aa660328e37c26eccb
---
M includes/objectcache/MemcachedBagOStuff.php
M includes/objectcache/MemcachedPeclBagOStuff.php
M tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
3 files changed, 35 insertions(+), 57 deletions(-)
Approvals:
Aaron Schulz: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/objectcache/MemcachedBagOStuff.php
b/includes/objectcache/MemcachedBagOStuff.php
index 12ba83e..0489531 100644
--- a/includes/objectcache/MemcachedBagOStuff.php
+++ b/includes/objectcache/MemcachedBagOStuff.php
@@ -65,25 +65,25 @@
}
protected function getWithToken( $key, &$casToken, $flags = 0 ) {
- return $this->client->get( $this->encodeKey( $key ), $casToken
);
+ return $this->client->get( $this->validateKeyEncoding( $key ),
$casToken );
}
public function set( $key, $value, $exptime = 0, $flags = 0 ) {
- return $this->client->set( $this->encodeKey( $key ), $value,
+ return $this->client->set( $this->validateKeyEncoding( $key ),
$value,
$this->fixExpiry( $exptime ) );
}
protected function cas( $casToken, $key, $value, $exptime = 0 ) {
- return $this->client->cas( $casToken, $this->encodeKey( $key ),
+ return $this->client->cas( $casToken,
$this->validateKeyEncoding( $key ),
$value, $this->fixExpiry( $exptime ) );
}
public function delete( $key ) {
- return $this->client->delete( $this->encodeKey( $key ) );
+ return $this->client->delete( $this->validateKeyEncoding( $key
) );
}
public function add( $key, $value, $exptime = 0 ) {
- return $this->client->add( $this->encodeKey( $key ), $value,
+ return $this->client->add( $this->validateKeyEncoding( $key ),
$value,
$this->fixExpiry( $exptime ) );
}
@@ -121,10 +121,14 @@
$that = $this;
$args = array_map(
function ( $arg ) use ( $that, &$charsLeft ) {
- // Because MemcachedBagOStuff::encodeKey() will
be called again
- // with this input once the key is actually
used, we have to
- // encode pound signs here rather than in
encodeKey().
- $arg = $that->encodeKey( str_replace( '#',
'%23', $arg ) );
+ // Make sure %, #, and non-ASCII chars are
escaped
+ $arg = preg_replace_callback(
+ '/[^\x21-\x22\x24\x26-\x7e]+/',
+ function ( $m ) {
+ return rawurlencode( $m[0] );
+ },
+ $arg
+ );
// 33 = 32 characters for the MD5 + 1 for the
'#' prefix.
if ( $charsLeft > 33 && strlen( $arg ) >
$charsLeft ) {
@@ -145,26 +149,18 @@
}
/**
- * Encode a key for use on the wire inside the memcached protocol.
+ * Ensure that a key is safe to use (contains no control characters and
no
+ * characters above the ASCII range.)
*
- * We encode spaces and line breaks to avoid protocol errors. We encode
- * the other control characters for compatibility with libmemcached
- * verify_key. We leave other punctuation alone, to maximise backwards
- * compatibility.
* @param string $key
* @return string
+ * @throws Exception
*/
- public function encodeKey( $key ) {
- return preg_replace_callback( '/[^\x21-\x7e]+/',
- array( $this, 'encodeKeyCallback' ), $key );
- }
-
- /**
- * @param array $m
- * @return string
- */
- protected function encodeKeyCallback( $m ) {
- return rawurlencode( $m[0] );
+ public function validateKeyEncoding( $key ) {
+ if ( preg_match( '/[^\x21-\x7e]+/', $key ) ) {
+ throw new Exception( "Key contains invalid characters:
$key" );
+ }
+ return $key;
}
/**
@@ -181,21 +177,6 @@
$expiry = 2592000;
}
return (int)$expiry;
- }
-
- /**
- * Decode a key encoded with encodeKey(). This is provided as a
convenience
- * function for debugging.
- *
- * @param string $key
- *
- * @return string
- */
- public function decodeKey( $key ) {
- // matches %00-%20, %25, %7F (=decoded alternatives for those
encoded in encodeKey)
- return preg_replace_callback( '/%([0-1][0-9]|20|25|7F)/i',
function ( $match ) {
- return urldecode( $match[0] );
- }, $key );
}
/**
diff --git a/includes/objectcache/MemcachedPeclBagOStuff.php
b/includes/objectcache/MemcachedPeclBagOStuff.php
index 365236d..e6df900 100644
--- a/includes/objectcache/MemcachedPeclBagOStuff.php
+++ b/includes/objectcache/MemcachedPeclBagOStuff.php
@@ -117,7 +117,7 @@
protected function getWithToken( $key, &$casToken, $flags = 0 ) {
$this->debugLog( "get($key)" );
- $result = $this->client->get( $this->encodeKey( $key ), null,
$casToken );
+ $result = $this->client->get( $this->validateKeyEncoding( $key
), null, $casToken );
$result = $this->checkResult( $key, $result );
return $result;
}
@@ -202,14 +202,10 @@
public function getMulti( array $keys, $flags = 0 ) {
$this->debugLog( 'getMulti(' . implode( ', ', $keys ) . ')' );
- $callback = array( $this, 'encodeKey' );
- $encodedResult = $this->client->getMulti( array_map( $callback,
$keys ) );
- $encodedResult = $encodedResult ?: array(); // must be an array
- $result = array();
- foreach ( $encodedResult as $key => $value ) {
- $key = $this->decodeKey( $key );
- $result[$key] = $value;
+ foreach ( $keys as $key ) {
+ $this->validateKeyEncoding( $key );
}
+ $result = $this->client->getMulti( $keys ) ?: array();
return $this->checkResult( false, $result );
}
@@ -219,14 +215,10 @@
* @return bool
*/
public function setMulti( array $data, $exptime = 0 ) {
- foreach ( $data as $key => $value ) {
- $encKey = $this->encodeKey( $key );
- if ( $encKey !== $key ) {
- $data[$encKey] = $value;
- unset( $data[$key] );
- }
- }
$this->debugLog( 'setMulti(' . implode( ', ', array_keys( $data
) ) . ')' );
+ foreach ( array_keys( $data ) as $key ) {
+ $this->validateKeyEncoding( $key );
+ }
$result = $this->client->setMulti( $data, $this->fixExpiry(
$exptime ) );
return $this->checkResult( false, $result );
}
diff --git a/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
b/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
index 64478a2..938f3b9 100644
--- a/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
@@ -21,8 +21,13 @@
);
$this->assertEquals(
- $this->cache->makeKey( 'punctuation_marks_are_ok',
'!@$%^&*()' ),
- 'test:punctuation_marks_are_ok:!@$%^&*()'
+ $this->cache->makeKey( 'punctuation_marks_are_ok',
'!@$^&*()' ),
+ 'test:punctuation_marks_are_ok:!@$^&*()'
+ );
+
+ $this->assertEquals(
+ $this->cache->makeKey( 'percent_is_escaped',
'!@$%^&*()' ),
+ 'test:percent_is_escaped:!@$%25^&*()'
);
$this->assertEquals(
--
To view, visit https://gerrit.wikimedia.org/r/248582
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If3e20c6a1a1b42fc1f2823aa660328e37c26eccb
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Ori.livneh <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
