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.

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

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.

It appears from git bisect that 23ea2e4c2966f381eb7fd69b66a8d738bb24cc60 is where things went wrong, specifically checkUserConfigPermissions https://phabricator.wikimedia.org/rMW23ea2e4c2966f381eb7fd69b66a8d738bb24cc60#change-ykQW1ZZ6UXaj

In https://phabricator.wikimedia.org/rMW23ea2e4c2966f381eb7fd69b66a8d738bb24cc60#change-ykQW1ZZ6UXaj, we should return $errors not [] for the $action === 'patrol' check.

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

Here's a version with a test.

In T206130#4639111, @kostajh wrote:

Here's a version with a test.

0001-Permissions-Return-existing-errors-instead-of-empty-.patch5 KBDownload

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

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.

In T206130#4639385, @Catrope wrote:

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.

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

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

In T206130#4640223, @gerritbot wrote:

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

In T206130#4640349, @Krinkle wrote:

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 ?

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

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