From 0bb1538929f09ba3302e52f7c399cf071ee15491 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 19 May 2022 14:11:51 +0100 Subject: [PATCH] REST: Hide exception message when wgShowExceptionDetails=false This reverts parts of I8520d8cb16 and Ib941c22d6b7e. The documentation of ShowExceptionDetails, as well as all other uses of it (e.g. MWExeceptionRenderer for index.php, API, and ResourceLoader) take it to mean to hide both exception message and details. This is why MWExceptionHandler didn't have, and didn't need, the added complexity of this as a parameter as this method simply wouldn't be called at all in that case. * Rename the method added in I8520d8cb16 to match the one in MWExceptionRenderer. * Update REST handling to now print any exception details when it is true. * Remove the now-unused code introduced in Ib941c22d6b7e. Change-Id: I1a9920dea0bafe315a20489efbe46ea9b55b0f74 --- docs/config-schema.yaml | 8 ++--- includes/MainConfigSchema.php | 9 +++--- includes/Rest/EntryPoint.php | 2 +- includes/Rest/ResponseFactory.php | 21 ++++++++----- includes/exception/MWExceptionHandler.php | 21 +++---------- .../includes/Rest/ResponseFactoryTest.php | 2 +- .../phpunit/unit/includes/Rest/RouterTest.php | 4 --- .../exception/MWExceptionHandlerTest.php | 31 ++++++------------- 8 files changed, 38 insertions(+), 60 deletions(-) diff --git a/docs/config-schema.yaml b/docs/config-schema.yaml index 4424b7f65b5..6c57e5be854 100644 --- a/docs/config-schema.yaml +++ b/docs/config-schema.yaml @@ -5701,10 +5701,10 @@ config-schema: ShowExceptionDetails: default: false description: |- - If set to true, uncaught exceptions will print the exception message and a - complete stack trace to output. This should only be used for debugging, as it - may reveal private information in function parameters due to PHP's backtrace - formatting. If set to false, only the exception's class will be shown. + Show exception message and stack trace when printing details about uncaught exceptions + in web response output. + This may reveal private information in error messages or function parameters. + If set to false, only the exception type or class name will be exposed. LogExceptionBacktrace: default: true description: 'If true, send the exception backtrace to the error log' diff --git a/includes/MainConfigSchema.php b/includes/MainConfigSchema.php index c224608c7a3..c349862e966 100644 --- a/includes/MainConfigSchema.php +++ b/includes/MainConfigSchema.php @@ -9036,10 +9036,11 @@ class MainConfigSchema { ]; /** - * If set to true, uncaught exceptions will print the exception message and a - * complete stack trace to output. This should only be used for debugging, as it - * may reveal private information in function parameters due to PHP's backtrace - * formatting. If set to false, only the exception's class will be shown. + * Show exception message and stack trace when printing details about uncaught exceptions + * in web response output. + * + * This may reveal private information in error messages or function parameters. + * If set to false, only the exception type or class name will be exposed. */ public const ShowExceptionDetails = [ 'default' => false, diff --git a/includes/Rest/EntryPoint.php b/includes/Rest/EntryPoint.php index 965e0de582e..08f0e1ed896 100644 --- a/includes/Rest/EntryPoint.php +++ b/includes/Rest/EntryPoint.php @@ -111,7 +111,7 @@ class EntryPoint { $conf = $services->getMainConfig(); $responseFactory = new ResponseFactory( self::getTextFormatters( $services ) ); - $responseFactory->setSendExceptionBacktrace( MWExceptionRenderer::shouldShowExceptionDetails() ); + $responseFactory->setShowExceptionDetails( MWExceptionRenderer::shouldShowExceptionDetails() ); $cors = new CorsUtils( new ServiceOptions( diff --git a/includes/Rest/ResponseFactory.php b/includes/Rest/ResponseFactory.php index d04da5df134..84b3e01508c 100644 --- a/includes/Rest/ResponseFactory.php +++ b/includes/Rest/ResponseFactory.php @@ -23,7 +23,7 @@ class ResponseFactory { private $textFormatters; /** @var bool Whether to send exception backtraces to the client */ - private $sendExceptionBacktrace = false; + private $showExceptionDetails = false; /** * @param ITextFormatter[] $textFormatters @@ -33,12 +33,14 @@ class ResponseFactory { } /** - * Controls whether error responses should include a backtrace + * Control whether web responses may include a exception messager and backtrace + * + * @see $wgShowExceptionDetails * @since 1.39 - * @param bool $sendExceptionBacktrace + * @param bool $showExceptionDetails */ - public function setSendExceptionBacktrace( bool $sendExceptionBacktrace ): void { - $this->sendExceptionBacktrace = $sendExceptionBacktrace; + public function setShowExceptionDetails( bool $showExceptionDetails ): void { + $this->showExceptionDetails = $showExceptionDetails; } /** @@ -241,18 +243,21 @@ class ResponseFactory { ) ); } - } else { + } elseif ( $this->showExceptionDetails ) { $response = $this->createHttpError( 500, [ 'message' => 'Error: exception of type ' . get_class( $exception ) . ': ' . $exception->getMessage(), 'exception' => MWExceptionHandler::getStructuredExceptionData( $exception, - MWExceptionHandler::CAUGHT_BY_OTHER, - $this->sendExceptionBacktrace + MWExceptionHandler::CAUGHT_BY_OTHER ) ] ); // XXX: should we try to do something useful with ILocalizedException? // XXX: should we try to do something useful with common MediaWiki errors like ReadOnlyError? + } else { + $response = $this->createHttpError( 500, [ + 'message' => 'Error: exception of type ' . get_class( $exception ), + ] ); } return $response; } diff --git a/includes/exception/MWExceptionHandler.php b/includes/exception/MWExceptionHandler.php index 355be7eede5..decd20a0fb5 100644 --- a/includes/exception/MWExceptionHandler.php +++ b/includes/exception/MWExceptionHandler.php @@ -605,22 +605,13 @@ TXT; * * @param Throwable $e * @param string $catcher CAUGHT_BY_* class constant indicating what caught the error - * @param bool|null $includeBacktrace Whether to include a backtrace. If null, - * the value of $includeBacktrace that was provided to installHandler() will be - * used. - * * @return array * @since 1.26 */ public static function getStructuredExceptionData( Throwable $e, - $catcher = self::CAUGHT_BY_OTHER, - ?bool $includeBacktrace = null + $catcher = self::CAUGHT_BY_OTHER ) { - if ( $includeBacktrace === null ) { - $includeBacktrace = self::$logExceptionBacktrace; - } - $data = [ 'id' => WebRequest::getRequestId(), 'type' => get_class( $e ), @@ -639,7 +630,7 @@ TXT; $data['suppressed'] = true; } - if ( $includeBacktrace ) { + if ( self::$logExceptionBacktrace ) { $data['backtrace'] = self::getRedactedTrace( $e ); } @@ -703,20 +694,16 @@ TXT; * @param bool $pretty Add non-significant whitespace to improve readability (default: false). * @param int $escaping Bitfield consisting of FormatJson::.*_OK class constants. * @param string $catcher CAUGHT_BY_* class constant indicating what caught the error - * @param bool|null $includeBacktrace Whether to include a backtrace. If null, - * the value of $includeBacktrace that was provided to installHandler() will be - * used. * @return string|false JSON string if successful; false upon failure */ public static function jsonSerializeException( Throwable $e, $pretty = false, $escaping = 0, - $catcher = self::CAUGHT_BY_OTHER, - ?bool $includeBacktrace = null + $catcher = self::CAUGHT_BY_OTHER ) { return FormatJson::encode( - self::getStructuredExceptionData( $e, $catcher, $includeBacktrace ), + self::getStructuredExceptionData( $e, $catcher ), $pretty, $escaping ); diff --git a/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php b/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php index 18baae57583..bc84b9510a1 100644 --- a/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php +++ b/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php @@ -162,7 +162,7 @@ class ResponseFactoryTest extends MediaWikiUnitTestCase { $body->rewind(); $data = json_decode( $body->getContents(), true ); $this->assertSame( 500, $data['httpCode'] ); - $this->assertSame( 'Error: exception of type Exception: hello', $data['message'] ); + $this->assertSame( 'Error: exception of type Exception', $data['message'] ); } public function testCreateFromExceptionWrapped() { diff --git a/tests/phpunit/unit/includes/Rest/RouterTest.php b/tests/phpunit/unit/includes/Rest/RouterTest.php index 5c69e5e52f0..492b148816b 100644 --- a/tests/phpunit/unit/includes/Rest/RouterTest.php +++ b/tests/phpunit/unit/includes/Rest/RouterTest.php @@ -161,10 +161,6 @@ class RouterTest extends \MediaWikiUnitTestCase { $body->rewind(); $data = json_decode( $body->getContents(), true ); $this->assertStringContainsString( 'RuntimeException', $data['message'] ); - $this->assertStringContainsString( 'Fatal mock error', $data['message'] ); - $this->assertSame( RuntimeException::class, $data['exception']['type'] ); - $this->assertSame( 12345, $data['exception']['code'] ); - $this->assertSame( 'Fatal mock error', $data['exception']['message'] ); $this->assertNotEmpty( $this->reportedErrors ); $this->assertInstanceOf( RuntimeException::class, $this->reportedErrors[0] ); } diff --git a/tests/phpunit/unit/includes/exception/MWExceptionHandlerTest.php b/tests/phpunit/unit/includes/exception/MWExceptionHandlerTest.php index 55349c40814..0074d5ffb10 100644 --- a/tests/phpunit/unit/includes/exception/MWExceptionHandlerTest.php +++ b/tests/phpunit/unit/includes/exception/MWExceptionHandlerTest.php @@ -1,6 +1,7 @@ logExceptionBacktrace = true; ini_set( 'zend.exception_ignore_args', $this->oldSettingValue ); parent::tearDown(); } @@ -151,13 +154,7 @@ TEXT; */ public function testJsonserializeexceptionKeys( $expectedKeyType, $exClass, $key ) { $json = json_decode( - MWExceptionHandler::jsonSerializeException( - new $exClass(), - true, - 0, - MWExceptionHandler::CAUGHT_BY_OTHER, - true - ) + MWExceptionHandler::jsonSerializeException( new $exClass() ) ); $this->assertObjectHasAttribute( $key, $json ); $this->assertSame( $expectedKeyType, gettype( $json->$key ), "Type of the '$key' key" ); @@ -185,14 +182,10 @@ TEXT; * @covers MWExceptionHandler::jsonSerializeException */ public function testJsonserializeexceptionBacktracingEnabled() { + TestingAccessWrapper::newFromClass( MWExceptionHandler::class ) + ->logExceptionBacktrace = true; $json = json_decode( - MWExceptionHandler::jsonSerializeException( - new Exception(), - true, - 0, - MWExceptionHandler::CAUGHT_BY_OTHER, - true - ) + MWExceptionHandler::jsonSerializeException( new Exception() ) ); $this->assertObjectHasAttribute( 'backtrace', $json ); } @@ -204,14 +197,10 @@ TEXT; * @covers MWExceptionHandler::jsonSerializeException */ public function testJsonserializeexceptionBacktracingDisabled() { + TestingAccessWrapper::newFromClass( MWExceptionHandler::class ) + ->logExceptionBacktrace = false; $json = json_decode( - MWExceptionHandler::jsonSerializeException( - new Exception(), - true, - 0, - MWExceptionHandler::CAUGHT_BY_OTHER, - false - ) + MWExceptionHandler::jsonSerializeException( new Exception() ) ); $this->assertObjectNotHasAttribute( 'backtrace', $json ); }