Page MenuHomePhabricator

Edit on user talkpage with expired block shows Blocked-notice-logextract
Open, Needs TriagePublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Edit the talk page of a user with an expired block (show with user Neto 27 in ptwiki
  • See Blocked-notice-logextract stating the user is currently blocked and showing the log extract of a 3 year old block

What should have happened instead?:
Not shown the blocked-notice-logextract message.

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):
Screenshot with &uselang=qqx

image.png (1×2 px, 685 KB)

Event Timeline

Umherirrender added a subscriber: Umherirrender.

Is the account where the screenshot is taken from blocked/locked? I do not see that log extract.

The use of User::getBlock leads to return your block own status and than the log extract is requested for the user name of the talk page.

Special:Contributions does not show a block and that is using DatabaseBlock::newFromTarget, seems to do things right (Possible wrong code in ApiVisualEditor)

VisualEditor just uses some MediaWiki functions. https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/659b7989f01d9167aa38f10724863bd4d124b983/includes/ApiVisualEditor.php#573

If you got this message, then MediaWiki must have thought that the user is blocked. I didn't get the message when I checked just now: https://pt.wikipedia.org/w/index.php?title=Usuário(a)_Discussão:Neto_27&veaction=edit

I think the active block is not necessarily the one from the listed block entry – maybe the user was caught by some completely unrelated global block or something that isn't listed in local logs?

Change 881001 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/VisualEditor@master] Replace User::getBlock with DatabaseBlock::newFromTarget

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

What's happening?

I think the active block is not necessarily the one from the listed block entry – maybe the user was caught by some completely unrelated global block or something that isn't listed in local logs?

This is correct - I can reproduce this locally by setting a global block on my IP address, then trying to edit a user page of someone who is not blocked. If they have been blocked before, I see the latest block log entry, otherwise I see:

image.png (100×451 px, 13 KB)

The use of User::getBlock leads to return your block own status [...]

This shouldn't be the case, since the global user and some other user are treated differently:

// User::getBlockedStatus passes in request if the user being checked is the global (accessing) user:

$request = null;
if ( $this->isGlobalSessionUser() ) {
	// This is the global user, so we need to pass the request
	$request = $this->getRequest();
}
$block = MediaWikiServices::getInstance()->getBlockManager()->getUserBlock(
	$this,
	$request,
	$fromReplica,
	$disableIpBlockExemptChecking
);
// BlockManager::getUserBlock just checks DatabaseBlock::newFromTarget if no request was passed:

$ip = null;

if ( $request && $checkIpBlocks ) {
	// Case #1: checking the global user, including IP blocks
	$ip = $request->getIP();
	$isAnon = !$user->isRegistered();
	$xff = $request->getHeader( 'X-Forwarded-For' );
	$blocks = array_merge(
		DatabaseBlock::newListFromTarget( $user, $ip, $fromPrimary ),
		$this->getSystemIpBlocks( $ip, $isAnon ),
		$this->getXffBlocks( $ip, $xff, $isAnon, $fromPrimary ),
		$this->getCookieBlock( $user, $request )
	);
} else {
	// Case #2: checking the global user, but they are exempt from IP blocks
	// and cookie blocks, so we only check for a user account block.
	// Case #3: checking whether another user's account is blocked.
	$blocks = DatabaseBlock::newListFromTarget( $user, null, $fromPrimary );
}

// ...

$legacyUser = $this->userFactory->newFromUserIdentity( $user );
$this->hookRunner->onGetUserBlock( clone $legacyUser, $ip, $block );

However, the problem comes from how GlobalBlocking handles the GetUserBlock hook: when passed a null IP address, it gets the IP address from the current request. This bug will have been introduced by T257701: Add global blocks into CompositeBlocks rather than treating them separately, which disentangled global blocks from core by adding this handler.

What should we do?

I think the fix may be needed in the GlobalBlocking extension. GlobalBlockingHooks::onGetUserBlock should probably do the following:

  • If checking blocks for the global session user, GlobalBlocking should return any blocks against the request IP(s)
  • If checking blocks for a user account that is not the global user, GlobalBlocking shouldn't find any blocks. Global blocks can only be made against an IP address, and we shouldn't know some other user's IP address(es) anyway
  • If checking blocks for an IP user that is not the global user (e.g. editing the talk page for an IP user), GlobalBlocking should find any global blocks against that IP address

GlobalBlocking::getUserBlockDetails is caching the instance (844176453209b60a6510a498b09f3f2969ffa9f7), assuming it is only called for the global user, returning the block info for the global user also when blocks for other users are requested, for example when the block log extract is build for another's user page via visualeditor code (classic EditPage does not check that).

The xff check is using the global request and therefor for the global session user, but there is no protection against that in GlobalBlocking. The BlockManager in core is checking xff only for the global session user.
There is no code (no call to WebRequest::getIP) which would call the ip of the current request, only xff headers.

When the global session user has exempt rights, the cache is not filled (within GlobalBlocking::getUserBlockDetails it is filled, but the hook handler checks is as well beforehand without the cache), when the global block of the page's user is requested, the exempt check is passed with false (except the user to check has one) and the xff check of my xff header leaks into the result, because no $ip to check is given to that function. There is no code to check if the given $user could be an ip address to check against that (No calls to User::getName in that code path). So the hook behind User::getBlock is not usable to check if the ip of page "User:127.0.0.1" is global blocked or not. GlobalBlocking extension works only for a given $ip (my ip), every hook call without $ip returns no block (except the leaked xff or the leaked cache). From the given $user only the check for isAnon is used to apply the anon_only check. When visiting pages of non-existing users that would also true and could show wrong things as well (isAnon != isIP, when working with non-session users).

For Special:Contributions there is an own hook (onSpecialContributionsBeforeMainOutput) to check the global state. But the hint on user page that the user is global blocked is not possible.

Also checking exempt rights for non-session user feels wrong, it would check for my ip and using rights of other users. But that does not happen, my ip is only passed for my user, so that user rights check is just waste, but looks scary in the code. Core skips is for non-session user.

So I still say that User::getBlock is only useful to check if the global session user should do an action or is blocked for actions (directly or indirectly by autoblock/globalblock), not when the code wants to know if there is a block for that user account/user name.

Sorry, some parts could be off-topic to the requested bug report, but needed to understand the patch set.

It is time to deprecate the block parts of User:.class (cross-linking T221067: Move block-related methods on User to the BlockManager)

Change 883568 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/GlobalBlocking@master] Return early from GetUserBlock hook handler if no IP to check

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

Sorry I missed the followup questions, but I think you have solved the mystery. Since I have ipblockexempt it didn't stop me from editing, but its likely my vpn provider was blocked.

Change 883568 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/GlobalBlocking@master] Return early from GetUserBlock hook handler if no IP to check

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

I think this change is worth making to GlobalBlocking, for the reasons laid out in T326256#8543152 and T326256#8545448

What this fixes:

  • A warning is no longer shown if the user is not blocked
  • A warning is still shown if an IP address/range is globally blocked only

What's still broken:

  • ApiVisualEditor still attempts to show a log extract if it finds a block. If the block is global, a misleading log extract is shown, since global blocks do not get logged in the same way, so the most recent log extract is not the right one

Should ApiVisualEditor be aware of GlobalBlocking and check for a global block, and show a different message if so?