Page MenuHomePhabricator
Files F44087073

T361479-2.patch
Dreamy_Jazz (WBrown (WMF))
Actions

Authored By
Dreamy_Jazz
Apr 1 2024, 11:57 PM
Size
8 KB
Referenced Files
None
Subscribers
None

T361479-2.patch
View Options

From c1bef410391d74a83c1df41fa035637bab9c2d69 Mon Sep 17 00:00:00 2001
From: Dreamy Jazz <wpgbrown@wikimedia.org>
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..9c412d5e 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 ( $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 @@
<li>
- {{{links}}}
- <span class="mw-changeslist-separator"></span>
+ {{#showLinks}}
+ {{{links}}}
+ <span class="mw-changeslist-separator"></span>
+ {{/showLinks}}
{{timestamp}}
<span class="mw-changeslist-separator"></span>
<span class="mw-checkuser-user-link{{#userLinkClass}} {{userLinkClass}}{{/userLinkClass}}">
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

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
15268917
Default Alt Text
T361479-2.patch (8 KB)

Event Timeline

Dreamy_Jazz updated the name for this file from "T36147-2.patch" to "T361479-2.patch".Apr 1 2024, 11:58 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits