From 94ba53f67731b0553a6178841d9506e384f74496 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 22 Sep 2015 10:33:24 -0400 Subject: [PATCH] Move CSRF token handling into MediaWiki\Session\Session User keeps most of its token-related methods because anon edit tokens are special. Login and createaccount tokens are completely moved. Change-Id: I524218fab7e2d78fd24482ad364428e98dc48bdf --- RELEASE-NOTES-1.27 | 6 + autoload.php | 2 + docs/hooks.txt | 3 +- includes/api/ApiBase.php | 9 +- includes/api/ApiCheckToken.php | 11 +- includes/api/ApiCreateAccount.php | 11 +- includes/api/ApiLogin.php | 18 ++- includes/api/ApiQueryTokens.php | 42 +++++- includes/api/ApiTokens.php | 2 +- includes/session/Session.php | 52 ++++++++ includes/session/Token.php | 125 ++++++++++++++++++ includes/specials/SpecialChangePassword.php | 11 +- includes/specials/SpecialUserlogin.php | 63 ++++----- includes/user/LoggedOutEditToken.php | 47 +++++++ includes/user/User.php | 76 ++++------- .../includes/api/ApiCreateAccountTest.php | 9 +- tests/phpunit/includes/api/ApiLoginTest.php | 4 +- tests/phpunit/includes/api/ApiTestCase.php | 4 +- .../phpunit/includes/session/SessionTest.php | 47 +++++++ tests/phpunit/includes/session/TokenTest.php | 66 +++++++++ 20 files changed, 484 insertions(+), 124 deletions(-) create mode 100644 includes/session/Token.php create mode 100644 includes/user/LoggedOutEditToken.php create mode 100644 tests/phpunit/includes/session/TokenTest.php diff --git a/RELEASE-NOTES-1.27 b/RELEASE-NOTES-1.27 index 50d40a6145..2ce804674d 100644 --- a/RELEASE-NOTES-1.27 +++ b/RELEASE-NOTES-1.27 @@ -81,6 +81,8 @@ production. MediaWiki\Session\SessionProvider. ** The User cannot be loaded from session until after Setup.php completes. Attempts to do so will be ignored and the User will remain unloaded. +** CSRF tokens may be fetched from the MediaWiki\Session\Session, which uses + the MediaWiki\Session\Token class. * MediaWiki will now auto-create users as necessary, removing the need for extensions to do so. An 'autocreateaccount' right is added to allow auto-creation when 'createaccount' is not granted to all users. @@ -88,6 +90,10 @@ production. * Most cookie-handling methods in User are deprecated. * $wgAllowAsyncCopyUploads and $CopyUploadAsyncTimeout were removed. This was an experimental feature that has never worked. +* Login and createaccount tokens now vary by timestamp. +* LoginForm::getLoginToken() and LoginForm::getCreateaccountToken() + return a MediaWiki\Session\Token, and tokens must be checked using that + class's methods. === New features in 1.27 === * $wgDataCenterId and $wgDataCenterRoles where added, which will serve as diff --git a/autoload.php b/autoload.php index 2f23924175..6de1d79759 100644 --- a/autoload.php +++ b/autoload.php @@ -721,6 +721,7 @@ $wgAutoloadLocalClasses = array( 'LogFormatter' => __DIR__ . '/includes/logging/LogFormatter.php', 'LogPage' => __DIR__ . '/includes/logging/LogPage.php', 'LogPager' => __DIR__ . '/includes/logging/LogPager.php', + 'LoggedOutEditToken' => __DIR__ . '/includes/user/LoggedOutEditToken.php', 'LoggedUpdateMaintenance' => __DIR__ . '/maintenance/Maintenance.php', 'LoginForm' => __DIR__ . '/includes/specials/SpecialUserlogin.php', 'LonelyPagesPage' => __DIR__ . '/includes/specials/SpecialLonelypages.php', @@ -797,6 +798,7 @@ $wgAutoloadLocalClasses = array( 'MediaWiki\\Session\\SessionManagerInterface' => __DIR__ . '/includes/session/SessionManagerInterface.php', 'MediaWiki\\Session\\SessionProvider' => __DIR__ . '/includes/session/SessionProvider.php', 'MediaWiki\\Session\\SessionProviderInterface' => __DIR__ . '/includes/session/SessionProviderInterface.php', + 'MediaWiki\\Session\\Token' => __DIR__ . '/includes/session/Token.php', 'MediaWiki\\Session\\UserInfo' => __DIR__ . '/includes/session/UserInfo.php', 'MediaWiki\\Site\\MediaWikiPageNameNormalizer' => __DIR__ . '/includes/site/MediaWikiPageNameNormalizer.php', 'MediaWiki\\Tidy\\Html5Depurate' => __DIR__ . '/includes/tidy/Html5Depurate.php', diff --git a/docs/hooks.txt b/docs/hooks.txt index 2b5e1e0d7e..b516122fc3 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -513,7 +513,8 @@ sites statistics information. 'ApiQueryTokensRegisterTypes': Use this hook to add additional token types to action=query&meta=tokens. Note that most modules will probably be able to use the 'csrf' token instead of creating their own token types. -&$salts: array( type => salt to pass to User::getEditToken() ) +&$salts: array( type => salt to pass to User::getEditToken() or array of salt + and key to pass to Session::getToken() ) 'APIQueryUsersTokens': DEPRECATED! Use ApiQueryTokensRegisterTypes instead. Use this hook to add custom token to list=users. Every token has an action, diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index 13d13a6853..425a337193 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -1260,11 +1260,10 @@ abstract class ApiBase extends ContextSource { ); } - if ( $this->getUser()->matchEditToken( - $token, - $salts[$tokenType], - $this->getRequest() - ) ) { + $tokenObj = ApiQueryTokens::getToken( + $this->getUser(), $this->getRequest()->getSession(), $salts[$tokenType] + ); + if ( $tokenObj->match( $token ) ) { return true; } diff --git a/includes/api/ApiCheckToken.php b/includes/api/ApiCheckToken.php index 28c6ece7c0..dfcbaf890e 100644 --- a/includes/api/ApiCheckToken.php +++ b/includes/api/ApiCheckToken.php @@ -32,21 +32,22 @@ class ApiCheckToken extends ApiBase { $params = $this->extractRequestParams(); $token = $params['token']; $maxage = $params['maxtokenage']; - $request = $this->getRequest(); $salts = ApiQueryTokens::getTokenTypeSalts(); - $salt = $salts[$params['type']]; $res = array(); - if ( $this->getUser()->matchEditToken( $token, $salt, $request, $maxage ) ) { + $tokenObj = ApiQueryTokens::getToken( + $this->getUser(), $this->getRequest()->getSession(), $salts[$params['type']] + ); + if ( $tokenObj->match( $token, $maxage ) ) { $res['result'] = 'valid'; - } elseif ( $maxage !== null && $this->getUser()->matchEditToken( $token, $salt, $request ) ) { + } elseif ( $maxage !== null && $tokenObj->match( $token ) ) { $res['result'] = 'expired'; } else { $res['result'] = 'invalid'; } - $ts = User::getEditTokenTimestamp( $token ); + $ts = MediaWiki\Session\Token::getTimestamp( $token ); if ( $ts !== null ) { $mwts = new MWTimestamp(); $mwts->timestamp->setTimestamp( $ts ); diff --git a/includes/api/ApiCreateAccount.php b/includes/api/ApiCreateAccount.php index a044be2565..d6baf34588 100644 --- a/includes/api/ApiCreateAccount.php +++ b/includes/api/ApiCreateAccount.php @@ -149,8 +149,11 @@ class ApiCreateAccount extends ApiBase { // Token was incorrect, so add it to result, but don't throw an exception // since not having the correct token is part of the normal // flow of events. - $result['token'] = LoginForm::getCreateaccountToken(); + $result['token'] = LoginForm::getCreateaccountToken()->toString(); $result['result'] = 'NeedToken'; + $this->setWarning( 'Fetching a token via action=createaccount is deprecated. ' . + 'Use action=query&meta=tokens&type=createaccount instead.' ); + $this->logFeatureUsage( 'action=createaccount&!token' ); } elseif ( !$status->isOK() ) { // There was an error. Die now. $this->dieStatus( $status ); @@ -200,7 +203,11 @@ class ApiCreateAccount extends ApiBase { ApiBase::PARAM_TYPE => 'password', ), 'domain' => null, - 'token' => null, + 'token' => array( + ApiBase::PARAM_TYPE => 'string', + ApiBase::PARAM_REQUIRED => false, // for BC + ApiBase::PARAM_HELP_MSG => array( 'api-help-param-token', 'createaccount' ), + ), 'email' => array( ApiBase::PARAM_TYPE => 'string', ApiBase::PARAM_REQUIRED => $this->getConfig()->get( 'EmailConfirmToEdit' ), diff --git a/includes/api/ApiLogin.php b/includes/api/ApiLogin.php index 860e3b205f..03cd666e7e 100644 --- a/includes/api/ApiLogin.php +++ b/includes/api/ApiLogin.php @@ -84,12 +84,9 @@ class ApiLogin extends ApiBase { // Check login token $token = LoginForm::getLoginToken(); - if ( !$token ) { - LoginForm::setLoginToken(); + if ( $token->wasNew() || !$params['token'] ) { $authRes = LoginForm::NEED_TOKEN; - } elseif ( !$params['token'] ) { - $authRes = LoginForm::NEED_TOKEN; - } elseif ( $token !== $params['token'] ) { + } elseif ( !$token->match( $params['token'] ) ) { $authRes = LoginForm::WRONG_TOKEN; } @@ -159,7 +156,10 @@ class ApiLogin extends ApiBase { case LoginForm::NEED_TOKEN: $result['result'] = 'NeedToken'; - $result['token'] = LoginForm::getLoginToken(); + $result['token'] = LoginForm::getLoginToken()->toString(); + $this->setWarning( 'Fetching a token via action=login is deprecated. ' . + 'Use action=query&meta=tokens&type=login instead.' ); + $this->logFeatureUsage( 'action=login&!lgtoken' ); // @todo: See above about deprecation $result['cookieprefix'] = $this->getConfig()->get( 'CookiePrefix' ); @@ -254,7 +254,11 @@ class ApiLogin extends ApiBase { ApiBase::PARAM_TYPE => 'password', ), 'domain' => null, - 'token' => null, + 'token' => array( + ApiBase::PARAM_TYPE => 'string', + ApiBase::PARAM_REQUIRED => false, // for BC + ApiBase::PARAM_HELP_MSG => array( 'api-help-param-token', 'login' ), + ), ); } diff --git a/includes/api/ApiQueryTokens.php b/includes/api/ApiQueryTokens.php index f88766461b..3f3464b184 100644 --- a/includes/api/ApiQueryTokens.php +++ b/includes/api/ApiQueryTokens.php @@ -44,16 +44,24 @@ class ApiQueryTokens extends ApiQueryBase { return; } + $user = $this->getUser(); + $session = $this->getRequest()->getSession(); $salts = self::getTokenTypeSalts(); foreach ( $params['type'] as $type ) { - $salt = $salts[$type]; - $val = $this->getUser()->getEditToken( $salt, $this->getRequest() ); - $res[$type . 'token'] = $val; + $res[$type . 'token'] = self::getToken( $user, $session, $salts[$type] )->toString(); } $this->getResult()->addValue( 'query', $this->getModuleName(), $res ); } + /** + * Get the salts for known token types + * @return (string|array)[] Returning a string will use that as the salt + * for User::getEditTokenObject() to fetch the token, which will give a + * LoggedOutEditToken (always "+\\") for anonymous users. Returning an + * array will use it as parameters to MediaWiki\\Session\\Session::getToken(), + * which will always return a full token even for anonymous users. + */ public static function getTokenTypeSalts() { static $salts = null; if ( !$salts ) { @@ -63,6 +71,8 @@ class ApiQueryTokens extends ApiQueryBase { 'patrol' => 'patrol', 'rollback' => 'rollback', 'userrights' => 'userrights', + 'login' => array( '', 'login' ), + 'createaccount' => array( '', 'createaccount' ), ); Hooks::run( 'ApiQueryTokensRegisterTypes', array( &$salts ) ); ksort( $salts ); @@ -71,6 +81,27 @@ class ApiQueryTokens extends ApiQueryBase { return $salts; } + /** + * Get a token from a salt + * @param User $user + * @param MediaWiki\\Session\\Session $session + * @param string|array $salt A string will be used as the salt for + * User::getEditTokenObject() to fetch the token, which will give a + * LoggedOutEditToken (always "+\\") for anonymous users. An array will + * be used as parameters to MediaWiki\\Session\\Session::getToken(), which + * will always return a full token even for anonymous users. An array will + * also persist the session. + * @return MediaWiki\\Session\\Token + */ + public static function getToken( User $user, MediaWiki\Session\Session $session, $salt ) { + if ( is_array( $salt ) ) { + $session->persist(); + return call_user_func_array( array( $session, 'getToken' ), $salt ); + } else { + return $user->getEditTokenObject( $salt, $session->getRequest() ); + } + } + public function getAllowedParams() { return array( 'type' => array( @@ -90,6 +121,11 @@ class ApiQueryTokens extends ApiQueryBase { ); } + public function isReadMode() { + // So login tokens can be fetched on private wikis + return false; + } + public function getCacheMode( $params ) { return 'private'; } diff --git a/includes/api/ApiTokens.php b/includes/api/ApiTokens.php index f92526da76..c10c9383de 100644 --- a/includes/api/ApiTokens.php +++ b/includes/api/ApiTokens.php @@ -81,7 +81,7 @@ class ApiTokens extends ApiBase { foreach ( ApiQueryTokens::getTokenTypeSalts() as $name => $salt ) { if ( !isset( $types[$name] ) ) { $types[$name] = function () use ( $salt, $user, $request ) { - return $user->getEditToken( $salt, $request ); + return ApiQueryTokens::getToken( $user, $request->getSession(), $salt )->toString(); }; } } diff --git a/includes/session/Session.php b/includes/session/Session.php index 840baa70fe..4ad69ae802 100644 --- a/includes/session/Session.php +++ b/includes/session/Session.php @@ -314,6 +314,58 @@ final class Session implements \Countable, \Iterator { } } + /** + * Fetch a CSRF token from the session + * + * Note that this does not persist the session, which you'll probably want + * to do if you want the token to actually be useful. + * + * @param string|string[] $salt Token salt + * @param string $key Token key + * @return MediaWiki\\Session\\SessionToken + */ + public function getToken( $salt = '', $key = 'default' ) { + $new = false; + $secrets = $this->get( 'wsTokenSecrets' ); + if ( !is_array( $secrets ) ) { + $secrets = array(); + } + if ( isset( $secrets[$key] ) && is_string( $secrets[$key] ) ) { + $secret = $secrets[$key]; + } else { + $secret = \MWCryptRand::generateHex( 32 ); + $secrets[$key] = $secret; + $this->set( 'wsTokenSecrets', $secrets ); + $new = true; + } + if ( is_array( $salt ) ) { + $salt = join( '|', $salt ); + } + return new Token( $secret, (string)$salt, $new ); + } + + /** + * Remove a CSRF token from the session + * + * The next call to self::getToken() with $key will generate a new secret. + * + * @param string $key Token key + */ + public function resetToken( $key = 'default' ) { + $secrets = $this->get( 'wsTokenSecrets' ); + if ( is_array( $secrets ) && isset( $secrets[$key] ) ) { + unset( $secrets[$key] ); + $this->set( 'wsTokenSecrets', $secrets ); + } + } + + /** + * Remove all CSRF tokens from the session + */ + public function resetAllTokens() { + $this->remove( 'wsTokenSecrets' ); + } + /** * Delay automatic saving while multiple updates are being made * diff --git a/includes/session/Token.php b/includes/session/Token.php new file mode 100644 index 0000000000..9b4a73cb78 --- /dev/null +++ b/includes/session/Token.php @@ -0,0 +1,125 @@ +secret = $secret; + $this->salt = $salt; + $this->new = $new; + } + + /** + * Decode the timestamp from a token string + * + * Does not validate the token beyond the syntactic checks necessary to + * be able to extract the timestamp. + * + * @param string $token + * @param int|null + */ + public static function getTimestamp( $token ) { + $suffixLen = strlen( self::SUFFIX ); + $len = strlen( $token ); + if ( $len <= 32 + $suffixLen || + substr( $token, -$suffixLen ) !== self::SUFFIX || + strspn( $token, '0123456789abcdef' ) + $suffixLen !== $len + ) { + return null; + } + + return hexdec( substr( $token, 32, -$suffixLen ) ); + } + + /** + * Get the string representation of the token at a timestamp + * @param int timestamp + * @return string + */ + protected function toStringAtTimestamp( $timestamp ) { + return hash_hmac( 'md5', $timestamp . $this->salt, $this->secret, false ) . + dechex( $timestamp ) . + self::SUFFIX; + } + + /** + * Get the string representation of the token + * @return string + */ + public function toString() { + return $this->toStringAtTimestamp( wfTimestamp() ); + } + + public function __toString() { + return $this->toString(); + } + + /** + * Test if the token-string matches this token + * @param string $userToken + * @param int|null $maxAge Return false if $userToken is older than this many seconds + * @return bool + */ + public function match( $userToken, $maxAge = null ) { + $timestamp = self::getTimestamp( $userToken ); + if ( $timestamp === null ) { + return false; + } + if ( $maxAge !== null && $timestamp < wfTimestamp() - $maxAge ) { + // Expired token + return false; + } + + $sessionToken = $this->toStringAtTimestamp( $timestamp ); + return hash_equals( $sessionToken, $userToken ); + } + + /** + * Indicate whether this token was just created + * @return bool + */ + public function wasNew() { + return $this->new; + } + +} diff --git a/includes/specials/SpecialChangePassword.php b/includes/specials/SpecialChangePassword.php index 8656798915..3e5a936ba3 100644 --- a/includes/specials/SpecialChangePassword.php +++ b/includes/specials/SpecialChangePassword.php @@ -111,13 +111,10 @@ class SpecialChangePassword extends FormSpecialPage { ); if ( !$this->getUser()->isLoggedIn() ) { - if ( !LoginForm::getLoginToken() ) { - LoginForm::setLoginToken(); - } $fields['LoginOnChangeToken'] = array( 'type' => 'hidden', 'label' => 'Change Password Token', - 'default' => LoginForm::getLoginToken(), + 'default' => LoginForm::getLoginToken()->toString(), ); } @@ -179,7 +176,7 @@ class SpecialChangePassword extends FormSpecialPage { } if ( !$this->getUser()->isLoggedIn() - && $request->getVal( 'wpLoginOnChangeToken' ) !== LoginForm::getLoginToken() + && !LoginForm::getLoginToken()->match( $request->getVal( 'wpLoginOnChangeToken' ) ) ) { // Potential CSRF (bug 62497) return false; @@ -218,8 +215,8 @@ class SpecialChangePassword extends FormSpecialPage { $this->getOutput()->returnToMain(); } else { $request = $this->getRequest(); - LoginForm::setLoginToken(); - $token = LoginForm::getLoginToken(); + LoginForm::clearLoginToken(); + $token = LoginForm::getLoginToken()->toString(); $data = array( 'action' => 'submitlogin', 'wpName' => $this->mUserName, diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 9473dff101..d9a5e4f772 100644 --- a/includes/specials/SpecialUserlogin.php +++ b/includes/specials/SpecialUserlogin.php @@ -531,9 +531,8 @@ class LoginForm extends SpecialPage { } # Request forgery checks. - if ( !self::getCreateaccountToken() ) { - self::setCreateaccountToken(); - + $token = self::getCreateaccountToken(); + if ( $token->wasNew() ) { return Status::newFatal( 'nocookiesfornew' ); } @@ -543,7 +542,7 @@ class LoginForm extends SpecialPage { } # Validate the createaccount token - if ( $this->mToken !== self::getCreateaccountToken() ) { + if ( !$token->match( $this->mToken ) ) { return Status::newFatal( 'sessionfailure' ); } @@ -737,9 +736,8 @@ class LoginForm extends SpecialPage { // but wrong-token attempts do. // If the user doesn't have a login token yet, set one. - if ( !self::getLoginToken() ) { - self::setLoginToken(); - + $token = self::getLoginToken(); + if ( $token->wasNew() ) { return self::NEED_TOKEN; } // If the user didn't pass a login token, tell them we need one @@ -753,7 +751,7 @@ class LoginForm extends SpecialPage { } // Validate the login token - if ( $this->mToken !== self::getLoginToken() ) { + if ( !$token->match( $this->mToken ) ) { return self::WRONG_TOKEN; } @@ -1492,15 +1490,9 @@ class LoginForm extends SpecialPage { $template->set( 'loggedinuser', $user->getName() ); if ( $this->mType == 'signup' ) { - if ( !self::getCreateaccountToken() ) { - self::setCreateaccountToken(); - } - $template->set( 'token', self::getCreateaccountToken() ); + $template->set( 'token', self::getCreateaccountToken()->toString() ); } else { - if ( !self::getLoginToken() ) { - self::setLoginToken(); - } - $template->set( 'token', self::getLoginToken() ); + $template->set( 'token', self::getLoginToken()->toString() ); } # Prepare language selection links as needed @@ -1576,22 +1568,25 @@ class LoginForm extends SpecialPage { /** * Get the login token from the current session - * @return mixed + * @since 1.27 returns a MediaWiki\\Session\\Token instead of a string + * @return MediaWiki\\Session\\Token */ public static function getLoginToken() { global $wgRequest; - - return $wgRequest->getSessionData( 'wsLoginToken' ); + return $wgRequest->getSession()->getToken( '', 'login' ); } /** - * Randomly generate a new login token and attach it to the current session + * Formerly randomly generated a login token that would be returned by + * $this->getLoginToken(). + * + * Since 1.27, this is a no-op. The token is generated as necessary by + * $this->getLoginToken(). + * + * @deprecated since 1.27 */ public static function setLoginToken() { - global $wgRequest; - // Generate a token directly instead of using $user->getEditToken() - // because the latter reuses wsEditToken in the session - $wgRequest->setSessionData( 'wsLoginToken', MWCryptRand::generateHex( 32 ) ); + wfDeprecated( __METHOD__, '1.27' ); } /** @@ -1599,24 +1594,30 @@ class LoginForm extends SpecialPage { */ public static function clearLoginToken() { global $wgRequest; - $wgRequest->setSessionData( 'wsLoginToken', null ); + $wgRequest->getSession()->resetToken( 'login' ); } /** * Get the createaccount token from the current session - * @return mixed + * @since 1.27 returns a MediaWiki\\Session\\Token instead of a string + * @return MediaWiki\\Session\\Token */ public static function getCreateaccountToken() { global $wgRequest; - return $wgRequest->getSessionData( 'wsCreateaccountToken' ); + return $wgRequest->getSession()->getToken( '', 'createaccount' ); } /** - * Randomly generate a new createaccount token and attach it to the current session + * Formerly randomly generated a createaccount token that would be returned + * by $this->getCreateaccountToken(). + * + * Since 1.27, this is a no-op. The token is generated as necessary by + * $this->getCreateaccountToken(). + * + * @deprecated since 1.27 */ public static function setCreateaccountToken() { - global $wgRequest; - $wgRequest->setSessionData( 'wsCreateaccountToken', MWCryptRand::generateHex( 32 ) ); + wfDeprecated( __METHOD__, '1.27' ); } /** @@ -1624,7 +1625,7 @@ class LoginForm extends SpecialPage { */ public static function clearCreateaccountToken() { global $wgRequest; - $wgRequest->setSessionData( 'wsCreateaccountToken', null ); + $wgRequest->getSession()->resetToken( 'createaccount' ); } /** diff --git a/includes/user/LoggedOutEditToken.php b/includes/user/LoggedOutEditToken.php new file mode 100644 index 0000000000..14548f409b --- /dev/null +++ b/includes/user/LoggedOutEditToken.php @@ -0,0 +1,47 @@ +isAnon() ) { - return self::EDIT_TOKEN_SUFFIX; - } else { - $token = $request->getSessionData( 'wsEditToken' ); - if ( $token === null ) { - $token = MWCryptRand::generateHex( 32 ); - $request->setSessionData( 'wsEditToken', $token ); - } - if ( is_array( $salt ) ) { - $salt = implode( '|', $salt ); - } - return hash_hmac( 'md5', $timestamp . $salt, $token, false ) . - dechex( $timestamp ) . - self::EDIT_TOKEN_SUFFIX; + return new LoggedOutEditToken(); + } + + if ( !$request ) { + $request = $this->getRequest(); } + return $request->getSession()->getToken( $salt ); } /** @@ -4099,29 +4096,23 @@ class User implements IDBAccessObject { * submission. * * @since 1.19 - * * @param string|array $salt Array of Strings Optional function-specific data for hashing * @param WebRequest|null $request WebRequest object to use or null to use $wgRequest * @return string The new edit token */ public function getEditToken( $salt = '', $request = null ) { - return $this->getEditTokenAtTimestamp( - $salt, $request ?: $this->getRequest(), wfTimestamp() - ); + return $this->getEditTokenObject( $salt, $request )->toString(); } /** * Get the embedded timestamp from a token. + * @deprecated since 1.27, use \\MediaWiki\\Session\\Token::getTimestamp instead. * @param string $val Input token * @return int|null */ public static function getEditTokenTimestamp( $val ) { - $suffixLen = strlen( self::EDIT_TOKEN_SUFFIX ); - if ( strlen( $val ) <= 32 + $suffixLen ) { - return null; - } - - return hexdec( substr( $val, 32, -$suffixLen ) ); + wfDeprecated( __METHOD__, '1.27' ); + return MediaWiki\Session\Token::getTimestamp( $val ); } /** @@ -4137,28 +4128,7 @@ class User implements IDBAccessObject { * @return bool Whether the token matches */ public function matchEditToken( $val, $salt = '', $request = null, $maxage = null ) { - if ( $this->isAnon() ) { - return $val === self::EDIT_TOKEN_SUFFIX; - } - - $timestamp = self::getEditTokenTimestamp( $val ); - if ( $timestamp === null ) { - return false; - } - if ( $maxage !== null && $timestamp < wfTimestamp() - $maxage ) { - // Expired token - return false; - } - - $sessionToken = $this->getEditTokenAtTimestamp( - $salt, $request ?: $this->getRequest(), $timestamp - ); - - if ( !hash_equals( $sessionToken, $val ) ) { - wfDebug( "User::matchEditToken: broken session data\n" ); - } - - return hash_equals( $sessionToken, $val ); + return $this->getEditTokenObject( $salt, $request )->match( $val, $maxage ); } /** diff --git a/tests/phpunit/includes/api/ApiCreateAccountTest.php b/tests/phpunit/includes/api/ApiCreateAccountTest.php index 3945102bb7..35905c5564 100644 --- a/tests/phpunit/includes/api/ApiCreateAccountTest.php +++ b/tests/phpunit/includes/api/ApiCreateAccountTest.php @@ -10,7 +10,6 @@ class ApiCreateAccountTest extends ApiTestCase { protected function setUp() { parent::setUp(); - LoginForm::setCreateaccountToken(); $this->setMwGlobals( array( 'wgEnableEmail' => true ) ); } @@ -114,7 +113,7 @@ class ApiCreateAccountTest extends ApiTestCase { public function testNoName() { $this->doApiRequest( array( 'action' => 'createaccount', - 'token' => LoginForm::getCreateaccountToken(), + 'token' => LoginForm::getCreateaccountToken()->toString(), 'password' => 'password', ) ); } @@ -127,7 +126,7 @@ class ApiCreateAccountTest extends ApiTestCase { $this->doApiRequest( array( 'action' => 'createaccount', 'name' => 'testName', - 'token' => LoginForm::getCreateaccountToken(), + 'token' => LoginForm::getCreateaccountToken()->toString(), ) ); } @@ -139,7 +138,7 @@ class ApiCreateAccountTest extends ApiTestCase { $this->doApiRequest( array( 'action' => 'createaccount', 'name' => 'Apitestsysop', - 'token' => LoginForm::getCreateaccountToken(), + 'token' => LoginForm::getCreateaccountToken()->toString(), 'password' => 'password', 'email' => 'test@domain.test', ) ); @@ -153,7 +152,7 @@ class ApiCreateAccountTest extends ApiTestCase { $this->doApiRequest( array( 'action' => 'createaccount', 'name' => 'Test User', - 'token' => LoginForm::getCreateaccountToken(), + 'token' => LoginForm::getCreateaccountToken()->toString(), 'password' => 'password', 'email' => 'invalid', ) ); diff --git a/tests/phpunit/includes/api/ApiLoginTest.php b/tests/phpunit/includes/api/ApiLoginTest.php index 4085925dd0..894feb7f9d 100644 --- a/tests/phpunit/includes/api/ApiLoginTest.php +++ b/tests/phpunit/includes/api/ApiLoginTest.php @@ -14,11 +14,11 @@ class ApiLoginTest extends ApiTestCase { */ public function testApiLoginNoName() { $session = array( - 'wsLoginToken' => 'foobar' + 'wsTokenSecrets' => array( 'login' => 'foobar' ), ); $data = $this->doApiRequest( array( 'action' => 'login', 'lgname' => '', 'lgpassword' => self::$users['sysop']->password, - 'lgtoken' => 'foobar', + 'lgtoken' => (string)( new MediaWiki\Session\Token( 'foobar', '' ) ) ), $session ); $this->assertEquals( 'NoName', $data[0]['login']['result'] ); } diff --git a/tests/phpunit/includes/api/ApiTestCase.php b/tests/phpunit/includes/api/ApiTestCase.php index 25ffcb7a92..52c9fec57e 100644 --- a/tests/phpunit/includes/api/ApiTestCase.php +++ b/tests/phpunit/includes/api/ApiTestCase.php @@ -148,12 +148,12 @@ abstract class ApiTestCase extends MediaWikiLangTestCase { if ( isset( $session['wsToken'] ) && $session['wsToken'] ) { // @todo Why does this directly mess with the session? Fix that. // add edit token to fake session - $session['wsEditToken'] = $session['wsToken']; + $session['wsTokenSecrets']['default'] = $session['wsToken']; // add token to request parameters $timestamp = wfTimestamp(); $params['token'] = hash_hmac( 'md5', $timestamp, $session['wsToken'] ) . dechex( $timestamp ) . - User::EDIT_TOKEN_SUFFIX; + MediaWiki\Session\Token::SUFFIX; return $this->doApiRequest( $params, $session, false, $user ); } else { diff --git a/tests/phpunit/includes/session/SessionTest.php b/tests/phpunit/includes/session/SessionTest.php index efc92f7ff7..858996d63a 100644 --- a/tests/phpunit/includes/session/SessionTest.php +++ b/tests/phpunit/includes/session/SessionTest.php @@ -199,4 +199,51 @@ class SessionTest extends MediaWikiTestCase { $this->assertTrue( $backend->dirty ); } + public function testTokens() { + $rc = new \ReflectionClass( 'MediaWiki\\Session\\Session' ); + if ( !method_exists( $rc, 'newInstanceWithoutConstructor' ) ) { + $this->markTestSkipped( + 'ReflectionClass::newInstanceWithoutConstructor isn\'t available' + ); + } + + // Instead of actually constructing the Session, we use reflection to + // bypass the constructor and plug a mock SessionBackend into the + // private fields to avoid having to actually create a SessionBackend. + $backend = new DummySessionBackend; + $session = $rc->newInstanceWithoutConstructor(); + $priv = \TestingAccessWrapper::newFromObject( $session ); + $priv->backend = $backend; + $priv->index = 42; + + $token = \TestingAccessWrapper::newFromObject( $session->getToken() ); + $this->assertArrayHasKey( 'wsTokenSecrets', $backend->data ); + $this->assertArrayHasKey( 'default', $backend->data['wsTokenSecrets'] ); + $secret = $backend->data['wsTokenSecrets']['default']; + $this->assertSame( $secret, $token->secret ); + $this->assertSame( '', $token->salt ); + $this->assertTrue( $token->wasNew() ); + + $token = \TestingAccessWrapper::newFromObject( $session->getToken( 'foo' ) ); + $this->assertSame( $secret, $token->secret ); + $this->assertSame( 'foo', $token->salt ); + $this->assertFalse( $token->wasNew() ); + + $backend->data['wsTokenSecrets']['secret'] = 'sekret'; + $token = \TestingAccessWrapper::newFromObject( + $session->getToken( array( 'bar', 'baz' ), 'secret' ) + ); + $this->assertSame( 'sekret', $token->secret ); + $this->assertSame( 'bar|baz', $token->salt ); + $this->assertFalse( $token->wasNew() ); + + $session->resetToken( 'secret' ); + $this->assertArrayHasKey( 'wsTokenSecrets', $backend->data ); + $this->assertArrayHasKey( 'default', $backend->data['wsTokenSecrets'] ); + $this->assertArrayNotHasKey( 'secret', $backend->data['wsTokenSecrets'] ); + + $session->resetAllTokens(); + $this->assertArrayNotHasKey( 'wsTokenSecrets', $backend->data ); + + } } diff --git a/tests/phpunit/includes/session/TokenTest.php b/tests/phpunit/includes/session/TokenTest.php new file mode 100644 index 0000000000..113f409e0c --- /dev/null +++ b/tests/phpunit/includes/session/TokenTest.php @@ -0,0 +1,66 @@ +getMockBuilder( 'MediaWiki\\Session\\Token' ) + ->setMethods( array( 'toStringAtTimestamp' ) ) + ->setConstructorArgs( array( 'sekret', 'salty', true ) ) + ->getMock(); + $token->expects( $this->any() )->method( 'toStringAtTimestamp' ) + ->will( $this->returnValue( 'faketoken+\\' ) ); + + $this->assertSame( 'faketoken+\\', $token->toString() ); + $this->assertSame( 'faketoken+\\', (string)$token ); + $this->assertTrue( $token->wasNew() ); + + $token = new Token( 'sekret', 'salty', false ); + $this->assertFalse( $token->wasNew() ); + } + + public function testToStringAtTimestamp() { + $token = \TestingAccessWrapper::newFromObject( new Token( 'sekret', 'salty', false ) ); + + $this->assertSame( + 'd9ade0c7d4349e9df9094e61c33a5a0d5644fde2+\\', + $token->toStringAtTimestamp( 1447362018 ) + ); + $this->assertSame( + 'ee2f7a2488dea9176c224cfb400d43be5644fdea+\\', + $token->toStringAtTimestamp( 1447362026 ) + ); + } + + public function testGetTimestamp() { + $this->assertSame( + 1447362018, Token::getTimestamp( 'd9ade0c7d4349e9df9094e61c33a5a0d5644fde2+\\' ) + ); + $this->assertSame( + 1447362026, Token::getTimestamp( 'ee2f7a2488dea9176c224cfb400d43be5644fdea+\\' ) + ); + $this->assertNull( Token::getTimestamp( 'ee2f7a2488dea9176c224cfb400d43be5644fdea-\\' ) ); + $this->assertNull( Token::getTimestamp( 'ee2f7a2488dea9176c224cfb400d43be+\\' ) ); + + $this->assertNull( Token::getTimestamp( 'ee2f7a2488dea9x76c224cfb400d43be5644fdea+\\' ) ); + } + + public function testMatch() { + $token = \TestingAccessWrapper::newFromObject( new Token( 'sekret', 'salty', false ) ); + + $test = $token->toStringAtTimestamp( time() - 10 ); + $this->assertTrue( $token->match( $test ) ); + $this->assertTrue( $token->match( $test, 12 ) ); + $this->assertFalse( $token->match( $test, 8 ) ); + + $this->assertFalse( $token->match( 'ee2f7a2488dea9176c224cfb400d43be5644fdea-\\' ) ); + } + +} -- 2.20.1