From ddd1d4b9203aa3b516afbb099b6819a42df6faec Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Mon, 6 May 2019 11:58:09 +0300 Subject: [PATCH] Log warning and show error on empty username Historically it seems that if Linker::userLink or friends were passed an empty username (probably due to an incorrect database entry), they would produce bogus output, e.g., an with no contents or a link to the invalid page "User_talk:" or similar. In b6e1e99bec8d we replaced an occurrence of Title::makeTitle() (no safety checks!) with creating a TitleValue, which asserts in its constructor that the title text is not empty. This made such pages fail an assertion and stop displaying at all. Now there's a proper check for the error. Such cases will log a production error and return "(no username available)". Bug: T222529 Change-Id: Id65bdf9666b0d16e5553b8f38c7cf8fce2e37a25 --- RELEASE-NOTES-1.34 | 5 +- includes/Linker.php | 30 ++++++ languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + tests/phpunit/includes/LinkerTest.php | 130 +++++++++++++++++++++++++- 5 files changed, 161 insertions(+), 6 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 98ed961605..22b744345f 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -55,7 +55,10 @@ For notes on 1.33.x and older releases, see HISTORY. * … === Bug fixes in 1.34 === -* … +* (T222529) If a log entry or page revision is recorded in the database with an + empty username, attempting to display it will log an error and return a "no + username available" to the user instead of silently displaying nothing or + invalid links. === Action API changes in 1.34 === * The 'recenteditcount' response property from action=query list=allusers, diff --git a/includes/Linker.php b/includes/Linker.php index 9cca0be9d3..6acfda3121 100644 --- a/includes/Linker.php +++ b/includes/Linker.php @@ -894,6 +894,12 @@ class Linker { * @since 1.16.3. $altUserName was added in 1.19. */ public static function userLink( $userId, $userName, $altUserName = false ) { + if ( $userName === '' ) { + wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' . + 'that need to be fixed?' ); + return wfMessage( 'empty-username' )->parse(); + } + $classes = 'mw-userlink'; $page = null; if ( $userId == 0 ) { @@ -936,6 +942,12 @@ class Linker { $userId, $userText, $redContribsWhenNoEdits = false, $flags = 0, $edits = null, $useParentheses = true ) { + if ( $userText === '' ) { + wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' . + 'that need to be fixed?' ); + return ' ' . wfMessage( 'empty-username' )->parse(); + } + global $wgUser, $wgDisableAnonTalk, $wgLang; $talkable = !( $wgDisableAnonTalk && $userId == 0 ); $blockable = !( $flags & self::TOOL_LINKS_NOBLOCK ); @@ -1018,6 +1030,12 @@ class Linker { * @return string HTML fragment with user talk link */ public static function userTalkLink( $userId, $userText ) { + if ( $userText === '' ) { + wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' . + 'that need to be fixed?' ); + return wfMessage( 'empty-username' )->parse(); + } + $userTalkPage = new TitleValue( NS_USER_TALK, strtr( $userText, ' ', '_' ) ); $moreLinkAttribs['class'] = 'mw-usertoollinks-talk'; @@ -1034,6 +1052,12 @@ class Linker { * @return string HTML fragment with block link */ public static function blockLink( $userId, $userText ) { + if ( $userText === '' ) { + wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' . + 'that need to be fixed?' ); + return wfMessage( 'empty-username' )->parse(); + } + $blockPage = SpecialPage::getTitleFor( 'Block', $userText ); $moreLinkAttribs['class'] = 'mw-usertoollinks-block'; @@ -1049,6 +1073,12 @@ class Linker { * @return string HTML fragment with e-mail user link */ public static function emailLink( $userId, $userText ) { + if ( $userText === '' ) { + wfLogWarning( __METHOD__ . ' received an empty username. Are there database errors ' . + 'that need to be fixed?' ); + return wfMessage( 'empty-username' )->parse(); + } + $emailPage = SpecialPage::getTitleFor( 'Emailuser', $userText ); $moreLinkAttribs['class'] = 'mw-usertoollinks-mail'; return self::link( $emailPage, diff --git a/languages/i18n/en.json b/languages/i18n/en.json index 5eeaf936f9..15edfc0160 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -2711,6 +2711,7 @@ "blocklink": "block", "unblocklink": "unblock", "change-blocklink": "change block", + "empty-username": "(no username available)", "contribslink": "contribs", "emaillink": "send email", "autoblocker": "Autoblocked because your IP address has been recently used by \"[[User:$1|$1]]\".\nThe reason given for $1's block is \"$2\"", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index 8a776dc2b9..9dfa861444 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -2918,6 +2918,7 @@ "blocklink": "Display name for a link that, when selected, leads to a form where a user can be blocked. Used in page history and recent changes pages. Example: \"''UserName (Talk | contribs | '''block''')''\".\n\nUsed as link title in [[Special:Contributions]] and in [[Special:DeletedContributions]].\n\nSee also:\n* {{msg-mw|Sp-contributions-talk}}\n* {{msg-mw|Change-blocklink}}\n* {{msg-mw|Unblocklink}}\n* {{msg-mw|Sp-contributions-blocklog}}\n* {{msg-mw|Sp-contributions-uploads}}\n* {{msg-mw|Sp-contributions-logs}}\n* {{msg-mw|Sp-contributions-deleted}}\n* {{msg-mw|Sp-contributions-userrights}}\n{{Identical|Block}}", "unblocklink": "Used as link title in [[Special:Contributions]] and in [[Special:DeletedContributions]].\n\nSee also:\n* {{msg-mw|Sp-contributions-talk}}\n* {{msg-mw|change-blocklink}}\n* {{msg-mw|blocklink}}\n* {{msg-mw|sp-contributions-blocklog}}\n* {{msg-mw|sp-contributions-uploads}}\n* {{msg-mw|sp-contributions-logs}}\n* {{msg-mw|sp-contributions-deleted}}\n* {{msg-mw|sp-contributions-userrights}}\n{{Identical|Unblock}}", "change-blocklink": "Used to name the link on [[Special:Log]].\n\nAlso used as link title in [[Special:Contributions]] and in [[Special:DeletedContributions]].\n\nSee also:\n* {{msg-mw|Sp-contributions-talk}}\n* {{msg-mw|unblocklink}}\n* {{msg-mw|blocklink}}\n* {{msg-mw|sp-contributions-blocklog}}\n* {{msg-mw|sp-contributions-uploads}}\n* {{msg-mw|sp-contributions-logs}}\n* {{msg-mw|sp-contributions-deleted}}\n* {{msg-mw|sp-contributions-userrights}}", + "empty-username": "If no username is available to display for a log or history entry, such as because of an incorrect database entry, this message is displayed in place of the links to the user page/talk/contribs/etc.", "contribslink": "Short for \"contributions\". Used as display name for a link to user contributions on history pages, [[Special:RecentChanges]], [[Special:Watchlist]], etc.\n{{Identical|Contribution}}", "emaillink": "Used as display name for a link to send an e-mail to a user in the user tool links. Example: \"(Talk | contribs | block | send e-mail)\".\n\n{{Identical|E-mail}}", "autoblocker": "Used in [[Special:Block]].\n* $1 - target username\n* $2 - reason", diff --git a/tests/phpunit/includes/LinkerTest.php b/tests/phpunit/includes/LinkerTest.php index 438d3e7bca..2362961ea7 100644 --- a/tests/phpunit/includes/LinkerTest.php +++ b/tests/phpunit/includes/LinkerTest.php @@ -14,11 +14,16 @@ class LinkerTest extends MediaWikiLangTestCase { 'wgArticlePath' => '/wiki/$1', ] ); - $this->assertEquals( - $expected, - Linker::userLink( $userId, $userName, $altUserName ), - $msg - ); + // We'd also test the warning, but injecting a mock logger into a static method is tricky. + if ( $userName === '' ) { + Wikimedia\suppressWarnings(); + } + $actual = Linker::userLink( $userId, $userName, $altUserName ); + if ( $userName === '' ) { + Wikimedia\restoreWarnings(); + } + + $this->assertEquals( $expected, $actual, $msg ); } public static function provideCasesForUserLink() { @@ -29,6 +34,9 @@ class LinkerTest extends MediaWikiLangTestCase { # - optional altUserName # - optional message return [ + # Empty name (T222529) + 'Empty username, userid 0' => [ '(no username available)', 0, '' ], + 'Empty username, userid > 0' => [ '(no username available)', 73, '' ], # ## ANONYMOUS USER ######################################## [ @@ -87,6 +95,118 @@ class LinkerTest extends MediaWikiLangTestCase { ]; } + /** + * @dataProvider provideUserToolLinks + * @covers Linker::userToolLinks + * @param string $expected + * @param int $userId + * @param string $userText + */ + public function testUserToolLinks( $expected, $userId, $userText ) { + // We'd also test the warning, but injecting a mock logger into a static method is tricky. + if ( $userText === '' ) { + Wikimedia\suppressWarnings(); + } + $actual = Linker::userToolLinks( $userId, $userText ); + if ( $userText === '' ) { + Wikimedia\restoreWarnings(); + } + + $this->assertSame( $expected, $actual ); + } + + public static function provideUserToolLinks() { + return [ + // Empty name (T222529) + 'Empty username, userid 0' => [ ' (no username available)', 0, '' ], + 'Empty username, userid > 0' => [ ' (no username available)', 73, '' ], + ]; + } + + /** + * @dataProvider provideUserTalkLink + * @covers Linker::userTalkLink + * @param string $expected + * @param int $userId + * @param string $userText + */ + public function testUserTalkLink( $expected, $userId, $userText ) { + // We'd also test the warning, but injecting a mock logger into a static method is tricky. + if ( $userText === '' ) { + Wikimedia\suppressWarnings(); + } + $actual = Linker::userTalkLink( $userId, $userText ); + if ( $userText === '' ) { + Wikimedia\restoreWarnings(); + } + + $this->assertSame( $expected, $actual ); + } + + public static function provideUserTalkLink() { + return [ + // Empty name (T222529) + 'Empty username, userid 0' => [ '(no username available)', 0, '' ], + 'Empty username, userid > 0' => [ '(no username available)', 73, '' ], + ]; + } + + /** + * @dataProvider provideBlockLink + * @covers Linker::blockLink + * @param string $expected + * @param int $userId + * @param string $userText + */ + public function testBlockLink( $expected, $userId, $userText ) { + // We'd also test the warning, but injecting a mock logger into a static method is tricky. + if ( $userText === '' ) { + Wikimedia\suppressWarnings(); + } + $actual = Linker::blockLink( $userId, $userText ); + if ( $userText === '' ) { + Wikimedia\restoreWarnings(); + } + + $this->assertSame( $expected, $actual ); + } + + public static function provideBlockLink() { + return [ + // Empty name (T222529) + 'Empty username, userid 0' => [ '(no username available)', 0, '' ], + 'Empty username, userid > 0' => [ '(no username available)', 73, '' ], + ]; + } + + /** + * @dataProvider provideEmailLink + * @covers Linker::emailLink + * @param string $expected + * @param int $userId + * @param string $userText + */ + public function testEmailLink( $expected, $userId, $userText ) { + // We'd also test the warning, but injecting a mock logger into a static method is tricky. + if ( $userText === '' ) { + Wikimedia\suppressWarnings(); + } + $actual = Linker::emailLink( $userId, $userText ); + if ( $userText === '' ) { + Wikimedia\restoreWarnings(); + } + + $this->assertSame( $expected, $actual ); + } + + public static function provideEmailLink() { + return [ + // Empty name (T222529) + 'Empty username, userid 0' => [ '(no username available)', 0, '' ], + 'Empty username, userid > 0' => [ '(no username available)', 73, '' ], + ]; + } + /** * @dataProvider provideCasesForFormatComment * @covers Linker::formatComment -- 2.20.1