Page MenuHomePhabricator

Globally blocked users can still use Thanks
Closed, ResolvedPublic

Description

Globally blocked users (using Special:GlobalBlock, provided by extension GlobalBlocking) can still use Thanks. The extension only checks for local blocks, with $user->isBlocked().

The simplest solution is to also check for $user->isBlockedGlobally(). That method is documented as: "Do not use for actual edit permission checks! This is intended for quick UI checks.", but it looks like it runs the complete checks.

Alternatively, we could introduce a new permission like 'thank', granted to all users – checks with Title::userCan() etc. will all fail if the user is globally blocked.

Event Timeline

matmarex created this task.Nov 29 2016, 6:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 29 2016, 6:14 PM
matmarex added a subscriber: DragonflySixtyseven.

@DragonflySixtyseven pointed out that this could allow for cross-wiki harassment of users active in multiple projects (especially since notifications are cross-wiki now). I guess offending accounts can be also locked, though, which prevents them from logging in (this is from extension CentralAuth), and logged out users can't send thanks.

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptNov 29 2016, 6:17 PM
Anomie added a subscriber: Anomie.Nov 29 2016, 6:17 PM

Why do we have a $user->isBlocked() method that doesn't fully indicate that the user is blocked and a $user->isBlockedGlobally() method that isn't supposed to actually be called?

No idea. They're ancient. If we wanted to change User::isBlocked() to call User::isBlockedGlobally(), we'd first have to audit all uses of the former to see whether they want to check for permissions (where the change is OK), or for presence of a block (where it may not be OK, e.g. if we're checking to see whether we should apply another block).

And User::isBlockedGlobally() is definitely supposed to be called, but for UI checks (e.g. whether we should show an error message before the user spends an hour writing an article, only to find out they can't save), rather than for permissions checks (e.g. whether we should allow the user to save said article).

This seems similar to T150796.

Bawolff triaged this task as High priority.Dec 13 2016, 10:38 PM
Bawolff added a subscriber: Bawolff.

This seems very misleading. We should have isBlocked() mean any type of block, and then we could have more specific isBlockedGlobally() and isBlockedLocally()

Ladsgroup added a subscriber: Ladsgroup.

I think fixing "isBlocked" method would fix this issue too but can we tackle this high-priority security bug for now?
I made this quick fix for thanks extension:

Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 12 2018, 1:52 PM

I think fixing "isBlocked" method would fix this issue too but can we tackle this high-priority security bug for now?
I made this quick fix for thanks extension:

I approve of this patch.

@Bawolff / @Reedy et al, what's the protocol for getting this patch onto the cluster and into the repo?

Catrope moved this task from Inbox to Blocked on the Growth-Team board.
Reedy added a comment.Aug 21 2018, 2:17 AM

@Bawolff / @Reedy et al, what's the protocol for getting this patch onto the cluster and into the repo?

So we don't bundle this in the tarballs, so the process is easier.

Do we still agree this is a High priority? I guess we probably want to avoid BEANS because it's an easy to use abuse vector, so probably don't want to have it in the open.

So... It needs deploying on all active WMF branches (just missed this weeks security window, but feel free to deploy yourselves if you want), and then it can go into gerrit into master. Then cherry picked to any supported MW branches too. No real need to cleanup the wmf branches in use particularly (ie no need to cherry pick the patch to the branches on gerrit). It'll ride the train in newer versions

Optionally you can send a message mediawiki-announce (et al) after it's released.

Thanks @Reedy! I'll deploy it myself once I get my patch on T199993 (another non-bundled extension security issue) reviewed.

Just for documentation purposes on here because I know some task readers were confused (others are not, it's just not clear in the writing :) ): Global blocks are IP based instead of user based so there are no "globally blocked users" in this case. This particular bug comes up when someone is editing on an (unblocked) local account but connecting via a globally blocked IP that is "hard" blocked (does not allow editing even when logged in unless you have the ip block exemption user right). Still should be fixed (and Roan and I chatted so i know he was fixing it with this in mind) just clarifying for the audience :).

Catrope changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 23 2018, 6:37 PM

Change 454884 had a related patch set uploaded (by Catrope; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@master] Security: Disable thank when the user is globally blocked

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

Change 454884 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] Security: Disable thank when the user is globally blocked

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

Catrope closed this task as Resolved.Aug 23 2018, 7:32 PM
Catrope claimed this task.

Change 454911 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_31] Security: Disable thank when the user is globally blocked

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

Change 454916 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_30] Security: Disable thank when the user is globally blocked

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

Change 454917 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_29] Security: Disable thank when the user is globally blocked

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

Change 454918 had a related patch set uploaded (by Reedy; owner: Amir Sarabadani):
[mediawiki/extensions/Thanks@REL1_27] Security: Disable thank when the user is globally blocked

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

Change 454917 abandoned by Reedy:
Security: Disable thank when the user is globally blocked

Reason:
No one cares about REL1_29 anymore

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

Change 454916 merged by Umherirrender:
[mediawiki/extensions/Thanks@REL1_30] Security: Disable thank when the user is globally blocked

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

Change 454918 merged by Umherirrender:
[mediawiki/extensions/Thanks@REL1_27] Security: Disable thank when the user is globally blocked

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

Change 454911 merged by Umherirrender:
[mediawiki/extensions/Thanks@REL1_31] Security: Disable thank when the user is globally blocked

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