Page MenuHomePhabricator

Mobile version of Special:Contributions leaks existence of globally suppressed users
Open, LowPublicSecurity

Description

The account Suppressed is globally suppressed in my local wiki.

On the desktop view of Special:Contributions it is treated like a non-existent account.

contributions_desktop.png (326×715 px, 25 KB)

On mobile view not. For globally suppressed accounts it looks like the following.

contributions_mobile.png (252×478 px, 12 KB)

While for nonexistent account it looks like the following (yes it basically shows nothing, but that is unrelated).

mobile_contributions_expected.png (219×389 px, 11 KB)

I think this was fixed in the desktop version as part of T120883 (I can't say for sure since that ticket is still protected).

Event Timeline

Confirmed to also apply to WMF prod, checked on enwiki
Relevant place to fix is probably https://gerrit.wikimedia.org/g/mediawiki/extensions/MobileFrontend/+/c16d95225b6195f19296d1b62c5391902b85a4b9/includes/specials/SpecialMobileContributions.php#81, something like (untested, I don't have mobile frontend installed locally)

if ( $this->user->isHidden() && !$this->getUser()->isAllowed( 'hideuser' ) ) {
	// Pretend like the user doesn't exist if they are hidden and the viewer
	// lacks the necessary rights
	$this->showPageNotFound();
	return;
}

@DannyS712's suggested fix is correct. I simply combined it with the line above the one they linked to:

T298581.patch
From dca9d3805893e1d3308475acbe55575c80291c6f Mon Sep 17 00:00:00 2001
From: Sam Smith <phuedx@wikimedia.org>
Date: Wed, 5 Jan 2022 11:40:09 +0000
Subject: [PATCH] SECURITY: contributions: Do not show contributions for hidden
 users

Bug: T298581
Change-Id: I4dbb315226af267d43154d1a5a5c4635d68d1038
---
 includes/specials/SpecialMobileContributions.php | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/includes/specials/SpecialMobileContributions.php b/includes/specials/SpecialMobileContributions.php
index 7a3ff3836..c00f64668 100644
--- a/includes/specials/SpecialMobileContributions.php
+++ b/includes/specials/SpecialMobileContributions.php
@@ -74,7 +74,10 @@ class SpecialMobileContributions extends SpecialMobileHistory {
 			}
 		}
 
-		if ( !$this->user ) {
+		if (
+			!$this->user
+			|| ( $this->user->isHidden() && !$this->getUser()->isAllowed( 'hideuser' ) )
+		) {
 			$this->showPageNotFound();
 			return;
 		}
-- 
2.33.1

With the patch applied and attempting to view the contributions of a suppressed user, I see the see the "Cannot look for contributions…" message as expected.

Adding more Readers Web folks so that they can review the patch.

@ovasileva another reason to prioritize the work in T219349. We keep getting errors like this while we use our bespoke version of this page.

@phuedx @Jdlrobson - if @phuedx's patch from T298581#7598274 looks good, I can deploy it to wmf.13 and wmf.16 soon, before the backport window in a couple of hours. It looks fine to me, but if anyone else who's closer to the code wanted to have a quick look, that would be nice.

@Zabe -

I think this was fixed in the desktop version as part of T120883 (I can't say for sure since that ticket is still protected).

Unfortunately that task needs to stay permanently private and it appears I can't just sub you to it for some reason, perhaps because it's also NDA-protected. But yes, it does appear that this issue was addressed across OutputPage, Article, Skin and SpecialContributions there, at least for the non-mobile views.

sbassett lowered the priority of this task from High to Low.Wed, Jan 5, 5:33 PM

The patch from T298581#7598274 has been deployed to wmf.13 and wmf.16. mediawiki-errors seems fine for now. Just testing a few globally suppressed accounts on enwiki, the patch appears to be working for me. Also now tracking at T276237 and T297839.

One small difference I noticed is that the fix for SpecialContributions for T120883 uses the PermissionsManager service to check the hideuser right instead of $this->getUser()->isAllowed( 'hideuser' ). I believe either are fine, but the PM service (or Authority) are the more modern approaches.

sbassett changed Risk Rating from N/A to Low.

Just to check -we can post this to Gerrit now? if so I will happily merge.

Just to check -we can post this to Gerrit now? if so I will happily merge.

Yes. Once it's patched in WMF production, we can make the task public (I can do that now) and push any relevant backports through gerrit, if the relevant extension, etc. is not part of the bundled release. Which in this case, MobileFrontend is not. This issue will likely get a CVE for the next supplemental security release, where it will be re-disclosed/announced, sometime around the end of March 2022.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".
sbassett added a subscriber: gerritbot.

One small difference I noticed is that the fix for SpecialContributions for T120883 uses the PermissionsManager service to check the hideuser right instead of $this->getUser()->isAllowed( 'hideuser' ). I believe either are fine, but the PM service (or Authority) are the more modern approaches.

I'm happy to move to the more modern approach in a follow-on patch.

One small difference I noticed is that the fix for SpecialContributions for T120883 uses the PermissionsManager service to check the hideuser right instead of $this->getUser()->isAllowed( 'hideuser' ). I believe either are fine, but the PM service (or Authority) are the more modern approaches.

I'm happy to move to the more modern approach in a follow-on patch.

Sounds good, thanks.

@phuedx the web team committed to T293268 today so hopefully that means we'll see the back of this page in about a month or so.

Change 751811 had a related patch set uploaded (by SBassett; author: Phuedx):

[mediawiki/extensions/MobileFrontend@master] SECURITY: contributions: Do not show contributions for hidden users

https://gerrit.wikimedia.org/r/751811

Change 751827 had a related patch set uploaded (by SBassett; author: Phuedx):

[mediawiki/extensions/MobileFrontend@REL1_37] SECURITY: contributions: Do not show contributions for hidden users

https://gerrit.wikimedia.org/r/751827

Change 751812 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/MobileFrontend@master] Use ->getAuthority() instead of ->getUser()

https://gerrit.wikimedia.org/r/751812

Change 751828 had a related patch set uploaded (by RhinosF1; author: Phuedx):

[mediawiki/extensions/MobileFrontend@REL1_35] SECURITY: contributions: Do not show contributions for hidden users

https://gerrit.wikimedia.org/r/751828

Change 751827 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@REL1_37] SECURITY: contributions: Do not show contributions for hidden users

https://gerrit.wikimedia.org/r/751827

Change 751828 abandoned by RhinosF1:

[mediawiki/extensions/MobileFrontend@REL1_35] SECURITY: contributions: Do not show contributions for hidden users

Reason:

https://gerrit.wikimedia.org/r/751828

Change 751811 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] SECURITY: contributions: Do not show contributions for hidden users

https://gerrit.wikimedia.org/r/751811

Change 751812 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Use ->getAuthority() instead of ->getUser()

https://gerrit.wikimedia.org/r/751812

Change 751934 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/MobileFrontend@master] contributions: Use PermissionManager service

https://gerrit.wikimedia.org/r/751934

Change 751934 abandoned by Phuedx:

[mediawiki/extensions/MobileFrontend@master] contributions: Use PermissionManager service

Reason:

Per Daniel's comment above.

https://gerrit.wikimedia.org/r/751934

Seems /srv/patches/1.38.0-wmf.17/extensions/MobileFrontend/01-T298581.patch has been merged already so I've removed it from the deployment server.

Seems /srv/patches/1.38.0-wmf.17/extensions/MobileFrontend/01-T298581.patch has been merged already so I've removed it from the deployment server.

Ah, yes. I've updated T276237 to reflect this as well.