- Patch #736298 by effulgentsia, eojthebrave, andypost, robeano: fixed Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security.

3 files changed, 179 insertions, 38 deletions

diff --git a/modules/file/file.field.inc b/modules/file/file.field.inc
index 12420d846..e0d452bd1 100644
--- a/modules/file/file.field.inc
+++ b/modules/file/file.field.inc
@@ -767,32 +767,45 @@ function theme_file_widget_multiple($variables) {
       continue;
     }
 
-    // Render all the buttons in the field as an "operation".
-    $operations = '';
+    // Delay rendering of the buttons, so that they can be rendered later in the
+    // "operations" column.
+    $operations_elements = array();
     foreach (element_children($element[$key]) as $sub_key) {
       if (isset($element[$key][$sub_key]['#type']) && $element[$key][$sub_key]['#type'] == 'submit') {
-        $operations .= drupal_render($element[$key][$sub_key]);
+        hide($element[$key][$sub_key]);
+        $operations_elements[] = &$element[$key][$sub_key];
       }
     }
 
-    // Render the "Display" option in its own own column.
+    // Delay rendering of the "Display" option and the weight selector, so that
+    // each can be rendered later in its own column.
+    if ($element['#display_field']) {
+      hide($element[$key]['display']);
+    }
+    hide($element[$key]['_weight']);
+
+    // Render everything else together in a column, without the normal wrappers.
+    $element[$key]['#theme_wrappers'] = array();
+    $information = drupal_render($element[$key]);
+
+    // Render the previously hidden elements, using render() instead of
+    // drupal_render(), to undo the earlier hide().
+    $operations = '';
+    foreach ($operations_elements as $operation_element) {
+      $operations .= render($operation_element);
+    }
     $display = '';
     if ($element['#display_field']) {
       unset($element[$key]['display']['#title']);
       $display = array(
-        'data' => drupal_render($element[$key]['display']),
+        'data' => render($element[$key]['display']),
         'class' => array('checkbox'),
       );
     }
-
-    // Render the weight in its own column.
     $element[$key]['_weight']['#attributes']['class'] = array($weight_class);
-    $weight = drupal_render($element[$key]['_weight']);
-
-    // Render everything else together in a column, without the normal wrappers.
-    $element[$key]['#theme_wrappers'] = array();
-    $information = drupal_render($element[$key]);
+    $weight = render($element[$key]['_weight']);
 
+    // Arrange the row with all of the rendered columns.
     $row = array();
     $row[] = $information;
     if ($element['#display_field']) {
diff --git a/modules/file/file.module b/modules/file/file.module
index 2f73d7267..8d923ac6f 100644
--- a/modules/file/file.module
+++ b/modules/file/file.module
@@ -65,6 +65,7 @@ function file_element_info() {
     '#process' => array('file_managed_file_process'),
     '#value_callback' => 'file_managed_file_value',
     '#element_validate' => array('file_managed_file_validate'),
+    '#pre_render' => array('file_managed_file_pre_render'),
     '#theme' => 'file_managed_file',
     '#theme_wrappers' => array('form_element'),
     '#progress_indicator' => 'throbber',
@@ -384,25 +385,6 @@ function file_managed_file_process($element, &$form_state, $form) {
     '#weight' => -5,
   );
 
-  // @todo It is not good to call these private functions. This should be
-  //   refactored so that the file deletion happens during a submit handler,
-  //   and form changes affected by that (such as toggling the upload and remove
-  //   buttons) happens during the 2nd run of this function that is triggered by
-  //   a form rebuild: http://drupal.org/node/736298.
-  if (_form_button_was_clicked($element['remove_button'], $form_state) || _form_element_triggered_scripted_submission($element['remove_button'], $form_state)) {
-    // If it's a temporary file we can safely remove it immediately, otherwise
-    // it's up to the implementing module to clean up files that are in use.
-    if ($element['#file'] && $element['#file']->status == 0) {
-      file_delete($element['#file']);
-    }
-    $element['#file'] = FALSE;
-    $fid = 0;
-  }
-
-  // Set access on the buttons.
-  $element['upload_button']['#access'] = empty($fid);
-  $element['remove_button']['#access'] = !empty($fid);
-
   $element['fid'] = array(
     '#type' => 'hidden',
     '#value' => $fid,
@@ -436,7 +418,6 @@ function file_managed_file_process($element, &$form_state, $form) {
     '#name' => 'files[' . implode('_', $element['#parents']) . ']',
     '#type' => 'file',
     '#size' => 22,
-    '#access' => empty($fid),
     '#theme_wrappers' => array(),
     '#weight' => -10,
   );
@@ -565,13 +546,50 @@ function file_managed_file_validate(&$element, &$form_state) {
 }
 
 /**
- * Submit handler for non-JavaScript uploads.
+ * Submit handler for upload and remove buttons of managed_file elements.
  */
 function file_managed_file_submit($form, &$form_state) {
-  // Do not redirect and leave the page after uploading a file. This keeps
-  // all the current form values in place. The file is saved by the
-  // #value_callback on the form element.
-  $form_state['redirect'] = FALSE;
+  // Determine whether it was the upload or the remove button that was clicked,
+  // and set $element to the managed_file element that contains that button.
+  $parents = $form_state['triggering_element']['#array_parents'];
+  $button_key = array_pop($parents);
+  $element = $form;
+  foreach ($parents as $parent) {
+    $element = $element[$parent];
+  }
+
+  // No action is needed here for the upload button, because all file uploads on
+  // the form are processed by file_managed_file_value() regardless of which
+  // button was clicked. Action is needed here for the remove button, because we
+  // only remove a file in response to its remove button being clicked.
+  if ($button_key == 'remove_button') {
+    // If it's a temporary file we can safely remove it immediately, otherwise
+    // it's up to the implementing module to clean up files that are in use.
+    if ($element['#file'] && $element['#file']->status == 0) {
+      file_delete($element['#file']);
+    }
+    // Update both $form_state['values'] and $form_state['input'] to reflect
+    // that the file has been removed, so that the form is rebuilt correctly.
+    // $form_state['values'] must be updated in case additional submit handlers
+    // run, and for form building functions that run during the rebuild, such as
+    // when the managed_file element is part of a field widget.
+    // $form_state['input'] must be updated so that file_managed_file_value()
+    // has correct information during the rebuild. The Form API provides no
+    // equivalent of form_set_value() for updating $form_state['input'], so
+    // inline that implementation with the same logic that form_set_value()
+    // uses.
+    $values_element = $element['#extended'] ? $element['fid'] : $element;
+    form_set_value($values_element, NULL, $form_state);
+    _form_set_value($form_state['input'], $values_element, $values_element['#parents'], NULL);
+  }
+
+  // Set the form to rebuild so that $form is correctly updated in response to
+  // processing the file removal. Since this function did not change $form_state
+  // if the upload button was clicked, a rebuild isn't necessary in that
+  // situation and setting $form_state['redirect'] to FALSE would suffice.
+  // However, we choose to always rebuild, to keep the form processing workflow
+  // consistent between the two buttons.
+  $form_state['rebuild'] = TRUE;
 }
 
 /**
@@ -626,6 +644,38 @@ function theme_file_managed_file($variables) {
 }
 
 /**
+ * #pre_render callback to hide display of the upload or remove controls.
+ *
+ * Upload controls are hidden when a file is already uploaded. Remove controls
+ * are hidden when there is no file attached. Controls are hidden here instead
+ * of in file_managed_file_process(), because #access for these buttons depends
+ * on the managed_file element's #value. See the documentation of form_builder()
+ * for more detailed information about the relationship between #process,
+ * #value, and #access.
+ *
+ * Because #access is set here, it affects display only and does not prevent
+ * JavaScript or other untrusted code from submitting the form as though access
+ * were enabled. The form processing functions for these elements should not
+ * assume that the buttons can't be "clicked" just because they are not
+ * displayed.
+ *
+ * @see file_managed_file_process()
+ * @see form_builder()
+ */
+function file_managed_file_pre_render($element) {
+  // If we already have a file, we don't want to show the upload controls.
+  if (!empty($element['#value']['fid'])) {
+    $element['upload']['#access'] = FALSE;
+    $element['upload_button']['#access'] = FALSE;
+  }
+  // If we don't already have a file, there is nothing to remove.
+  else {
+    $element['remove_button']['#access'] = FALSE;
+  }
+  return $element;
+}
+
+/**
  * Returns HTML for a link to a file.
  *
  * @param $variables
diff --git a/modules/file/tests/file.test b/modules/file/tests/file.test
index b228cb329..ee01ffa61 100644
--- a/modules/file/tests/file.test
+++ b/modules/file/tests/file.test
@@ -14,7 +14,7 @@ class FileFieldTestCase extends DrupalWebTestCase {
 
   function setUp() {
     parent::setUp('file');
-    $this->admin_user = $this->drupalCreateUser(array('access content', 'access administration pages', 'administer site configuration', 'administer users', 'administer content types', 'administer nodes', 'create article content', 'edit any article content', 'delete any article content'));
+    $this->admin_user = $this->drupalCreateUser(array('access content', 'access administration pages', 'administer site configuration', 'administer users', 'administer content types', 'administer nodes', 'bypass node access'));
     $this->drupalLogin($this->admin_user);
   }
 
@@ -202,6 +202,84 @@ class FileFieldTestCase extends DrupalWebTestCase {
   }
 }
 
+
+/**
+ * Test class to test file field upload and remove buttons, with and without AJAX.
+ */
+class FileFieldWidgetTestCase extends FileFieldTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'File field widget test',
+      'description' => 'Test upload and remove buttons, with and without AJAX.',
+      'group' => 'File',
+    );
+  }
+
+  /**
+   * Tests upload and remove buttons, with and without AJAX.
+   *
+   * @todo This function currently only tests the "remove" button of a single-
+   *   valued field. Tests should be added for the "upload" button and for each
+   *   button of a multi-valued field. Tests involving multiple AJAX steps on
+   *   the same page will become easier after http://drupal.org/node/789186
+   *   lands. Testing the "upload" button in AJAX context requires more
+   *   investigation into how jQuery uploads files, so that drupalPostAJAX() can
+   *   emulate that correctly.
+   */
+  function testWidget() {
+    // Use 'page' instead of 'article', so that the 'article' image field does
+    // not conflict with this test. If in the future the 'page' type gets its
+    // own default file or image field, this test can be made more robust by
+    // using a custom node type.
+    $type_name = 'page';
+    $field_name = strtolower($this->randomName());
+    $this->createFileField($field_name, $type_name);
+    $field = field_info_field($field_name);
+    $instance = field_info_instance('node', $field_name, $type_name);
+
+    $test_file = $this->getTestFile('text');
+
+    foreach (array('nojs', 'js') as $type) {
+      // Create a new node with the uploaded file and ensure it got uploaded
+      // successfully.
+      $nid = $this->uploadNodeFile($test_file, $field_name, $type_name);
+      $node = node_load($nid, NULL, TRUE);
+      $node_file = (object) $node->{$field_name}[LANGUAGE_NONE][0];
+      $this->assertFileExists($node_file, t('New file saved to disk on node creation.'));
+
+      // Ensure the edit page has a remove button instead of an upload button.
+      $this->drupalGet("node/$nid/edit");
+      $this->assertNoFieldByXPath('//input[@type="submit"]', t('Upload'), t('Node with file does not display the "Upload" button.'));
+      $this->assertFieldByXpath('//input[@type="submit"]', t('Remove'), t('Node with file displays the "Remove" button.'));
+
+      // "Click" the remove button (emulating either a nojs or js submission).
+      switch ($type) {
+        case 'nojs':
+          $this->drupalPost(NULL, array(), t('Remove'));
+          break;
+        case 'js':
+          // @todo This can be simplified after http://drupal.org/node/789186
+          //   lands.
+          preg_match('/jQuery\.extend\(Drupal\.settings, (.*?)\);/', $this->content, $matches);
+          $settings = drupal_json_decode($matches[1]);
+          $button = $this->xpath('//input[@type="submit" and @value="' . t('Remove') . '"]');
+          $button_id = (string) $button[0]['id'];
+          $this->drupalPostAJAX(NULL, array(), array((string) $button[0]['name'] => (string) $button[0]['value']), $settings['ajax'][$button_id]['url'], array(), array(), NULL, $settings['ajax'][$button_id]);
+          break;
+      }
+
+      // Ensure the page now has an upload button instead of a remove button.
+      $this->assertNoFieldByXPath('//input[@type="submit"]', t('Remove'), t('After clicking the "Remove" button, it is no longer displayed.'));
+      $this->assertFieldByXpath('//input[@type="submit"]', t('Upload'), t('After clicking the "Remove" button, the "Upload" button is displayed.'));
+
+      // Save the node and ensure it does not have the file.
+      $this->drupalPost(NULL, array(), t('Save'));
+      $node = node_load($nid, NULL, TRUE);
+      $this->assertTrue(empty($node->{$field_name}[LANGUAGE_NONE][0]['fid']), t('File was successfully removed from the node.'));
+    }
+  }
+}
+
 /**
  * Test class to test file handling with node revisions.
  */