From 4eace785e66d199cb8fe1ec224bdc49831949a6d Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 7 Nov 2018 11:25:40 -0500 Subject: [PATCH] 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 --- RELEASE-NOTES-1.33 | 4 ++ includes/api/ApiErrorFormatter.php | 20 ++++++++ includes/api/ApiMain.php | 24 +++++++-- includes/api/ApiMessageTrait.php | 5 +- .../includes/api/ApiErrorFormatterTest.php | 19 +++++++ tests/phpunit/includes/api/ApiMainTest.php | 51 +++++++++++++++++++ tests/phpunit/includes/api/ApiMessageTest.php | 7 +++ 7 files changed, 126 insertions(+), 4 deletions(-) diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index 1021cb382f..422bbfa621 100644 --- 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 847afd8634..ef28b73715 100644 --- 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 22232dd753..162f0ce152 100644 --- 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[''] = 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 18b6bc42c1..6894d2809f 100644 --- 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 d11e3143ea..65a511d85b 100644 --- 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 a6083cda89..3299e73dc9 100644 --- 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', '' ], + [ + '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 <https://lists.wikimedia.org/mailman/listinfo/mediawiki-api-announce> " . + "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 c6f5a8e791..70114c2593 100644 --- 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( " 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" ], ]; } -- 2.20.1