Page MenuHomePhabricator

File Metadata

Author
sbassett
Created
Apr 16 2019, 10:15 PM

T25227.patch

From 43c00bff590322a65f5ef6cae653ec70648f766e Mon Sep 17 00:00:00 2001
From: Brian Wolff <bawolff+wn@gmail.com>
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

Event Timeline