From 13acba25a05c39dd5709477e9ebabfbf98804107 Mon Sep 17 00:00:00 2001 From: daniel Date: Fri, 2 Jul 2021 13:12:00 +0200 Subject: [PATCH] REST: gracefully handle all exceptions. ResponseFactory::createFromException already had support for arbitrary exceptions, but Router was so far only using it for HttpExceptions, leaving other kinds of exceptions uncaught. In addition to catching all exceptions and generating an appropriate JSON response for them, this patch introduces the ErrorReporter interface, with an MWErrorReporter implementation which calls MWExceptionHandler::rollbackMasterChangesAndLog(). This is how uncaught errors are handled for requests coming in via api.php, so it seems appropriate to use the same approach for requests coming in via rest.php. Bug: T285984 Change-Id: I0605a7693821ef58fac80ab67f51a742556a37fd --- includes/Rest/EntryPoint.php | 2 + includes/Rest/Reporter/ErrorReporter.php | 24 +++++++++++ includes/Rest/Reporter/MWErrorReporter.php | 29 +++++++++++++ includes/Rest/Reporter/PHPErrorReporter.php | 35 +++++++++++++++ includes/Rest/ResponseFactory.php | 7 +-- includes/Rest/Router.php | 11 +++++ .../phpunit/includes/Rest/EntryPointTest.php | 2 + .../MWBasicRequestAuthorizerTest.php | 2 + .../includes/Rest/ResponseFactoryTest.php | 2 +- .../phpunit/unit/includes/Rest/RouterTest.php | 43 ++++++++++++++++++- .../unit/includes/Rest/testRoutes.json | 4 ++ 11 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 includes/Rest/Reporter/ErrorReporter.php create mode 100644 includes/Rest/Reporter/MWErrorReporter.php create mode 100644 includes/Rest/Reporter/PHPErrorReporter.php diff --git a/includes/Rest/EntryPoint.php b/includes/Rest/EntryPoint.php index 97f5d39ff6a..145282e84d2 100644 --- a/includes/Rest/EntryPoint.php +++ b/includes/Rest/EntryPoint.php @@ -9,6 +9,7 @@ use MediaWiki\Config\ServiceOptions; use MediaWiki\MediaWikiServices; use MediaWiki\Rest\BasicAccess\CompoundAuthorizer; use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer; +use MediaWiki\Rest\Reporter\MWErrorReporter; use MediaWiki\Rest\Validator\Validator; use RequestContext; use Title; @@ -75,6 +76,7 @@ class EntryPoint { $authority, $objectFactory, $restValidator, + new MWErrorReporter(), $services->getHookContainer() ) )->setCors( $cors ); } diff --git a/includes/Rest/Reporter/ErrorReporter.php b/includes/Rest/Reporter/ErrorReporter.php new file mode 100644 index 00000000000..55cf1fa4c0d --- /dev/null +++ b/includes/Rest/Reporter/ErrorReporter.php @@ -0,0 +1,24 @@ +level = $level; + } + + /** + * @param Throwable $error + * @param Handler $handler + * @param RequestInterface $request + */ + public function reportError( Throwable $error, Handler $handler, RequestInterface $request ) { + $firstLine = preg_split( '#$#m', (string)$error, 0 )[0]; + trigger_error( $firstLine, $this->level ); + } + +} diff --git a/includes/Rest/ResponseFactory.php b/includes/Rest/ResponseFactory.php index 22baa54bb46..7658eb68660 100644 --- a/includes/Rest/ResponseFactory.php +++ b/includes/Rest/ResponseFactory.php @@ -231,11 +231,12 @@ class ResponseFactory { } } else { $response = $this->createHttpError( 500, [ - 'message' => 'Error: exception of type ' . get_class( $exception ), + 'message' => 'Error: exception of type ' . get_class( $exception ) . ': ' + . $exception->getMessage(), 'exception' => MWExceptionHandler::getStructuredExceptionData( $exception ) ] ); - // FIXME should we try to do something useful with ILocalizedException? - // FIXME should we try to do something useful with common MediaWiki errors like ReadOnlyError? + // XXX: should we try to do something useful with ILocalizedException? + // XXX: should we try to do something useful with common MediaWiki errors like ReadOnlyError? } return $response; } diff --git a/includes/Rest/Router.php b/includes/Rest/Router.php index fae9f50e694..a89fdc35e04 100644 --- a/includes/Rest/Router.php +++ b/includes/Rest/Router.php @@ -8,7 +8,9 @@ use MediaWiki\HookContainer\HookContainer; use MediaWiki\Permissions\Authority; use MediaWiki\Rest\BasicAccess\BasicAuthorizerInterface; use MediaWiki\Rest\PathTemplateMatcher\PathMatcher; +use MediaWiki\Rest\Reporter\ErrorReporter; use MediaWiki\Rest\Validator\Validator; +use Throwable; use Wikimedia\Message\MessageValue; use Wikimedia\ObjectFactory; @@ -63,6 +65,9 @@ class Router { /** @var CorsUtils|null */ private $cors; + /** @var ErrorReporter */ + private $errorReporter; + /** @var HookContainer */ private $hookContainer; @@ -77,6 +82,7 @@ class Router { * @param Authority $authority * @param ObjectFactory $objectFactory * @param Validator $restValidator + * @param ErrorReporter $errorReporter * @param HookContainer $hookContainer * @internal */ @@ -91,6 +97,7 @@ class Router { Authority $authority, ObjectFactory $objectFactory, Validator $restValidator, + ErrorReporter $errorReporter, HookContainer $hookContainer ) { $this->routeFiles = $routeFiles; @@ -103,6 +110,7 @@ class Router { $this->authority = $authority; $this->objectFactory = $objectFactory; $this->restValidator = $restValidator; + $this->errorReporter = $errorReporter; $this->hookContainer = $hookContainer; } @@ -330,6 +338,9 @@ class Router { return $this->executeHandler( $handler ); } catch ( HttpException $e ) { return $this->responseFactory->createFromException( $e ); + } catch ( Throwable $e ) { + $this->errorReporter->reportError( $e, $handler, $request ); + return $this->responseFactory->createFromException( $e ); } } diff --git a/tests/phpunit/includes/Rest/EntryPointTest.php b/tests/phpunit/includes/Rest/EntryPointTest.php index a8f91207f93..ca199d16cc9 100644 --- a/tests/phpunit/includes/Rest/EntryPointTest.php +++ b/tests/phpunit/includes/Rest/EntryPointTest.php @@ -10,6 +10,7 @@ use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer; use MediaWiki\Rest\CorsUtils; use MediaWiki\Rest\EntryPoint; use MediaWiki\Rest\Handler; +use MediaWiki\Rest\Reporter\PHPErrorReporter; use MediaWiki\Rest\RequestData; use MediaWiki\Rest\RequestInterface; use MediaWiki\Rest\ResponseFactory; @@ -47,6 +48,7 @@ class EntryPointTest extends \MediaWikiIntegrationTestCase { $authority, $objectFactory, new Validator( $objectFactory, $request, $authority ), + new PHPErrorReporter(), $this->createHookContainer() ); } diff --git a/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php b/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php index 3e38b2601ea..ec615a97419 100644 --- a/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php +++ b/tests/phpunit/unit/includes/Rest/BasicAccess/MWBasicRequestAuthorizerTest.php @@ -6,6 +6,7 @@ use GuzzleHttp\Psr7\Uri; use MediaWiki\Permissions\SimpleAuthority; use MediaWiki\Rest\BasicAccess\MWBasicAuthorizer; use MediaWiki\Rest\Handler; +use MediaWiki\Rest\Reporter\PHPErrorReporter; use MediaWiki\Rest\RequestData; use MediaWiki\Rest\ResponseFactory; use MediaWiki\Rest\Router; @@ -41,6 +42,7 @@ class MWBasicRequestAuthorizerTest extends MediaWikiUnitTestCase { $authority, $objectFactory, new Validator( $objectFactory, $request, $authority ), + new PHPErrorReporter(), $this->createHookContainer() ); } diff --git a/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php b/tests/phpunit/unit/includes/Rest/ResponseFactoryTest.php index bc84b9510a1..18baae57583 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', $data['message'] ); + $this->assertSame( 'Error: exception of type Exception: hello', $data['message'] ); } public function testCreateFromExceptionWrapped() { diff --git a/tests/phpunit/unit/includes/Rest/RouterTest.php b/tests/phpunit/unit/includes/Rest/RouterTest.php index 2c26ff0b1bb..5c4f08b7a75 100644 --- a/tests/phpunit/unit/includes/Rest/RouterTest.php +++ b/tests/phpunit/unit/includes/Rest/RouterTest.php @@ -7,6 +7,7 @@ use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer; use MediaWiki\Rest\Handler; use MediaWiki\Rest\HttpException; use MediaWiki\Rest\RedirectException; +use MediaWiki\Rest\Reporter\ErrorReporter; use MediaWiki\Rest\RequestData; use MediaWiki\Rest\RequestInterface; use MediaWiki\Rest\ResponseException; @@ -14,7 +15,10 @@ use MediaWiki\Rest\ResponseFactory; use MediaWiki\Rest\Router; use MediaWiki\Rest\Validator\Validator; use MediaWiki\Tests\Unit\Permissions\MockAuthorityTrait; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Container\ContainerInterface; +use RuntimeException; +use Throwable; use Wikimedia\ObjectFactory; /** @@ -23,6 +27,9 @@ use Wikimedia\ObjectFactory; class RouterTest extends \MediaWikiUnitTestCase { use MockAuthorityTrait; + /** @var Throwable[] */ + private $reportedErrors = []; + /** * @param RequestInterface $request * @param string|null $authError @@ -39,6 +46,14 @@ class RouterTest extends \MediaWikiUnitTestCase { ); $routeFiles = array_merge( [ __DIR__ . '/testRoutes.json' ], $additionalRouteFiles ); $authority = $this->mockAnonUltimateAuthority(); + + /** @var MockObject|ErrorReporter $mockErrorReporter */ + $mockErrorReporter = $this->createNoOpMock( ErrorReporter::class, [ 'reportError' ] ); + $mockErrorReporter->method( 'reportError' ) + ->willReturnCallback( function ( $e ) { + $this->reportedErrors[] = $e; + } ); + return new Router( $routeFiles, [], @@ -50,6 +65,7 @@ class RouterTest extends \MediaWikiUnitTestCase { $authority, $objectFactory, new Validator( $objectFactory, $request, $authority ), + $mockErrorReporter, $this->createHookContainer() ); } @@ -99,6 +115,14 @@ class RouterTest extends \MediaWikiUnitTestCase { }; } + public static function fatalHandlerFactory() { + return new class extends Handler { + public function execute() { + throw new RuntimeException( 'Fatal mock error', 12345 ); + } + }; + } + public static function throwRedirectHandlerFactory() { return new class extends Handler { public function execute() { @@ -117,7 +141,7 @@ class RouterTest extends \MediaWikiUnitTestCase { }; } - public function testException() { + public function testHttpException() { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throw' ) ] ); $router = $this->createRouter( $request ); $response = $router->execute( $request ); @@ -128,6 +152,23 @@ class RouterTest extends \MediaWikiUnitTestCase { $this->assertSame( 'Mock error', $data['message'] ); } + public function testFatalException() { + $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/fatal' ) ] ); + $router = $this->createRouter( $request ); + $response = $router->execute( $request ); + $this->assertSame( 500, $response->getStatusCode() ); + $body = $response->getBody(); + $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] ); + } + public function testRedirectException() { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/RouterTest/throwRedirect' ) ] ); $router = $this->createRouter( $request ); diff --git a/tests/phpunit/unit/includes/Rest/testRoutes.json b/tests/phpunit/unit/includes/Rest/testRoutes.json index 767d204833b..5a704a4ccc7 100644 --- a/tests/phpunit/unit/includes/Rest/testRoutes.json +++ b/tests/phpunit/unit/includes/Rest/testRoutes.json @@ -15,6 +15,10 @@ "path": "/mock/RouterTest/throw", "factory": "MediaWiki\\Tests\\Rest\\RouterTest::throwHandlerFactory" }, + { + "path": "/mock/RouterTest/fatal", + "factory": "MediaWiki\\Tests\\Rest\\RouterTest::fatalHandlerFactory" + }, { "path": "/mock/RouterTest/throwRedirect", "factory": "MediaWiki\\Tests\\Rest\\RouterTest::throwRedirectHandlerFactory"