Page MenuHomePhabricator

Loading Special:IPContributions for a username target displays the block log extracts
Closed, ResolvedPublic2 Estimated Story PointsBUG REPORT

Description

Special:IPContributions is a page used to find temporary account IP contributions. The form validates that the target input is a valid IP address or range. However, the error page contains the block log extracts as shown below:

image.png (1,612×971 px, 163 KB)

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

  1. Install CheckUser
  2. Block a user, noting down the username
  3. Load Special:IPContributions
  4. Enter the username from step 2 into target into the IP address or range field
  5. Click Search

What happens?:
The block log extract is shown for the username below the error banner

What should have happened instead?:
No block log should be shown for the user, as it is being shown on an error page.

Event Timeline

Hi folks! I stumbled onto this bug report because the backlog was linked from the Trust and Safety Product engineer opening. After noodling around I believe I've found the cause.

The block logs are displayed based on $shouldShowBlocks in ContributionsSpecialPage.php in core.

Whereas the Special:IPContributions error that appears when entering a username instead of an IP address is controlled by the following code block beginning on line 167 in SpecialIPContributions.php:

parent::execute( $par );

$target = $this->opts['target'] ?? null;
if (
	$target &&
	!$this->lookupUtils->isValidIPOrRange( $target ) &&
	!IPUtils::isIPAddress( $target )
) {
	$this->getOutput()->setSubtitle(
		new MessageWidget( [
			'type' => 'error',
			'label' => new HtmlSnippet(
				$this->msg( 'checkuser-ip-contributions-target-error-no-ip-banner', $target )->parse()
			)
		] )
	);
} else {
	$this->getOutput()->addJsConfigVars( 'wgIPRangeTarget', $target );
}

The parent class' execute method is run before the logic determining whether or not to show the error (and thus ensures the block logs are displayed regardless).

I played around with managing the logic for this via an additional variable in ContributionsSpecialPage.php:

$shouldShowBlocks = !$this->including() && $this->opts['shouldShowBlocks'];

However, $target relies on the parent class being run first in order to properly handle the logic in the if statement. I figure I shouldn't be presumptuous and start changing code in core without first bringing this to other people's attention. Hope I saved somebody a few minutes of digging at least!

Hi folks! I stumbled onto this bug report because the backlog was linked from the Trust and Safety Product engineer opening. After noodling around I believe I've found the cause.

Thanks for taking a look at this!

I believe we should be able to fix this by adding a method to ContributionsSpecialPage that determines the value of $shouldShowBlocks (something like ::shouldShowBlockLogExtract). Then SpecialIPContributions can override it to return false if the IP target is not valid. When that method is called, we can assume that ->opts['target'] is set.

Change #1072765 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/core@master] Add ContributionsSpecialPage::shouldShowBlockLogExtract

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

Change #1072772 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] Hide block log extract when target is invalid for IPContributions

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

Change #1072765 merged by jenkins-bot:

[mediawiki/core@master] Add ContributionsSpecialPage::shouldShowBlockLogExtract

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

Change #1072772 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Hide block log extract when target is invalid for IPContributions

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

@Djackson-ctr found a bug during QA where the global blocking block notice is still displayed. Moving this back to In progress.

Change #1073230 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/GlobalBlocking@master] Hide GlobalBlocking log extract when core block log extract hidden

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

Change #1073231 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/core@master] Make ContributionsSpecialPage::shouldShowBlockLogExtract public

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

Change #1073239 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] Make SpecialIPContributions::shouldShowBlockLogExtract public

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

Change #1073239 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Make SpecialIPContributions::shouldShowBlockLogExtract public

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

Change #1073231 merged by jenkins-bot:

[mediawiki/core@master] Make ContributionsSpecialPage::shouldShowBlockLogExtract public

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

Change #1073230 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Hide GlobalBlocking log extract when core block log extract hidden

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

QA has been completed, and the new code changes are functioning as expected ("No block log should be shown for the user, as it is being shown on an error page").

image.png (1,133×291 px, 28 KB)