From 63a9098e9cafcbf8bb4bd02f20cf5643c61c2472 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Wed, 9 May 2018 14:53:32 -0400 Subject: [PATCH] Add getLoginSecurityLevel() support to FormSpecialPage The base SpecialPage will handle reauthentication automatically if you just implement getLoginSecurityLevel() to return an appropriate string. But it doesn't work with FormSpecialPage, and if you try calling checkLoginSecurityLevel() manually it'll lose any post data if the reauth happens when the form is posted. So this patch has SpecialPage::checkLoginSecurityLevel() preserve post data across reauth (using logic similar to that in AuthManagerSpecialPage), and has FormSpecialPage call checkLoginSecurityLevel() in the same way the base SpecialPage does. It also fixes the SpecialPage logic to not call checkLoginSecurityLevel() when the special page doesn't implement getLoginSecurityLevel(), as was the originally-intended behavior. Apparently almost nothing actually gets to SpecialPage::execute() or this would probably have been noticed already. Change-Id: Ic89dc1b6583aaecd2efe3f5109896148a188c271 (cherry picked from commit bfc4e41636aca33b943f8522024bd9f8eeac1977) --- RELEASE-NOTES-1.31 | 4 ++ includes/specialpage/FormSpecialPage.php | 43 ++++++++++++++++++- includes/specialpage/SpecialPage.php | 54 ++++++++++++++++++++++-- 3 files changed, 95 insertions(+), 6 deletions(-) diff --git a/RELEASE-NOTES-1.31 b/RELEASE-NOTES-1.31 index 3e0efe4a56..54672be5e3 100644 --- a/RELEASE-NOTES-1.31 +++ b/RELEASE-NOTES-1.31 @@ -79,6 +79,10 @@ Required PHP version has been increased from 7.0.0 to 7.0.13. saveFileDependencies(). * (T224374) Fix message parameters so that the message that says SQLite is out of date makes sense. +* SpecialPage::checkLoginSecurityLevel() will now preserve POST data when + reauthenticating. +* FormSpecialPage::execute() will now call checkLoginSecurityLevel() if + getLoginSecurityLevel() returns non-false. == MediaWiki 1.31.1 == diff --git a/includes/specialpage/FormSpecialPage.php b/includes/specialpage/FormSpecialPage.php index 66c7d47ea9..81a0036e83 100644 --- a/includes/specialpage/FormSpecialPage.php +++ b/includes/specialpage/FormSpecialPage.php @@ -35,6 +35,12 @@ abstract class FormSpecialPage extends SpecialPage { */ protected $par = null; + /** + * @var array|null POST data preserved across re-authentication + * @since 1.32 + */ + protected $reauthPostData = null; + /** * Get an HTMLForm descriptor array * @return array @@ -89,13 +95,31 @@ abstract class FormSpecialPage extends SpecialPage { * @return HTMLForm|null */ protected function getForm() { + $context = $this->getContext(); + $onSubmit = [ $this, 'onSubmit' ]; + + if ( $this->reauthPostData ) { + // Restore POST data + $context = new DerivativeContext( $context ); + $oldRequest = $this->getRequest(); + $context->setRequest( new DerivativeRequest( + $oldRequest, $this->reauthPostData + $oldRequest->getQueryValues(), true + ) ); + + // But don't treat it as a "real" submission just in case of some + // crazy kind of CSRF. + $onSubmit = function () { + return false; + }; + } + $form = HTMLForm::factory( $this->getDisplayFormat(), $this->getFormFields(), - $this->getContext(), + $context, $this->getMessagePrefix() ); - $form->setSubmitCallback( [ $this, 'onSubmit' ] ); + $form->setSubmitCallback( $onSubmit ); if ( $this->getDisplayFormat() !== 'ooui' ) { // No legend and wrapper by default in OOUI forms, but can be set manually // from alterForm() @@ -151,6 +175,11 @@ abstract class FormSpecialPage extends SpecialPage { // This will throw exceptions if there's a problem $this->checkExecutePermissions( $this->getUser() ); + $securityLevel = $this->getLoginSecurityLevel(); + if ( $securityLevel !== false && !$this->checkLoginSecurityLevel( $securityLevel ) ) { + return; + } + $form = $this->getForm(); if ( $form->show() ) { $this->onSuccess(); @@ -199,4 +228,14 @@ abstract class FormSpecialPage extends SpecialPage { public function requiresUnblock() { return true; } + + /** + * Preserve POST data across reauthentication + * + * @since 1.32 + * @param array $data + */ + protected function setReauthPostData( array $data ) { + $this->reauthPostData = $data; + } } diff --git a/includes/specialpage/SpecialPage.php b/includes/specialpage/SpecialPage.php index 6828b4a400..317aa0d7c8 100644 --- a/includes/specialpage/SpecialPage.php +++ b/includes/specialpage/SpecialPage.php @@ -352,6 +352,23 @@ class SpecialPage implements MessageLocalizer { return false; } + /** + * Record preserved POST data after a reauthentication. + * + * This is called from checkLoginSecurityLevel() when returning from the + * redirect for reauthentication, if the redirect had been served in + * response to a POST request. + * + * The base SpecialPage implementation does nothing. If your subclass uses + * getLoginSecurityLevel() or checkLoginSecurityLevel(), it should probably + * implement this to do something with the data. + * + * @since 1.32 + * @param array $data + */ + protected function setReauthPostData( array $data ) { + } + /** * Verifies that the user meets the security level, possibly reauthenticating them in the process. * @@ -378,16 +395,42 @@ class SpecialPage implements MessageLocalizer { */ protected function checkLoginSecurityLevel( $level = null ) { $level = $level ?: $this->getName(); + $key = 'SpecialPage:reauth:' . $this->getName(); + $request = $this->getRequest(); + $securityStatus = AuthManager::singleton()->securitySensitiveOperationStatus( $level ); if ( $securityStatus === AuthManager::SEC_OK ) { + $uniqueId = $request->getVal( 'postUniqueId' ); + if ( $uniqueId ) { + $key = $key . ':' . $uniqueId; + $session = $request->getSession(); + $data = $session->getSecret( $key ); + if ( $data ) { + $session->remove( $key ); + $this->setReauthPostData( $data ); + } + } return true; } elseif ( $securityStatus === AuthManager::SEC_REAUTH ) { - $request = $this->getRequest(); $title = self::getTitleFor( 'Userlogin' ); + $queryParams = $request->getQueryValues(); + + if ( $request->wasPosted() ) { + $data = array_diff_assoc( $request->getValues(), $request->getQueryValues() ); + if ( $data ) { + // unique ID in case the same special page is open in multiple browser tabs + $uniqueId = MWCryptRand::generateHex( 6 ); + $key = $key . ':' . $uniqueId; + $queryParams['postUniqueId'] = $uniqueId; + $session = $request->getSession(); + $session->persist(); // Just in case + $session->setSecret( $key, $data ); + } + } + $query = [ 'returnto' => $this->getFullTitle()->getPrefixedDBkey(), - 'returntoquery' => wfArrayToCgi( array_diff_key( $request->getQueryValues(), - [ 'title' => true ] ) ), + 'returntoquery' => wfArrayToCgi( array_diff_key( $queryParams, [ 'title' => true ] ) ), 'force' => $level, ]; $url = $title->getFullURL( $query, false, PROTO_HTTPS ); @@ -568,7 +611,10 @@ class SpecialPage implements MessageLocalizer { public function execute( $subPage ) { $this->setHeaders(); $this->checkPermissions(); - $this->checkLoginSecurityLevel( $this->getLoginSecurityLevel() ); + $securityLevel = $this->getLoginSecurityLevel(); + if ( $securityLevel !== false && !$this->checkLoginSecurityLevel( $securityLevel ) ) { + return; + } $this->outputHeader(); } -- 2.20.1