From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Tue, 8 Sep 2015 15:40:23 +0000 (-0400)
Subject: ApiResult: Fix size checking
X-Git-Tag: 1.31.0-rc.0~10077^2
X-Git-Url: http://git.cyclocoop.org/%22.%24image2.%22?a=commitdiff_plain;h=46322fff07f204793d60c2e64171c90e5e29b17f;p=lhc%2Fweb%2Fwiklou.git

ApiResult: Fix size checking

Two bugs here:
* Setting NO_SIZE_CHECK also bypassed validation
* ApiResult::valueSize() didn't handle ApiSerializable, which is fixed
  by defining that the value needs to be passed through
  ApiResult::validateValue() first.

Bug: T111796
Change-Id: I7c00d8ee53364a26f8f63f82a4d83b92baf5383e
---

diff --git a/includes/api/ApiResult.php b/includes/api/ApiResult.php
index 014ca85bee..608e9e1213 100644
--- a/includes/api/ApiResult.php
+++ b/includes/api/ApiResult.php
@@ -288,7 +288,7 @@ class ApiResult implements ApiSerializable {
 	 * @param int $flags Zero or more OR-ed flags like OVERRIDE | ADD_ON_TOP.
 	 */
 	public static function setValue( array &$arr, $name, $value, $flags = 0 ) {
-		if ( !( $flags & ApiResult::NO_VALIDATE ) ) {
+		if ( ( $flags & ApiResult::NO_VALIDATE ) !== ApiResult::NO_VALIDATE ) {
 			$value = self::validateValue( $value );
 		}
 
@@ -402,6 +402,11 @@ class ApiResult implements ApiSerializable {
 		$arr = &$this->path( $path, ( $flags & ApiResult::ADD_ON_TOP ) ? 'prepend' : 'append' );
 
 		if ( $this->checkingSize && !( $flags & ApiResult::NO_SIZE_CHECK ) ) {
+			// self::valueSize needs the validated value. Then flag
+			// to not re-validate later.
+			$value = self::validateValue( $value );
+			$flags |= ApiResult::NO_VALIDATE;
+
 			$newsize = $this->size + self::valueSize( $value );
 			if ( $this->maxSize !== false && $newsize > $this->maxSize ) {
 				/// @todo Add i18n message when replacing calls to ->setWarning()
@@ -1079,14 +1084,12 @@ class ApiResult implements ApiSerializable {
 	 * or the sum of the strlen()s of the elements if the item is an array.
 	 * @note Once the deprecated public self::size is removed, we can rename
 	 *       this back to a less awkward name.
-	 * @param mixed $value
+	 * @param mixed $value Validated value (see self::validateValue())
 	 * @return int
 	 */
 	private static function valueSize( $value ) {
 		$s = 0;
-		if ( is_array( $value ) ||
-			is_object( $value ) && !is_callable( array( $value, '__toString' ) )
-		) {
+		if ( is_array( $value ) ) {
 			foreach ( $value as $k => $v ) {
 				if ( !self::isMetadataKey( $s ) ) {
 					$s += self::valueSize( $v );
@@ -1488,7 +1491,7 @@ class ApiResult implements ApiSerializable {
 	 */
 	public static function size( $value ) {
 		wfDeprecated( __METHOD__, '1.25' );
-		return self::valueSize( $value );
+		return self::valueSize( self::validateValue( $value ) );
 	}
 
 	/**
diff --git a/tests/phpunit/includes/api/ApiResultTest.php b/tests/phpunit/includes/api/ApiResultTest.php
index affb0fa9a5..cff2328dc8 100644
--- a/tests/phpunit/includes/api/ApiResultTest.php
+++ b/tests/phpunit/includes/api/ApiResultTest.php
@@ -181,6 +181,19 @@ class ApiResultTest extends MediaWikiTestCase {
 			);
 		}
 
+		ApiResult::setValue( $arr, null, NAN, ApiResult::NO_VALIDATE );
+
+		try {
+			ApiResult::setValue( $arr, null, NAN, ApiResult::NO_SIZE_CHECK );
+			$this->fail( 'Expected exception not thrown' );
+		} catch ( InvalidArgumentException $ex ) {
+			$this->assertSame(
+				'Cannot add non-finite floats to ApiResult',
+				$ex->getMessage(),
+				'Expected exception'
+			);
+		}
+
 		$arr = array();
 		$result2 = new ApiResult( 8388608 );
 		$result2->addValue( null, 'foo', 'bar' );
@@ -408,6 +421,19 @@ class ApiResultTest extends MediaWikiTestCase {
 			);
 		}
 
+		$result->addValue( null, null, NAN, ApiResult::NO_VALIDATE );
+
+		try {
+			$result->addValue( null, null, NAN, ApiResult::NO_SIZE_CHECK );
+			$this->fail( 'Expected exception not thrown' );
+		} catch ( InvalidArgumentException $ex ) {
+			$this->assertSame(
+				'Cannot add non-finite floats to ApiResult',
+				$ex->getMessage(),
+				'Expected exception'
+			);
+		}
+
 		$result->reset();
 		$result->addParsedLimit( 'foo', 12 );
 		$this->assertSame( array(
@@ -444,6 +470,12 @@ class ApiResultTest extends MediaWikiTestCase {
 		$result->removeValue( null, 'foo' );
 		$this->assertTrue( $result->addValue( null, 'foo', '1' ) );
 
+		$result = new ApiResult( 10 );
+		$obj = new ApiResultTestSerializableObject( 'ok' );
+		$obj->foobar = 'foobaz';
+		$this->assertTrue( $result->addValue( null, 'foo', $obj ) );
+		$this->assertSame( 2, $result->getSize() );
+
 		$result = new ApiResult( 8388608 );
 		$result2 = new ApiResult( 8388608 );
 		$result2->addValue( null, 'foo', 'bar' );