[SECURITY] [API BREAKING CHANGE] Require logout token.

Special:Userlogout now requires a token

Api action=logout requires a csrf token and the request to be POSTed

Patch author: bawolff

Bug: T25227
Change-Id: Icb674095956bb3f6c847c9553c53e404402ea774

diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33
index f9b2826..f9826e2 100644 (file)
--- a/RELEASE-NOTES-1.33
+++ b/RELEASE-NOTES-1.33
@@ -171,6 +171,7 @@ For notes on 1.32.x and older releases, see HISTORY.
 * (T216245) Autoblocks will now be spread by action=edit and action=move.
 * action=query&meta=userinfo has a new uiprop, 'latestcontrib', that returns
   the date of user's latest contribution.
 * (T216245) Autoblocks will now be spread by action=edit and action=move.
 * action=query&meta=userinfo has a new uiprop, 'latestcontrib', that returns
   the date of user's latest contribution.

+* (T25227) action=logout now requires to be posted and have a csrf token.

 
 === Action API internal changes in 1.33 ===
 * A number of deprecated methods for API documentation, intended for overriding
 
 === Action API internal changes in 1.33 ===
 * A number of deprecated methods for API documentation, intended for overriding

diff --git a/includes/api/ApiLogout.php b/includes/api/ApiLogout.php
index c663d1e..39a96ac 100644 (file)
--- a/includes/api/ApiLogout.php
+++ b/includes/api/ApiLogout.php
@@ -59,13 +59,21 @@ class ApiLogout extends ApiBase {
                Hooks::run( 'UserLogoutComplete', [ &$user, &$injected_html, $oldName ] );
        }
 
                Hooks::run( 'UserLogoutComplete', [ &$user, &$injected_html, $oldName ] );
        }
 

+       public function mustBePosted() {
+               return true;
+       }
+
+       public function needsToken() {
+               return 'csrf';
+       }
+

        public function isReadMode() {
                return false;
        }
 
        protected function getExamplesMessages() {
                return [
        public function isReadMode() {
                return false;
        }
 
        protected function getExamplesMessages() {
                return [

-                       'action=logout'
+                       'action=logout&token=123ABC'

                                => 'apihelp-logout-example-logout',
                ];
        }
                                => 'apihelp-logout-example-logout',
                ];
        }

diff --git a/includes/skins/SkinTemplate.php b/includes/skins/SkinTemplate.php
index 8f45f28..25771bc 100644 (file)
--- a/includes/skins/SkinTemplate.php
+++ b/includes/skins/SkinTemplate.php
@@ -617,16 +617,18 @@ class SkinTemplate extends Skin {
                        $page = Title::newFromText( $request->getVal( 'title', '' ) );
                }
                $page = $request->getVal( 'returnto', $page );
                        $page = Title::newFromText( $request->getVal( 'title', '' ) );
                }
                $page = $request->getVal( 'returnto', $page );

-               $a = [];
+               $returnto = [];

                if ( strval( $page ) !== '' ) {
                if ( strval( $page ) !== '' ) {

-                       $a['returnto'] = $page;
+                       $returnto['returnto'] = $page;

                        $query = $request->getVal( 'returntoquery', $this->thisquery );
                        $query = $request->getVal( 'returntoquery', $this->thisquery );

+                       $paramsArray = wfCgiToArray( $query );
+                       unset( $paramsArray['logoutToken'] );
+                       $query = wfArrayToCgi( $paramsArray );

                        if ( $query != '' ) {
                        if ( $query != '' ) {

-                               $a['returntoquery'] = $query;
+                               $returnto['returntoquery'] = $query;

                        }
                }
 
                        }
                }
 

-               $returnto = wfArrayToCgi( $a );

                if ( $this->loggedin ) {
                        $personal_urls['userpage'] = [
                                'text' => $this->username,
                if ( $this->loggedin ) {
                        $personal_urls['userpage'] = [
                                'text' => $this->username,


@@ -691,9 +693,10 @@ class SkinTemplate extends Skin {
                                $personal_urls['logout'] = [
                                        'text' => $this->msg( 'pt-userlogout' )->text(),
                                        'href' => self::makeSpecialUrl( 'Userlogout',
                                $personal_urls['logout'] = [
                                        'text' => $this->msg( 'pt-userlogout' )->text(),
                                        'href' => self::makeSpecialUrl( 'Userlogout',

-                                               // userlogout link must always contain an & character, otherwise we might not be able
+                                               // Note: userlogout link must always contain an & character, otherwise we might not be able

                                                // to detect a buggy precaching proxy (T19790)
                                                // to detect a buggy precaching proxy (T19790)

-                                               $title->isSpecial( 'Preferences' ) ? 'noreturnto' : $returnto ),
+                                               ( $title->isSpecial( 'Preferences' ) ? [] : $returnto )
+                                               + [ 'logoutToken' => $this->getUser()->getEditToken( 'logoutToken', $this->getRequest() ) ] ),

                                        'active' => false
                                ];
                        }
                                        'active' => false
                                ];
                        }

diff --git a/includes/specials/SpecialUserLogout.php b/includes/specials/SpecialUserLogout.php
index a9b732e..568327d 100644 (file)
--- a/includes/specials/SpecialUserLogout.php
+++ b/includes/specials/SpecialUserLogout.php
@@ -48,6 +48,28 @@ class SpecialUserLogout extends UnlistedSpecialPage {
                $this->setHeaders();
                $this->outputHeader();
 
                $this->setHeaders();
                $this->outputHeader();
 

+               $out = $this->getOutput();
+               $user = $this->getUser();
+               $request = $this->getRequest();
+
+               $logoutToken = $request->getVal( 'logoutToken' );
+               $urlParams = [
+                       'logoutToken' => $user->getEditToken( 'logoutToken', $request )
+               ] + $request->getValues();
+               unset( $urlParams['title'] );
+               $continueLink = $this->getFullTitle()->getFullUrl( $urlParams );
+
+               if ( $logoutToken === null ) {
+                       $this->getOutput()->addWikiMsg( 'userlogout-continue', $continueLink );
+                       return;
+               }
+               if ( !$this->getUser()->matchEditToken(
+                       $logoutToken, 'logoutToken', $this->getRequest(), 24 * 60 * 60
+               ) ) {
+                       $this->getOutput()->addWikiMsg( 'userlogout-sessionerror', $continueLink );
+                       return;
+               }
+

                // Make sure it's possible to log out
                $session = MediaWiki\Session\SessionManager::getGlobalSession();
                if ( !$session->canSetUser() ) {
                // Make sure it's possible to log out
                $session = MediaWiki\Session\SessionManager::getGlobalSession();
                if ( !$session->canSetUser() ) {

diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 2b43dab..5eeaf93 100644 (file)
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -4220,5 +4220,7 @@
        "passwordpolicies-policyflag-forcechange": "must change on login",
        "passwordpolicies-policyflag-suggestchangeonlogin": "suggest change on login",
        "easydeflate-invaliddeflate": "Content provided is not properly deflated",
        "passwordpolicies-policyflag-forcechange": "must change on login",
        "passwordpolicies-policyflag-suggestchangeonlogin": "suggest change on login",
        "easydeflate-invaliddeflate": "Content provided is not properly deflated",

-       "unprotected-js": "For security reasons JavaScript cannot be loaded from unprotected pages. Please only create javascript in the MediaWiki: namespace or as a User subpage"
+       "unprotected-js": "For security reasons JavaScript cannot be loaded from unprotected pages. Please only create javascript in the MediaWiki: namespace or as a User subpage",
+       "userlogout-continue": "If you wish to log out please [$1 continue to the log out page].",
+       "userlogout-sessionerror": "Log out failed due to session error. Please [$1 try again]."

 }
 }

diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index 7aed514..8a776dc 100644 (file)
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -4427,5 +4427,7 @@
        "passwordpolicies-policyflag-forcechange": "Password policy flag that enforces changing invalid passwords on login.",
        "passwordpolicies-policyflag-suggestchangeonlogin": "Password policy flag that suggests changing invalid passwords on login.",
        "easydeflate-invaliddeflate": "Error message if the content passed to easydeflate was not deflated (compressed) properly",
        "passwordpolicies-policyflag-forcechange": "Password policy flag that enforces changing invalid passwords on login.",
        "passwordpolicies-policyflag-suggestchangeonlogin": "Password policy flag that suggests changing invalid passwords on login.",
        "easydeflate-invaliddeflate": "Error message if the content passed to easydeflate was not deflated (compressed) properly",

-       "unprotected-js": "Error message shown when trying to load javascript via action=raw that is not protected"
+       "unprotected-js": "Error message shown when trying to load javascript via action=raw that is not protected",
+       "userlogout-continue": "Shown if user attempted to log out without a token specified. Probably the user clicked on an old link that hasn't been updated to use the new system. $1 - url that user should click on in order to log out.",
+       "userlogout-sessionerror": "Shown when a user tries to log out with an invalid token. $1 is url with correct token that user should click."

 }
 }

diff --git a/tests/phpunit/includes/api/ApiLogoutTest.php b/tests/phpunit/includes/api/ApiLogoutTest.php
new file mode 100644 (file)
index 0000000..fcdb745
--- /dev/null
+++ b/tests/phpunit/includes/api/ApiLogoutTest.php
@@ -0,0 +1,55 @@
+<?php
+
+/**
+ * @group API
+ * @group Database
+ * @group medium
+ *
+ * @covers ApiLogout
+ */
+class ApiLogoutTest extends ApiTestCase {
+       public function setUp() {
+               parent::setUp();
+       }
+
+       public function testUserLogoutBadToken() {
+               try {
+                       $token = 'invalid token';
+                       $retLogout = $this->doUserLogout( $token );
+               }
+               catch ( ApiUsageException $e ) {
+                       $exceptionMsg = $e->getMessage();
+               }
+
+               $this->assertSame( "Invalid CSRF token.", $exceptionMsg );
+       }
+
+       public function testUserLogout() {
+               // TODO: there has to be a cleaner way to make User::doLogout happy
+               global $wgUser;
+               $wgUser = User::newFromId( '127.0.0.1' );
+
+               $token = $this->getUserCsrfTokenFromApi();
+               $retLogout = $this->doUserLogout( $token );
+               $this->assertFalse( $wgUser->isLoggedIn() );
+       }
+
+       public function getUserCsrfTokenFromApi() {
+               $retToken = $this->doApiRequest( [
+                       'action' => 'query',
+                       'meta' => 'tokens',
+                       'type' => 'csrf'
+               ] );
+
+               $this->assertArrayNotHasKey( 'warnings', $retToken );
+
+               return $retToken[0]['query']['tokens']['csrftoken'];
+       }
+
+       public function doUserLogout( $logoutToken ) {
+               return $this->doApiRequest( [
+                       'action' => 'logout',
+                       'token' => $logoutToken
+               ] );
+       }
+}