@Dylsss Thanks for the report. I can confirm the behavior with globally-blocked IPs. Local blocks work fine AFAICS:

I'm adding @Michael and @Lucas_Werkmeister_WMDE to this task: both were recently active in the repository and contributed with a significant amount of commit. Can you have a look please, or add someone else who is able to investigate?

This does fix the error, but leaves some room for fatal errors. Leaving here to trigger some discussion about this approach.

A real fix should likely call PermissionManager::userCan() at some point (which checks global blocks). I would also recommend moving to FormSpecialPage, which provides a framework you can use to simplify the code.

This whole PermissionManager API is confusing to me, tbh. If calling getBlock() is not enough (because it doesn’t include global blocks), does that mean PermissionManager::isBlockedFrom() is also buggy? (As far as I can tell, it doesn’t explicitly handle global blocks either; it runs the UserIsBlockedFrom hook, but codesearch isn’t showing any uses of that hook that look like they add the global blocks part.) And how is one supposed to use userCan() when there’s apparently no obvious way to get a throwable out of it? (getPermissionErrors() exists, but returns a list of message specifiers.)

(Edit: I didn’t notice, while writing this comment, that isBlockedFrom() is actually the method used in the existing code, which Urbanecm’s patch replaces with a different call.)

In T296578#7532508, @Urbanecm wrote:

T296578.patch1 KBDownload

This does fix the error, but leaves some room for fatal errors. Leaving here to trigger some discussion about this approach.

🤔 missing test aside, where do you see the room for a fatal error created by your patch?

Also, I echo Lucas' remarks about PermissionManager. There is no reason assume that

	/**
	 * Check if user is blocked from editing a particular article. If the user does not
	 * have a block, this will return false.
	 *
	 * @param User $user
	 * @param PageIdentity|LinkTarget $page Title to check
	 * @param bool $fromReplica Whether to check the replica DB instead of the primary DB
	 *
	 * @return bool
	 */
	public function isBlockedFrom( User $user, $page, $fromReplica = false ) {

is only checking local blocks.

In T296578#7533610, @Michael wrote:

In T296578#7532508, @Urbanecm wrote:

T296578.patch1 KBDownload

This does fix the error, but leaves some room for fatal errors. Leaving here to trigger some discussion about this approach.

🤔 missing test aside

Note: Tests shouldn't be included in security patches. A security patch should be as small as possible, as it isn't in gerrit.

where do you see the room for a fatal error created by your patch?

Explained in the commit message :-). If an user has the edit right, userCan says false and the user is not actually blocked. Calling throw new UserBlockedError with null instead of a block will fatal error.

Also, I echo Lucas' remarks about PermissionManager. There is no reason assume that

	/**
	 * Check if user is blocked from editing a particular article. If the user does not
	 * have a block, this will return false.
	 *
	 * @param User $user
	 * @param PageIdentity|LinkTarget $page Title to check
	 * @param bool $fromReplica Whether to check the replica DB instead of the primary DB
	 *
	 * @return bool
	 */
	public function isBlockedFrom( User $user, $page, $fromReplica = false ) {

is only checking local blocks.

I'm likely biased here, but since there are no actual global blocks (the global blocking interface is way more restricted than local blocks; ie. no user accounts or no partial blocks are allowed), it kinda makes sense. Can be at least made more explicit in the docs, definitely.

In T296578#7533518, @Lucas_Werkmeister_WMDE wrote:

This whole PermissionManager API is confusing to me, tbh. If calling getBlock() is not enough (because it doesn’t include global blocks), does that mean PermissionManager::isBlockedFrom() is also buggy? (As far as I can tell, it doesn’t explicitly handle global blocks either; it runs the UserIsBlockedFrom hook, but codesearch isn’t showing any uses of that hook that look like they add the global blocks part.) And how is one supposed to use userCan() when there’s apparently no obvious way to get a throwable out of it? (getPermissionErrors() exists, but returns a list of message specifiers.)

I don't understand why a throwable is needed? The current code throws manually when it finds a (local) block. I think the same can be done with userCan() too? Either PermissionsError can be thrown, or ErrorPageError if you need a very generic error message.

As an ideal solution, I'd switch to FormSpecialPage. That class doesn't seem to solve special pages either ATM, but that will be likely easier to fix (filled {T296593} for it).

In T296578#7533701, @Urbanecm wrote:

Note: Tests shouldn't be included in security patches. A security patch should be as small as possible, as it isn't in gerrit.

Gotcha, makes sense. I'll add an item in the acceptance criteria to add tests after the patch has been merged and released as public, so that we don't forget about it.

🤔 missing test aside, where do you see the room for a fatal error created by your patch?

Explained in the commit message :-). If an user has the edit right, userCan says false and the user is not actually blocked. Calling throw new UserBlockedError with null instead of a block will fatal error.

Doh! My eyes were too drawn to the code, so that I completely skipped the commit message. Thank you for explaining it 🙏. Those issues should be solvable by checking if there is some block and then maybe falling back to a more generic error. But that can probably also be done after the core security issue has been resolved and this task has become public.

That being said, looking at further usages of PermissionManager::isBlockedFrom, I think there are more places that might have similar issues, including the NewEntitySchema special page, some of EntitySchema's actions, and SpecialMergeLexemes. And also several places in core look suspicious?

In T296578#7533708, @Urbanecm wrote:

I don't understand why a throwable is needed? The current code throws manually when it finds a (local) block. I think the same can be done with userCan() too? Either PermissionsError can be thrown, or ErrorPageError if you need a very generic error message.

It looks like PermissionsError is exactly the class I was looking for, thank you. Suggested patch:

Edit: I locally ran the browser tests, which include a test for block behavior.

I would also suggest to add this function to PermissionManager, to make this pattern easier:

	public function throwPermissionErrors(
		$action,
		User $user,
		LinkTarget $page,
		$rigor = self::RIGOR_SECURE,
		$ignoreErrors = []
	) {
		$permissionErrors = $this->getPermissionErrors(
			$action, $user, $page, $rigor, $ignoreErrors );
		if ( $permissionErrors !== [] ) {
			throw new PermissionsError( $action, $permissionErrors );
		}
	}

What do you think? (If you don’t think it’s totally weird, I can also put it on Gerrit for wider discussion.)

+1 to the patch at T296578#7533904. I will try to get this deployed during today's security window (happening right now) if the first patch I'm working on allows for it.

Deployed to wmf.9. I don't have a great way to test this myself, so if @Dylsss, @Urbanecm, @Michael or @Lucas_Werkmeister_WMDE could help confirm everything looks good, that would be great. I'm not seeing any obvious errors on the page or in logstash and the patch seemed logical and trivial enough where it should work fine. If there are any issues though, we can obviously roll this back. Also now tracking at T292236 and T276237.

That being said, looking at further usages of PermissionManager::isBlockedFrom, I think there are more places that might have similar issues, including the NewEntitySchema special page, some of EntitySchema's actions, and SpecialMergeLexemes. And also several places in core look suspicious?

Yep, I was able to create a new EntitySchema item with Special:NewEntitySchema at https://test.wikidata.org/wiki/EntitySchema:E17 as a globally blocked IP.

Alright, here’s another patch updating more places that were only checking for blocks.

And I’ll upload a core patch for this little oddity:

			// reindex $permissionErrors:
			// the ignoreErrors param (ns-specialprotected) may have left holes,
			// but PermissionsError expects $errors[0] to exist

And I’ll upload a core patch for this little oddity:

			// reindex $permissionErrors:
			// the ignoreErrors param (ns-specialprotected) may have left holes,
			// but PermissionsError expects $errors[0] to exist

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/742742

In T296578#7533915, @Lucas_Werkmeister_WMDE wrote:

I would also suggest to add this function to PermissionManager, to make this pattern easier:

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/742744

In T296578#7536988, @Lucas_Werkmeister_WMDE wrote:

Alright, here’s another patch updating more places that were only checking for blocks.

T296578-2.patch6 KBDownload

I've tested this patch locally and it works as expected with

$wgHooks['getUserPermissionsErrorsExpensive'][] = function ( $title, $user, $action, &$result ) {
	if ( in_array( $action, [ 'edit', 'createpage' ] ) ) {
		$result = [ [ 'parentheses', 'ur blocked lol (expensive), with right: ' . $action ] ];
		return false;
	}
	return true;
};

However, it does not work with only

$wgHooks['UserIsBlockedGlobally'][] = function ( $user, $ip, &$blocked, &$block ) {
	$blocked = true;
};

The UserIsBlockedGlobally hook seems to be barely checked anywhere?

I guess that’s just a quirk of how global blocks are implemented… the GlobalBlocking extension also uses the getUserPermissionsErrorsExpensive hook, I think that one does the main permission checking work.

To be clear, my comment above is intended as a +1 to Lucas most recent patch.

Alright, second patch deployed as well (and added to /srv/patches too). Only deployed to wmf.9, as a rollback to wmf.7 seems exceedingly unlikely at this point; wmf.10 and wmf.11 were cancelled, and wmf.12 doesn’t exist yet.

In T296578#7540221, @Lucas_Werkmeister_WMDE wrote:

Alright, second patch deployed as well (and added to /srv/patches too). Only deployed to wmf.9, as a rollback to wmf.7 seems exceedingly unlikely at this point; wmf.10 and wmf.11 were cancelled, and wmf.12 doesn’t exist yet.

Thanks for the deploy! I assume everything looks good with the new permission checks.

Change 748076 had a related patch set uploaded (by Tobias Andersson; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@REL1_37] SECURITY: Do not let globally blocked users edit

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

Change 748079 had a related patch set uploaded (by Tobias Andersson; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@REL1_37] SECURITY: Replace more block checks permission checks

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

Change 748081 had a related patch set uploaded (by Tobias Andersson; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@REL1_36] SECURITY: Do not let globally blocked users edit

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

Change 748083 had a related patch set uploaded (by Tobias Andersson; author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/EntitySchema@REL1_36] SECURITY: Replace more block checks permission checks

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

Change 748076 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@REL1_37] SECURITY: Do not let globally blocked users edit

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

Change 748081 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@REL1_36] SECURITY: Do not let globally blocked users edit

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

Change 748079 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@REL1_37] SECURITY: Replace more block checks permission checks

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

Change 748083 merged by jenkins-bot:

[mediawiki/extensions/EntitySchema@REL1_36] SECURITY: Replace more block checks permission checks

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