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

@@ -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'

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,

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(

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;
 	}

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
 		);

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() {

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] );
 	}

tests/phpunit/unit/includes/exception/MWExceptionHandlerTest.php

@@ -1,6 +1,7 @@
 <?php
 
 use Wikimedia\NormalizedException\NormalizedException;
+use Wikimedia\TestingAccessWrapper;
 
 /**
  * @author Antoine Musso
@@ -21,6 +22,8 @@ class MWExceptionHandlerTest extends \MediaWikiUnitTestCase {
 	}
 
 	protected function tearDown(): void {
+		TestingAccessWrapper::newFromClass( MWExceptionHandler::class )
+			->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 );
 	}