Page MenuHomePhabricator

Make Consumer::normalizeValues() give consistent normalized values for null emailAuthenticated timestamp
Closed, ResolvedPublic

Description

Consumer::normalizeValues() adjusts various fields to consistent values. For timestamps, it does something like this:

$this->emailAuthenticated = wfTimestamp( TS_MW, $this->emailAuthenticated );
$this->registration = wfTimestamp( TS_MW, $this->registration );
$this->stageTimestamp = wfTimestamp( TS_MW, $this->stageTimestamp );

"registration" and "stageTimestamp" cannot be null in the db, but emailAuthenticated can.

The current code is fine in normal usage, where emailAuthenticated always has a valid value. But if it is null, it is normalized to the current timestamp. Because this timestamp is used in MWOauthDAO::getChangeToken(), this introduces a subtle timing issue where change tokens do not match if the current timestamp has incremented by (at least) a second between change token calculations.

To my knowledge, this is not a production issue, as this timestamp is always initialized in code. However, it gave inconsistent and unexpected results in local unit testing.

Suggested fix: use wfTimestampOrNull rather than wfTimestamp to normalize the emailAuthenticated value. This better reflects reality, and also causes Consumer::normalizeValues() to behave consistently, thereby eliminating the timing issue.

Event Timeline

Actually, looking at the db schema, "registration" and "stageTimestamp" can't be null. So this needs to be done only for emailAuthenticated.

BPirkle renamed this task from Make Consumer::normalizeValues() give consistent normalized values for null timestamps to Make Consumer::normalizeValues() give consistent normalized values for null emailAuthenticated timestamp.Aug 26 2020, 5:02 PM
BPirkle updated the task description. (Show Details)

Change 622614 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/extensions/OAuth@master] Make Consumer::normalizeValues() consistently normalize emailAuthenticated

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

Change 622614 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] Make Consumer::normalizeValues() consistently normalize emailAuthenticated

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