Index: includes/form.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/form.inc,v
retrieving revision 1.451
diff -u -p -r1.451 form.inc
--- includes/form.inc	11 Apr 2010 19:00:27 -0000	1.451
+++ includes/form.inc	11 Apr 2010 21:11:20 -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,
@@ -1431,19 +1460,20 @@ function _form_builder_handle_input_elem
     }
   }
 
+  // With JavaScript or other easy hacking, input can be submitted even for
+  // elements with #access=FALSE or #disabled=TRUE. For security, these must
+  // not be processed. Forms that set #disabled=TRUE on an element do not
+  // expect input for the element, and even forms submitted with
+  // drupal_form_submit() must not be able to get around this. Forms that set
+  // #access=FALSE on an element usually allow access for some users, so forms
+  // submitted with drupal_form_submit() may bypass access restriction and be
+  // treated as high-privelege users instead.
+  $process_input = empty($element['#disabled']) && ($form_state['programmed'] || ($form_state['process_input'] && (!isset($element['#access']) || $element['#access'])));
+
   // Set the element's #value property.
   if (!isset($element['#value']) && !array_key_exists('#value', $element)) {
     $value_callback = !empty($element['#value_callback']) ? $element['#value_callback'] : 'form_type_' . $element['#type'] . '_value';
-
-    // With JavaScript or other easy hacking, input can be submitted even for
-    // elements with #access=FALSE or #disabled=TRUE. For security, these must
-    // not be processed. Forms that set #disabled=TRUE on an element do not
-    // expect input for the element, and even forms submitted with
-    // drupal_form_submit() must not be able to get around this. Forms that set
-    // #access=FALSE on an element usually allow access for some users, so forms
-    // submitted with drupal_form_submit() may bypass access restriction and be
-    // treated as high-privelege users instead.
-    if (empty($element['#disabled']) && ($form_state['programmed'] || ($form_state['process_input'] && (!isset($element['#access']) || $element['#access'])))) {
+    if ($process_input) {
       // Get the input for the current element. NULL values in the input need to
       // be explicitly distinguished from missing input. (see below)
       $input = $form_state['input'];
@@ -1506,18 +1536,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'])) {
+  // keep track of all the clickable buttons in the form for
+  // form_state_values_clean(). Enforce the same input processing restrictions
+  // as above.
+  if ($process_input) {
     // Detect if the element triggered the submission via AJAX.
     if (_form_element_triggered_scripted_submission($element, $form_state)) {
       $form_state['triggering_element'] = $element;
@@ -1530,11 +1552,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	11 Apr 2010 21:11:22 -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;
+  // 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']);
+    }
+    form_set_value($element['#extended'] ? $element['fid'] : $element, NULL, $form_state);
+  }
+
+  // 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.
+  // When setting an entity editing form to rebuild, the form's
+  // #builder_function must be executed, or else other fields in the entity form
+  // won't retain user input correctly: http://drupal.org/node/735800.
+  if (isset($form['#builder_function']) && function_exists($form['#builder_function'])) {
+    $form['#builder_function']($form, $form_state);
+  }
+  $form_state['rebuild'] = TRUE;
 }
 
 /**
@@ -607,6 +619,37 @@ function file_managed_file_save_upload($
 }
 
 /**
+ * #pre_render callback to hide display of the upload or remove controls depending on which is needed.
+ *
+ * This is done 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
+ * problems with #process functions setting #access conditional on #value.
+ *
+ * Because #access is set here, after form_builder() has already been executed
+ * for the elements affected, 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;
+}
+
+/**
  * 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.48
diff -u -p -r1.48 form.test
--- modules/simpletest/tests/form.test	11 Apr 2010 19:00:27 -0000	1.48
+++ modules/simpletest/tests/form.test	11 Apr 2010 21:11:24 -0000
@@ -935,14 +935,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 {
 
   public static 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',
     );
   }
@@ -952,59 +952,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.37
diff -u -p -r1.37 form_test.module
--- modules/simpletest/tests/form_test.module	11 Apr 2010 19:00:27 -0000	1.37
+++ modules/simpletest/tests/form_test.module	11 Apr 2010 21:11:24 -0000
@@ -1024,8 +1024,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.');
@@ -1039,7 +1039,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.
  */