From ea2f3d1c3adfa62b67f905d2795bc997b424a3f8 Mon Sep 17 00:00:00 2001 From: sbassett Date: Tue, 16 Apr 2019 17:09:43 -0500 Subject: [PATCH] [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 --- RELEASE-NOTES-1.31 | 1 + includes/api/ApiLogout.php | 10 +++- includes/skins/SkinTemplate.php | 15 +++--- includes/specials/SpecialUserLogout.php | 22 ++++++++ languages/i18n/en.json | 4 +- languages/i18n/qqq.json | 4 +- tests/phpunit/includes/api/ApiLogoutTest.php | 55 ++++++++++++++++++++ 7 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 tests/phpunit/includes/api/ApiLogoutTest.php diff --git a/RELEASE-NOTES-1.31 b/RELEASE-NOTES-1.31 index 621580a357..e71c1f2943 100644 --- 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" +* (T25227) action=logout now requires to be posted and have a csrf token. == MediaWiki 1.31.1 == diff --git a/includes/api/ApiLogout.php b/includes/api/ApiLogout.php index c663d1e4bb..39a96ac563 100644 --- 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 ] ); } + public function mustBePosted() { + return true; + } + + public function needsToken() { + return 'csrf'; + } + public function isReadMode() { return false; } protected function getExamplesMessages() { return [ - 'action=logout' + 'action=logout&token=123ABC' => 'apihelp-logout-example-logout', ]; } diff --git a/includes/skins/SkinTemplate.php b/includes/skins/SkinTemplate.php index 45875334bc..203326f126 100644 --- 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 ); - $a = []; + $returnto = []; if ( strval( $page ) !== '' ) { - $a['returnto'] = $page; + $returnto['returnto'] = $page; $query = $request->getVal( 'returntoquery', $this->thisquery ); + $paramsArray = wfCgiToArray( $query ); + unset( $paramsArray['logoutToken'] ); + $query = wfArrayToCgi( $paramsArray ); if ( $query != '' ) { - $a['returntoquery'] = $query; + $returnto['returntoquery'] = $query; } } - $returnto = wfArrayToCgi( $a ); 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', - // 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) - $title->isSpecial( 'Preferences' ) ? 'noreturnto' : $returnto ), + ( $title->isSpecial( 'Preferences' ) ? [] : $returnto ) + + [ 'logoutToken' => $this->getUser()->getEditToken( 'logoutToken', $this->getRequest() ) ] ), 'active' => false ]; } diff --git a/includes/specials/SpecialUserLogout.php b/includes/specials/SpecialUserLogout.php index a9b732efb1..568327d25b 100644 --- a/includes/specials/SpecialUserLogout.php +++ b/includes/specials/SpecialUserLogout.php @@ -48,6 +48,28 @@ class SpecialUserLogout extends UnlistedSpecialPage { $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() ) { diff --git a/languages/i18n/en.json b/languages/i18n/en.json index ded741d796..1c11deb4e4 100644 --- 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.", - "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 70582784d8..ba5bee606c 100644 --- 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= 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 index 0000000000..fcdb745aaa --- /dev/null +++ b/tests/phpunit/includes/api/ApiLogoutTest.php @@ -0,0 +1,55 @@ +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 + ] ); + } +} -- 2.20.1