From 99c6128bb5fd66b06c5b39789958217c5bd47c8a Mon Sep 17 00:00:00 2001 From: Dreamy Jazz Date: Tue, 2 Apr 2024 00:03:08 +0100 Subject: [PATCH] SECURITY: Hide 'logs' link if page is a hidden user in 'Get actions' Why: * Special:CheckUser 'Get actions' has links shown before a row in the results which provide links to the diff for an edit and the logs page for a log action. * When a user is blocked with 'hideuser' enabled, Special:CheckUser still adds the 'logs' link for actions which are associated with this user. * This association is partly done through the username being used as the 'page' query paramter value, so that a user can filter for logs performed by this user. * However, in the case of a user which the current authority cannot see this causes the username to be leaked via the URL of that 'logs' link. * Removing the 'logs' link if the 'page' query parameter is a hidden user should fix this issue. What: * Update CheckUserGetActionsPager::getLinksFromRow to check if the 'page' title is a username (NS_USER) and if so, whether the username is hidden from the current authority. If it is hidden, then don't add the 'logs' link to the links shown at the start of the result line. * Don't display the links at the start of the row if there are no links to display, including the separator which is usually added after the links. * Add tests to verify that the security fix has worked. Bug: T361479 Change-Id: I67d76232b131054713534d619d04972f4d84e232 --- .../Pagers/CheckUserGetActionsPager.php | 65 ++++++++++++++----- templates/GetActionsLine.mustache | 6 +- .../Pagers/CheckUserGetActionsPagerTest.php | 37 +++++++++++ 3 files changed, 89 insertions(+), 19 deletions(-) diff --git a/src/CheckUser/Pagers/CheckUserGetActionsPager.php b/src/CheckUser/Pagers/CheckUserGetActionsPager.php index a017a40c..a0b06749 100644 --- a/src/CheckUser/Pagers/CheckUserGetActionsPager.php +++ b/src/CheckUser/Pagers/CheckUserGetActionsPager.php @@ -4,6 +4,7 @@ namespace MediaWiki\CheckUser\CheckUser\Pagers; use HtmlArmor; use IContextSource; +use LogEventsList; use LogFormatter; use LogicException; use LogPage; @@ -166,8 +167,6 @@ class CheckUserGetActionsPager extends AbstractCheckUserPager { */ public function formatRow( $row ): string { $templateParams = []; - // Create diff/hist/page links - $templateParams['links'] = $this->getLinksFromRow( $row ); // Show date $templateParams['timestamp'] = $this->getLanguage()->userTime( wfTimestamp( TS_MW, $row->timestamp ), $this->getUser() ); @@ -189,6 +188,9 @@ class CheckUserGetActionsPager extends AbstractCheckUserPager { $hidden = $this->userFactory->newFromUserIdentity( $user )->isHidden() && !$this->getAuthority()->isAllowed( 'hideuser' ); } + // Create diff/hist/page links + $templateParams['links'] = $this->getLinksFromRow( $row, $user ); + $templateParams['showLinks'] = $templateParams['links'] !== ''; if ( $hidden ) { $templateParams['userLink'] = Html::element( 'span', @@ -299,16 +301,17 @@ class CheckUserGetActionsPager extends AbstractCheckUserPager { /** * @param stdClass $row + * @param UserIdentity $performer The user that performed the action represented by this row. * @return string diff, hist and page other links related to the change */ - protected function getLinksFromRow( stdClass $row ): string { + protected function getLinksFromRow( stdClass $row, UserIdentity $performer ): string { $links = []; // Log items // Due to T315224 triple equals for type does not work for sqlite. if ( $row->type == RC_LOG ) { $title = Title::makeTitle( $row->namespace, $row->title ); $links['log'] = ''; - if ( $this->eventTableReadNew && isset( $row->log_id ) ) { + if ( $this->eventTableReadNew && isset( $row->log_id ) && $row->log_id ) { $links['log'] = Html::rawElement( 'span', [], $this->getLinkRenderer()->makeKnownLink( SpecialPage::getTitleFor( 'Log' ), @@ -316,21 +319,49 @@ class CheckUserGetActionsPager extends AbstractCheckUserPager { [], [ 'logid' => $row->log_id ] ) - ) . ' '; + ); + } + // Hide the 'logs' link if the page is a username and the current authority does not have permission to see + // the username in question (T361479). + $hidden = false; + if ( $title->getNamespace() === NS_USER ) { + $user = $this->userFactory->newFromName( $title->getBaseText() ); + if ( isset( $row->log_deleted ) && $performer->getName() === $title->getText() ) { + // If the username of the performer is the same as the title, we can also check whether the + // performer of the log entry is hidden. + $hidden = !LogEventsList::userCanBitfield( + $row->log_deleted, + LogPage::DELETED_USER, + $this->getContext()->getAuthority() + ); + } + if ( $user !== null && !$hidden ) { + // If LogEventsList::userCanBitfield said the log entry isn't hidden, then also check if the user + // is hidden in general (via a block with 'hideuser' set). + // LogEventsList::userCanBitfield can return false while this is true for events from + // cu_private_event, as log_deleted is always 0 for those rows (as they cannot be revision deleted). + $hidden = $user->isHidden() && !$this->getAuthority()->isAllowed( 'hideuser' ); + } } - $links['log'] .= Html::rawElement( 'span', [], + if ( !$hidden ) { + $links['log'] .= Html::rawElement( 'span', [], $this->getLinkRenderer()->makeKnownLink( - SpecialPage::getTitleFor( 'Log' ), - new HtmlArmor( $this->message['checkuser-logs-link-text'] ), - [], - [ 'page' => $title->getPrefixedText() ] - ) - ); - $links['log'] = Html::rawElement( - 'span', - [ 'class' => 'mw-changeslist-links' ], - $links['log'] - ); + SpecialPage::getTitleFor( 'Log' ), + new HtmlArmor( $this->message['checkuser-logs-link-text'] ), + [], + [ 'page' => $title->getPrefixedText() ] + ) + ); + } + // Only add the log related links if we have any to add. There may be none for cu_private_event rows when + // the username listed as the title is blocked with 'hideuser' enabled. + if ( $links['log'] !== '' ) { + $links['log'] = Html::rawElement( + 'span', + [ 'class' => 'mw-changeslist-links' ], + $links['log'] + ); + } } else { $title = Title::makeTitle( $row->namespace, $row->title ); // New pages diff --git a/templates/GetActionsLine.mustache b/templates/GetActionsLine.mustache index 9f5d4f9e..50aeafeb 100644 --- a/templates/GetActionsLine.mustache +++ b/templates/GetActionsLine.mustache @@ -1,6 +1,8 @@
  • - {{{links}}} - + {{#showLinks}} + {{{links}}} + + {{/showLinks}} {{timestamp}} diff --git a/tests/phpunit/integration/CheckUser/Pagers/CheckUserGetActionsPagerTest.php b/tests/phpunit/integration/CheckUser/Pagers/CheckUserGetActionsPagerTest.php index 0421d080..a37012f5 100644 --- a/tests/phpunit/integration/CheckUser/Pagers/CheckUserGetActionsPagerTest.php +++ b/tests/phpunit/integration/CheckUser/Pagers/CheckUserGetActionsPagerTest.php @@ -260,6 +260,43 @@ class CheckUserGetActionsPagerTest extends CheckUserPagerTestBase { ]; } + public function testFormatRowWhenTitleIsHiddenUser() { + // Get a user which has been blocked with the 'hideuser' enabled. + $hiddenUser = $this->getTestUser()->getUser(); + $blockStatus = $this->getServiceContainer()->getBlockUserFactory() + ->newBlockUser( + $hiddenUser, + $this->getTestUser( [ 'suppress', 'sysop' ] )->getAuthority(), + 'infinity', + 'block to hide the test user', + [ 'isHideUser' => true ] + )->placeBlock(); + $this->assertStatusGood( $blockStatus ); + // Test that when the title is the username of a hidden user, the 'logs' link is not set (as this uses the + // the title for the row). + $this->testFormatRow( + [ + 'log_type' => 'test', + 'log_action' => 'phpunit', + 'log_deleted' => 0, + 'namespace' => NS_USER, + 'title' => $hiddenUser->getUserPage()->getText(), + 'user_text' => $hiddenUser->getName(), + 'user' => $hiddenUser->getId(), + 'log_params' => LogEntryBase::makeParamBlob( [] ), + 'client_hints_reference_id' => 1, + 'client_hints_reference_type' => UserAgentClientHintsManager::IDENTIFIER_CU_CHANGES, + ], + [ $hiddenUser->getName() => '' ], + [ $hiddenUser->getId() => true ], + [], + new ClientHintsBatchFormatterResults( [], [] ), + [ 'showLinks' => false ], + SCHEMA_COMPAT_NEW, + true, + ); + } + public static function provideFormatRow() { // @todo test the rest of the template parameters. return [ -- 2.34.1