aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--RELEASE-NOTES-1.334
-rw-r--r--includes/api/ApiErrorFormatter.php20
-rw-r--r--includes/api/ApiMain.php24
-rw-r--r--includes/api/ApiMessageTrait.php5
-rw-r--r--tests/phpunit/includes/api/ApiErrorFormatterTest.php19
-rw-r--r--tests/phpunit/includes/api/ApiMainTest.php51
-rw-r--r--tests/phpunit/includes/api/ApiMessageTest.php7
7 files changed, 126 insertions, 4 deletions
diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33
index 1021cb382fc2..422bbfa621b5 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 847afd8634f3..ef28b73715f6 100644
--- a/includes/api/ApiErrorFormatter.php
+++ b/includes/api/ApiErrorFormatter.php
@@ -59,6 +59,26 @@ class ApiErrorFormatter {
}
/**
+ * 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
*
* @since 1.32
diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 22232dd7531b..162f0ce152a8 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['<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 18b6bc42c1d8..6894d2809f66 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 d11e3143ea01..65a511d85bec 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 a6083cda8977..3299e73dc9f2 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,
@@ -1056,6 +1072,24 @@ class ApiMainTest extends ApiTestCase {
]
],
[
+ $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 c6f5a8e791f8..70114c2593b6 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( "<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" ],
];
}
generated by cgit v1.2.3 (git 2.25.1) at 2025-04-27 04:21:52 +0000