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
2020-03-12 13:54:51 +01:00 · 2020-03-12 13:54:51 +01:00 · 905f6fc890
commit 905f6fc890
parent c6ac92433e
8 changed files with 282 additions and 29 deletions
--- 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
 	 */
--- 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(
--- 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."
 }
--- 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"
 }
--- 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;

--- 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
 	 */
--- 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',
--- 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;
+	}
+
 }