From: Brad Jorsch Date: Thu, 18 Aug 2016 17:36:11 +0000 (-0400) Subject: API: Insist authn parameters be in the POST body X-Git-Tag: 1.31.0-rc.0~5995^2 X-Git-Url: https://git.heureux-cyclage.org/index.php?a=commitdiff_plain;h=6a068d18e1e34c8744d12cfee08fe998828f9387;p=lhc%2Fweb%2Fwiklou.git 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 f6c3530316..eac4e2c32f 100644 --- 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 d330862e79..a4f54ee6d8 100644 --- 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 4a1a520f12..fcb748c202 100644 --- 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 28937f7873..9bc0b3a433 100644 --- 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 565e829f79..22b079dee1 100644 --- 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' );