[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.31 b/RELEASE-NOTES-1.31
index 621580a..e71c1f2 100644 (file)
--- a/RELEASE-NOTES-1.31
+++ b/RELEASE-NOTES-1.31
@@ -65,6 +65,7 @@ THIS IS NOT A RELEASE YET
 * (T216029) Chrome redirects to Special:BadTitle after editing a section with
   a non-Latin name on a page with non-Latin characters in title.
 * (T219728) Added support for new Japanese era name "Reiwa"
 * (T216029) Chrome redirects to Special:BadTitle after editing a section with
   a non-Latin name on a page with non-Latin characters in title.
 * (T219728) Added support for new Japanese era name "Reiwa"

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

 
 == MediaWiki 1.31.1 ==
 
 
 == MediaWiki 1.31.1 ==
 

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 4587533..203326f 100644 (file)
--- a/includes/skins/SkinTemplate.php
+++ b/includes/skins/SkinTemplate.php
@@ -627,16 +627,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,


@@ -699,9 +701,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 ded741d..1c11deb 100644 (file)
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -4468,5 +4468,7 @@
        "pagedata-not-acceptable": "No matching format found. Supported MIME types: $1",
        "pagedata-bad-title": "Invalid title: $1.",
        "unregistered-user-config": "For security reasons JavaScript, CSS and JSON user subpages cannot be loaded for unregistered users.",
        "pagedata-not-acceptable": "No matching format found. Supported MIME types: $1",
        "pagedata-bad-title": "Invalid title: $1.",
        "unregistered-user-config": "For security reasons JavaScript, CSS and JSON user subpages cannot be loaded for unregistered users.",

-       "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 7058278..ba5bee6 100644 (file)
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -4665,5 +4665,7 @@
        "pagedata-not-acceptable": "No matching format found. Supported MIME types: $1",
        "pagedata-bad-title": "Error shown when the requested title is invalid. Parameters:\n* $1: the malformed ID",
        "unregistered-user-config": "Shown when viewing a user JS, CSS or JSON subpage with ?action=raw&ctype=<mime type> where there is no such user. It is shown as a paragraph after a header saying 'Forbidden'.",
        "pagedata-not-acceptable": "No matching format found. Supported MIME types: $1",
        "pagedata-bad-title": "Error shown when the requested title is invalid. Parameters:\n* $1: the malformed ID",
        "unregistered-user-config": "Shown when viewing a user JS, CSS or JSON subpage with ?action=raw&ctype=<mime type> where there is no such user. It is shown as a paragraph after a header saying 'Forbidden'.",

-       "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
+               ] );
+       }
+}