Page MenuHomePhabricator

Non logged in users are able to review and patrol pages on enwiki using the API
Closed, ResolvedPublicPRODUCTION ERROR

Description

See log https://en.wikipedia.org/wiki/Special:Log?type=patrol&user=2606%3AA000%3A83C5%3A7200%3A556E%3A7DD2%3ABCA6%3A57AD&page=&wpdate=&tagfilter=

Not sure if this is a "patrol" problem and/or a "page triage" problem

Please also assess for security bug

Event Timeline

Xaosflux created this task.Oct 3 2018, 1:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 3 2018, 1:36 PM
JJMC89 added a subscriber: JJMC89.Oct 3 2018, 1:59 PM
JJMC89 renamed this task from non logged in user is able to review / patrol pages on enwiki to Non logged in users are able to review and patrol pages on enwiki using the API.Oct 3 2018, 2:16 PM
JJMC89 edited projects, added PageCuration; removed MediaWiki-extensions-Patroller.
JJMC89 added a subscriber: MMiller_WMF.
Restricted Application added a project: Growth-Team. · View Herald TranscriptOct 3 2018, 2:16 PM
JJMC89 added a comment.Oct 3 2018, 2:21 PM

While not logged in: I couldn't see the buttons for reviewing or patrolling while using the desktop web interface, but it is possible to use the API for reviewing and patrolling.

This comment was removed by Abelmoschus_Esculentus.
kostajh added a subscriber: kostajh.Oct 3 2018, 2:37 PM

Noting that PageTriage Action API checks the patrol right for a user, which is why PageCuration is affected.

kostajh set Security to Software security bug.Oct 3 2018, 3:09 PM
kostajh changed the visibility from "Public (No Login Required)" to "Custom Policy".

It looks like anonymous users can patrol articles and thus also use APIs that rely on checking "patrol" right for the anonymous user. My suspicion is that bd3bf5d2a7e6b4daf6c7ee10c505832eae83a539 introduced this.

Reedy merged a task: Restricted Task.Oct 3 2018, 3:46 PM
Reedy added subscribers: Domdeparis, SBisson, Krenair.
Reedy added a subscriber: Tgr.Oct 3 2018, 4:42 PM
kostajh moved this task from Backlog / Other to Patch pending review on the acl*security board.

I'll work on a test next but wanted to get this up here first.

Here's a version with a test.

Anomie added a subscriber: Anomie.Oct 3 2018, 5:49 PM

Here's a version with a test.

Change to Title.php looks good, Code-Review+1. Didn't look too closely at the test changes, but they pass for me locally.

Catrope added a subscriber: Catrope.Oct 3 2018, 6:08 PM

Looks good to me. I'd prefer it if the test changes included a test for the case when the user is, but we can fix that in a follow-up patch.

In about 30 minutes I'll start deploying this and doing the rest of the security patch process.

Reedy added a subscriber: Reedy.Oct 3 2018, 6:28 PM

Looks good to me. I'd prefer it if the test changes included a test for the case when the user is, but we can fix that in a follow-up patch.

In about 30 minutes I'll start deploying this and doing the rest of the security patch process.

Cheers Roan.

Obviously after it's on the cluster (both branches), it can just be made public into gerrit and merged as it was introduced during the 1.32 development cycle

That wasn't obvious to me, so thanks for pointing that out :)

Also, I just realized why patrol checks aren't completely broken everywhere. There's some inconsistent and bizarre behavior going on, but I now understand why.

> $user = User::newFromName('Catrope');

> var_dump($user->isAllowed('patrol'));
bool(false)

> $title = Title::newFromText('User:Catrope');

> var_dump($title->getUserPermissionsErrors('patrol', $user));
array(0) {
}

> var_dump($title->userCan('patrol', $user));
bool(false)

Since getUserPermissionsErrors returns an empty array, you'd expect userCan to return true. But that doesn't happen, because userCan invokes getUserPermissionsErrorsInternal with $short=true, which stops as soon as it encounters an error. That means that the broken JS/CSS permissions code doesn't get a chance to wipe out that error later, because it's never called.

Patch is deployed, making this public now.

Catrope changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 3 2018, 6:51 PM

Change 464386 had a related patch set uploaded (by Catrope; owner: Kosta Harlan):
[mediawiki/core@master] SECURITY: Fix permissions check for patrol action

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

@Xaosflux and @JJMC89 thank you for reporting this.

Change 464400 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/PageTriage@master] Tests: Check if anonymous users can use action API

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

kostajh moved this task from Inbox to Current Sprint on the Growth-Team board.Oct 3 2018, 7:50 PM
kostajh edited projects, added Growth-Team (Current Sprint); removed Growth-Team.
kostajh moved this task from Incoming to Code Review on the Growth-Team (Current Sprint) board.

Change 464386 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Fix permissions check for patrol action

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

Change 464423 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Move test assertion to mirror parameter order

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

mysql:research@s3-analytics-slave [enwiki]> SELECT COUNT(log_id) FROM logging WHERE log_user = 0 AND log_type = 'pagetriage-curation';  
+---------------+
| COUNT(log_id) |
+---------------+
|            25 |
+---------------+
1 row in set (0.09 sec)

mysql:research@s3-analytics-slave [enwiki]> SELECT COUNT(log_id) FROM logging WHERE log_user = 0 AND log_type = 'patrol';             
+---------------+
| COUNT(log_id) |
+---------------+
|            24 |
+---------------+
1 row in set (0.03 sec)

Looks like this wasn't widely abused.

Change 464423 merged by jenkins-bot:
[mediawiki/core@master] Move test assertion to mirror parameter order

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

Change 464423 merged by jenkins-bot:
[mediawiki/core@master] Move test assertion to mirror parameter order

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

This broke the Travis CI tests for all PHP/DB versions:

-        1 => '[[Traviswiki:Administrators|Administrators]]'
+        1 => '[[MyWiki:Administrators|Administrators]]'

Looks like its currently getting the state of the configuration in a way that assumes the default database name, rather than the actual one. This has bit us a few times before because our Jenkins jobs actually use the default dbname for its actual db.

Change 464566 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/core@master] Tests: Simplify badaccess group check for patrol action

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

Catrope added a comment.EditedOct 4 2018, 5:27 PM

Looks like its currently getting the state of the configuration in a way that assumes the default database name, rather than the actual one. This has bit us a few times before because our Jenkins jobs actually use the default dbname for its actual db.

How is it possible, then, that lots of other pre-existing tests using things like [[$prefix:Administrators|Administrators]] (see here), where $prefix is generated exactly the same way, don't fail in Travis, but these do?

Hm.. yeah. Might have to do with (missing) MediaWikiService resets.

But, it's quite difficult now to prevent other failures from creeping in, which almost always happens when they are failing for over day. Maybe looking at MediaWikiService will lead to a simple solution, but if not, could the failing test be disabled for the time being?

Maybe looking at MediaWikiService will lead to a simple solution, but if not, could the failing test be disabled for the time being?

@Krinkle what are your thoughts on the patch I posted in T206130#4641817 ?

Tgr added a comment.Oct 4 2018, 10:29 PM

Ugh, I am very sorry about this :( I broke it in gerrit 455481 and meant to fix it in 456140 but somehow did not realize the security implications and left the patch stalled when I got other urgent stuff to do, and forgot about it...

Change 465537 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] title: Disable flaky TitlePermission test cases

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

Change 465537 abandoned by Krinkle:
title: Fix flaky TitlePermission test cases

Reason:
I don't know what's wrong with it. But I do know the bloody thing is more complicated than it should be.

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

Change 465537 restored by Krinkle:
title: Fix flaky TitlePermission test cases

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

Change 465537 abandoned by Krinkle:
title: Fix flaky TitlePermission test cases

Reason:
I give up. Something somewhere is messing with the global state in the shared job. Presumably in such a way that TestCase is unable to reset.

That probably means our resets are too brave by stashing and restoring, instead of actually clearing/resetting.

Anyhow, given we've spent 7 days and over a dozen commits trying to do this, I give up. Kosta has a patch that reduces the test to not needlessly assert what the wgSitename value is, which makes a ton of sense, given the test isn't about that at all. I'm merging that now.

We can continue pondering about why we can't have nice resets after that, including how to make them work with our existing problems in SimpleCaptcha, Flow, and Wikibase.

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

Change 464566 merged by jenkins-bot:
[mediawiki/core@master] Tests: Simplify badaccess group check for patrol action

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

SBisson assigned this task to kostajh.Oct 10 2018, 5:33 PM
SBisson moved this task from Code Review to QA on the Growth-Team (Current Sprint) board.

Change 464400 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Tests: Check if anonymous users can use action API

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

Krinkle removed a subscriber: Krinkle.Oct 10 2018, 11:24 PM
Etonkovidova closed this task as Resolved.Oct 12 2018, 12:17 AM
Etonkovidova added a subscriber: Etonkovidova.

Checked in betalabs and testwiki.

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:08 PM
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:38 PM