Checkuser API does not validate cutimecond
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

None

Authored By

	Dreamy_Jazz
	May 27 2022, 3:15 PM

Description

List of steps to reproduce (step by step, including full links if applicable):

Go to Special:ApiSandbox or directly via an API endpoint
Choose query as the action and checkuser as list.
Fill in the other fields as appropriate but choose a very old cultimecond (such as -2000000000000 weeks) or a cultimecond in the positive.
Submit the request

What happens?:
Instead of gracefully saying that the expiry is invalid, the request fails with errors as follows:

For -2000000000000 weeks:

{
    "error": {
        "code": "internal_api_error_Wikimedia\\Timestamp\\TimestampException",
        "info": "[483de639-5f67-4d41-beea-337ecc681f82] Caught exception of type Wikimedia\\Timestamp\\TimestampException",
        "errorclass": "Wikimedia\\Timestamp\\TimestampException"
    },
    "servedby": "mw1423"
}

For 222220000 weeks

{
    "error": {
        "code": "internal_api_error_Wikimedia\\Timestamp\\TimestampException",
        "info": "[b86ab9b6-d9bb-4352-bb30-43dcd76c8a6f] Caught exception of type Wikimedia\\Timestamp\\TimestampException",
        "errorclass": "Wikimedia\\Timestamp\\TimestampException"
    },
    "servedby": "mw1444"
}

For 22 weeks:

{
    "batchcomplete": "",
    "query": {
        "checkuser": {
            "edits": []
        }
    }
}

What should have happened instead?:
For the first two, instead of a confusing error of Caught exception of type Wikimedia\\Timestamp\\TimestampException something more user friendly should be returned such as invalid timecond provided.

For the last, a positive timecond should not be possible as this means that you want to run a check for anything past a future date. As that future date has not happened yet there is obviously no CU data. Either this should be treated as the equivalent but into the past or an error of an invalid timecond should be returned. Returning an error seems best here.

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc.:
Tested on local instance of MediaWiki with CentralAuth and CheckUser installed. Also tested on enwiki using Special:ApiSandbox.

Details on how to address this
Any thrown TimestampException when parsing the cutimecond should be returned as a more user-readable error. If the parsing was possible the code should also check that the timestamp generated from cutimecond is not in the future.

Error

mwversion: 1.39.0-wmf.13
reqId: 483de639-5f67-4d41-beea-337ecc681f82
Find reqId in Logstash

normalized_message

[{reqId}] {exception_url}   Wikimedia\Timestamp\TimestampException: Wikimedia\Timestamp\ConvertibleTimestamp::setTimestamp: Invalid timestamp - -1209599998346335759

exception.trace

from /srv/mediawiki/php-1.39.0-wmf.13/vendor/wikimedia/timestamp/src/ConvertibleTimestamp.php(240)
#0 /srv/mediawiki/php-1.39.0-wmf.13/vendor/wikimedia/timestamp/src/ConvertibleTimestamp.php(165): Wikimedia\Timestamp\ConvertibleTimestamp->setTimestamp(integer)
#1 /srv/mediawiki/php-1.39.0-wmf.13/includes/libs/rdbms/platform/SQLPlatform.php(429): Wikimedia\Timestamp\ConvertibleTimestamp->__construct(integer)
#2 /srv/mediawiki/php-1.39.0-wmf.13/includes/libs/rdbms/database/Database.php(4532): Wikimedia\Rdbms\Platform\SQLPlatform->timestamp(integer)
#3 /srv/mediawiki/php-1.39.0-wmf.13/includes/libs/rdbms/database/DBConnRef.php(68): Wikimedia\Rdbms\Database->timestamp(integer)
#4 /srv/mediawiki/php-1.39.0-wmf.13/includes/libs/rdbms/database/DBConnRef.php(627): Wikimedia\Rdbms\DBConnRef->__call(string, array)
#5 /srv/mediawiki/php-1.39.0-wmf.13/extensions/CheckUser/src/Api/ApiQueryCheckUser.php(51): Wikimedia\Rdbms\DBConnRef->timestamp(integer)
#6 /srv/mediawiki/php-1.39.0-wmf.13/includes/api/ApiQuery.php(656): MediaWiki\CheckUser\Api\ApiQueryCheckUser->execute()
#7 /srv/mediawiki/php-1.39.0-wmf.13/includes/api/ApiMain.php(1900): ApiQuery->execute()
#8 /srv/mediawiki/php-1.39.0-wmf.13/includes/api/ApiMain.php(874): ApiMain->executeAction()
#9 /srv/mediawiki/php-1.39.0-wmf.13/includes/api/ApiMain.php(845): ApiMain->executeActionWithErrorHandling()
#10 /srv/mediawiki/php-1.39.0-wmf.13/api.php(90): ApiMain->execute()
#11 /srv/mediawiki/php-1.39.0-wmf.13/api.php(45): wfApiMain()
#12 /srv/mediawiki/w/api.php(3): require(string)
#13 {main}

Details

	Subject	Repo	Branch	Lines +/-
	ApiQueryCheckUser: Perform further validation on timecond	mediawiki/extensions/CheckUser	master	+1 -1

Customize query in gerrit

Event Timeline

Dreamy_Jazz created this task.May 27 2022, 3:15 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 27 2022, 3:15 PM

Dreamy_Jazz renamed this task from Checkuser API does not properly validate cutimecond to Checkuser API does not validate cutimecond.May 27 2022, 3:24 PM

Dreamy_Jazz updated the task description. (Show Details)

Dreamy_Jazz updated the task description. (Show Details)May 27 2022, 3:27 PM

TheresNoTime updated the task description. (Show Details)May 27 2022, 3:50 PM

TheresNoTime subscribed.

Dreamy_Jazz updated the task description. (Show Details)May 27 2022, 4:16 PM

So you're meant to get You need use correct time limit (like "-2 weeks" or "2 weeks ago"). if the apierror-checkuser-timelimit key in i18n/api/en.json is to be believed.

However, this part of src/Api/ApiQueryCheckUser.php has a flaw:

$timeCutoff = strtotime( $timecond );
if ( !$timeCutoff ) {
    $this->dieWithError( 'apierror-checkuser-timelimit', 'invalidtime' );
}

Using your example of $timecond = '-2000000000000 weeks',

$timeCutoff = strtotime( '-2000000000000 weeks' ); // -1209599998346329826

// -1209599998346329826 evaluates as truthy
if ( !$timeCutoff ) { 
    $this->dieWithError( 'apierror-checkuser-timelimit', 'invalidtime' );
}

$this->addTables( 'cu_changes' );
$this->addOption( 'LIMIT', $limit + 1 );
$this->addOption( 'ORDER BY', 'cuc_timestamp DESC' );
$this->addWhere( "cuc_timestamp > " . $db->addQuotes( $db->timestamp( $timeCutoff ) ) ); // invalid timestamp, throw TimestampException

-1209599998346329826 is not a supported timestamp format (unsurprisingly)

Fixing the behaviour of line 45's check should resolve this, as a nicer message would be returned

I did some testing, seems this can be solved fairly easily.

We should check if $timeCutoff is

falsy (failed to be parsed by strtotime)
a negative number (parsed to be before 1st Jan 1970)
greater than strtotime( 'now' ) (a future date)

$timeCutoff = strtotime( $timecond );
if ( !$timeCutoff || $timeCutoff < 0 || $timeCutoff > strtotime( 'now' ) ) {
    $this->dieWithError( 'apierror-checkuser-timelimit', 'invalidtime' );
}

Change 800789 had a related patch set uploaded (by Samtar; author: Samtar):

[mediawiki/extensions/CheckUser@master] ApiQueryCheckUser: Perform further validation on timecond

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

gerritbot added a project: Patch-For-Review.May 27 2022, 7:29 PM

Dreamy_Jazz moved this task from General / Unsorted to Patches for review on the CheckUser board.May 27 2022, 9:18 PM

Change 800789 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] ApiQueryCheckUser: Perform further validation on timecond

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

A patch has been merged so marking this as resolved.

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.14; 2022-05-30).May 27 2022, 10:00 PM

Maintenance_bot removed a project: Patch-For-Review.May 27 2022, 10:30 PM

Dreamy_Jazz moved this task from Patches for review to Done on the CheckUser board.Jun 14 2022, 4:02 PM

Checkuser API does not validate cutimecondClosed, ResolvedPublicBUG REPORTActions

Description

Error

Details

Event Timeline

Checkuser API does not validate cutimecond
Closed, ResolvedPublicBUG REPORT
Actions