Page MenuHomePhabricator

Email address confirmation broken due to incorrect or missing assignment of $expiration in User::confirmationToken()
Closed, ResolvedPublic

Description

Fix for email confirmation token expiration setting in 1.17.3

The recent patches break email confirmation in 1.17.3 and likely break or degrade it on other versions. Here is the result of attempting to set email in 1.17.3:

Internal error
Non-string key given
Backtrace:
#0 [....]/w/includes/GlobalFunctions.php(781): MessageCache->get(NULL, true, Object(Language))
#1 [....]/w/includes/GlobalFunctions.php(902): wfMsgGetKey(NULL, true, Object(Language), false)
#2 [....]/w/languages/Language.php(513): wfMsgExt(NULL, Array)
#3 [....]/w/languages/Language.php(525): Language->getMessageFromDB(NULL)
#4 [....]/w/languages/Language.php(799): Language->getMonthName(false)
#5 [....]/w/languages/Language.php(1560): Language->sprintfDate('H:i, j F Y', false)
#6 [....]/w/includes/User.php(2966): Language->timeanddate('f4e8c51bf5ef375...', false)
#7 [....]/w/includes/specials/SpecialConfirmemail.php(84): User->sendConfirmationMail()
#8 [....]/w/includes/specials/SpecialConfirmemail.php(58): EmailConfirmation->showRequestForm()

This is because assignment of $expiration incorrectly falls through to the assignment of $token, which is not a valid date:

function confirmationToken( &$expiration ) {

        $now = time();
        $expires = $now + 7 * 24 * 60 * 60;
        $expiration = 
        $token = MWCryptRand::generateHex( 32 );
        $hash = md5( $token );
        $this->load();
        $this->mEmailToken = $hash;
        $this->mEmailTokenExpires = wfTimestamp( TS_MW, $expires );
        return $token;
}

The relevant section of the 1.17.3 patch is:

@@ -3005,12 +3009,12 @@

	function confirmationToken( &$expiration ) {
		$now = time();
		$expires = $now + 7 * 24 * 60 * 60;
  • $expiration = wfTimestamp( TS_MW, $expires );
  • $token = self::generateToken( $this->mId . $this->mEmail . $expires );

+ $expiration =
+ $token = MWCryptRand::generateHex( 32 );

		$hash = md5( $token );
		$this->load();
		$this->mEmailToken = $hash;
  • $this->mEmailTokenExpires = $expiration;

+ $this->mEmailTokenExpires = wfTimestamp( TS_MW, $expires );

		return $token;
	}

It appears that this is an attempt to incorrectly optimize the setting of $expiration. This is backed up by the patch in 1.18.2, which removes access to the variable from the function:
@@ -3268,12 +3272,11 @@

		global $wgUserEmailConfirmationTokenExpiry;
		$now = time();
		$expires = $now + $wgUserEmailConfirmationTokenExpiry;
  • $expiration = wfTimestamp( TS_MW, $expires );
  • $token = self::generateToken( $this->mId . $this->mEmail . $expires );
  • $hash = md5( $token ); $this->load();

+ $token = MWCryptRand::generateHex( 32 );
+ $hash = md5( $token );

		$this->mEmailToken = $hash;
  • $this->mEmailTokenExpires = $expiration;

+ $this->mEmailTokenExpires = wfTimestamp( TS_MW, $expires );

		return $token;
	}

I have attached a patch to 1.17.3 that replaces $expiration and uses it to set $this->mEmailTokenExpires again. (It will be offset by a few lines on the branch due to other local patches.)

I do not have later versions around right now, but $expiration is likely still used by the parent function to display the expiry time in the email address, and so this should be fixed in 1.18, 1.19 and SVN. It might break the sending of the email, depending on the impact of $wgLang->{timeanddate|date|time}(null, false).


Version: 1.17.x
Severity: major

Attached:

Details

Reference
bz35441

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 12:12 AM
bzimport added a project: MediaWiki-Email.
bzimport set Reference to bz35441.
bzimport added a subscriber: Unknown Object (MLST).

sumanah wrote:

Thanks for the patch, Laurence. By the way, we'd be happy to give you a Git account

https://www.mediawiki.org/wiki/Project:Labsconsole_accounts

so that you can submit this and other future patches directly if you want, and get them reviewed faster:

https://www.mediawiki.org/wiki/Git/Workflow#How_to_submit_a_patch

Yes, it also needs fixing in 1.18, 1.19 and trunk. Dantman, you're working on that, right?

In 1.17, why call $this->load() twice? (It's there already, a few lines on.)

(In reply to comment #4)

In 1.17, why call $this->load() twice? (It's there already, a few lines on.)

Gah... that was from the cherry pick. I'll submit a new patch set.

sumanah wrote:

Daniel, were you able to submit a new patch set?

Oh right. This should already be fixed and released.