Page MenuHomePhabricator

Echo ignores hideuser for non-revision based notifications (e.g. thanks)
Closed, ResolvedPublic


Echo only checks revdel/suppression if the notification is revision based (edit-user-talk for example). So...

  • Create user "AbusiveUsername"
  • Thank "GoodUser" a bunch of times
  • Admin hideusers "AbusiveUsername"
  • "GoodUser" looks at their notifications and sees "AbusiveUsername" everywhere instead of "username removed"

Hilariously, if you revdel the username from the edit that was thanked (GoodUser's name), Echo will properly display "username removed".

This also works with the user-rights notification type (but that is unlikely to happen practically).

I think the fix here is to check User::isHidden() in addition to the revdel flags?

Event Timeline

Legoktm created this task.Aug 27 2015, 5:17 PM
Legoktm raised the priority of this task from to Needs Triage.
Legoktm updated the task description. (Show Details)
Legoktm changed the visibility from "Public (No Login Required)" to "Custom Policy".
Legoktm changed the edit policy from "All Users" to "Custom Policy".
Legoktm changed Security from None to Software security bug.
Legoktm added a subscriber: Legoktm.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 27 2015, 5:17 PM

I'm concerned about the performance of User::isHidden(). In a worst case, we have to query the ipblocks table, hit memcache, then hit centralauth, all for a case that is 99% going to be a miss.

I think we should have keep track of the log id for user rights and thanks (need to modify the UserRights hook for this), and check the log_deleted of that, since it will be updated whenever a user is hidden. For notification types that can't be associated with either a log entry or revision id, we should fallback to User::isHidden(). We'll probably have to do that for EducationProgram's notifications (due to T96526).

@csteipp and I discussed this on IRC, and decided that using User::isHidden() is the best solution for now, because we'd only be querying the log table for log_deleted, which is equivalent to querying ipblocks once, and the notification is recent, we should be able to get the data out of memcache. And we'll avoid issues like T106219 if the local *_deleted columns are not up to date.

After writing this I realized this doesn't work for the Thanks case because it sets 'revid' to the edit that was being thanked, and that user is the current user, not the thankee.

This version handles the Thanks case properly.

I'm concerned about the performance of User::isHidden(). In a worst case, we have to query the ipblocks table, hit memcache, then hit centralauth, all for a case that is 99% going to be a miss.

I was going to say, that sounds like a good use case for the bloom filter code, but I guess that got removed.

This version handles the Thanks case properly.

Looks good to me. @csteipp, how should we handle this further?

csteipp added a subscriber: greg.Sep 1 2015, 4:09 PM

@Catrope, since you approve of that patch, let's go ahead and deploy it. It would be great if someone on the team coordinated with @greg and actually did the deploy so they can watch for issues afterward, but I'm happy to deploy it for you as well.

If you're worried about people abusing this on 3rd party wikis (it looks like there are a few on wikiapiary), then we can hold this until the next mediawiki release. But since it relatively low impact, I'd be fine if once that's deployed, go ahead and merge this patch in gerrit (and backport to all supported branches) and notify wikitech-l about it. We can just mention it in the next mediawiki release announcement for any admins who missed it on wikitech.

greg added a comment.Sep 1 2015, 8:50 PM

What kind of issues should RelEng look out for after this is deployed?

Regarding timing: Fire at will (is my normal answer for security patches, unless there's something else happening, of course). Just let us know what we should look out for before :)

dpatrick triaged this task as Medium priority.Sep 1 2015, 9:18 PM

I've deployed the patch to 1.26wmf20 and 1.26wmf21 branches, and sent an email to the ops list.

DannyH removed a subscriber: DannyH.Oct 5 2015, 10:45 PM
csteipp closed this task as Resolved.Oct 16 2015, 6:10 PM

We'll announce this later today

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 16 2015, 10:34 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.

Assigned CVE-2015-8007