From 24be43b9aaffa5b723f5074bef10485cf34c3788 Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Mon, 28 May 2018 18:45:24 -0400
Subject: [PATCH] API: ApiBase::getParameter() shouldn't throw on other params'
 errors

This regression was introduced in Ia19a1617b7.

Bug: T195777
Change-Id: I1e1eb3861ced83f79e56d2325ab693ef4e393999
---
 includes/api/ApiBase.php                   | 51 +++++++++++++++++++---
 tests/phpunit/includes/api/ApiBaseTest.php | 38 ++++++++++++++++
 2 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php
index b18bf58dcc..02a635a712 100644
--- a/includes/api/ApiBase.php
+++ b/includes/api/ApiBase.php
@@ -761,10 +761,23 @@ abstract class ApiBase extends ContextSource {
 	 * value - validated value from user or default. limits will not be
 	 * parsed if $parseLimit is set to false; use this when the max
 	 * limit is not definitive yet, e.g. when getting revisions.
-	 * @param bool $parseLimit True by default
+	 * @param bool|array $options If a boolean, uses that as the value for 'parseLimit'
+	 *  - parseLimit: (bool, default true) Whether to parse the 'max' value for limit types
+	 *  - safeMode: (bool, default false) If true, avoid throwing for parameter validation errors.
+	 *    Returned parameter values might be ApiUsageException instances.
 	 * @return array
 	 */
-	public function extractRequestParams( $parseLimit = true ) {
+	public function extractRequestParams( $options = [] ) {
+		if ( is_bool( $options ) ) {
+			$options = [ 'parseLimit' => $options ];
+		}
+		$options += [
+			'parseLimit' => true,
+			'safeMode' => false,
+		];
+
+		$parseLimit = (bool)$options['parseLimit'];
+
 		// Cache parameters, for performance and to avoid T26564.
 		if ( !isset( $this->mParamCache[$parseLimit] ) ) {
 			$params = $this->getFinalParams() ?: [];
@@ -778,9 +791,13 @@ abstract class ApiBase extends ContextSource {
 				if ( isset( $paramSettings[self::PARAM_TEMPLATE_VARS] ) ) {
 					$toProcess[] = [ $paramName, $paramSettings[self::PARAM_TEMPLATE_VARS], $paramSettings ];
 				} else {
-					$results[$paramName] = $this->getParameterFromSettings(
-						$paramName, $paramSettings, $parseLimit
-					);
+					try {
+						$results[$paramName] = $this->getParameterFromSettings(
+							$paramName, $paramSettings, $parseLimit
+						);
+					} catch ( ApiUsageException $ex ) {
+						$results[$paramName] = $ex;
+					}
 				}
 			}
 
@@ -826,7 +843,11 @@ abstract class ApiBase extends ContextSource {
 
 						$newName = str_replace( $placeholder, $value, $name );
 						if ( !$targets ) {
-							$results[$newName] = $this->getParameterFromSettings( $newName, $settings, $parseLimit );
+							try {
+								$results[$newName] = $this->getParameterFromSettings( $newName, $settings, $parseLimit );
+							} catch ( ApiUsageException $ex ) {
+								$results[$newName] = $ex;
+							}
 						} else {
 							$newTargets = [];
 							foreach ( $targets as $k => $v ) {
@@ -842,6 +863,15 @@ abstract class ApiBase extends ContextSource {
 			$this->mParamCache[$parseLimit] = $results;
 		}
 
+		$ret = $this->mParamCache[$parseLimit];
+		if ( !$options['safeMode'] ) {
+			foreach ( $ret as $v ) {
+				if ( $v instanceof ApiUsageException ) {
+					throw $v;
+				}
+			}
+		}
+
 		return $this->mParamCache[$parseLimit];
 	}
 
@@ -852,7 +882,14 @@ abstract class ApiBase extends ContextSource {
 	 * @return mixed Parameter value
 	 */
 	protected function getParameter( $paramName, $parseLimit = true ) {
-		return $this->extractRequestParams( $parseLimit )[$paramName];
+		$ret = $this->extractRequestParams( [
+			'parseLimit' => $parseLimit,
+			'safeMode' => true,
+		] )[$paramName];
+		if ( $ret instanceof ApiUsageException ) {
+			throw $ret;
+		}
+		return $ret;
 	}
 
 	/**
diff --git a/tests/phpunit/includes/api/ApiBaseTest.php b/tests/phpunit/includes/api/ApiBaseTest.php
index 4e3e3fc962..cce99cebe0 100644
--- a/tests/phpunit/includes/api/ApiBaseTest.php
+++ b/tests/phpunit/includes/api/ApiBaseTest.php
@@ -212,6 +212,44 @@ class ApiBaseTest extends ApiTestCase {
 		$mock->getTitleFromTitleOrPageId( [ 'pageid' => 298401643 ] );
 	}
 
+	public function testGetParameter() {
+		$mock = $this->getMockBuilder( MockApi::class )
+			->setMethods( [ 'getAllowedParams' ] )
+			->getMock();
+		$mock->method( 'getAllowedParams' )->willReturn( [
+			'foo' => [
+				ApiBase::PARAM_TYPE => [ 'value' ],
+			],
+			'bar' => [
+				ApiBase::PARAM_TYPE => [ 'value' ],
+			],
+		] );
+		$wrapper = TestingAccessWrapper::newFromObject( $mock );
+
+		$context = new DerivativeContext( $mock );
+		$context->setRequest( new FauxRequest( [ 'foo' => 'bad', 'bar' => 'value' ] ) );
+		$wrapper->mMainModule = new ApiMain( $context );
+
+		// Even though 'foo' is bad, getParameter( 'bar' ) must not fail
+		$this->assertSame( 'value', $wrapper->getParameter( 'bar' ) );
+
+		// But getParameter( 'foo' ) must throw.
+		try {
+			$wrapper->getParameter( 'foo' );
+			$this->fail( 'Expected exception not thrown' );
+		} catch ( ApiUsageException $ex ) {
+			$this->assertTrue( $this->apiExceptionHasCode( $ex, 'unknown_foo' ) );
+		}
+
+		// And extractRequestParams() must throw too.
+		try {
+			$mock->extractRequestParams();
+			$this->fail( 'Expected exception not thrown' );
+		} catch ( ApiUsageException $ex ) {
+			$this->assertTrue( $this->apiExceptionHasCode( $ex, 'unknown_foo' ) );
+		}
+	}
+
 	/**
 	 * @dataProvider provideGetParameterFromSettings
 	 * @param string|null $input
-- 
2.20.1