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.
   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 ===
 * …
 
 === 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;
        }
 
                $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
         *
        /**
         * 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] ) ) {
                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;
                        }
                }
                                return false;
                        }
                }


@@ -1025,6 +1029,7 @@ class ApiMain extends ApiBase {
                } else {
                        // Something is seriously wrong
                        $config = $this->getConfig();
                } 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' ) ) {
                        $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 ) {
                // 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 ) {
                        $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(
                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"
                        );
                                "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',
 
                        $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;
                        }
                        } 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 ) {
                }
                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\"" );
                }
 
                        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();
 
                        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';
                $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' ) );
 
                $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,
                return [
                        [
                                $ex,


@@ -1055,6 +1071,24 @@ class ApiMainTest extends ApiTestCase {
                                        'servedby' => wfHostname(),
                                ]
                        ],
                                        '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' ],
                        [
                                $apiEx1,
                                [ 'existing-error', 'sv-error1', 'sv-error2' ],


@@ -1075,6 +1109,23 @@ class ApiMainTest extends ApiTestCase {
                                        'servedby' => wfHostname(),
                                ]
                        ],
                                        '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() );
 
                $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() );
                // BC case
                $msg = new ApiMessage( 'actionthrottledtext' );
                $this->assertSame( 'ratelimited', $msg->getApiCode() );


@@ -72,6 +76,9 @@ class ApiMessageTest extends MediaWikiTestCase {
                return [
                        [ '' ],
                        [ 42 ],
                return [
                        [ '' ],
                        [ 42 ],

+                       [ 'A bad code' ],
+                       [ 'Project:A_page_title' ],
+                       [ "WTF\nnewlines" ],

                ];
        }
 
                ];
        }