From d927830935f5eecf13aada330111f2e61e4e3c04 Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Wed, 20 Jun 2018 10:32:03 -0400
Subject: [PATCH] API: Check assert parameters earlier in the request

Specifically, check the assert and assertuser parameters before setting
up the action module, so errors in parsing the module's parameters due
to being logged out don't override the client's intended "am I logged
in?" check.

Note this means that assertion failures will no longer use custom module
output formatters. This seems like an acceptable tradeoff: on Wikimedia
sites in May 2018 there were no requests that would have been affected
by this change.

Bug: T197672
Change-Id: I02a71395d5ed9f445e57162f2136292825f8dbb5
---
 RELEASE-NOTES-1.32                         |  3 +++
 includes/api/ApiMain.php                   |  7 ++++--
 tests/phpunit/includes/api/ApiMainTest.php | 29 ++++++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32
index 731f874d16..261f4b824f 100644
--- a/RELEASE-NOTES-1.32
+++ b/RELEASE-NOTES-1.32
@@ -78,6 +78,9 @@ production.
     templated parameters.
 * It is now an error to submit too many values for a multi-valued parameter.
   This has generated a warning since MediaWiki 1.14.
+* Assertion failures from the 'assert' and 'assertuser' parameters will no
+  longer use the action module's custom response format, for the few modules
+  that use custom formatters that handle errors.
 
 === Action API internal changes in 1.32 ===
 * Added 'ApiParseMakeOutputPage' hook.
diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index c0c489555d..f324effb37 100644
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -1554,6 +1554,11 @@ class ApiMain extends ApiBase {
 	 */
 	protected function executeAction() {
 		$params = $this->setupExecuteAction();
+
+		// Check asserts early so e.g. errors in parsing a module's parameters due to being
+		// logged out don't override the client's intended "am I logged in?" check.
+		$this->checkAsserts( $params );
+
 		$module = $this->setupModule();
 		$this->mModule = $module;
 
@@ -1575,8 +1580,6 @@ class ApiMain extends ApiBase {
 			$this->setupExternalResponse( $module, $params );
 		}
 
-		$this->checkAsserts( $params );
-
 		// Execute
 		$module->execute();
 		Hooks::run( 'APIAfterExecute', [ &$module ] );
diff --git a/tests/phpunit/includes/api/ApiMainTest.php b/tests/phpunit/includes/api/ApiMainTest.php
index 584c60c4ae..f06d97efd9 100644
--- a/tests/phpunit/includes/api/ApiMainTest.php
+++ b/tests/phpunit/includes/api/ApiMainTest.php
@@ -434,6 +434,35 @@ class ApiMainTest extends ApiTestCase {
 		}
 	}
 
+	/**
+	 * Test that 'assert' is processed before module errors
+	 */
+	public function testAssertBeforeModule() {
+		// Sanity check that the query without assert throws too-many-titles
+		try {
+			$this->doApiRequest( [
+				'action' => 'query',
+				'titles' => implode( '|', range( 1, ApiBase::LIMIT_SML1 + 1 ) ),
+			], null, null, new User );
+			$this->fail( 'Expected exception not thrown' );
+		} catch ( ApiUsageException $e ) {
+			$this->assertTrue( self::apiExceptionHasCode( $e, 'too-many-titles' ), 'sanity check' );
+		}
+
+		// Now test that the assert happens first
+		try {
+			$this->doApiRequest( [
+				'action' => 'query',
+				'titles' => implode( '|', range( 1, ApiBase::LIMIT_SML1 + 1 ) ),
+				'assert' => 'user',
+			], null, null, new User );
+			$this->fail( 'Expected exception not thrown' );
+		} catch ( ApiUsageException $e ) {
+			$this->assertTrue( self::apiExceptionHasCode( $e, 'assertuserfailed' ),
+				"Error '{$e->getMessage()}' matched expected 'assertuserfailed'" );
+		}
+	}
+
 	/**
 	 * Test if all classes in the main module manager exists
 	 */
-- 
2.20.1