Fix login API for users with @ in their usernames

An @ in the username caused the password to be treated as a bot password,
but apparently some real usernames still contain it. Try both logins
instead. Security considerations are the same as for the other bot
password syntax: the length check makes sure we do not provide any
information on a timing side channel about the password unless it is
extremely long.

Change-Id: I58f42544a08c3208c41f54cfae932632d9c5affa

diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php
index 0bbe12e..eae57f4 100644 (file)
--- a/includes/user/BotPassword.php
+++ b/includes/user/BotPassword.php
@@ -411,11 +411,13 @@ class BotPassword implements IDBAccessObject {
         */
        public static function canonicalizeLoginData( $username, $password ) {
                $sep = BotPassword::getSeparator();
-               if ( strpos( $username, $sep ) !== false ) {
-                       // the separator is not valid in usernames so this must be a bot login
-                       return [ $username, $password, false ];
+               // the strlen check helps minimize the password information obtainable from timing
+               if ( strlen( $password ) >= 32 && strpos( $username, $sep ) !== false ) {
+                       // the separator is not valid in new usernames but might appear in legacy ones
+                       if ( preg_match( '/^[0-9a-w]{32,}$/', $password ) ) {
+                               return [ $username, $password, true ];
+                       }
                } elseif ( strlen( $password ) > 32 && strpos( $password, $sep ) !== false ) {
-                       // the strlen check helps minimize the password information obtainable from timing
                        $segments = explode( $sep, $password );
                        $password = array_pop( $segments );
                        $appId = implode( $sep, $segments );

diff --git a/tests/phpunit/includes/api/ApiLoginTest.php b/tests/phpunit/includes/api/ApiLoginTest.php
index 487ab84..97681eb 100644 (file)
--- a/tests/phpunit/includes/api/ApiLoginTest.php
+++ b/tests/phpunit/includes/api/ApiLoginTest.php
@@ -231,10 +231,11 @@ class ApiLoginTest extends ApiTestCase {
                $centralId = CentralIdLookup::factory()->centralIdFromLocalUser( $user->getUser() );
                $this->assertNotEquals( 0, $centralId, 'sanity check' );
 
+               $password = 'ngfhmjm64hv0854493hsj5nncjud2clk';
                $passwordFactory = new PasswordFactory();
                $passwordFactory->init( RequestContext::getMain()->getConfig() );
                // A is unsalted MD5 (thus fast) ... we don't care about security here, this is test only
-               $passwordHash = $passwordFactory->newFromPlaintext( 'foobaz' );
+               $passwordHash = $passwordFactory->newFromPlaintext( $password );
 
                $dbw = wfGetDB( DB_MASTER );
                $dbw->insert(
@@ -255,7 +256,7 @@ class ApiLoginTest extends ApiTestCase {
                $ret = $this->doApiRequest( [
                        'action' => 'login',
                        'lgname' => $lgName,
-                       'lgpassword' => 'foobaz',
+                       'lgpassword' => $password,
                ] );
 
                $result = $ret[0];
@@ -270,7 +271,7 @@ class ApiLoginTest extends ApiTestCase {
                        'action' => 'login',
                        'lgtoken' => $token,
                        'lgname' => $lgName,
-                       'lgpassword' => 'foobaz',
+                       'lgpassword' => $password,
                ], $ret[2] );
 
                $result = $ret[0];

diff --git a/tests/phpunit/includes/user/BotPasswordTest.php b/tests/phpunit/includes/user/BotPasswordTest.php
index d637704..cb27fde 100644 (file)
--- a/tests/phpunit/includes/user/BotPasswordTest.php
+++ b/tests/phpunit/includes/user/BotPasswordTest.php
@@ -244,8 +244,9 @@ class BotPasswordTest extends MediaWikiTestCase {
                return [
                        [ 'user', 'pass', false ],
                        [ 'user', 'abc@def', false ],
+                       [ 'legacy@user', 'pass', false ],
                        [ 'user@bot', '12345678901234567890123456789012',
-                               [ 'user@bot', '12345678901234567890123456789012', false ] ],
+                               [ 'user@bot', '12345678901234567890123456789012', true ] ],
                        [ 'user', 'bot@12345678901234567890123456789012',
                                [ 'user@bot', '12345678901234567890123456789012', true ] ],
                        [ 'user', 'bot@12345678901234567890123456789012345',