API: Insist authn parameters be in the POST body

Passwords should always be submitted in the POST body, not in the query
string. Thus, a warning will now be returned if the password for
action=login or any sensitive authentication request parameters for
AuthManager actions are found in the query string.

These warnings should be upgraded to errors in 1.29.

Change-Id: Ifb2c684bb28c9acc004be2b0c2fef839eb7624aa

diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28
index f6c3530..eac4e2c 100644 (file)
--- a/RELEASE-NOTES-1.28
+++ b/RELEASE-NOTES-1.28
@@ -61,6 +61,13 @@ production.
 * The following response properties from action=login, deprecated in 1.27, are
   now removed: lgtoken, cookieprefix, sessionid. Clients should handle cookies
   to properly manage session state.
+* Submitting the lgtoken and lgpassword parameters in the query string to
+  action=login is now deprecated and outputs a warning. They should be submitted
+  in the POST body instead.
+* Submitting sensitive authentication request parameters to action=clientlogin,
+  action=createaccount, action=linkaccount, and action=changeauthenticationdata
+  in the query string is now deprecated and outputs a warning. They should be
+  submitted in the POST body instead.
 
 === Action API internal changes in 1.28 ===
 * Added a new hook, 'ApiMakeParserOptions', to allow extensions to better

diff --git a/includes/api/ApiAuthManagerHelper.php b/includes/api/ApiAuthManagerHelper.php
index d330862..a4f54ee 100644 (file)
--- a/includes/api/ApiAuthManagerHelper.php
+++ b/includes/api/ApiAuthManagerHelper.php
@@ -157,8 +157,13 @@ class ApiAuthManagerHelper {
 
                // Collect the fields for all the requests
                $fields = [];
+               $sensitive = [];
                foreach ( $reqs as $req ) {
-                       $fields += (array)$req->getFieldInfo();
+                       $info = (array)$req->getFieldInfo();
+                       $fields += $info;
+                       $sensitive += array_filter( $info, function ( $opts ) {
+                               return !empty( $opts['sensitive'] );
+                       } );
                }
 
                // Extract the request data for the fields and mark those request
@@ -166,6 +171,16 @@ class ApiAuthManagerHelper {
                $data = array_intersect_key( $this->module->getRequest()->getValues(), $fields );
                $this->module->getMain()->markParamsUsed( array_keys( $data ) );
 
+               if ( $sensitive ) {
+                       try {
+                               $this->module->requirePostedParameters( array_keys( $sensitive ), 'noprefix' );
+                       } catch ( UsageException $ex ) {
+                               // Make this a warning for now, upgrade to an error in 1.29.
+                               $this->module->setWarning( $ex->getMessage() );
+                               $this->module->logFeatureUsage( $this->module->getModuleName() . '-params-in-query-string' );
+                       }
+               }
+
                return AuthenticationRequest::loadRequestsFromSubmission( $reqs, $data );
        }
 

diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php
index 4a1a520..fcb748c 100644 (file)
--- a/includes/api/ApiBase.php
+++ b/includes/api/ApiBase.php
@@ -776,6 +776,39 @@ abstract class ApiBase extends ContextSource {
                }
        }
 
+       /**
+        * Die if any of the specified parameters were found in the query part of
+        * the URL rather than the post body.
+        * @since 1.28
+        * @param string[] $params Parameters to check
+        * @param string $prefix Set to 'noprefix' to skip calling $this->encodeParamName()
+        */
+       public function requirePostedParameters( $params, $prefix = 'prefix' ) {
+               // Skip if $wgDebugAPI is set or we're in internal mode
+               if ( $this->getConfig()->get( 'DebugAPI' ) || $this->getMain()->isInternalMode() ) {
+                       return;
+               }
+
+               $queryValues = $this->getRequest()->getQueryValues();
+               $badParams = [];
+               foreach ( $params as $param ) {
+                       if ( $prefix !== 'noprefix' ) {
+                               $param = $this->encodeParamName( $param );
+                       }
+                       if ( array_key_exists( $param, $queryValues ) ) {
+                               $badParams[] = $param;
+                       }
+               }
+
+               if ( $badParams ) {
+                       $this->dieUsage(
+                               'The following parameters were found in the query string, but must be in the POST body: '
+                                       . join( ', ', $badParams ),
+                               'mustpostparams'
+                       );
+               }
+       }
+
        /**
         * Callback function used in requireOnlyOneParameter to check whether required parameters are set
         *
@@ -2197,7 +2230,7 @@ abstract class ApiBase extends ContextSource {
         * analysis.
         * @param string $feature Feature being used.
         */
-       protected function logFeatureUsage( $feature ) {
+       public function logFeatureUsage( $feature ) {
                $request = $this->getRequest();
                $s = '"' . addslashes( $feature ) . '"' .
                        ' "' . wfUrlencode( str_replace( ' ', '_', $this->getUser()->getName() ) ) . '"' .

diff --git a/includes/api/ApiLogin.php b/includes/api/ApiLogin.php
index 28937f7..9bc0b3a 100644 (file)
--- a/includes/api/ApiLogin.php
+++ b/includes/api/ApiLogin.php
@@ -70,6 +70,14 @@ class ApiLogin extends ApiBase {
                        return;
                }
 
+               try {
+                       $this->requirePostedParameters( [ 'password', 'token' ] );
+               } catch ( UsageException $ex ) {
+                       // Make this a warning for now, upgrade to an error in 1.29.
+                       $this->setWarning( $ex->getMessage() );
+                       $this->logFeatureUsage( 'login-params-in-query-string' );
+               }
+
                $params = $this->extractRequestParams();
 
                $result = [];

diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 565e829..22b079d 100644 (file)
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -1103,18 +1103,7 @@ class ApiMain extends ApiBase {
                                $this->dieUsageMsg( [ 'missingparam', 'token' ] );
                        }
 
-                       if ( !$this->getConfig()->get( 'DebugAPI' ) &&
-                               array_key_exists(
-                                       $module->encodeParamName( 'token' ),
-                                       $this->getRequest()->getQueryValues()
-                               )
-                       ) {
-                               $this->dieUsage(
-                                       "The '{$module->encodeParamName( 'token' )}' parameter was " .
-                                               'found in the query string, but must be in the POST body',
-                                       'mustposttoken'
-                               );
-                       }
+                       $module->requirePostedParameters( [ 'token' ] );
 
                        if ( !$module->validateToken( $moduleParams['token'], $moduleParams ) ) {
                                $this->dieUsageMsg( 'sessionfailure' );