Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security

Thanks for the work on this!

I still want to review it in more detail. In the meantime, this patch doesn't change what's already in #10, but adds new hunks: the removal of the @todo in form.inc (the security issue for why this issue is critical) and a test to go with it. If the file tests still pass, then we're making good progress and just need to do some cleanup.

Awesome, I'm really happy #11 passes! This one removes a bunch of hunks that aren't within the scope of this issue. @eojthebrave: I admire your attempt to clean up some of the mess in file_ajax_upload(), and I hope we're able to clean that function up more before D7 is released, but let's keep this issue focused to just what is needed for FAPI security. That will make it easier to review. This patch just leaves #10's critical refactoring of file.module's #process and #submit functions and #11's fix to form.inc and tests.

This should still pass tests and be a manageable patch to review.

Man, this issue is quite tricky. This patch cleans it up a little more, but even though it passes tests, it does not work. More tests need to be added.

Here's a remaining bug that needs to be added as a test: go to node/add/article, type in a title, browse for an image, click upload. You will get a message saying the article node was saved, even though you did not click "save" but "upload".

The problem is that in the 1st run through form_builder() (not the one triggered by drupal_rebuild_form()), when $form_state['triggering_element'] is not yet determined and no submit handlers have yet run, file_managed_file_value() runs before file_managed_file_process(). file_managed_file_value() has the code to save the uploaded file (something I expected to be movable to the #submit function). But it can't be moved to file_managed_file_submit(), because we want the user to be able to click "save" without clicking "upload" and still have the file saved. But that means by the time file_managed_file_process() is called the first time, there exists a fid, and the upload button is given #access=FALSE. And that prevents the upload button from being a candidate for $form_state['triggering_element'] even though it was clicked (see the @todo in form.inc that this patch tries to remove). So as far as FAPI is concerned, nothing matches $form_state['triggering_element'], which makes it default it to the "Save" button because that's the first one (this defaulting is needed to handle the fact that Internet Explorer sometimes doesn't submit button information in the POST).

Anyway, some help here from fago, chx, or another multistep form guru would be very nice. Our normal FAPI workflow works well when #value callbacks don't do #submit actions. But here we have a case where that's exactly what's happening. Do we try to find a way to make this form work despite that? Or do we find a way to move the file saving to a #submit, but make sure it runs for the "save" button too?

Tagging this since it touches on deep FAPI questions.

Another question related to this. Is the following behavior of HEAD a bug or a feature?

Go to node/add/article, leave title blank, browse for an image (but do not click upload), click "save". You get a validation error (missing title), but the image has been uploaded, so now the "upload" button has been changed to "remove".

Does this make sense? That even though you got a validation error on the form, your image has been uploaded and saved on the server, and in the redisplay of the form, the "upload" button changed to "remove"? If this is a feature, it points to additional reasons for the file saving to not be movable to the #submit phase.

Hm, if #16 is a feature, then it's very similar to the CAPTCHA example in #642702: Form validation handlers cannot alter $form structure. Perhaps we just need to move the file saving from file_managed_file_value() to file_managed_file_validate() or something like that...

This one is ready for review.

It's a little ugly (read the phpdoc for file_managed_file_after_build()), but I haven't thought of an alternative. It seems reasonable in the case of file.module that the value callback handles the upload. And FAPI is setup to run #process functions after #value is determined (and that is by design, so no changing that for D7), so we don't have a nice mechanism to get back the original fully built form that was initially presented to the user to use for #access checking. Normally, it's not a problem as most #process functions don't change structure/#access based on what's in #value, but for those that do, we have a problem. Seems like an unfortunate limitation of FAPI, but better to work around it in the way done in this patch than to not implement any #access security for button click determination. If anyone has a better idea, please post it.

The reasoning here reminds me of #pre_render.

Thanks! That makes much more sense. Updated patch and added more documentation explaining the situation.

Aside from that, I wanted to mention that #element_validate handlers are now also able to alter the form structure - if that helps in any way. (Form validation runs last.)

There's potentially a separate need unrelated to this issue to evaluate what is done in file_managed_file_(value|process|validate)(). For example, does it make sense that we save the file in _value() rather than in _validate()? And if we decide to change some of the workflow of managed_file around, then #element_validate being able to change $form is awesome! But with the workflow as it is, #pre_render is the correct place for toggling the display of the buttons.

Sometimes we miss a kind of "value extraction" step in the form API...

So currently, we have #value_callback followed by #process in pre-order traversal, and #after_build in post-order traversal. If I'm understanding correctly, you're suggesting we may want some kind of #preprocess followed by #value_extract in post-order traversal to happen before the existing stuff. Seems like good stuff to consider for D8, but are you suggesting pipeline changes for D7? Currently, #process functions all over the place (certainly in contrib) do conditional stuff on #value, so I don't think it would be wise to change that order for D7, but I do think it makes sense to re-architect the entire pipeline in D8.

Though if adding new callbacks like #preprocess to run before #value_callback would solve the problems with #builder_function, perhaps that should be considered even for D7, since adding new stuff wouldn't break BC.

[EDIT: Seems like material for a separate issue, though, as I don't think even that would help us solve this issue better than how #20 does it. The only thing I don't like about #20 is that it doesn't offer a way to correctly enforce the prior page request's #access of the upload/remove buttons. Not a big deal for this particular use-case, but potentially frustrating for other use-cases (note that in D6 and in D7 HEAD, #access isn't enforced AT ALL for button click detection, so #20 is a huge improvement, just leaves this unsolved problem in these kinds of dynamic #process workflows). But solving this remaining problem would require retaining the prior step's input or values in $form_state or retaining the prior step's fully built $form in addition to the $original_form, not just adjusting the sequence of callbacks.]

Oh, and by the way, @fago, thanks so much for reviewing this issue! It helps a lot to have someone with your insight of multistep forms helping out on this and the other FAPI WTF issues that you've been giving attention to!

Marked #737686: Image field 'Remove' button broken a duplicate, because #20 fixes the problem (I tested both with JavaScript disabled and enabled). But, would be great to add a test for it as part of this issue, since HEAD is broken in this regard but not failing any tests. I think we're missing quite a bit of test coverage for file.module. So, I'd like to use the occasion to shamelessly plug #384992: drupal_html_id does not work correctly in AJAX context with multiple forms on page, which in addition to solving what the title states, also improves our ability to add tests for multistep AJAX workflows.

With #28 and #29.

Is #access ever unset in _form_builder_handle_input_element()?

It's never set to begin with most of the time. Things only set it to FALSE. Not set needs to be interpreted as TRUE. I changed the if statement to be exactly the same as what is used above in the same function for deciding whether to process input for an input control. Same principle here: a button is a kind of input control.

.

+++ includes/form.inc	11 Apr 2010 02:07:53 -0000
@@ -1477,17 +1506,10 @@ function _form_builder_handle_input_elem
+  if ($form_state['programmed'] || ($form_state['process_input'] && (!isset($element['#access']) || $element['#access']))) {

#426056: Server-side enforcement of #disabled is inconsistent landed so this patch applies the #disabled enforcement for triggering_element determination as well. Since this if statement is getting quite long, this patch cleans it up to only exist once in the function.

I think this is ready to fly. Any chance of getting an RTBC or additional feedback before DrupalCon?

+++ modules/file/tests/file.test	14 Apr 2010 11:25:14 -0000
@@ -329,6 +331,10 @@ class FileFieldDisplayTestCase extends F
+    // Check file removal.
+    $this->removeNodeFile($nid, FALSE);
+    $this->assertFileNotExists($node_file, t('File is deleted.'));
+    $this->assertFileEntryNotExists($node_file, t('File entry is deleted.'));

Unfortunately, this test does not fail in HEAD. The "remove" button works in HEAD with JS disabled. It is the AJAX submission that's broken due to missing test coverage. We need to improve file.module testing to cover both AJAX and non-AJAX variants of the upload and remove buttons to avoid future regressions.

112 critical left. Go review some!

Re #40: The "remove" button *should* always work regardless of JS availability, but the way that's implemented involves two different workflows. Without JS availability, the form is submitted to its normal "action". With JS availability, it is submitted to 'file/ajax' which runs file_ajax_upload(). The latter is where the bug is, but the test coverage is only for the former, which is how the bug made it into HEAD without bot catching it, so we need to add test coverage for the latter, which we should do as part of this issue. There's also #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions for helping to keep #ajax['path'] callbacks correctly implemented, to reduce these kinds of bugs in the future by eliminating needless deviation between non-AJAX and AJAX FAPI processing.

Because both this issue and #756762: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions will take some time to move through the process, I posted a simple fix to the broken remove button to #737686: Image field 'Remove' button broken.

Yes, this is critical, as it's preventing an important security improvement, and also solves the problem of the image field "remove" button not working. What's holding it up is a desire to roll in some better AJAX test coverage (since the "remove" button is being fixed, we need a test for that), but I'll try to work on that and post a new patch this week.

I posted a patch that makes it easier to write AJAX tests in a separate issue: #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage. I still haven't gotten around to writing the necessary AJAX tests for file.module, but I think it makes sense for those to be included with this issue. Meanwhile, the other issue is ready for review.

Sorry to not have gotten this done yet: I've been giving attention to some other issues. I do plan on resuming this one soon. As an FYI, this issue also fixes #disabled buttons to not be recognized as clicked as per #803212-12: Protection against forgery of input selection value doesn't work with checkboxes.

Changing title and tagging to clarify what the point of this issue is and why it's critical. See issue description and early comments for details.

Re #48: not sure. Here's a re-roll in case that helps.

Setting to "needs review" for a bot-check. If bot comes back green, I will reset to "needs work".

#49: file_managed_file_process-736298-49.patch queued for re-testing.

back to "needs work" as per #49.

Sorry for the delay on this. Other issues have been eating my time. Anyway, we're hopefully close to beta 1, so time to get moving on this again. There are still a couple issues outstanding that could simplify this patch once they land: #735800: node form triggers form level submit functions on button level submits, without validation and #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage. This patch comments where there's WTFs due to those issues. But, although the WTF cleanup once those issues land will be nice, they don't have to hold up this patch, which is otherwise ready I think (though, perhaps the comments can still use some polish).

This adds tests for the "remove" button, both under non-JS and JS emulation, because the JS one fails with HEAD, and since this issue happens to fix that, #770880: Files and images are not deleted after node saved and #737686: Image field 'Remove' button broken were marked as duplicates of this issue, and therefore, this issue is responsible for adding the test to prevent regression. More test coverage of the file field widget (i.e., the "upload" button under JS, and various combinations of clicks with a multi-valued field) would be nice, but requires effort, and I think would be best handled as a separate issue, and after #789186: Improve drupalPostAJAX() to be used more easily, leading to better AJAX test coverage lands.

+++ modules/file/file.module	16 Jun 2010 19:32:46 -0000
@@ -565,13 +546,44 @@ function file_managed_file_validate(&$el
+  // 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);
+  }

Yay, the other issue landed, so this patch removes this little bit of nastiness.

Also, I improved the prior patch's PHPDoc of form_builder() and file_managed_file_pre_render(). I think this is ready to fly. Any chance of this making it in before alpha6/beta1?

56 critical left. Go review some!

Awesome! The change in #735800: node form triggers form level submit functions on button level submits, without validation that made entity forms follow normal FAPI persistence of $form_state['input'] exposed a bug in the patch's file_managed_file_submit() function. This is great, because it means the bug existed for non-entity forms using the managed_file element, and wasn't caught by bot, because we don't have test coverage for that. But now that entity forms follow the same rebuild rules as all other forms, we have the necessary test coverage. This patch fixes the bug by updating $form_state['input'] to reflect the file deletion. Note, most elements aren't so delicate with the need to update $form_state['values'] and $form_state['input'] for rebuilding to function correctly, so I added code comments explaining why this is needed for the managed_file element.

This merges in additional form_builder() documentation that is being dropped from #266246-79: Add smart defaults for system_settings_form. The documentation being added is not related to this issue, but because this issue also adds a lot of form_builder() documentation, I think it makes sense for all of it to be reviewed together. Plus because of the new documentation, I made some tweaks to the documentation that was in #57. It's quite a lot of explanation about form_builder(), but let's face it, form_builder() is a complex function that does a lot, with implications that aren't all that intuitive for most people.

Thanks!

This patch touches the "file.module" and "forms system" components, but re-assigning to "forms system", because it touches that in a more significant way, and because even the changes to file.module should be reviewed by FAPI experts.

Technically, this issue involves an "API change" in that, much like file.module is doing in HEAD, if any contrib module is setting #access=FALSE or #disabled=TRUE of a clicked button within a #process function, it will need to change that into either a proper multi-step form or move that code into a #pre_render function, as per the new form_builder() docs. I believe this is acceptable, because:

1) Using a #process function in the aforementioned way was always questionable, and only "worked" because FAPI didn't implement the security it should have.

2) This allows us to improve FAPI security, which justifies a minor BC break.

I'm not sure why the button elements aren't simply moved out of the renderable array into a separate one here?

#843818: What pattern should we use for hide()/render() within theme functions?