From 3b3115e7f33dc91c05c62dd5b385a37f51f051b5 Mon Sep 17 00:00:00 2001 From: daniel Date: Fri, 14 Jun 2019 11:01:22 +0200 Subject: [PATCH] Title: ensure getBaseTitle and getRootTitle return valid Titles Since getBaseText() and getRootText() may return text with trailing whitespace, getBaseTitle and getRootTitle must use makeTitleSafe instead of makeTitle. Bug: T225585 Change-Id: Id92df552f05e6a9ed7c9259a8779fa94c3587a3e --- includes/Title.php | 43 ++++++++++++++++++++++++---- tests/phpunit/includes/TitleTest.php | 40 ++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/includes/Title.php b/includes/Title.php index f69f1a4503..b27baa8db1 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -23,6 +23,7 @@ */ use MediaWiki\Permissions\PermissionManager; +use Wikimedia\Assert\Assert; use Wikimedia\Rdbms\Database; use Wikimedia\Rdbms\IDatabase; use MediaWiki\Linker\LinkTarget; @@ -851,7 +852,10 @@ class Title implements LinkTarget, IDBAccessObject { /** * Returns true if the title is valid, false if it is invalid. * - * Valid titles can be round-tripped via makeTitleSafe() and newFromText(). + * Valid titles can be round-tripped via makeTitle() and newFromText(). + * Their DB key can be used in the database, though it may not have the correct + * capitalization. + * * Invalid titles may get returned from makeTitle(), and it may be useful to * allow them to exist, e.g. in order to process log entries about pages in * namespaces that belong to extensions that are no longer installed. @@ -870,10 +874,23 @@ class Title implements LinkTarget, IDBAccessObject { try { $services->getTitleParser()->parseTitle( $this->mDbkeyform, $this->mNamespace ); - return true; } catch ( MalformedTitleException $ex ) { return false; } + + try { + // Title value applies basic syntax checks. Should perhaps be moved elsewhere. + new TitleValue( + $this->mNamespace, + $this->mDbkeyform, + $this->mFragment, + $this->mInterwiki + ); + } catch ( InvalidArgumentException $ex ) { + return false; + } + + return true; } /** @@ -1728,6 +1745,9 @@ class Title implements LinkTarget, IDBAccessObject { /** * Get the root page name text without a namespace, i.e. the leftmost part before any slashes * + * @note the return value may contain trailing whitespace and is thus + * not safe for use with makeTitle or TitleValue. + * * @par Example: * @code * Title::newFromText('User:Foo/Bar/Baz')->getRootText(); @@ -1761,12 +1781,20 @@ class Title implements LinkTarget, IDBAccessObject { * @since 1.20 */ public function getRootTitle() { - return self::makeTitle( $this->mNamespace, $this->getRootText() ); + $title = self::makeTitleSafe( $this->mNamespace, $this->getRootText() ); + Assert::postcondition( + $title !== null, + 'makeTitleSafe() should always return a Title for the text returned by getRootText().' + ); + return $title; } /** * Get the base page name without a namespace, i.e. the part before the subpage name * + * @note the return value may contain trailing whitespace and is thus + * not safe for use with makeTitle or TitleValue. + * * @par Example: * @code * Title::newFromText('User:Foo/Bar/Baz')->getBaseText(); @@ -1794,7 +1822,7 @@ class Title implements LinkTarget, IDBAccessObject { } /** - * Get the base page name title, i.e. the part before the subpage name + * Get the base page name title, i.e. the part before the subpage name. * * @par Example: * @code @@ -1806,7 +1834,12 @@ class Title implements LinkTarget, IDBAccessObject { * @since 1.20 */ public function getBaseTitle() { - return self::makeTitle( $this->mNamespace, $this->getBaseText() ); + $title = self::makeTitleSafe( $this->mNamespace, $this->getBaseText() ); + Assert::postcondition( + $title !== null, + 'makeTitleSafe() should always return a Title for the text returned by getBaseText().' + ); + return $title; } /** diff --git a/tests/phpunit/includes/TitleTest.php b/tests/phpunit/includes/TitleTest.php index 225a786b21..9b021c40ee 100644 --- a/tests/phpunit/includes/TitleTest.php +++ b/tests/phpunit/includes/TitleTest.php @@ -494,17 +494,31 @@ class TitleTest extends MediaWikiTestCase { */ public function testGetBaseText( $title, $expected, $msg = '' ) { $title = Title::newFromText( $title ); - $this->assertEquals( $expected, + $this->assertSame( $expected, $title->getBaseText(), $msg ); } + /** + * @dataProvider provideBaseTitleCases + * @covers Title::getBaseTitle + */ + public function testGetBaseTitle( $title, $expected, $msg = '' ) { + $title = Title::newFromText( $title ); + $base = $title->getBaseTitle(); + $this->assertTrue( $base->isValid(), $msg ); + $this->assertTrue( + $base->equals( Title::makeTitleSafe( $title->getNamespace(), $expected ) ), + $msg + ); + } + public static function provideBaseTitleCases() { return [ # Title, expected base, optional message [ 'User:John_Doe/subOne/subTwo', 'John Doe/subOne' ], - [ 'User:Foo/Bar/Baz', 'Foo/Bar' ], + [ 'User:Foo / Bar / Baz', 'Foo / Bar ' ], ]; } @@ -520,11 +534,25 @@ class TitleTest extends MediaWikiTestCase { ); } + /** + * @dataProvider provideRootTitleCases + * @covers Title::getRootTitle + */ + public function testGetRootTitle( $title, $expected, $msg = '' ) { + $title = Title::newFromText( $title ); + $root = $title->getRootTitle(); + $this->assertTrue( $root->isValid(), $msg ); + $this->assertTrue( + $root->equals( Title::makeTitleSafe( $title->getNamespace(), $expected ) ), + $msg + ); + } + public static function provideRootTitleCases() { return [ # Title, expected base, optional message [ 'User:John_Doe/subOne/subTwo', 'John Doe' ], - [ 'User:Foo/Bar/Baz', 'Foo' ], + [ 'User:Foo / Bar / Baz', 'Foo ' ], ]; } @@ -709,6 +737,12 @@ class TitleTest extends MediaWikiTestCase { [ Title::makeTitle( NS_MAIN, '|' ), false ], [ Title::makeTitle( NS_MAIN, '#' ), false ], [ Title::makeTitle( NS_MAIN, 'Test' ), true ], + [ Title::makeTitle( NS_MAIN, ' Test' ), false ], + [ Title::makeTitle( NS_MAIN, '_Test' ), false ], + [ Title::makeTitle( NS_MAIN, 'Test ' ), false ], + [ Title::makeTitle( NS_MAIN, 'Test_' ), false ], + [ Title::makeTitle( NS_MAIN, "Test\nthis" ), false ], + [ Title::makeTitle( NS_MAIN, "Test\tthis" ), false ], [ Title::makeTitle( -33, 'Test' ), false ], [ Title::makeTitle( 77663399, 'Test' ), false ], ]; -- 2.20.1