From b5237cfc1b4757527648381f57f6cc23882a9afe Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Mon, 30 May 2016 23:11:33 +0200 Subject: [PATCH] HTMLForm: Allow distinguishing between form views and submission attempts Calling HTMLForm::setFormIdentifier() will set an internal identifier for this form. It will be submitted as a hidden form field, allowing HTMLForm to determine whether the form was submitted (or just viewed). Setting this serves two purposes: * If you use two or more forms on one page, it allows HTMLForm to identify which of the forms was submitted, and not attempt to validate the other ones. (T102114) * If you use checkbox or multiselect fields inside a form using the GET method, it allows HTMLForm to distinguish between the initial page view and a form submission with all checkboxes or select options unchecked. (T29676) Bug: T102114 Bug: T29676 Change-Id: Ib6ce3fd8941be86211cff5c6932b5e84982490fa --- includes/htmlform/HTMLCheckField.php | 4 +- includes/htmlform/HTMLCheckMatrix.php | 21 +++------ includes/htmlform/HTMLForm.php | 54 +++++++++++++++++++--- includes/htmlform/HTMLFormField.php | 14 ++++++ includes/htmlform/HTMLMultiSelectField.php | 22 +++------ 5 files changed, 76 insertions(+), 39 deletions(-) diff --git a/includes/htmlform/HTMLCheckField.php b/includes/htmlform/HTMLCheckField.php index 4a6b804730..a553839b09 100644 --- a/includes/htmlform/HTMLCheckField.php +++ b/includes/htmlform/HTMLCheckField.php @@ -118,9 +118,9 @@ class HTMLCheckField extends HTMLFormField { // GetCheck won't work like we want for checks. // Fetch the value in either one of the two following case: - // - we have a valid token (form got posted or GET forged by the user) + // - we have a valid submit attempt (form was just submitted, or a GET URL forged by the user) // - checkbox name has a value (false or true), ie is not null - if ( $request->getCheck( 'wpEditToken' ) || $request->getVal( $this->mName ) !== null ) { + if ( $this->isSubmitAttempt( $request ) || $request->getVal( $this->mName ) !== null ) { return $invert ? !$request->getBool( $this->mName ) : $request->getBool( $this->mName ); diff --git a/includes/htmlform/HTMLCheckMatrix.php b/includes/htmlform/HTMLCheckMatrix.php index 9f67233641..b324fb6afd 100644 --- a/includes/htmlform/HTMLCheckMatrix.php +++ b/includes/htmlform/HTMLCheckMatrix.php @@ -225,22 +225,13 @@ class HTMLCheckMatrix extends HTMLFormField implements HTMLNestedFilterable { * @return array */ function loadDataFromRequest( $request ) { - if ( $this->mParent->getMethod() == 'post' ) { - if ( $request->wasPosted() ) { - // Checkboxes are not added to the request arrays if they're not checked, - // so it's perfectly possible for there not to be an entry at all - return $request->getArray( $this->mName, [] ); - } else { - // That's ok, the user has not yet submitted the form, so show the defaults - return $this->getDefault(); - } - } else { - // This is the impossible case: if we look at $_GET and see no data for our - // field, is it because the user has not yet submitted the form, or that they - // have submitted it with all the options unchecked. We will have to assume the - // latter, which basically means that you can't specify 'positive' defaults - // for GET forms. + if ( $this->isSubmitAttempt( $request ) ) { + // Checkboxes are just not added to the request arrays if they're not checked, + // so it's perfectly possible for there not to be an entry at all return $request->getArray( $this->mName, [] ); + } else { + // That's ok, the user has not yet submitted the form, so show the defaults + return $this->getDefault(); } } diff --git a/includes/htmlform/HTMLForm.php b/includes/htmlform/HTMLForm.php index 8ac4cf2a29..2b7417e468 100644 --- a/includes/htmlform/HTMLForm.php +++ b/includes/htmlform/HTMLForm.php @@ -190,6 +190,7 @@ class HTMLForm extends ContextSource { protected $mSubmitText; protected $mSubmitTooltip; + protected $mFormIdentifier; protected $mTitle; protected $mMethod = 'post'; protected $mWasSubmitted = false; @@ -480,7 +481,14 @@ class HTMLForm extends ContextSource { } # Load data from the request. - $this->loadData(); + if ( + $this->mFormIdentifier === null || + $this->getRequest()->getVal( 'wpFormIdentifier' ) === $this->mFormIdentifier + ) { + $this->loadData(); + } else { + $this->mFieldData = []; + } return $this; } @@ -492,22 +500,29 @@ class HTMLForm extends ContextSource { public function tryAuthorizedSubmit() { $result = false; - $submit = false; + $identOkay = false; + if ( $this->mFormIdentifier === null ) { + $identOkay = true; + } else { + $identOkay = $this->getRequest()->getVal( 'wpFormIdentifier' ) === $this->mFormIdentifier; + } + + $tokenOkay = false; if ( $this->getMethod() !== 'post' ) { - $submit = true; // no session check needed + $tokenOkay = true; // no session check needed } elseif ( $this->getRequest()->wasPosted() ) { $editToken = $this->getRequest()->getVal( 'wpEditToken' ); if ( $this->getUser()->isLoggedIn() || $editToken !== null ) { // Session tokens for logged-out users have no security value. // However, if the user gave one, check it in order to give a nice // "session expired" error instead of "permission denied" or such. - $submit = $this->getUser()->matchEditToken( $editToken, $this->mTokenSalt ); + $tokenOkay = $this->getUser()->matchEditToken( $editToken, $this->mTokenSalt ); } else { - $submit = true; + $tokenOkay = true; } } - if ( $submit ) { + if ( $tokenOkay && $identOkay ) { $this->mWasSubmitted = true; $result = $this->trySubmit(); } @@ -1042,6 +1057,12 @@ class HTMLForm extends ContextSource { */ public function getHiddenFields() { $html = ''; + if ( $this->mFormIdentifier !== null ) { + $html .= Html::hidden( + 'wpFormIdentifier', + $this->mFormIdentifier + ) . "\n"; + } if ( $this->getMethod() === 'post' ) { $html .= Html::hidden( 'wpEditToken', @@ -1327,6 +1348,27 @@ class HTMLForm extends ContextSource { return $this; } + /** + * Set an internal identifier for this form. It will be submitted as a hidden form field, allowing + * HTMLForm to determine whether the form was submitted (or merely viewed). Setting this serves + * two purposes: + * + * - If you use two or more forms on one page, it allows HTMLForm to identify which of the forms + * was submitted, and not attempt to validate the other ones. + * - If you use checkbox or multiselect fields inside a form using the GET method, it allows + * HTMLForm to distinguish between the initial page view and a form submission with all + * checkboxes or select options unchecked. + * + * @since 1.28 + * @param string $ident + * @return $this + */ + public function setFormIdentifier( $ident ) { + $this->mFormIdentifier = $ident; + + return $this; + } + /** * Stop a default submit button being shown for this form. This implies that an * alternate submit method must be provided manually. diff --git a/includes/htmlform/HTMLFormField.php b/includes/htmlform/HTMLFormField.php index 9f5e72838b..7cb497d11c 100644 --- a/includes/htmlform/HTMLFormField.php +++ b/includes/htmlform/HTMLFormField.php @@ -349,6 +349,20 @@ abstract class HTMLFormField { $this->mShowEmptyLabels = $show; } + /** + * Can we assume that the request is an attempt to submit a HTMLForm, as opposed to an attempt to + * just view it? This can't normally be distinguished for e.g. checkboxes. + * + * Returns true if the request has a field for a CSRF token (wpEditToken) or a form identifier + * (wpFormIdentifier). + * + * @param WebRequest $request + * @return boolean + */ + protected function isSubmitAttempt( WebRequest $request ) { + return $request->getCheck( 'wpEditToken' ) || $request->getCheck( 'wpFormIdentifier' ); + } + /** * Get the value that this input has been set to from a posted form, * or the input's default value if it has not been set. diff --git a/includes/htmlform/HTMLMultiSelectField.php b/includes/htmlform/HTMLMultiSelectField.php index 23125bd465..a231b2f6b1 100644 --- a/includes/htmlform/HTMLMultiSelectField.php +++ b/includes/htmlform/HTMLMultiSelectField.php @@ -123,23 +123,13 @@ class HTMLMultiSelectField extends HTMLFormField implements HTMLNestedFilterable * @return string */ function loadDataFromRequest( $request ) { - if ( $this->mParent->getMethod() == 'post' ) { - if ( $request->wasPosted() ) { - # Checkboxes are just not added to the request arrays if they're not checked, - # so it's perfectly possible for there not to be an entry at all - return $request->getArray( $this->mName, [] ); - } else { - # That's ok, the user has not yet submitted the form, so show the defaults - return $this->getDefault(); - } - } else { - # This is the impossible case: if we look at $_GET and see no data for our - # field, is it because the user has not yet submitted the form, or that they - # have submitted it with all the options unchecked? We will have to assume the - # latter, which basically means that you can't specify 'positive' defaults - # for GET forms. - # @todo FIXME... + if ( $this->isSubmitAttempt( $request ) ) { + // Checkboxes are just not added to the request arrays if they're not checked, + // so it's perfectly possible for there not to be an entry at all return $request->getArray( $this->mName, [] ); + } else { + // That's ok, the user has not yet submitted the form, so show the defaults + return $this->getDefault(); } } -- 2.20.1