From 905f6fc890f9ee2c1ab5900a72e62058b2021959 Mon Sep 17 00:00:00 2001 From: daniel Date: Thu, 12 Mar 2020 13:54:51 +0100 Subject: [PATCH] REST: page/ endpoints: don't use tokens with OAuth CSRF tokens should only be required (and only be allowed) if the current session isn't already inherently safe against CSRF due to the way the authentication mechanism works. This allows (and requires) tokens to be omitted for requests that use an OAuth Authorization header. Bug: T230843 Bug: T230842 Bug: T237852 Change-Id: Ib2922d556ff2470d4bf8c386c18986ca9f37d1b5 --- .../Rest/Handler/ActionModuleBasedHandler.php | 26 ++++ includes/Rest/Handler/EditHandler.php | 17 ++- includes/Rest/i18n/en.json | 1 + includes/Rest/i18n/qqq.json | 1 + includes/session/Session.php | 2 +- .../Rest/Handler/CreationHandlerTest.php | 104 +++++++++++++-- .../Rest/Handler/UpdateHandlerTest.php | 120 +++++++++++++++--- .../ActionModuleBasedHandlerTestTrait.php | 40 ++++++ 8 files changed, 282 insertions(+), 29 deletions(-) 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; + } + }