jenkins-bot has submitted this change and it was merged.
Change subject: API: Insist authn parameters be in the POST body
......................................................................
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
---
M RELEASE-NOTES-1.28
M includes/api/ApiAuthManagerHelper.php
M includes/api/ApiBase.php
M includes/api/ApiLogin.php
M includes/api/ApiMain.php
5 files changed, 66 insertions(+), 14 deletions(-)
Approvals:
Gergő Tisza: Looks good to me, approved
jenkins-bot: Verified
diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28
index f6c3530..eac4e2c 100644
--- a/RELEASE-NOTES-1.28
+++ b/RELEASE-NOTES-1.28
@@ -61,6 +61,13 @@
* 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
--- a/includes/api/ApiAuthManagerHelper.php
+++ b/includes/api/ApiAuthManagerHelper.php
@@ -157,8 +157,13 @@
// 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 @@
$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
--- a/includes/api/ApiBase.php
+++ b/includes/api/ApiBase.php
@@ -777,6 +777,39 @@
}
/**
+ * 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
*
* @param object $x Parameter to check is not null/false
@@ -2197,7 +2230,7 @@
* 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
--- a/includes/api/ApiLogin.php
+++ b/includes/api/ApiLogin.php
@@ -70,6 +70,14 @@
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
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -1103,18 +1103,7 @@
$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' );
--
To view, visit https://gerrit.wikimedia.org/r/305545
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifb2c684bb28c9acc004be2b0c2fef839eb7624aa
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: Gergő Tisza <gti...@wikimedia.org>
Gerrit-Reviewer: Legoktm <legoktm.wikipe...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
