REST /page/{title}: Fix title encoding.

Page titles used in URL paths, such as the Location header returned
after a page was created, must use the correct encoding for spaces and
pluses.

Bug: T258606
Change-Id: I75e91ac8f8da4eb183a9c8f1a682ea08c2225227

includes/Rest/Handler.php

@@ -94,6 +94,32 @@ abstract class Handler {
 		return $this->router->getRouteUrl( $path, $pathParams, $queryParams );
 	}
 
+	/**
+	 * URL-encode titles in a "pretty" way.
+	 *
+	 * Keeps intact ;@$!*(),~: (urlencode does not, but wfUrlencode does).
+	 * Encodes spaces as underscores (wfUrlencode does not).
+	 * Encodes slashes (wfUrlencode does not, but keeping them messes with REST pathes).
+	 * Encodes pluses (this is not necessary, and may change).
+	 *
+	 * @see wfUrlencode
+	 *
+	 * @param string $title
+	 *
+	 * @return string
+	 */
+	protected function urlEncodeTitle( $title ) {
+		$title = str_replace( ' ', '_', $title );
+		$title = urlencode( $title );
+
+		// %3B_a_%40_b_%24_c_%21_d_%2A_e_%28_f_%29_g_%2C_h_~_i_%3A
+		$replace = [ '%3B', '%40', '%24', '%21', '%2A', '%28', '%29', '%2C', '%7E', '%3A' ];
+		$with = [ ';', '@', '$', '!', '*', '(', ')', ',', '~', ':' ];
+		$title = str_replace( $replace, $with, $title );
+
+		return $title;
+	}
+
 	/**
 	 * Get the current request. The return type declaration causes it to raise
 	 * a fatal error if init() has not yet been called.

includes/Rest/Handler/CreationHandler.php

@@ -108,7 +108,8 @@ class CreationHandler extends EditHandler {
 			$response
 		);
 
-		$title = urlencode( $this->getTitleParameter() );
+		$title = $this->urlEncodeTitle( $actionModuleResult['edit']['title'] );
+
 		$url = $this->getRouter()->getRouteUrl( '/v1/page/' . $title );
 		$response->setHeader( 'Location', $url );
 	}

includes/Rest/Handler/EditHandler.php

@@ -93,7 +93,7 @@ abstract class EditHandler extends ActionModuleBasedHandler {
 			throw new HttpException( $data['edit']['result'], 409 );
 		}
 
-		$title = $this->titleParser->parseTitle( $this->getTitleParameter() );
+		$title = $this->titleParser->parseTitle( $data['edit']['title'] );
 
 		// This seems wasteful. This is the downside of delegating to the action API module:
 		// if we need additional data in the response, we have to load it.

tests/api-testing/REST/Creation.js

@@ -43,7 +43,15 @@ describe( 'POST /page', () => {
 
 	describe( 'successful operation', () => {
 		it( 'should create a page if it does not exist', async () => {
-			const title = utils.title( 'Edit Test ' );
+			const titleSuffix = utils.title();
+			const title = 'A B+C:D@E-' + titleSuffix;
+
+			// In "title style" encoding, spaces turn to underscores,
+			// colons are preserved, and slashes and pluses get encoded.
+			// FIXME: correct handling of encoded slashes depends on
+			//        the server setup and can't be tested reliably.
+			const encodedTitle = 'A_B%2BC:D@E-' + titleSuffix;
+
 			const reqBody = {
 				token: anonToken,
 				source: 'Lörem Ipsüm',
@@ -56,13 +64,12 @@ describe( 'POST /page', () => {
 			assert.equal( editStatus, 201 );
 
 			assert.nestedProperty( header, 'location' );
-			const encodedTitle = encodeURI( title );
-			const normalizedLocation = header.location.replace( /\+/g, '%20' );
-			assert.match( normalizedLocation, new RegExp( `^https?://.*/v1/page/${encodedTitle}$` ) );
+			const location = header.location;
+			assert.match( location, new RegExp( `^https?://.*/v1/page/${encodedTitle}$` ) );
 			checkEditResponse( title, reqBody, editBody );
 
 			// follow redirect
-			const { status: redirStatus, body: redirBody } = await supertest.agent( normalizedLocation ).get( '' );
+			const { status: redirStatus, body: redirBody } = await supertest.agent( location ).get( '' );
 			assert.equal( redirStatus, 200 );
 			checkSourceResponse( title, reqBody, redirBody );
 

tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php

@@ -42,6 +42,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 			->method( 'isDefinedModel' )
 			->willReturnMap( [
 				[ CONTENT_MODEL_WIKITEXT, true ],
+				[ CONTENT_MODEL_TEXT, true ],
 			] );
 
 		/** @var MockObject|MediaWikiTitleCodec $titleCodec */
@@ -97,6 +98,9 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 	}
 
 	public function provideExecute() {
+		// NOTE: Prefix hard coded in a fake for Router::getRouteUrl() in HandlerTestTrait
+		$baseUrl = 'https://wiki.example.com/rest/v1/page/';
+
 		yield "create with token" => [
 			[ // Request data received by CreationHandler
 				'method' => 'POST',
@@ -121,7 +125,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 					"new" => true,
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
+					"title" => "Foo",
 					"contentmodel" => "wikitext",
 					"oldrevid" => 0,
 					"newrevid" => 371707,
@@ -143,6 +147,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 				],
 				'source' => 'Content of revision 371707'
 			],
+			$baseUrl . 'Foo',
 			false
 		];
 
@@ -153,17 +158,17 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 					'Content-Type' => 'application/json',
 				],
 				'bodyContents' => json_encode( [
-					'title' => 'Foo',
+					'title' => 'Talk:Foo',
 					'source' => 'Lorem Ipsum',
 					'comment' => 'Testing',
-					'content_model' => CONTENT_MODEL_WIKITEXT,
+					'content_model' => CONTENT_MODEL_TEXT,
 				] ),
 			],
 			[ // Fake request expected to be passed into ApiEditPage
-				'title' => 'Foo',
+				'title' => 'Talk:Foo',
 				'text' => 'Lorem Ipsum',
 				'summary' => 'Testing',
-				'contentmodel' => 'wikitext',
+				'contentmodel' => CONTENT_MODEL_TEXT,
 				'createonly' => '1',
 				'token' => '+\\',
 			],
@@ -172,8 +177,8 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 					"new" => true,
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
-					"contentmodel" => "wikitext",
+					"title" => "Talk:Foo",
+					"contentmodel" => CONTENT_MODEL_TEXT,
 					"oldrevid" => 0,
 					"newrevid" => 371707,
 					"newtimestamp" => "2018-12-18T16:59:42Z",
@@ -181,9 +186,9 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 			],
 			[ // Response expected to be generated by CreationHandler
 				'id' => 94542,
-				'title' => 'ns:0:Foo',
-				'key' => 'ns:0:Foo',
-				'content_model' => 'wikitext',
+				'title' => 'ns:0:Talk:Foo', // our mock TitleCodec doesn't parse namespaces
+				'key' => 'ns:0:Talk:Foo',
+				'content_model' => CONTENT_MODEL_TEXT,
 				'latest' => [
 					'id' => 371707,
 					'timestamp' => "2018-12-18T16:59:42Z"
@@ -194,6 +199,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 				],
 				'source' => 'Content of revision 371707'
 			],
+			$baseUrl . 'Talk:Foo',
 			true
 		];
 
@@ -204,14 +210,14 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 					'Content-Type' => 'application/json',
 				],
 				'bodyContents' => json_encode( [
-					'title' => 'Foo',
+					'title' => 'foo/bar',
 					'source' => 'Lorem Ipsum',
 					'comment' => 'Testing',
 					'content_model' => CONTENT_MODEL_WIKITEXT,
 				] ),
 			],
 			[ // Fake request expected to be passed into ApiEditPage
-				'title' => 'Foo',
+				'title' => 'foo/bar',
 				'text' => 'Lorem Ipsum',
 				'summary' => 'Testing',
 				'contentmodel' => 'wikitext',
@@ -223,7 +229,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 					"new" => true,
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
+					"title" => "Foo/bar",
 					"contentmodel" => "wikitext",
 					"oldrevid" => 0,
 					"newrevid" => 371707,
@@ -232,8 +238,8 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 			],
 			[ // Response expected to be generated by CreationHandler
 				'id' => 94542,
-				'title' => 'ns:0:Foo',
-				'key' => 'ns:0:Foo',
+				'title' => 'ns:0:Foo/bar',
+				'key' => 'ns:0:Foo/bar',
 				'content_model' => 'wikitext',
 				'latest' => [
 					'id' => 371707,
@@ -245,6 +251,57 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 				],
 				'source' => 'Content of revision 371707'
 			],
+			$baseUrl . 'Foo%2Fbar',
+			true
+		];
+
+		yield "create with space" => [
+			[ // Request data received by CreationHandler
+				'method' => 'POST',
+				'headers' => [
+					'Content-Type' => 'application/json',
+				],
+				'bodyContents' => json_encode( [
+					'title' => 'foo (ba+r)',
+					'source' => 'Lorem Ipsum',
+					'comment' => 'Testing'
+				] ),
+			],
+			[ // Fake request expected to be passed into ApiEditPage
+				'title' => 'foo (ba+r)',
+				'text' => 'Lorem Ipsum',
+				'summary' => 'Testing',
+				'createonly' => '1',
+				'token' => '+\\', // use known-good token for current user (anon)
+			],
+			[ // Mock response returned by ApiEditPage
+				"edit" => [
+					"new" => true,
+					"result" => "Success",
+					"pageid" => 94542,
+					"title" => "Foo (ba+r)",
+					"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 (ba+r)',
+				'key' => 'ns:0:Foo_(ba+r)',
+				'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'
+			],
+			$baseUrl . 'Foo_(ba%2Br)',
 			true
 		];
 	}
@@ -257,6 +314,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 		$expectedActionParams,
 		$actionResult,
 		$expectedResponse,
+		$expectedRedirect,
 		$csrfSafe
 	) {
 		$request = new RequestData( $requestData );
@@ -267,8 +325,7 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 
 		$this->assertSame( 201, $response->getStatusCode() );
 		$this->assertSame(
-			// NOTE: Prefix hard coded in a fake for Router::getRouteUrl() in HandlerTestTrait
-			'https://wiki.example.com/rest/v1/page/Foo',
+			$expectedRedirect,
 			$response->getHeaderLine( 'Location' )
 		);
 		$this->assertSame( 'application/json', $response->getHeaderLine( 'Content-Type' ) );
@@ -279,8 +336,8 @@ class CreationHandlerTest extends MediaWikiIntegrationTestCase {
 		// Check parameters passed to ApiEditPage by CreationHandler based on $requestData
 		foreach ( $expectedActionParams as $key => $value ) {
 			$this->assertSame(
-				$handler->getApiMain()->getVal( $key ),
 				$value,
+				$handler->getApiMain()->getVal( $key ),
 				"ApiEditPage param: $key"
 			);
 		}

tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php

@@ -141,7 +141,7 @@ class UpdateHandlerTest extends \MediaWikiLangTestCase {
 					"new" => true,
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
+					"title" => "Foo",
 					"contentmodel" => "wikitext",
 					"oldrevid" => 0,
 					"newrevid" => 371707,
@@ -193,7 +193,7 @@ class UpdateHandlerTest extends \MediaWikiLangTestCase {
 					"new" => true,
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
+					"title" => "Foo",
 					"contentmodel" => "wikitext",
 					"oldrevid" => 0,
 					"newrevid" => 371707,
@@ -244,7 +244,7 @@ class UpdateHandlerTest extends \MediaWikiLangTestCase {
 				"edit" => [
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
+					"title" => "Foo_bar",
 					"contentmodel" => "wikitext",
 					"oldrevid" => 371705,
 					"newrevid" => 371707,
@@ -296,7 +296,7 @@ class UpdateHandlerTest extends \MediaWikiLangTestCase {
 				"edit" => [
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
+					"title" => "Foo_bar",
 					"contentmodel" => "wikitext",
 					"oldrevid" => 371705,
 					"newrevid" => 371707,
@@ -348,7 +348,7 @@ class UpdateHandlerTest extends \MediaWikiLangTestCase {
 				"edit" => [
 					"result" => "Success",
 					"pageid" => 94542,
-					"title" => "Sandbox",
+					"title" => "Foo",
 					"contentmodel" => "wikitext",
 					"oldrevid" => 371705,
 					"newrevid" => 371707,
@@ -390,8 +390,8 @@ class UpdateHandlerTest extends \MediaWikiLangTestCase {
 		// Check parameters passed to ApiEditPage by UpdateHandler based on $requestData
 		foreach ( $expectedActionParams as $key => $value ) {
 			$this->assertSame(
-				$handler->getApiMain()->getVal( $key ),
 				$value,
+				$handler->getApiMain()->getVal( $key ),
 				"ApiEditPage param: $key"
 			);
 		}