Issue #2380053 by klausi, pwolanin, tsphethean, sun, David_Rothstein: Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields

4 files changed, 104 insertions, 3 deletions

diff --git a/CHANGELOG.txt b/CHANGELOG.txt
index 4e0f43386..d03e71cc3 100644
--- a/CHANGELOG.txt
+++ b/CHANGELOG.txt
@@ -1,6 +1,8 @@
 
 Drupal 7.35, xxxx-xx-xx (development version)
 -----------------------
+- Prevented the form API from allowing arrays to be submitted for various form
+  elements (such as textfields, textareas, and password fields).
 - Fixed a bug in the Contact module which caused the global user object to have
   the incorrect name and e-mail address during the remainder of the page
   request after the contact form is submitted.
diff --git a/includes/form.inc b/includes/form.inc
index da1caa819..223c4cd68 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -2451,6 +2451,17 @@ function form_type_password_confirm_value($element, $input = FALSE) {
     $element += array('#default_value' => array());
     return $element['#default_value'] + array('pass1' => '', 'pass2' => '');
   }
+  $value = array('pass1' => '', 'pass2' => '');
+  // Throw out all invalid array keys; we only allow pass1 and pass2.
+  foreach ($value as $allowed_key => $default) {
+    // These should be strings, but allow other scalars since they might be
+    // valid input in programmatic form submissions. Any nested array values
+    // are ignored.
+    if (isset($input[$allowed_key]) && is_scalar($input[$allowed_key])) {
+      $value[$allowed_key] = (string) $input[$allowed_key];
+    }
+  }
+  return $value;
 }
 
 /**
@@ -2495,6 +2506,27 @@ function form_type_select_value($element, $input = FALSE) {
 }
 
 /**
+ * Determines the value for a textarea form element.
+ *
+ * @param array $element
+ *   The form element whose value is being populated.
+ * @param mixed $input
+ *   The incoming input to populate the form element. If this is FALSE,
+ *   the element's default value should be returned.
+ *
+ * @return string
+ *   The data that will appear in the $element_state['values'] collection
+ *   for this element. Return nothing to use the default.
+ */
+function form_type_textarea_value($element, $input = FALSE) {
+  if ($input !== FALSE) {
+    // This should be a string, but allow other scalars since they might be
+    // valid input in programmatic form submissions.
+    return is_scalar($input) ? (string) $input : '';
+  }
+}
+
+/**
  * Determines the value for a textfield form element.
  *
  * @param $element
@@ -2509,9 +2541,12 @@ function form_type_select_value($element, $input = FALSE) {
  */
 function form_type_textfield_value($element, $input = FALSE) {
   if ($input !== FALSE && $input !== NULL) {
-    // Equate $input to the form value to ensure it's marked for
-    // validation.
-    return str_replace(array("\r", "\n"), '', $input);
+    // This should be a string, but allow other scalars since they might be
+    // valid input in programmatic form submissions.
+    if (!is_scalar($input)) {
+      $input = '';
+    }
+    return str_replace(array("\r", "\n"), '', (string) $input);
   }
 }
 
diff --git a/modules/simpletest/tests/form.test b/modules/simpletest/tests/form.test
index f90b854c7..0bf6c8c65 100644
--- a/modules/simpletest/tests/form.test
+++ b/modules/simpletest/tests/form.test
@@ -470,6 +470,64 @@ class FormsTestCase extends DrupalWebTestCase {
     $this->drupalPost(NULL, array('checkboxes[one]' => TRUE, 'checkboxes[two]' => TRUE), t('Submit'));
     $this->assertText('An illegal choice has been detected.', 'Input forgery was detected.');
   }
+
+  /**
+   * Tests that submitted values are converted to scalar strings for textfields.
+   */
+  public function testTextfieldStringValue() {
+    // Check multivalued submissions.
+    $multivalue = array('evil' => 'multivalue', 'not so' => 'good');
+    $this->checkFormValue('textfield', $multivalue, '');
+    $this->checkFormValue('password', $multivalue, '');
+    $this->checkFormValue('textarea', $multivalue, '');
+    $this->checkFormValue('machine_name', $multivalue, '');
+    $this->checkFormValue('password_confirm', $multivalue, array('pass1' => '', 'pass2' => ''));
+    // Check integer submissions.
+    $integer = 5;
+    $string = '5';
+    $this->checkFormValue('textfield', $integer, $string);
+    $this->checkFormValue('password', $integer, $string);
+    $this->checkFormValue('textarea', $integer, $string);
+    $this->checkFormValue('machine_name', $integer, $string);
+    $this->checkFormValue('password_confirm', array('pass1' => $integer, 'pass2' => $integer), array('pass1' => $string, 'pass2' => $string));
+    // Check that invalid array keys are ignored for password confirm elements.
+    $this->checkFormValue('password_confirm', array('pass1' => 'test', 'pass2' => 'test', 'extra' => 'invalid'), array('pass1' => 'test', 'pass2' => 'test'));
+  }
+
+  /**
+   * Checks that a given form input value is sanitized to the expected result.
+   *
+   * @param string $element_type
+   *   The form element type. Example: textfield.
+   * @param mixed $input_value
+   *   The submitted user input value for the form element.
+   * @param mixed $expected_value
+   *   The sanitized result value in the form state after calling
+   *   form_builder().
+   */
+  protected function checkFormValue($element_type, $input_value, $expected_value) {
+    $form_id = $this->randomName();
+    $form = array();
+    $form_state = form_state_defaults();
+    $form['op'] = array('#type' => 'submit', '#value' => t('Submit'));
+    $form[$element_type] = array(
+      '#type' => $element_type,
+      '#title' => 'test',
+    );
+
+    $form_state['input'][$element_type] = $input_value;
+    $form_state['input']['form_id'] = $form_id;
+    $form_state['method'] = 'post';
+    $form_state['values'] = array();
+    drupal_prepare_form($form_id, $form, $form_state);
+
+    // This is the main function we want to test: it is responsible for
+    // populating user supplied $form_state['input'] to sanitized
+    // $form_state['values'].
+    form_builder($form_id, $form, $form_state);
+
+    $this->assertIdentical($form_state['values'][$element_type], $expected_value, format_string('Form submission for the "@element_type" element type has been correctly sanitized.', array('@element_type' => $element_type)));
+  }
 }
 
 /**
diff --git a/modules/system/system.module b/modules/system/system.module
index a27493579..6a6200ea1 100644
--- a/modules/system/system.module
+++ b/modules/system/system.module
@@ -374,6 +374,9 @@ function system_element_info() {
     '#element_validate' => array('form_validate_machine_name'),
     '#theme' => 'textfield',
     '#theme_wrappers' => array('form_element'),
+    // Use the same value callback as for textfields; this ensures that we only
+    // get string values.
+    '#value_callback' => 'form_type_textfield_value',
   );
   $types['password'] = array(
     '#input' => TRUE,
@@ -382,6 +385,9 @@ function system_element_info() {
     '#process' => array('ajax_process_form'),
     '#theme' => 'password',
     '#theme_wrappers' => array('form_element'),
+    // Use the same value callback as for textfields; this ensures that we only
+    // get string values.
+    '#value_callback' => 'form_type_textfield_value',
   );
   $types['password_confirm'] = array(
     '#input' => TRUE,