From 43c00bff590322a65f5ef6cae653ec70648f766e Mon Sep 17 00:00:00 2001 From: Brian Wolff 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 Bug: T25227 --- RELEASE-NOTES-1.33 | 1 + includes/api/ApiLogout.php | 10 +++++++++- includes/skins/SkinTemplate.php | 12 ++++++------ includes/specials/SpecialUserLogout.php | 22 ++++++++++++++++++++++ languages/i18n/en.json | 4 +++- languages/i18n/qqq.json | 4 +++- 6 files changed, 44 insertions(+), 9 deletions(-) diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index db5fea0..4f99e86 100644 --- a/RELEASE-NOTES-1.33 +++ b/RELEASE-NOTES-1.33 @@ -169,6 +169,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. +* (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 diff --git a/includes/api/ApiLogout.php b/includes/api/ApiLogout.php index c663d1e..39a96ac 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 8f45f28..068e43a 100644 --- a/includes/skins/SkinTemplate.php +++ b/includes/skins/SkinTemplate.php @@ -617,16 +617,15 @@ 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 ); if ( $query != '' ) { - $a['returntoquery'] = $query; + $returnto['returntoquery'] = $query; } } - $returnto = wfArrayToCgi( $a ); if ( $this->loggedin ) { $personal_urls['userpage'] = [ 'text' => $this->username, @@ -691,9 +690,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 a9b732e..aa5ad75 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 = array( + '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 56ff467..b92fee7 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -4216,5 +4216,7 @@ "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 c7f2733..6731a76 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -4423,5 +4423,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", - "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." } -- 2.20.1