Page MenuHomePhabricator

Unable to get edits of ip addresses in CU
Closed, ResolvedPublic

Description

Unable to get edits of certain ip address in Check User, getting Exception encountered, of type "Wikimedia\Assert\ParameterAssertionException"

Refer https://www.wikidata.org/wiki/Special:CheckUserLog .

Currently unable to decipher what the trigger is for 'some' IPs working and others not.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 2 2016, 6:08 AM
Shanmugamp7 renamed this task from Unable to get edits of ip addresses to Unable to get edits of ip addresses in CU.Jun 2 2016, 6:08 AM
Shanmugamp7 updated the task description. (Show Details)

This is an odd one because it's happening on many IPs but not, in any way, all of them. Originally I thought it was only on IPs with smaller number of edits but 127.0.0.1 lets me search it... and that has a ton.

One good one to test on so far is 198.73.209.1 (This is a WMF Office IP)

Jalexander triaged this task as Unbreak Now! priority.Jun 2 2016, 6:14 AM
Jalexander updated the task description. (Show Details)

Appears to be related to @Legoktm's recent work:

/wiki/Special:CheckUser   Wikimedia\Assert\ParameterAssertionException from line 63 of /srv/mediawiki/php-1.28.0-wmf.4/vendor/wikimedia/assert/src/Assert.php: Bad value for parameter $dbkey: should not be empty {"exception_id":"V0@YPgpAIDQAAEn@HjQAAABR"}
[Exception Wikimedia\Assert\ParameterAssertionException] (/srv/mediawiki/php-1.28.0-wmf.4/vendor/wikimedia/assert/src/Assert.php:63) Bad value for parameter $dbkey: should not be empty
  #0 /srv/mediawiki/php-1.28.0-wmf.4/includes/title/TitleValue.php(82): Wikimedia\Assert\Assert::parameter(boolean, string, string)
  #1 /srv/mediawiki/php-1.28.0-wmf.4/includes/cache/LinkBatch.php(171): TitleValue->__construct(integer, string)
  #2 /srv/mediawiki/php-1.28.0-wmf.4/includes/cache/LinkBatch.php(135): LinkBatch->addResultToCache(LinkCache, ResultWrapper)
  #3 /srv/mediawiki/php-1.28.0-wmf.4/includes/cache/LinkBatch.php(122): LinkBatch->executeInto(LinkCache)
  #4 /srv/mediawiki/php-1.28.0-wmf.4/extensions/CheckUser/specials/SpecialCheckUser.php(748): LinkBatch->execute()
  #5 /srv/mediawiki/php-1.28.0-wmf.4/extensions/CheckUser/specials/SpecialCheckUser.php(83): CheckUser->doIPEditsRequest(string, boolean, string, integer)
  #6 /srv/mediawiki/php-1.28.0-wmf.4/includes/specialpage/SpecialPage.php(479): CheckUser->execute(NULL)
  #7 /srv/mediawiki/php-1.28.0-wmf.4/includes/specialpage/SpecialPageFactory.php(591): SpecialPage->run(NULL)
  #8 /srv/mediawiki/php-1.28.0-wmf.4/includes/MediaWiki.php(282): SpecialPageFactory::executePath(Title, RequestContext)
  #9 /srv/mediawiki/php-1.28.0-wmf.4/includes/MediaWiki.php(746): MediaWiki->performRequest()
  #10 /srv/mediawiki/php-1.28.0-wmf.4/includes/MediaWiki.php(520): MediaWiki->main()
  #11 /srv/mediawiki/php-1.28.0-wmf.4/index.php(43): MediaWiki->run()
  #12 /srv/mediawiki/w/index.php(3): include(string)
  #13 {main}
Krenair added a subscriber: Krenair.Jun 2 2016, 6:55 AM
2016-06-02 06:06:39 [V0-M7wpAICgAAH-kkbEAAABG] mw1170 wikidatawiki 1.28.0-wmf.4 exception ERROR: [V0-M7wpAICgAAH-kkbEAAABG] /wiki/Special:CheckUser   Wikimedia\Assert\ParameterAssertionException from line 63 of /srv/mediawiki/php-1.28.0-wmf.4/vendor/wikimedia/assert/src/Assert.php: Bad value for parameter $dbkey: should not be empty {"exception_id":"V0-M7wpAICgAAH-kkbEAAABG"} 
[Exception Wikimedia\Assert\ParameterAssertionException] (/srv/mediawiki/php-1.28.0-wmf.4/vendor/wikimedia/assert/src/Assert.php:63) Bad value for parameter $dbkey: should not be empty
  #0 /srv/mediawiki/php-1.28.0-wmf.4/includes/title/TitleValue.php(82): Wikimedia\Assert\Assert::parameter(boolean, string, string)
  #1 /srv/mediawiki/php-1.28.0-wmf.4/includes/cache/LinkBatch.php(171): TitleValue->__construct(integer, string)
  #2 /srv/mediawiki/php-1.28.0-wmf.4/includes/cache/LinkBatch.php(135): LinkBatch->addResultToCache(LinkCache, ResultWrapper)
  #3 /srv/mediawiki/php-1.28.0-wmf.4/includes/cache/LinkBatch.php(122): LinkBatch->executeInto(LinkCache)
  #4 /srv/mediawiki/php-1.28.0-wmf.4/extensions/CheckUser/specials/SpecialCheckUser.php(748): LinkBatch->execute()
  #5 /srv/mediawiki/php-1.28.0-wmf.4/extensions/CheckUser/specials/SpecialCheckUser.php(83): CheckUser->doIPEditsRequest(string, boolean, string, integer)
  #6 /srv/mediawiki/php-1.28.0-wmf.4/includes/specialpage/SpecialPage.php(479): CheckUser->execute(NULL)
  #7 /srv/mediawiki/php-1.28.0-wmf.4/includes/specialpage/SpecialPageFactory.php(591): SpecialPage->run(NULL)
  #8 /srv/mediawiki/php-1.28.0-wmf.4/includes/MediaWiki.php(282): SpecialPageFactory::executePath(Title, RequestContext)
  #9 /srv/mediawiki/php-1.28.0-wmf.4/includes/MediaWiki.php(746): MediaWiki->performRequest()
  #10 /srv/mediawiki/php-1.28.0-wmf.4/includes/MediaWiki.php(520): MediaWiki->main()
  #11 /srv/mediawiki/php-1.28.0-wmf.4/index.php(43): MediaWiki->run()
  #12 /srv/mediawiki/w/index.php(3): include(string)
  #13 {main}

Similar to T136615

Unlike T136615, this appear to be a real problem with the data. $dbkey definitely should not be empty. If cuc_user_text or cuc_title are allowed to be empty, then the code shouldn't add those rows to the LinkBatch.

matmarex removed a subscriber: matmarex.Jun 2 2016, 8:49 AM

(By the way, while it's related to the CheckUser extension, I don't think this is a security bug. Unless you just don't want anyone to hear that you temporarily can't access some data.)

Unless you just don't want anyone to hear that you temporarily can't access some data.

Vandals knowing that some anti-abuse tools are currently broken would be bad

matmarex added a subscriber: Bsadowski1.

@Bsadowski1 also reported this issue on IRC just now.

Anyway, here's the patch:

(untested, I don't have CheckUser set up locally). cuc_title can indeed be empty for log actions (e.g. user autocreation).

Looks like this has a patch, are we comfortable rolling forward with that patch in place, or should we hold the train until this patch has been vetted?

I'm pretty much just looking for the equivalent of a +1 for @matmarex 's patch.

Bawolff added a subscriber: Bawolff.Jun 2 2016, 7:21 PM

Anyway, here's the patch:

(untested, I don't have CheckUser set up locally). cuc_title can indeed be empty for log actions (e.g. user autocreation).

Tested. consider this a +2

Well it makes sense to keep this private well figuring it out, I think its fine to deploy the change publically, as long as its not sitting in gerrit waiting for a super long time. So with that in mind I submitted this to gerrit: https://gerrit.wikimedia.org/r/#/q/I66fc2ae5f85a481e7f6f7b36e15f4184aa07ca82,n,z

Bug should probably be made non-private once this is deployed everywhere.

Bawolff closed this task as Resolved.Jun 2 2016, 7:56 PM
Bawolff claimed this task.

@Shanmugamp7 @Bsadowski1 @Jalexander : This should be fixed now, can you confirm you are no longer experiencing the issue?

This change has now been deployed.

I am going to remove this as a wmf.4 blocker, roll wmf.4 forward, and make this task public in 10 minutes or so.

Thanks for your help all!

thcipriani changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 2 2016, 8:17 PM
thcipriani changed Security from Software security bug to None.
Restricted Application added subscribers: TerraCodes, Malyacko, JEumerus. · View Herald TranscriptJun 2 2016, 8:17 PM

@Shanmugamp7 @Bsadowski1 @Jalexander : This should be fixed now, can you confirm you are no longer experiencing the issue?

Can confirm that the test cases all seem to work now. Thanks!

Re this being a security bug: Agreed that making it public now (as has already been done) is good but anything like this is definetly best to be private while still in effect. We not only 'can' have people try to take advantage of it but we definetly 'have' had people use Bugzilla/Phabricator to try and get around bugs and hiding the attack vectors like that until they're closed is important imo.

MarcoAurelio moved this task from Backlog to Closed on the CheckUser board.Jan 22 2017, 4:31 PM
Restricted Application added a subscriber: Jay8g. · View Herald TranscriptJan 22 2017, 4:31 PM