Maniphest T282070

After unblocking autoblock, Special:Log and Special:RecentChanges gives ParameterAssertionException: Bad value for parameter $dbKey
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	tstarling
	May 6 2021, 12:49 AM

Description

I noticed this on my local test wiki and then reproduced it in production at https://test.wikipedia.org/wiki/Special:Log . Steps to reproduce:

A user edits
Block that user
Go to Special:BlockList
Unblock the autoblock
Go to Special:Log

It probably affects everything that shows log entries.

[4377a5c9-59b7-4697-a66b-d813f1b8c1c3] /wiki/Special:Log Wikimedia\Assert\ParameterAssertionException: Bad value for parameter $dbKey: must be a valid title: #20411

Backtrace:

from /srv/mediawiki/php-1.37.0-wmf.4/vendor/wikimedia/assert/src/Assert.php(72)
#0 /srv/mediawiki/php-1.37.0-wmf.4/includes/page/PageReferenceValue.php(75): Wikimedia\Assert\Assert::parameter(boolean, string, string)
#1 /srv/mediawiki/php-1.37.0-wmf.4/includes/page/PageIdentityValue.php(56): MediaWiki\Page\PageReferenceValue->__construct(integer, string, boolean)
#2 /srv/mediawiki/php-1.37.0-wmf.4/includes/cache/LinkBatch.php(275): MediaWiki\Page\PageIdentityValue->__construct(integer, integer, string, boolean)
#3 /srv/mediawiki/php-1.37.0-wmf.4/includes/cache/LinkBatch.php(214): LinkBatch->addResultToCache(LinkCache, Wikimedia\Rdbms\ResultWrapper)
#4 /srv/mediawiki/php-1.37.0-wmf.4/includes/cache/LinkBatch.php(186): LinkBatch->executeInto(LinkCache)
#5 /srv/mediawiki/php-1.37.0-wmf.4/includes/logging/LogPager.php(433): LinkBatch->execute()
#6 /srv/mediawiki/php-1.37.0-wmf.4/includes/pager/IndexPager.php(599): LogPager->getStartBody()
#7 /srv/mediawiki/php-1.37.0-wmf.4/includes/specials/SpecialLog.php(274): IndexPager->getBody()
#8 /srv/mediawiki/php-1.37.0-wmf.4/includes/specials/SpecialLog.php(156): SpecialLog->show(FormOptions, array)
#9 /srv/mediawiki/php-1.37.0-wmf.4/includes/specialpage/SpecialPage.php(646): SpecialLog->execute(NULL)
#10 /srv/mediawiki/php-1.37.0-wmf.4/includes/specialpage/SpecialPageFactory.php(1396): SpecialPage->run(NULL)
#11 /srv/mediawiki/php-1.37.0-wmf.4/includes/MediaWiki.php(313): MediaWiki\SpecialPage\SpecialPageFactory->executePath(string, RequestContext)
#12 /srv/mediawiki/php-1.37.0-wmf.4/includes/MediaWiki.php(916): MediaWiki->performRequest()
#13 /srv/mediawiki/php-1.37.0-wmf.4/includes/MediaWiki.php(550): MediaWiki->main()
#14 /srv/mediawiki/php-1.37.0-wmf.4/index.php(53): MediaWiki->run()
#15 /srv/mediawiki/php-1.37.0-wmf.4/index.php(46): wfIndexMain()
#16 /srv/mediawiki/w/index.php(3): require(string)
#17 {main}

The invalid title is in log_namespace/log_title so is added to LinkBatch at LogPager.php:425.

Details

Subject	Repo	Branch	Lines +/-
PageIdentityValue: apply basic validation of titles	mediawiki/core	master	+107 -15
LinkBatch: skip bad input	mediawiki/core	wmf/1.37.0-wmf.4	+84 -24
Remove harmful validation regex in PageReferenceValue	mediawiki/core	wmf/1.37.0-wmf.4	+1 -17
LinkBatch: skip bad input	mediawiki/core	master	+84 -24
Remove harmful validation regex in PageReferenceValue	mediawiki/core	master	+1 -17

Customize query in gerrit

Related Objects
Search...

Status	Subtype	Assigned	Task
Resolved	Release	brennen	T281145 1.37.0-wmf.4 deployment blockers
Resolved		daniel	T282070 After unblocking autoblock, Special:Log and Special:RecentChanges gives ParameterAssertionException: Bad value for parameter $dbKey
Open		None	T282088 Create a way to consistently validate titles (dbKeys) for use in the database.

Event Timeline

tstarling created this task.May 6 2021, 12:49 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 6 2021, 12:49 AM

tstarling triaged this task as Unbreak Now! priority.May 6 2021, 12:50 AM

tstarling moved this task from Inbox to Doing(WIP:5) on the Platform Team Workboards (Clinic Duty Team) board.

Likely caused by c829bfc8dde66094638996127be6ac4b233d5dde which added PageIdentityValue construction for bad links at line 275 of LinkBatch.php.

I don't really understand why PageReferenceValue::__construct() is introducing yet another ad-hoc title validation regex, redundant with the similar regex in TitleValue::assertValidSpec() and with proper title validation in MediaWikiTitleCodec::splitTitleString(). Why do we need (at least) three definitions of title validity?

Change 685623 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Remove harmful validation regex in PageReferenceValue

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

gerritbot added a project: Patch-For-Review.May 6 2021, 1:10 AM

tstarling added a subscriber: • Pchelolo.May 6 2021, 1:36 AM

Please don't close this after patch is merged, there's a problem in LinkBatch as well this validation exposes. We'd want to look at that.

Change 685623 merged by jenkins-bot:

[mediawiki/core@master] Remove harmful validation regex in PageReferenceValue

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

Change 685596 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@wmf/1.37.0-wmf.4] Remove harmful validation regex in PageReferenceValue

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

Change 685596 merged by jenkins-bot:

[mediawiki/core@wmf/1.37.0-wmf.4] Remove harmful validation regex in PageReferenceValue

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.5; 2021-05-11).May 6 2021, 5:00 AM

Maintenance_bot removed a project: Patch-For-Review.May 6 2021, 5:10 AM

Mentioned in SAL (#wikimedia-operations) [2021-05-06T05:32:16Z] <tstarling@deploy1002> Synchronized php-1.37.0-wmf.4/includes/page/PageReferenceValue.php: fixing T282070 RC/log breakage due to unblocking autoblocks (duration: 01m 09s)

I confirmed that Special:Log and Special:RecentChanges on testwiki works again.

In T282070#7064426, @tstarling wrote:

I don't really understand why PageReferenceValue::__construct() is introducing yet another ad-hoc title validation regex, redundant with the similar regex in TitleValue::assertValidSpec() and with proper title validation in MediaWikiTitleCodec::splitTitleString(). Why do we need (at least) three definitions of title validity?

The only proper definition of validity is MediaWikiTitleCodec::splitTitleString().

Assertions in the constructors of TitleValue and PageReferenceValue are intended to expose situations in which the caller failed to apply this validation, to prevent data corruption down the line. This of course has the side-effect of also exposing corrupt data that has already made it into the database. Which is a good thing to detect, but of course shouldn't break the site. I'll work on it.

In effect, the assertion checks the assumption that things coming from the database are known to be valid. Which I keep learning is not an assumption that can be made safely...

But in this case it's not because of the database, that's the issue: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/682144/3/includes/cache/LinkBatch.php#274

We are making PageIdenitityValue out of titles that were NOT found in the DB, but passed TitleValue validation - that's obviously incorrect, TitleValue can be much broader then PageidentityValue.

The api shows that the database contains

{
    "logid": 260450,
    "ns": 2,
    "title": "User:#20411",
    "pageid": 0,
    "logpage": 0,
    "params": {},
    "type": "block",
    "action": "unblock",
    "user": "Tim Starling",
    "timestamp": "2021-05-06T00:36:28Z",
    "comment": ""
},

where User:#20411 is an invalid title (Stored with log_namespace = 2 and log_title = #20411). LinkBatch is using TitleValue::tryNew which allows to have a # in the dbkey part and created a title value for it, which than gets PageIdentityValue created for and fails.

TitleValue needs to check for # in dbkey and make that invalid? It is possible to use the LinkTarget on Title::castFromLinkTarget, but when calling Title::isValid that returns false/invalid.

In T282070#7066064, @Pchelolo wrote:

We are making PageIdenitityValue out of titles that were NOT found in the DB, but passed TitleValue validation - that's obviously incorrect, TitleValue can be much broader then PageidentityValue.

Yes, that needs better handling

In T282070#7067169, @Umherirrender wrote:

TitleValue needs to check for # in dbkey and make that invalid? It is possible to use the LinkTarget on Title::castFromLinkTarget, but when calling Title::isValid that returns false/invalid.

TitleValue will consider "#foo" valid, because it is: [[#foo]] is valid a relative section link. The dbKey here would be empty. Which, again, is valid in the context of TitleValue. "User:#foo" however shouldn't be considered valid.

Looks very similar to another new wmf.4 production error, but with a different parameter name:
T282180: Special:Log/move: ParameterTypeException: Bad value for parameter $link: must be a MediaWiki\Linker\LinkTarget|MediaWiki\Page\PageReference

If similar enough, consider merging, or otherwise looking at separately.

Krinkle added a parent task: T281145: 1.37.0-wmf.4 deployment blockers.May 7 2021, 3:42 AM

In T282070#7068420, @Krinkle wrote:

Looks very similar to another new wmf.4 production error, but with a different parameter name:
T282180: Special:Log/move: ParameterTypeException: Bad value for parameter $link: must be a MediaWiki\Linker\LinkTarget|MediaWiki\Page\PageReference

If similar enough, consider merging, or otherwise looking at separately.

No, I think this has a different cause.

Change 686481 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] LinkBatch: skip bad input

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

gerritbot added a project: Patch-For-Review.May 7 2021, 11:10 AM

Change 686591 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] PageIdentityValue: apply basic validation of titles

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

Train blockers are UBN!.

Change 686481 merged by jenkins-bot:

[mediawiki/core@master] LinkBatch: skip bad input

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

Change 685901 had a related patch set uploaded (by Jforrester; author: Daniel Kinzler):

[mediawiki/core@wmf/1.37.0-wmf.4] LinkBatch: skip bad input

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

For clarity, is LinkBatch: skip bad input sufficient to unblock wmf.4?

Change 685901 merged by jenkins-bot:

[mediawiki/core@wmf/1.37.0-wmf.4] LinkBatch: skip bad input

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

Mentioned in SAL (#wikimedia-operations) [2021-05-07T17:50:51Z] <brennen@deploy1002> Synchronized php-1.37.0-wmf.4/includes/cache/LinkBatch.php: Backport: [[gerrit:685901|LinkBatch: skip bad input (T282180 T282070)]] (duration: 01m 06s)

ReleaseTaggerBot edited projects, added MW-1.37-notes (1.37.0-wmf.4; 2021-05-04); removed MW-1.37-notes (1.37.0-wmf.5; 2021-05-11).May 7 2021, 6:00 PM

In T282070#7069905, @brennen wrote:

For clarity, is LinkBatch: skip bad input sufficient to unblock wmf.4?

If wmf.4 is blocked on this ticket, I believe so, yes.
Tim's patch from last night should have been sufficient for that, really.

Is there a good way to test autoblocks on beta?...

To verify, check https://test.wikipedia.org/wiki/Special:Log?type=block&user=&page=User%3A%2320411&wpdate=&tagfilter=&wpfilters%5B%5D=newusers. With admin rights, you should see a log line saying something like "Tim Starling unblocked #20411" at the bottom. If you see that, the bug is fixed. If you see nothing, you don't have sufficient rights. If the bug is not fixed and you have sufficient rights, you get a 500.

Confirm, Resolved as far as the train blocker part matters. If there's follow-up work, please re-open and detach.

Change 686591 merged by jenkins-bot: