Page MenuHomePhabricator

MobileFrontend contributions display edits where username is revdel'd without having appropriate rights
Closed, ResolvedPublic

Description

In order to update your installation to fix this bug, you can:


Recreating this task (originally T132653) with PII removed.

Description

After oversighting an IP on the English Wikipedia I noticed there was an issue. The IP address was removed from public view on the desktop website, but was still visible on the mobile website.

The issue still persisted even after I purged my cache and using a different browser while logged out.

This request originated from an OTRS ticket in the mobile feedback queue. (Ticket # 2016041310003588 - now in the oversight::oversight-en-wp queue) The edit was made using the iOS Wikipedia app.

Thanks to @Josve05a for bringing it to my attention.

Relevant comments

In T132653#2205884, @Krenair wrote:

Haven't tested beyond a syntax check but we can probably add this to the end of the if ( $this->user ) block in SpecialMobileContributions::getQueryConditions:

			$dbr = wfGetDB( DB_SLAVE );
			// Copied from core Special:Contributions
			// Paranoia: avoid brute force searches (T19342)
			if ( !$this->user->isAllowed( 'deletedhistory' ) ) {
				$conds[] = $dbr->bitAnd( 'rev_deleted', Revision::DELETED_USER ) . ' = 0';
			} elseif ( !$this->user->isAllowedAny( 'suppressrevision', 'viewsuppressed' ) ) {
				$conds[] = $dbr->bitAnd( 'rev_deleted', Revision::SUPPRESSED_USER ) .
					' != ' . Revision::SUPPRESSED_USER;
			}

In T132653#2213719, @phuedx wrote:

F3888227 is an only-very-slightly modified version of @Krenair's suggestion in T132653#2205884.


In T132653#2213835, @Jhernandez wrote:

I've QAd @phuedx's patch locally and it works (cc/ @csteipp).

Tested before and after patch:

When:

  1. Create page 'X'
  2. Edit 'X' with user 'User' 3 times
  3. With Admin remove revision 2 of 'X'

Then (after patch applied):

  1. Admin can see all contributions in both desktop and mobile Special:Contributions/User
  2. Logged out user can only see contributions 1 and 3 in desktop and mobile Special:Contributions/User
  3. Logged in user 'User' (editor) can only see contributions 1 and 3 in desktop and mobile Special:Contributions/User.

In T132653#2217971, @Jdlrobson wrote:

@csteipp we are fine to do this but that said - there appears to be no deploys this week. How urgent do we want to get this up?


In T132653#2217971, @Jdlrobson wrote:

@csteipp we are fine to do this but that said - there appears to be no deploys this week. How urgent do we want to get this up?


In T132653#2218361, @csteipp wrote:
In T132653#2217971, @Jdlrobson wrote:

@csteipp we are fine to do this but that said - there appears to be no deploys this week. How urgent do we want to get this up?

No reason to be reckless, but if everything on the switch goes smoothly, I'd like to try to get it out this week. I'll ping you guys again if it looks like that will be possible.


In T132653#2237352, @Krenair wrote:

I just deployed this, ops post to follow shortly


In T132653#2237410, @Jdlrobson wrote:

@csteipp with your permission can we now:

  • submit this to master and merge
  • remove the security tag on this task
  • post to the public wikitech-l mailing list

If so any of the team can make this happen.


In T132653#2237476, @Mike_V wrote:

@csteipp I've sent you an email. Could you take a look before you remove the security tag?


In T132653#2237614, @Krenair wrote:

I don't think everyone on your team can make this task public @Jdlrobson
Also there is a remaining issue: People who can see these entries don't have them tagged as "[username or IP address removed - edit hidden from contributions]" on mobile like they are on desktop


In T132653#2237650, @csteipp wrote:

Mike_V edited the task description. (Show Details)

^ @demon, can you permanently kill the previous revision of the task's description?


In T132653#2237652, @csteipp wrote:

remove the security tag on this task

Please hold off until we get the PII removed

Event Timeline

In T132653#2237410, @Jdlrobson wrote:

@csteipp with your permission can we now:

  • submit this to master and merge
  • remove the security tag on this task
  • post to the public wikitech-l mailing list

If so any of the team can make this happen.

In T132653#2237614, @Krenair wrote:

I don't think everyone on your team can make this task public @Jdlrobson
Also there is a remaining issue: People who can see these entries don't have them tagged as "[username or IP address removed - edit hidden from contributions]" on mobile like they are on desktop

@Jdlrobson, I think you should address @Krenair's comment first, but after that, you can go ahead and push the patch publicly and announce it.

@Jdlrobson Just a reminder, we decided in standup that we need a new card for the remaining work, and it needs to be estimated with the team.

Given the problem raised in @Krenair's comment is not a security issue and we are unlikely to have time this sprint to fix it I'd rather create a new high priority card for this issue, wrap up the work in this one and post to the mailing list if that's okay?

Given the problem raised in @Krenair's comment is not a security issue and we are unlikely to have time this sprint to fix it I'd rather create a new high priority card for this issue, wrap up the work in this one and post to the mailing list if that's okay?

As discussed on irc, yep, works for me.

I've created T133722 and with permission from @csteipp dropped the security tag. Email to follow shortly.

csteipp changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 26 2016, 6:18 PM
csteipp changed Security from Software security bug to None.

Change 285433 merged by jenkins-bot:
SECURITY: Don't list deleted edits without rights

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

Email sent to wikitech-l
I've opened up T133722 so we will triage that and get that addressed separately to completely wrap up all the work relating to this feature.

Can this patch be backported to REL1_26, REL1_25 and REL1_23 too? The current security patch is only available in master, and these three branches still receive security support.

Also there should probably be an email to mediawiki-announce as well.

Non-bundled extensions shouldn't be on mediawiki-announce

Can this patch be backported to REL1_26, REL1_25 and REL1_23 too? The current security patch is only available in master, and these three branches still receive security support.

Since it's not bundled anyone is more than welcome to backport said patches :)

Change 285686 had a related patch set uploaded (by Phuedx):
SECURITY: Don't list deleted edits without rights

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

Change 285695 had a related patch set uploaded (by Phuedx):
SECURITY: Don't list deleted edits without rights

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

Change 285696 had a related patch set uploaded (by Phuedx):
SECURITY: Don't list deleted edits without rights

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

Change 285697 had a related patch set uploaded (by Phuedx):
SECURITY: Don't list deleted edits without rights

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

Change 285686 merged by Brian Wolff:
SECURITY: Don't list deleted edits without rights

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

Change 285696 merged by Brian Wolff:
SECURITY: Don't list deleted edits without rights

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

Change 285695 merged by Brian Wolff:
SECURITY: Don't list deleted edits without rights

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

Change 285697 merged by Brian Wolff:
SECURITY: Don't list deleted edits without rights

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

Thanks @Bawolff!

Btw, for future reference, if things like that are stuck, please dont hesitate to ping me. I only noticed because I was doing something unrelated with 1.23 release branch.

Btw, for future reference, if things like that are stuck, please dont hesitate to ping me. I only noticed because I was doing something unrelated with 1.23 release branch.

Duly noted. Thanks again!