Index: includes/form.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/form.inc,v
retrieving revision 1.448
diff -u -p -r1.448 form.inc
--- includes/form.inc	4 Apr 2010 13:22:51 -0000	1.448
+++ includes/form.inc	5 Apr 2010 16:01:33 -0000
@@ -1203,10 +1203,39 @@ function form_error(&$element, $message 
 }
 
 /**
- * Walk through the structured form array, adding any required
- * properties to each element and mapping the incoming input
- * data to the proper elements. Also, execute any #process handlers
- * attached to a specific element.
+ * Walk through the structured form array, adding any required properties to
+ * each element and mapping the incoming input data to the proper elements.
+ * Also, execute any #process handlers attached to a specific element.
+ *
+ * For each element that can receive user input data, the current request's
+ * incoming input data is used to set the element's #value property. The
+ * element's #process handlers are executed after its #value has been set,
+ * enabling those functions to execute conditional logic based on the current
+ * value. However, during the #process handler stage, the #value properties have
+ * not yet been validated: validation occurs after the entire form has passed
+ * through form_builder() and before submit handlers run, so any code that
+ * depends on validated values must reside within a submit handler.
+ *
+ * #value is set and #process functions are executed in a pre-order traversal,
+ * so input data for a child element is mapped after the parent element's
+ * #process functions have been executed. This requires #process functions to
+ * use special care when using #value to affect which children to add or how to
+ * set their #access properties. For example, the Form API determines which
+ * element (usually a button, but in the case of AJAX, could be any input
+ * element) triggered the submission of the form and sets
+ * $form_state['triggering_element'] accordingly and uses that in deciding which
+ * validation and submission handlers to execute. Making this determination is
+ * part of the input mapping stage, since the user input data contains the
+ * needed information. The Form API implements access control security during
+ * input data mapping, so that all user input for an element is ignored unless
+ * the element exists and does not have its #access set to FALSE. This means
+ * that regardless of what is in the user input data,
+ * $form_state['triggering_element'] will not be set to a button whose #access
+ * is FALSE during the input mapping stage. So, a #process function needs to be
+ * careful to not remove access to the button that was clicked. Functions later
+ * in the pipeline (#after_build callbacks, validation handlers, submission
+ * handlers, and #pre_render callbacks) can be used to remove buttons based on
+ * new user input.
  *
  * @param $form_id
  *   A unique string identifying the form for validation, submission,
@@ -1477,17 +1506,10 @@ function _form_builder_handle_input_elem
 
   // Determine which element (if any) triggered the submission of the form and
   // keep track of all the buttons in the form for form_state_values_clean().
-  // @todo We need to add a #access check here, so that someone can't fake the
-  //   click of a button they shouldn't have access to, but first we need to
-  //   fix file.module's managed_file element pipeline to handle the click of
-  //   the remove button in a submit handler instead of in a #process function.
-  //   During the first run of form_builder() after the form is submitted,
-  //   #process functions need to return the expanded element with child
-  //   elements' #access properties matching what they were when the form was
-  //   displayed to the user, since that is what we are processing input for.
-  //   Changes to the form (like toggling the upload/remove button) need to wait
-  //   until form rebuild: http://drupal.org/node/736298.
-  if (!empty($form_state['input'])) {
+  // Enforce access control so that a hacked form submission doesn't result in
+  // $form_state['triggering_element'] set to a restricted element, which would
+  // potentially result in insecure submit handler execution.
+  if (!empty($form_state['input']) && ($form_state['programmed'] || !isset($element['#access']) || $element['#access'])) {
     // Detect if the element triggered the submission via AJAX.
     if (_form_element_triggered_scripted_submission($element, $form_state)) {
       $form_state['triggering_element'] = $element;
@@ -1500,11 +1522,7 @@ function _form_builder_handle_input_elem
       // All buttons in the form need to be tracked for
       // form_state_values_clean() and for the form_builder() code that handles
       // a form submission containing no button information in $_POST.
-      // @todo When #access is checked in an outer if statement (see above), it
-      //   won't need to be checked here.
-      if ($form_state['programmed'] || !isset($element['#access']) || $element['#access']) {
-        $form_state['buttons'][] = $element;
-      }
+      $form_state['buttons'][] = $element;
       if (_form_button_was_clicked($element, $form_state)) {
         $form_state['triggering_element'] = $element;
       }
Index: modules/file/file.module
===================================================================
RCS file: /cvs/drupal/drupal/modules/file/file.module,v
retrieving revision 1.24
diff -u -p -r1.24 file.module
--- modules/file/file.module	31 Mar 2010 19:34:56 -0000	1.24
+++ modules/file/file.module	5 Apr 2010 16:01:34 -0000
@@ -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',
@@ -385,25 +386,6 @@ function file_managed_file_process($elem
     '#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,
@@ -437,7 +419,6 @@ function file_managed_file_process($elem
     '#name' => 'files[' . implode('_', $element['#parents']) . ']',
     '#type' => 'file',
     '#size' => 22,
-    '#access' => empty($fid),
     '#theme_wrappers' => array(),
     '#weight' => -10,
   );
@@ -566,13 +547,44 @@ function file_managed_file_validate(&$el
 }
 
 /**
- * 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;
+  // This is unfortunately necessary for the form to retain the input of other
+  // fields in the entity when submitting with JavaScript disabled.
+  // @see http://drupal.org/node/367006
+  // @see http://drupal.org/node/735800
+  if (isset($form['#builder_function']) && function_exists($form['#builder_function'])) {
+    $form['#builder_function']($form, $form_state);
+  }
+
+  // The triggering element is either the upload or remove button. Get the
+  // 'managed_file' element it controls: its parent in $form.
+  $parents = $form_state['triggering_element']['#array_parents'];
+  $button_key = array_pop($parents);
+  $element = $form;
+  foreach ($parents as $parent) {
+    $element = $element[$parent];
+  }
+
+  // Perform the button-specific logic.
+  switch ($button_key) {
+    case 'upload_button':
+      // No action needed, because file_managed_file_value() handles the upload
+      // which it must so that the form works as expected if the user clicks
+      // "Save" without ever clicking "Upload".
+      break;
+    case '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']);
+      }
+      form_set_value($element['#extended'] ? $element['fid'] : $element, NULL, $form_state);
+      break;
+  }
+
+  $form_state['rebuild'] = TRUE;
 }
 
 /**
@@ -607,6 +619,30 @@ function file_managed_file_save_upload($
 }
 
 /**
+ * #pre_render callback to hide display of the upload or remove controls depending on which is needed.
+ *
+ * See form_builder() for why this is done here instead of in
+ * file_managed_file_process(). This means that the use of #access here is for
+ * affecting display only and does not prevent JavaScript or other untrusted
+ * code from submitting the form as though an inappropriate button was clicked.
+ *
+ * @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;
+}
+
+/**
  * Theme a managed file element.
  */
 function theme_file_managed_file($variables) {
Index: modules/simpletest/tests/form.test
===================================================================
RCS file: /cvs/drupal/drupal/modules/simpletest/tests/form.test,v
retrieving revision 1.44
diff -u -p -r1.44 form.test
--- modules/simpletest/tests/form.test	31 Mar 2010 19:34:56 -0000	1.44
+++ modules/simpletest/tests/form.test	5 Apr 2010 16:01:36 -0000
@@ -908,14 +908,14 @@ class FormsProgrammaticTestCase extends 
 }
 
 /**
- * Test that FAPI correctly determines $form_state['clicked_button'].
+ * Test that FAPI correctly determines $form_state['triggering_element'].
  */
-class FormsClickedButtonTestCase extends DrupalWebTestCase {
+class FormsTriggeringElementTestCase extends DrupalWebTestCase {
 
   function getInfo() {
     return array(
-      'name' => 'Form clicked button determination',
-      'description' => 'Test the determination of $form_state[\'clicked_button\'].',
+      'name' => 'Form triggering element determination',
+      'description' => 'Test the determination of $form_state[\'triggering_element\'].',
       'group' => 'Form API',
     );
   }
@@ -925,59 +925,85 @@ class FormsClickedButtonTestCase extends
   }
 
   /**
-   * Test the determination of $form_state['clicked_button'] when no button
+   * Test the determination of $form_state['triggering_element'] when no button
    * information is included in the POST data, as is sometimes the case when
    * the ENTER key is pressed in a textfield in Internet Explorer.
    */
   function testNoButtonInfoInPost() {
     $path = 'form-test/clicked-button';
     $edit = array();
-    $form_id = 'form-test-clicked-button';
+    $form_html_id = 'form-test-clicked-button';
 
     // Ensure submitting a form with no buttons results in no
-    // $form_state['clicked_button'] and the form submit handler not running.
-    drupal_static_reset('drupal_html_id');
-    $this->drupalPost($path, $edit, NULL, array(), array(), $form_id);
-    $this->assertText('There is no clicked button.', t('$form_state[\'clicked_button\'] set to NULL.'));
+    // $form_state['triggering_element'] and the form submit handler not
+    // running.
+    $this->drupalPost($path, $edit, NULL, array(), array(), $form_html_id);
+    $this->assertText('There is no clicked button.', t('$form_state[\'triggering_element\'] set to NULL.'));
     $this->assertNoText('Submit handler for form_test_clicked_button executed.', t('Form submit handler did not execute.'));
 
     // Ensure submitting a form with one or more submit buttons results in
-    // $form_state['clicked_button'] being set to the first one the user has
+    // $form_state['triggering_element'] being set to the first one the user has
     // access to. An argument with 'r' in it indicates a restricted
     // (#access=FALSE) button.
-    drupal_static_reset('drupal_html_id');
-    $this->drupalPost($path . '/s', $edit, NULL, array(), array(), $form_id);
-    $this->assertText('The clicked button is button1.', t('$form_state[\'clicked_button\'] set to only button.'));
+    $this->drupalPost($path . '/s', $edit, NULL, array(), array(), $form_html_id);
+    $this->assertText('The clicked button is button1.', t('$form_state[\'triggering_element\'] set to only button.'));
     $this->assertText('Submit handler for form_test_clicked_button executed.', t('Form submit handler executed.'));
-    drupal_static_reset('drupal_html_id');
-    $this->drupalPost($path . '/s/s', $edit, NULL, array(), array(), $form_id);
-    $this->assertText('The clicked button is button1.', t('$form_state[\'clicked_button\'] set to first button.'));
+
+    $this->drupalPost($path . '/s/s', $edit, NULL, array(), array(), $form_html_id);
+    $this->assertText('The clicked button is button1.', t('$form_state[\'triggering_element\'] set to first button.'));
     $this->assertText('Submit handler for form_test_clicked_button executed.', t('Form submit handler executed.'));
-    drupal_static_reset('drupal_html_id');
-    $this->drupalPost($path . '/rs/s', $edit, NULL, array(), array(), $form_id);
-    $this->assertText('The clicked button is button2.', t('$form_state[\'clicked_button\'] set to first available button.'));
+
+    $this->drupalPost($path . '/rs/s', $edit, NULL, array(), array(), $form_html_id);
+    $this->assertText('The clicked button is button2.', t('$form_state[\'triggering_element\'] set to first available button.'));
     $this->assertText('Submit handler for form_test_clicked_button executed.', t('Form submit handler executed.'));
 
     // Ensure submitting a form with buttons of different types results in
-    // $form_state['clicked_button'] being set to the first button, regardless
-    // of type. For the FAPI 'button' type, this should result in the submit
-    // handler not executing. The types are 's'(ubmit), 'b'(utton), and
+    // $form_state['triggering_element'] being set to the first button,
+    // regardless of type. For the FAPI 'button' type, this should result in the
+    // submit handler not executing. The types are 's'(ubmit), 'b'(utton), and
     // 'i'(mage_button).
-    drupal_static_reset('drupal_html_id');
-    $this->drupalPost($path . '/s/b/i', $edit, NULL, array(), array(), $form_id);
-    $this->assertText('The clicked button is button1.', t('$form_state[\'clicked_button\'] set to first button.'));
+    $this->drupalPost($path . '/s/b/i', $edit, NULL, array(), array(), $form_html_id);
+    $this->assertText('The clicked button is button1.', t('$form_state[\'triggering_element\'] set to first button.'));
     $this->assertText('Submit handler for form_test_clicked_button executed.', t('Form submit handler executed.'));
-    drupal_static_reset('drupal_html_id');
-    $this->drupalPost($path . '/b/s/i', $edit, NULL, array(), array(), $form_id);
-    $this->assertText('The clicked button is button1.', t('$form_state[\'clicked_button\'] set to first button.'));
+
+    $this->drupalPost($path . '/b/s/i', $edit, NULL, array(), array(), $form_html_id);
+    $this->assertText('The clicked button is button1.', t('$form_state[\'triggering_element\'] set to first button.'));
     $this->assertNoText('Submit handler for form_test_clicked_button executed.', t('Form submit handler did not execute.'));
-    drupal_static_reset('drupal_html_id');
-    $this->drupalPost($path . '/i/s/b', $edit, NULL, array(), array(), $form_id);
-    $this->assertText('The clicked button is button1.', t('$form_state[\'clicked_button\'] set to first button.'));
+
+    $this->drupalPost($path . '/i/s/b', $edit, NULL, array(), array(), $form_html_id);
+    $this->assertText('The clicked button is button1.', t('$form_state[\'triggering_element\'] set to first button.'));
     $this->assertText('Submit handler for form_test_clicked_button executed.', t('Form submit handler executed.'));
   }
-}
 
+  /**
+   * Test that $form_state['triggering_element'] does not get set to a button
+   * with #access=FALSE.
+   */
+  function testAttemptAccessControlBypass() {
+    $path = 'form-test/clicked-button';
+    $form_html_id = 'form-test-clicked-button';
+
+    // Retrieve a form where 'button1' has #access=FALSE and 'button2' doesn't.
+    $this->drupalGet($path . '/rs/s');
+
+    // Submit the form with 'button1=button1' in the POST data, which someone
+    // trying to get around security safeguards could easily do. We have to do
+    // a little trickery here, to work around the safeguards in drupalPost(): by
+    // renaming the text field that is in the form to 'button1', we can get the
+    // data we want into $_POST.
+    $elements = $this->xpath('//form[@id="' . $form_html_id . '"]//input[@name="text"]');
+    $elements[0]['name'] = 'button1';
+    $this->drupalPost(NULL, array('button1' => 'button1'), NULL, array(), array(), $form_html_id);
+
+    // Ensure that $form_state['triggering_element'] was not set to the
+    // restricted button. Do this with both a negative and positive assertion,
+    // because negative assertions alone can be brittle. See
+    // testNoButtonInfoInPost() for why the triggering element gets set to
+    // 'button2'.
+    $this->assertNoText('The clicked button is button1.', t('$form_state[\'triggering_element\'] not set to a restricted button.'));
+    $this->assertText('The clicked button is button2.', t('$form_state[\'triggering_element\'] not set to a restricted button.'));
+  }
+}
 
 /**
  * Tests rebuilding of arbitrary forms by altering them.
Index: modules/simpletest/tests/form_test.module
===================================================================
RCS file: /cvs/drupal/drupal/modules/simpletest/tests/form_test.module,v
retrieving revision 1.33
diff -u -p -r1.33 form_test.module
--- modules/simpletest/tests/form_test.module	26 Mar 2010 18:58:12 -0000	1.33
+++ modules/simpletest/tests/form_test.module	5 Apr 2010 16:01:36 -0000
@@ -994,8 +994,8 @@ function form_test_clicked_button($form,
  * Form validation handler for the form_test_clicked_button() form.
  */
 function form_test_clicked_button_validate($form, &$form_state) {
-  if (isset($form_state['clicked_button'])) {
-    drupal_set_message(t('The clicked button is %name.', array('%name' => $form_state['clicked_button']['#name'])));
+  if (isset($form_state['triggering_element'])) {
+    drupal_set_message(t('The clicked button is %name.', array('%name' => $form_state['triggering_element']['#name'])));
   }
   else {
     drupal_set_message('There is no clicked button.');
@@ -1009,7 +1009,6 @@ function form_test_clicked_button_submit
   drupal_set_message('Submit handler for form_test_clicked_button executed.');
 }
 
-
 /**
  * Implements hook_form_FORM_ID_alter() for the registration form.
  */