Page MenuHomePhabricator

Edits using Help pane can bypass sysop-issued blocks
Closed, ResolvedPublic

Description

Hi,

I've found edits made using Help pane (and possibly other Growth-made products) can bypass sysop-issued blocks, see https://cs.wikipedia.org/w/index.php?title=Wikipedie:Pot%C5%99ebuji_pomoc&diff=prev&oldid=17175968, made by Daniel Hut. This user was blocked on 16 April 12:33 CEST by sysop OJJ. This question was posted on 26 April, which is _after_ this block was issued.

I don't think users should be able to use this module when blocked. Theoretically, we can amend the block interface to allow sysops to block all edits but questions, or simply ban edits by this interface.

Martin

Details

Related Gerrit Patches:
mediawiki/extensions/GrowthExperiments : wmf/1.34.0-wmf.1QuestionPoster: Check permissions and EditFilterMergedContent hook
mediawiki/extensions/GrowthExperiments : masterQuestionPoster: Check permissions and EditFilterMergedContent hook

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 27 2019, 10:37 AM
Urbanecm triaged this task as High priority.Apr 27 2019, 10:38 AM

Boldly triaging the task, bypassing blocks is rather urgent issue I think.

Urbanecm renamed this task from Edits using Help pane bypass sysop-issued blocks to Edits using Help pane can bypass sysop-issued blocks.Apr 27 2019, 10:38 AM
Krenair set Security to Software security bug.Apr 27 2019, 4:43 PM
Krenair added a project: Security.
Krenair changed the visibility from "Public (No Login Required)" to "Custom Policy".
Krenair added a subscriber: Krenair.
kostajh claimed this task.Apr 29 2019, 1:33 PM
kostajh moved this task from Incoming to In Progress on the Growth-Team (Current Sprint) board.

Catching any exception like that and replacing it with a generic permission denied seems like a bad idea.

I wish there was a better way to make an edit on behalf of a user with all the permission checks etc. This checks permissions, but I don't think it would check AbuseFilter for example. The only way that I'm aware of that does everything correctly is making an internal API call to ApiEditPage.

Catching any exception like that and replacing it with a generic permission denied seems like a bad idea.

That's fair. It looks like the exception is only thrown for an invalid rigor parameter in getPermissionErrorsInternal so I'm removing that try/catch.

Looking into ApiEditPage now.

This version runs EditFilterMergedContent hook; the same awkward implementation pattern from ApiEditPage and SpecialChangeContentModel is used. I've tested this patch with AbuseFilter, site-wide block, and partial block for a specific title.

Catrope changed the visibility from "Custom Policy" to "Public (No Login Required)".Apr 29 2019, 8:40 PM

Patch was deployed to production at 20:36 UTC; now in Gerrit as https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/507201

Change 507201 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] QuestionPoster: Check permissions and EditFilterMergedContent hook

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

Change 507203 had a related patch set uploaded (by Catrope; owner: Kosta Harlan):
[mediawiki/extensions/GrowthExperiments@wmf/1.34.0-wmf.1] QuestionPoster: Check permissions and EditFilterMergedContent hook

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

kostajh added a subscriber: Etonkovidova.

Change 507203 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@wmf/1.34.0-wmf.1] QuestionPoster: Check permissions and EditFilterMergedContent hook

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

Technically, it works. I can't post a question. Do we want users to get such nothing-telling error message?

Maybe we should display help pane without "ask a question" capability for blocked users? So they can search help on-wiki, but not edit by asking questions?

Right, improved error handling would be nice, but not in scope of yesterday's quick fix. @Urbanecm could you please file a new issue for improved error handling and tag with GrowthExperiments-Help panel ? Thanks!

In case when an edit hits Abuse filter there will be the same message as in @Urbanecm comment. For an edit that hit Abuse filter on a normal page, the message is much more specific:

Right, improved error handling would be nice, but not in scope of yesterday's quick fix. @Urbanecm could you please file a new issue for improved error handling and tag with GrowthExperiments-Help panel ? Thanks!

Certainly, filled as T222201.

In case when an edit hits Abuse filter there will be the same message as in @Urbanecm comment. For an edit that hit Abuse filter on a normal page, the message is much more specific:

Filled that part as T222204. Note the error message is (can be) filter-specific.

Etonkovidova closed this task as Resolved.May 2 2019, 4:52 PM
Etonkovidova moved this task from QA to Needs PM Review on the Growth-Team (Current Sprint) board.

Checked for site-wide blocked users and users blocked from specific namespaces (Wikipedia, User talk, Help).

Anomie added a subscriber: Anomie.May 24 2019, 2:38 PM

The only way that I'm aware of that does everything correctly is making an internal API call to ApiEditPage.

Note that making internal calls like that is a "code smell", though. I started trying to abstract the business logic of it from the API "front-end" in gerrit:460436, but priorities and team assignments changed and I haven't had time to work on it for a while.

Note that making internal calls like that is a "code smell", though. I started trying to abstract the business logic of it from the API "front-end" in gerrit:460436, but priorities and team assignments changed and I haven't had time to work on it for a while.

Completely agreed. It'd be great to have something that isn't as code-smelly as doing an internal API call and also doesn't skip half the steps we need, and it looks like your EditController class would be that.