diff --git a/includes/Rest/Handler/ActionModuleBasedHandler.php b/includes/Rest/Handler/ActionModuleBasedHandler.php index 9fcff7ece1c..9b7a53952b6 100644 --- a/includes/Rest/Handler/ActionModuleBasedHandler.php +++ b/includes/Rest/Handler/ActionModuleBasedHandler.php @@ -12,6 +12,7 @@ use MediaWiki\Rest\Handler; use MediaWiki\Rest\HttpException; use MediaWiki\Rest\LocalizedHttpException; use MediaWiki\Rest\Response; +use MediaWiki\Session\Session; use RequestContext; use WebResponse; use Wikimedia\Message\ListParam; @@ -25,6 +26,11 @@ use Wikimedia\Message\ScalarParam; */ abstract class ActionModuleBasedHandler extends Handler { + /** + * @var Session|null + */ + private $session = null; + /** * @var ApiMain|null */ @@ -34,6 +40,26 @@ abstract class ActionModuleBasedHandler extends Handler { return $this->getApiMain()->getUser(); } + /** + * @return Session + */ + protected function getSession() { + if ( !$this->session ) { + $this->session = $this->getApiMain()->getRequest()->getSession(); + } + + return $this->session; + } + + /** + * Set main action API entry point for testing. + * + * @param ApiMain $apiMain + */ + public function setApiMain( ApiMain $apiMain ) { + $this->apiMain = $apiMain; + } + /** * @return ApiMain */ diff --git a/includes/Rest/Handler/EditHandler.php b/includes/Rest/Handler/EditHandler.php index 431c205a537..f0d0fb0aa04 100644 --- a/includes/Rest/Handler/EditHandler.php +++ b/includes/Rest/Handler/EditHandler.php @@ -164,10 +164,21 @@ abstract class EditHandler extends ActionModuleBasedHandler { * @return string */ protected function getActionModuleToken() { - // TODO: if the request is known to be safe, return $this->getUser()->getEditToken(); - $body = $this->getValidatedBody(); - return $body['token'] ?? ''; + + if ( $this->getSession()->getProvider()->safeAgainstCsrf() ) { + if ( !empty( $body['token'] ) ) { + throw new LocalizedHttpException( + new MessageValue( 'rest-extraneous-csrf-token' ), + 400 + ); + } + + // Since the session is safe against CSRF, just use a known-good token. + return $this->getUser()->getEditToken(); + } else { + return $body['token'] ?? ''; + } } protected function mapActionModuleResponse( diff --git a/includes/Rest/i18n/en.json b/includes/Rest/i18n/en.json index 8838efb0ed5..a9cd4af9b75 100644 --- a/includes/Rest/i18n/en.json +++ b/includes/Rest/i18n/en.json @@ -35,5 +35,6 @@ "rest-missing-body-field": "Mandatory field `$1` missing from request body.", "rest-bad-content-model": "Bad content model: $1", "rest-update-cannot-create-page": "The page `$1` cannot be created since it already exists. To update the existing page, provide the base revision ID in the structure under `latest` key in the request body.", + "rest-extraneous-csrf-token": "Extraneous CSRF token found. CSRF tokens must not be used when using authentication mechanisms such as OAuth that are safe against CSRF attacks.", "rest-cannot-load-file": "The file for title `$1` cannot be loaded." } diff --git a/includes/Rest/i18n/qqq.json b/includes/Rest/i18n/qqq.json index bcfdcf22bf6..8902a3e922b 100644 --- a/includes/Rest/i18n/qqq.json +++ b/includes/Rest/i18n/qqq.json @@ -36,5 +36,6 @@ "rest-missing-body-field": "Error message for REST API debugging, shown when there is a mandatory field missing from the request body. Parameters:\n* $1: The field name", "rest-bad-content-model": "Error message for REST API debugging, shown when an unknown content model is specified. Parameters:\n* $1: The content model name", "rest-update-cannot-create-page": "Error message for REST API debugging, shown when creation of a page was requested via a PUT with no base revision ID, but the page already exists. Parameters:\n* $1: The title of the page", + "rest-extraneous-csrf-token": "Error message for REST API debugging, shown when an CSRF token was provided by the client, even though the authentication mechanisms used is safe against CSRF attacks.", "rest-cannot-load-file": "Error message for REST API debugging, shown when a media file could not be loaded. Parameters:\n* $1: The title of the page" } diff --git a/includes/session/Session.php b/includes/session/Session.php index 9bd329cab08..45e8b80aa15 100644 --- a/includes/session/Session.php +++ b/includes/session/Session.php @@ -45,7 +45,7 @@ use WebRequest; * @ingroup Session * @since 1.27 */ -final class Session implements \Countable, \Iterator, \ArrayAccess { +class Session implements \Countable, \Iterator, \ArrayAccess { /** @var null|string[] Encryption algorithm to use */ private static $encryptionAlgorithm = null; diff --git a/tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php b/tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php index 51584d4aceb..d32f593146b 100644 --- a/tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php +++ b/tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php @@ -28,7 +28,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { use ActionModuleBasedHandlerTestTrait; - private function newHandler( $resultData, $throwException = null ) { + private function newHandler( $resultData, $throwException = null, $csrfSafe = false ) { $config = new HashConfig( [ 'RightsUrl' => 'https://creativecommons.org/licenses/by-sa/4.0/', 'RightsText' => 'CC-BY-SA 4.0' @@ -82,17 +82,22 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { $titleCodec, $revisionLookup ); + + $apiMain = $this->getApiMain( $csrfSafe ); + $dummyModule = $this->getDummyApiModule( $apiMain, 'edit', $resultData, $throwException ); + + $handler->setApiMain( $apiMain ); $handler->overrideActionModule( 'edit', 'action', - $this->getDummyApiModule( $handler->getApiMain(), 'edit', $resultData, $throwException ) + $dummyModule ); return $handler; } public function provideExecute() { - yield "create" => [ + yield "create with token" => [ [ // Request data received by CreationHandler 'method' => 'POST', 'headers' => [ @@ -137,7 +142,8 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { 'title' => 'CC-BY-SA 4.0' ], 'source' => 'Content of revision 371707' - ] + ], + false ]; yield "create with model" => [ @@ -147,7 +153,6 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { 'Content-Type' => 'application/json', ], 'bodyContents' => json_encode( [ - 'token' => 'TOKEN', 'title' => 'Foo', 'source' => 'Lorem Ipsum', 'comment' => 'Testing', @@ -160,6 +165,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { 'summary' => 'Testing', 'contentmodel' => 'wikitext', 'createonly' => '1', + 'token' => '+\\', ], [ // Mock response returned by ApiEditPage "edit" => [ @@ -187,7 +193,59 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { 'title' => 'CC-BY-SA 4.0' ], 'source' => 'Content of revision 371707' - ] + ], + true + ]; + + yield "create without token" => [ + [ // Request data received by CreationHandler + 'method' => 'POST', + 'headers' => [ + 'Content-Type' => 'application/json', + ], + 'bodyContents' => json_encode( [ + 'title' => 'Foo', + 'source' => 'Lorem Ipsum', + 'comment' => 'Testing', + 'content_model' => CONTENT_MODEL_WIKITEXT, + ] ), + ], + [ // Fake request expected to be passed into ApiEditPage + 'title' => 'Foo', + 'text' => 'Lorem Ipsum', + 'summary' => 'Testing', + 'contentmodel' => 'wikitext', + 'createonly' => '1', + 'token' => '+\\', // use known-good token for current user (anon) + ], + [ // Mock response returned by ApiEditPage + "edit" => [ + "new" => true, + "result" => "Success", + "pageid" => 94542, + "title" => "Sandbox", + "contentmodel" => "wikitext", + "oldrevid" => 0, + "newrevid" => 371707, + "newtimestamp" => "2018-12-18T16:59:42Z", + ] + ], + [ // Response expected to be generated by CreationHandler + 'id' => 94542, + 'title' => 'ns:0:Foo', + 'key' => 'ns:0:Foo', + 'content_model' => 'wikitext', + 'latest' => [ + 'id' => 371707, + 'timestamp' => "2018-12-18T16:59:42Z" + ], + 'license' => [ + 'url' => 'https://creativecommons.org/licenses/by-sa/4.0/', + 'title' => 'CC-BY-SA 4.0' + ], + 'source' => 'Content of revision 371707' + ], + true ]; } @@ -198,11 +256,12 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { $requestData, $expectedActionParams, $actionResult, - $expectedResponse + $expectedResponse, + $csrfSafe ) { $request = new RequestData( $requestData ); - $handler = $this->newHandler( $actionResult ); + $handler = $this->newHandler( $actionResult, null, $csrfSafe ); $response = $this->executeHandler( $handler, $request ); @@ -320,6 +379,35 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase { ]; } + public function testBodyValidation_extraneousToken() { + $requestData = [ + 'method' => 'POST', + 'pathParams' => [ 'title' => 'Foo' ], + 'headers' => [ + 'Content-Type' => 'application/json', + ], + 'bodyContents' => json_encode( [ + 'title' => 'Foo', + 'token' => 'TOKEN', + 'comment' => 'Testing', + 'source' => 'Lorem Ipsum', + 'content_model' => 'wikitext' + ] ), + ]; + + $request = new RequestData( $requestData ); + + $handler = $this->newHandler( [], null, true ); + + $exception = $this->executeHandlerAndGetHttpException( $handler, $request ); + + $this->assertSame( 400, $exception->getCode(), 'HTTP status' ); + $this->assertInstanceOf( LocalizedHttpException::class, $exception ); + + $expectedMessage = new MessageValue( 'rest-extraneous-csrf-token' ); + $this->assertEquals( $expectedMessage, $exception->getMessageValue() ); + } + /** * @dataProvider provideHeaderValidation */ diff --git a/tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php b/tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php index cdded8f37bb..a7d76d68074 100644 --- a/tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php +++ b/tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php @@ -31,7 +31,7 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { use ActionModuleBasedHandlerTestTrait; - private function newHandler( $resultData, $throwException = null ) { + private function newHandler( $resultData, $throwException = null, $csrfSafe = false ) { $config = new HashConfig( [ 'RightsUrl' => 'https://creativecommons.org/licenses/by-sa/4.0/', 'RightsText' => 'CC-BY-SA 4.0' @@ -102,19 +102,24 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { $titleCodec, $revisionLookup ); + + $apiMain = $this->getApiMain( $csrfSafe ); + $dummyModule = $this->getDummyApiModule( $apiMain, 'edit', $resultData, $throwException ); + + $handler->setApiMain( $apiMain ); $handler->overrideActionModule( 'edit', 'action', - $this->getDummyApiModule( $handler->getApiMain(), 'edit', $resultData, $throwException ) + $dummyModule ); return $handler; } public function provideExecute() { - yield "create" => [ + yield "create with token" => [ [ // Request data received by UpdateHandler - 'method' => 'POST', + 'method' => 'PUT', 'pathParams' => [ 'title' => 'Foo' ], 'headers' => [ 'Content-Type' => 'application/json', @@ -158,7 +163,8 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { 'title' => 'CC-BY-SA 4.0' ], 'source' => 'Content of revision 371707' - ] + ], + false ]; yield "create with model" => [ @@ -209,12 +215,13 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { 'title' => 'CC-BY-SA 4.0' ], 'source' => 'Content of revision 371707' - ] + ], + false ]; - yield "update" => [ + yield "update with token" => [ [ // Request data received by UpdateHandler - 'method' => 'POST', + 'method' => 'PUT', 'pathParams' => [ 'title' => 'foo bar' ], 'headers' => [ 'Content-Type' => 'application/json', @@ -259,7 +266,8 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { 'title' => 'CC-BY-SA 4.0' ], 'source' => 'Content of revision 371707' - ] + ], + false ]; yield "update with model" => [ @@ -270,7 +278,6 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { 'Content-Type' => 'application/json', ], 'bodyContents' => json_encode( [ - 'token' => 'TOKEN', 'source' => 'Lorem Ipsum', 'comment' => 'Testing', 'content_model' => CONTENT_MODEL_WIKITEXT, @@ -284,7 +291,7 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { 'contentmodel' => 'wikitext', 'nocreate' => '1', 'baserevid' => '789123', - 'token' => 'TOKEN', + 'token' => '+\\', ], [ // Mock response returned by ApiEditPage "edit" => [ @@ -311,7 +318,57 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { 'title' => 'CC-BY-SA 4.0' ], 'source' => 'Content of revision 371707' - ] + ], + true + ]; + + yield "update without token" => [ + [ // Request data received by UpdateHandler + 'method' => 'PUT', + 'pathParams' => [ 'title' => 'Foo' ], + 'headers' => [ + 'Content-Type' => 'application/json', + ], + 'bodyContents' => json_encode( [ + 'source' => 'Lorem Ipsum', + 'comment' => 'Testing', + 'content_model' => CONTENT_MODEL_WIKITEXT, + 'latest' => [ 'id' => 789123 ], + ] ), + ], + [ // Fake request expected to be passed into ApiEditPage + 'title' => 'Foo', + 'text' => 'Lorem Ipsum', + 'summary' => 'Testing', + 'contentmodel' => 'wikitext', + 'nocreate' => '1', + 'baserevid' => '789123', + 'token' => '+\\', // use known-good token for current user (anon) + ], + [ // Mock response returned by ApiEditPage + "edit" => [ + "result" => "Success", + "pageid" => 94542, + "title" => "Sandbox", + "contentmodel" => "wikitext", + "oldrevid" => 371705, + "newrevid" => 371707, + "newtimestamp" => "2018-12-18T16:59:42Z", + ] + ], + [ // Response expected to be generated by UpdateHandler + 'id' => 94542, + 'content_model' => 'wikitext', + 'latest' => [ + 'id' => 371707, + 'timestamp' => "2018-12-18T16:59:42Z" + ], + 'license' => [ + 'url' => 'https://creativecommons.org/licenses/by-sa/4.0/', + 'title' => 'CC-BY-SA 4.0' + ], + ], + true ]; } @@ -322,11 +379,12 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { $requestData, $expectedActionParams, $actionResult, - $expectedResponse + $expectedResponse, + $csrfSafe ) { $request = new RequestData( $requestData ); - $handler = $this->newHandler( $actionResult ); + $handler = $this->newHandler( $actionResult, null, $csrfSafe ); $responseData = $this->executeHandlerAndGetBodyData( $handler, $request ); @@ -353,7 +411,7 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { public function provideBodyValidation() { yield "missing source field" => [ [ // Request data received by UpdateHandler - 'method' => 'POST', + 'method' => 'PUT', 'pathParams' => [ 'title' => 'Foo' ], 'headers' => [ 'Content-Type' => 'application/json', @@ -368,7 +426,7 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { ]; yield "missing comment field" => [ [ // Request data received by UpdateHandler - 'method' => 'POST', + 'method' => 'PUT', 'pathParams' => [ 'title' => 'Foo' ], 'headers' => [ 'Content-Type' => 'application/json', @@ -400,10 +458,38 @@ class UpdateHandlerTest extends MediaWikiIntegrationTestCase { $this->assertEquals( $expectedMessage, $exception->getMessageValue() ); } + public function testBodyValidation_extraneousToken() { + $requestData = [ + 'method' => 'PUT', + 'pathParams' => [ 'title' => 'Foo' ], + 'headers' => [ + 'Content-Type' => 'application/json', + ], + 'bodyContents' => json_encode( [ + 'token' => 'TOKEN', + 'comment' => 'Testing', + 'source' => 'Lorem Ipsum', + 'content_model' => 'wikitext' + ] ), + ]; + + $request = new RequestData( $requestData ); + + $handler = $this->newHandler( [], null, true ); + + $exception = $this->executeHandlerAndGetHttpException( $handler, $request ); + + $this->assertSame( 400, $exception->getCode(), 'HTTP status' ); + $this->assertInstanceOf( LocalizedHttpException::class, $exception ); + + $expectedMessage = new MessageValue( 'rest-extraneous-csrf-token' ); + $this->assertEquals( $expectedMessage, $exception->getMessageValue() ); + } + public function provideHeaderValidation() { yield "bad content type" => [ [ // Request data received by UpdateHandler - 'method' => 'POST', + 'method' => 'PUT', 'pathParams' => [ 'title' => 'Foo' ], 'headers' => [ 'Content-Type' => 'text/plain', diff --git a/tests/phpunit/unit/includes/Rest/Handler/ActionModuleBasedHandlerTestTrait.php b/tests/phpunit/unit/includes/Rest/Handler/ActionModuleBasedHandlerTestTrait.php index da3aaf5d350..455c08344fc 100644 --- a/tests/phpunit/unit/includes/Rest/Handler/ActionModuleBasedHandlerTestTrait.php +++ b/tests/phpunit/unit/includes/Rest/Handler/ActionModuleBasedHandlerTestTrait.php @@ -5,8 +5,13 @@ namespace MediaWiki\Tests\Rest\Handler; use ApiBase; use ApiMain; use Exception; +use FauxRequest; +use MediaWiki\Session\Session; +use MediaWiki\Session\SessionId; +use MediaWiki\Session\SessionProviderInterface; use PHPUnit\Framework\MockObject\MockBuilder; use PHPUnit\Framework\MockObject\MockObject; +use RequestContext; /** * A trait providing utility functions for testing Handler classes @@ -66,4 +71,39 @@ trait ActionModuleBasedHandlerTestTrait { return $module; } + /** + * @return ApiMain + */ + private function getApiMain( $csrfSafe = false ) { + /** @var SessionProviderInterface|MockObject $session */ + $sessionProvider = + $this->createNoOpMock( SessionProviderInterface::class, [ 'safeAgainstCsrf' ] ); + $sessionProvider->method( 'safeAgainstCsrf' )->willReturn( $csrfSafe ); + + /** @var Session|MockObject $session */ + $session = $this->createNoOpMock( Session::class, [ 'getSessionId', 'getProvider' ] ); + $session->method( 'getSessionId' )->willReturn( new SessionId( 'test' ) ); + $session->method( 'getProvider' )->willReturn( $sessionProvider ); + + // NOTE: This being a FauxRequest instance triggers special case behavior + // in ApiMain, causing ApiMain::isInternalMode() to return true. Among other things, + // this causes ApiMain to throw errors rather than encode them in the result data. + /** @var MockObject|FauxRequest $fauxRequest */ + $fauxRequest = $this->getMockBuilder( FauxRequest::class ) + ->onlyMethods( [ 'getSession', 'getSessionId' ] ) + ->getMock(); + $fauxRequest->method( 'getSession' )->willReturn( $session ); + $fauxRequest->method( 'getSessionId' )->willReturn( $session->getSessionId() ); + + $testContext = RequestContext::getMain(); + + $fauxContext = new RequestContext(); + $fauxContext->setRequest( $fauxRequest ); + $fauxContext->setUser( $testContext->getUser() ); + $fauxContext->setLanguage( $testContext->getLanguage() ); + + $apiMain = new ApiMain( $fauxContext, true ); + return $apiMain; + } + }