API: Validate API error codes

Validate them in ApiMessageTrait when the message is created, and again
in ApiMain before they're included in the header.

This also introduces an "api-warning" log channel, since "api" is too
spammy for real use, and converts a few existing things to use it.

Bug: T208926
Change-Id: Ib2d8bd4d4a5d58af76431835ba783c148de7792a
Depends-On: Iced44f2602d57eea9a2d15aee5b8c9a50092b49c
Depends-On: I5c2747f527c30ded7a614feb26f5777d901bd512
Depends-On: I9c9bd8f5309518fcbab7179fb71d209c005e5e64

diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33
index 1021cb3..422bbfa 100644 (file)
--- a/RELEASE-NOTES-1.33
+++ b/RELEASE-NOTES-1.33
@@ -68,6 +68,10 @@ production.
   Additionally, the  'APIGetDescription' and 'APIGetParamDescription' hooks have
   been removed, as their only use was to let extensions override values returned
   by getDescription() and getParamDescription(), respectively.
+* API error codes may only contain ASCII letters, numbers, underscore, and
+  hyphen. Methods such as ApiBase::dieWithError() and
+  ApiMessageTrait::setApiCode() will throw an InvalidArgumentException if
+  passed a bad code.
 * …
 
 === Languages updated in 1.33 ===

diff --git a/includes/api/ApiErrorFormatter.php b/includes/api/ApiErrorFormatter.php
index 847afd8..ef28b73 100644 (file)
--- a/includes/api/ApiErrorFormatter.php
+++ b/includes/api/ApiErrorFormatter.php
@@ -58,6 +58,26 @@ class ApiErrorFormatter {
                $this->format = $format;
        }
 
+       /**
+        * Test whether a code is a valid API error code
+        *
+        * A valid code contains only ASCII letters, numbers, underscore, and
+        * hyphen and is not the empty string.
+        *
+        * For backwards compatibility, any code beginning 'internal_api_error_' is
+        * also allowed.
+        *
+        * @param string $code
+        * @return bool
+        */
+       public static function isValidApiCode( $code ) {
+               return is_string( $code ) && (
+                       preg_match( '/^[a-zA-Z0-9_-]+$/', $code ) ||
+                       // TODO: Deprecate this
+                       preg_match( '/^internal_api_error_[^\0\r\n]+$/', $code )
+               );
+       }
+
        /**
         * Return a formatter like this one but with a different format
         *

diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 22232dd..162f0ce 100644 (file)
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -834,7 +834,11 @@ class ApiMain extends ApiBase {
                foreach ( $requestedHeaders as $rHeader ) {
                        $rHeader = strtolower( trim( $rHeader ) );
                        if ( !isset( $allowedAuthorHeaders[$rHeader] ) ) {
-                               wfDebugLog( 'api', 'CORS preflight failed on requested header: ' . $rHeader );
+                               LoggerFactory::getInstance( 'api-warning' )->warning(
+                                       'CORS preflight failed on requested header: {header}', [
+                                               'header' => $rHeader
+                                       ]
+                               );
                                return false;
                        }
                }
@@ -1025,6 +1029,7 @@ class ApiMain extends ApiBase {
                } else {
                        // Something is seriously wrong
                        $config = $this->getConfig();
+                       // TODO: Avoid embedding arbitrary class names in the error code.
                        $class = preg_replace( '#^Wikimedia\\\Rdbms\\\#', '', get_class( $e ) );
                        $code = 'internal_api_error_' . $class;
                        if ( $config->get( 'ShowExceptionDetails' ) ) {
@@ -1077,7 +1082,15 @@ class ApiMain extends ApiBase {
                // Add errors from the exception
                $modulePath = $e instanceof ApiUsageException ? $e->getModulePath() : null;
                foreach ( $this->errorMessagesFromException( $e, 'error' ) as $msg ) {
-                       $errorCodes[$msg->getApiCode()] = true;
+                       if ( ApiErrorFormatter::isValidApiCode( $msg->getApiCode() ) ) {
+                               $errorCodes[$msg->getApiCode()] = true;
+                       } else {
+                               LoggerFactory::getInstance( 'api-warning' )->error( 'Invalid API error code "{code}"', [
+                                       'code' => $msg->getApiCode(),
+                                       'exception' => $e,
+                               ] );
+                               $errorCodes['<invalid-code>'] = true;
+                       }
                        $formatter->addError( $modulePath, $msg );
                }
                foreach ( $this->errorMessagesFromException( $e, 'warning' ) as $msg ) {
@@ -1464,9 +1477,14 @@ class ApiMain extends ApiBase {
                if ( $numLagged >= ceil( $replicaCount / 2 ) ) {
                        $laggedServers = implode( ', ', $laggedServers );
                        wfDebugLog(
-                               'api-readonly',
+                               'api-readonly', // Deprecate this channel in favor of api-warning?
                                "Api request failed as read only because the following DBs are lagged: $laggedServers"
                        );
+                       LoggerFactory::getInstance( 'api-warning' )->warning(
+                               "Api request failed as read only because the following DBs are lagged: {laggeddbs}", [
+                                       'laggeddbs' => $laggedServers,
+                               ]
+                       );
 
                        $this->dieWithError(
                                'readonly_lag',

diff --git a/includes/api/ApiMessageTrait.php b/includes/api/ApiMessageTrait.php
index 18b6bc4..6894d28 100644 (file)
--- a/includes/api/ApiMessageTrait.php
+++ b/includes/api/ApiMessageTrait.php
@@ -105,12 +105,15 @@ trait ApiMessageTrait {
                        } else {
                                $this->apiCode = $key;
                        }
+
+                       // Ensure the code is actually valid
+                       $this->apiCode = preg_replace( '/[^a-zA-Z0-9_-]/', '_', $this->apiCode );
                }
                return $this->apiCode;
        }
 
        public function setApiCode( $code, array $data = null ) {
-               if ( $code !== null && !( is_string( $code ) && $code !== '' ) ) {
+               if ( $code !== null && !ApiErrorFormatter::isValidApiCode( $code ) ) {
                        throw new InvalidArgumentException( "Invalid code \"$code\"" );
                }
 

diff --git a/tests/phpunit/includes/api/ApiErrorFormatterTest.php b/tests/phpunit/includes/api/ApiErrorFormatterTest.php
index d11e314..65a511d 100644 (file)
--- a/tests/phpunit/includes/api/ApiErrorFormatterTest.php
+++ b/tests/phpunit/includes/api/ApiErrorFormatterTest.php
@@ -632,4 +632,23 @@ class ApiErrorFormatterTest extends MediaWikiLangTestCase {
                ];
        }
 
+       /**
+        * @dataProvider provideIsValidApiCode
+        * @covers ApiErrorFormatter::isValidApiCode
+        * @param string $code
+        * @param bool $expect
+        */
+       public function testIsValidApiCode( $code, $expect ) {
+               $this->assertSame( $expect, ApiErrorFormatter::isValidApiCode( $code ) );
+       }
+
+       public static function provideIsValidApiCode() {
+               return [
+                       [ 'foo-bar_Baz123', true ],
+                       [ 'foo bar', false ],
+                       [ 'foo\\bar', false ],
+                       [ 'internal_api_error_foo\\bar baz', true ],
+               ];
+       }
+
 }

diff --git a/tests/phpunit/includes/api/ApiMainTest.php b/tests/phpunit/includes/api/ApiMainTest.php
index a6083cd..3299e73 100644 (file)
--- a/tests/phpunit/includes/api/ApiMainTest.php
+++ b/tests/phpunit/includes/api/ApiMainTest.php
@@ -1010,6 +1010,15 @@ class ApiMainTest extends ApiTestCase {
                        MWExceptionHandler::getRedactedTraceAsString( $dbex )
                )->inLanguage( 'en' )->useDatabase( false )->text();
 
+               // The specific exception doesn't matter, as long as it's namespaced.
+               $nsex = new MediaWiki\ShellDisabledError();
+               $nstrace = wfMessage( 'api-exception-trace',
+                       get_class( $nsex ),
+                       $nsex->getFile(),
+                       $nsex->getLine(),
+                       MWExceptionHandler::getRedactedTraceAsString( $nsex )
+               )->inLanguage( 'en' )->useDatabase( false )->text();
+
                $apiEx1 = new ApiUsageException( null,
                        StatusValue::newFatal( new ApiRawMessage( 'An error', 'sv-error1' ) ) );
                TestingAccessWrapper::newFromObject( $apiEx1 )->modulePath = 'foo+bar';
@@ -1017,6 +1026,13 @@ class ApiMainTest extends ApiTestCase {
                $apiEx1->getStatusValue()->warning( new ApiRawMessage( 'Another warning', 'sv-warn2' ) );
                $apiEx1->getStatusValue()->fatal( new ApiRawMessage( 'Another error', 'sv-error2' ) );
 
+               $badMsg = $this->getMockBuilder( ApiRawMessage::class )
+                        ->setConstructorArgs( [ 'An error', 'ignored' ] )
+                        ->setMethods( [ 'getApiCode' ] )
+                        ->getMock();
+               $badMsg->method( 'getApiCode' )->willReturn( "bad\nvalue" );
+               $apiEx2 = new ApiUsageException( null, StatusValue::newFatal( $badMsg ) );
+
                return [
                        [
                                $ex,
@@ -1055,6 +1071,24 @@ class ApiMainTest extends ApiTestCase {
                                        'servedby' => wfHostname(),
                                ]
                        ],
+                       [
+                               $nsex,
+                               [ 'existing-error', 'internal_api_error_MediaWiki\ShellDisabledError' ],
+                               [
+                                       'warnings' => [
+                                               [ 'code' => 'existing-warning', 'text' => 'existing warning', 'module' => 'main' ],
+                                       ],
+                                       'errors' => [
+                                               [ 'code' => 'existing-error', 'text' => 'existing error', 'module' => 'main' ],
+                                               [
+                                                       'code' => 'internal_api_error_MediaWiki\ShellDisabledError',
+                                                       'text' => "[$reqId] Exception caught: " . $nsex->getMessage(),
+                                               ]
+                                       ],
+                                       'trace' => $nstrace,
+                                       'servedby' => wfHostname(),
+                               ]
+                       ],
                        [
                                $apiEx1,
                                [ 'existing-error', 'sv-error1', 'sv-error2' ],
@@ -1075,6 +1109,23 @@ class ApiMainTest extends ApiTestCase {
                                        'servedby' => wfHostname(),
                                ]
                        ],
+                       [
+                               $apiEx2,
+                               [ 'existing-error', '<invalid-code>' ],
+                               [
+                                       'warnings' => [
+                                               [ 'code' => 'existing-warning', 'text' => 'existing warning', 'module' => 'main' ],
+                                       ],
+                                       'errors' => [
+                                               [ 'code' => 'existing-error', 'text' => 'existing error', 'module' => 'main' ],
+                                               [ 'code' => "bad\nvalue", 'text' => 'An error' ],
+                                       ],
+                                       'docref' => "See $doclink for API usage. Subscribe to the mediawiki-api-announce mailing " .
+                                               "list at &lt;https://lists.wikimedia.org/mailman/listinfo/mediawiki-api-announce&gt; " .
+                                               "for notice of API deprecations and breaking changes.",
+                                       'servedby' => wfHostname(),
+                               ]
+                       ]
                ];
        }
 

diff --git a/tests/phpunit/includes/api/ApiMessageTest.php b/tests/phpunit/includes/api/ApiMessageTest.php
index c6f5a8e..70114c2 100644 (file)
--- a/tests/phpunit/includes/api/ApiMessageTest.php
+++ b/tests/phpunit/includes/api/ApiMessageTest.php
@@ -38,6 +38,10 @@ class ApiMessageTest extends MediaWikiTestCase {
                $msg = new ApiMessage( 'apiwarn-baz' );
                $this->assertSame( 'baz', $msg->getApiCode() );
 
+               // Weird "message key"
+               $msg = new ApiMessage( "<foo> bar\nbaz" );
+               $this->assertSame( '_foo__bar_baz', $msg->getApiCode() );
+
                // BC case
                $msg = new ApiMessage( 'actionthrottledtext' );
                $this->assertSame( 'ratelimited', $msg->getApiCode() );
@@ -72,6 +76,9 @@ class ApiMessageTest extends MediaWikiTestCase {
                return [
                        [ '' ],
                        [ 42 ],
+                       [ 'A bad code' ],
+                       [ 'Project:A_page_title' ],
+                       [ "WTF\nnewlines" ],
                ];
        }