From d2d2906ce77ec2a85b4d3458a5bad0c09956b8b2 Mon Sep 17 00:00:00 2001 From: daniel Date: Wed, 22 Jul 2020 21:12:00 +0200 Subject: [PATCH] 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 | 26 ++++++ includes/Rest/Handler/CreationHandler.php | 3 +- includes/Rest/Handler/EditHandler.php | 2 +- tests/api-testing/REST/Creation.js | 17 +++- .../Rest/Handler/CreationHandlerTest.php | 93 +++++++++++++++---- .../Rest/Handler/UpdateHandlerTest.php | 12 +-- 6 files changed, 122 insertions(+), 31 deletions(-) diff --git a/includes/Rest/Handler.php b/includes/Rest/Handler.php index dd8372c9976..df1cee57e21 100644 --- a/includes/Rest/Handler.php +++ b/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. diff --git a/includes/Rest/Handler/CreationHandler.php b/includes/Rest/Handler/CreationHandler.php index 9f72dfdf2c3..1e9b726b355 100644 --- a/includes/Rest/Handler/CreationHandler.php +++ b/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 ); } diff --git a/includes/Rest/Handler/EditHandler.php b/includes/Rest/Handler/EditHandler.php index f0d0fb0aa04..9c28ad210b2 100644 --- a/includes/Rest/Handler/EditHandler.php +++ b/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. diff --git a/tests/api-testing/REST/Creation.js b/tests/api-testing/REST/Creation.js index 1d5f0716652..48d23f10b05 100644 --- a/tests/api-testing/REST/Creation.js +++ b/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 ); diff --git a/tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php b/tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php index bc8aa4c9544..b8e74d32937 100644 --- a/tests/phpunit/integration/includes/Rest/Handler/CreationHandlerTest.php +++ b/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" ); } diff --git a/tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php b/tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php index ef75b6d806c..05c59aad100 100644 --- a/tests/phpunit/integration/includes/Rest/Handler/UpdateHandlerTest.php +++ b/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" ); }