Drupal 7.43 / 8.0.4 regression: When an anonymous user submits a form with an un-uploaded file that leads to a validation error, the file is lost on the next correct submission

I'm working on a test for this to confirm the bug in D7.

This just confirms that a basic node form works when tested by an anonymous visitor.

This adds another test method to confirm that a single file upload works.

This adds a test that shows the file is lost after the validation is triggered.

And this version runs a copy of testAnonNodeWithFileWithoutTitle() after first logging in as an admin user, just to confirm what's happening.

Bumping this to critical as it involves data loss, and to match #2675170.

Not sure where to go from here, besides diving deeper into the form and file handling, which I don't have time for today.

It loses the file because the default value is forced in this snippet:

     // Temporary files that belong to other users should never be allowed.
        // Since file ownership can't be determined for anonymous users, they
        // are not allowed to reuse temporary files at all.
        elseif ($file->status != FILE_STATUS_PERMANENT && (!$GLOBALS['user']->uid || $file->uid != $GLOBALS['user']->uid)) {

But at this point the default value is 0...

So maybe we can somehow set $element['#default_value']['fid'] on the managed file element before the $value_callback file_managed_file_value() is triggered?

Solution in #13 looks OK to me!

I just checked and Drupal 8 HEAD has the same issue, but a bit more of an edge case there since files are auto uploaded and HTML5 validation will catch the most common validation errors already.

Patch tested, confirmed fixes problems with Webform and File Resup modules.

Should the fid be passed as a 'value' instead of a 'hidden' field?

David_Rothstein: I tested Webform with your patch and it appears to solve the problem. I'd love to hear if others still have problems with it. I also wonder if we should move the test to file.test instead of node.test, given the problem is in the file handling?

This moves the tests to the File module.

Patched with #13 and it's working correctly. Files are attached to the webform result after anonymous user corrects validation (email field) and re-submits.
Our enviro D7.43 and Webform 7.x-4.10

Patch #22 - passed the included tests using the core Testing module. This was my first time using a test, so does this mean it's all good?

The only error I got before the results screen was:

An AJAX HTTP request terminated abnormally. Debugging information follows. Path: /mysite/batch?id=5715&op=do StatusText: ResponseText: ReadyState: 4

But everything was green when I clicked 'Continue to Results'

@amaisano: I don't know about that, sorry, I use the terminal for running tests.

David_Rothstein #29

Are you sure this fixed the File Resup issue also?

Sorry, yes should have said with the patch. The patch for File Resup only partially fixes the problem, server side validation errors still cause data loss. This patch fixes that issue with File Resup.

The problem was twofold with File Resup.

Is this something that will be included in the next core update? I'm not comfortable patching core so might be able to push off a week or 2.

We've had several clients who reported this bug to us and we've patched our 50+ Drupal installations with this patch. I'd say this needs to be included in the next Drupal core version.

I can reproduce the error with D7.43 and Webform 7.x-4.12 without any validation error.

Patch in #22 fails with git with

error: while searching for:
      '#markup' => theme('file_link', array('file' => $element['#file'])) . ' ',
      '#weight' => -10,
    );
  }

  // Add the extension list to the page as JavaScript settings.

Applied using patch -p1 and it fixed the issue.

We are having issues with webform submissions that include file uploads. We have applied the patch in comment #22, and this successfully mitigates the validation issue with file uploads.

We also have to use the patch -p1 option when applying the patch.

patch -p1 is the way how we should be applying patches.

We have apply the same patch from #22 and it seem to be working fine. We have test it out using - Webform 7.x-4.5. I did not see anything wrong with that patch for this reason I'm moving it to RCTB.

I apologize in advance if I'm not posting this question in the right place. I'm a site-builder who's not very experienced at back-end. This patch did not work for me (i.e., after updating to Drupal 7.43 and applying the patch using -p1, my file upload fields in webforms don't upload files). Could this be because I'm using a different version of PHP ? (PHP 5.6 on my local server and PHP 5.4 on production). How should I proceed? Is it safe to revert to an earlier version of PHP? Thanks in advance for your guidance and help.

@Debra G: Are you using Webform v7.x-3.x or v7.x-4.x? There are other problems with v7.x-3.x, as identified in #2675170, but 7.x-4.x should be working correctly.

@Damien: Thanks for responding. I'm using Webform 7.x-4.12, along with Commerce Webform 7.x-2.0-beta2, and Webform Multifile : 7.x-1.3. Maybe those additional modules are complicating things. I see that there's a development release of Commerce Webform that is dated a few day's later than the version that I'm using. I'll update that, try the patch again tonight, and let you know what happens. Thanks again.

@Debra G: You might also check the Webform Multifile issue queue to see if there's anything related, e.g. there might be some JS errors also showing which could be complicating things further.

Sorry, but I am confused. Is this issue gonna be patched in next Drupal 7 Core stable release or in next Webform module stable release?
Currently I am using a patch from the other issue (Webform) that seems to work but I am afraid that, in fact, the issue will be solved in the next Drupal 7 Core stable release.

We are using Webform 7.x-4.10. After encountering this problem in a webform with multiple file uploads we successfully applied and tested the patch from #2678822-22: Drupal 7.43 / 8.0.4 regression: When an anonymous user submits a form with an un-uploaded file that leads to a validation error, the file is lost on the next correct submission (drupal-n2678822-22.patch).

Is this going to be fixed on the Drupal side or on the Webform side?

Awesome! Thanks @David_Rothstein. I'll try to get time to work on the D8 bug next week.

D8 patch

@David_Rothstein Thanks for the commit! Does this wait for a May release despite having just made it into the "first Wednesday of every month" window (assuming I understand https://www.drupal.org/core/release-cycle-overview correctly)? Or is the fact that it was Critical (data loss) enough to invoke the "release an unscheduled minor release" clause? If not, I'll go ahead and patch knowing that I won't have to re-patch 7.44.

I applied patch from #22 and it worked perfectly. Thanks DamienMcKenna.

+++ b/core/modules/file/src/Tests/FileFieldAnonymousSubmissionTest.php
@@ -0,0 +1,174 @@
+
+/**
+ * @file
+ * Contains \Drupal\file\Tests\FileFieldAnonymousSubmissionTest.
+ */
+

Removed this.

Looks fine to me otherwise. Tested with my application specific behat tests where I had this bug as well on a public node form, they pass now.

I think I can RTBC this, since I just removed that @file.

I applied patch from #22 and it worked perfectly. Thanks DamienMcKenna!

Committed 6ae4278 and pushed to 8.1.x and 8.2.x. Thanks!

diff --git a/core/modules/file/src/Element/ManagedFile.php b/core/modules/file/src/Element/ManagedFile.php
index bcd33b4..1b0984b 100644
--- a/core/modules/file/src/Element/ManagedFile.php
+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -326,9 +326,9 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
         }
         // Anonymous users who have uploaded a temporary file need a
         // non-session-based token added so $this->valueCallback() can check
-        // that they have permission to use this file on subsequent submissions of
-        // the same form (for example, after an Ajax upload or form validation
-        // error).
+        // that they have permission to use this file on subsequent submissions
+        // of the same form (for example, after an Ajax upload or form
+        // validation error).
         if ($file->isTemporary() && \Drupal::currentUser()->isAnonymous()) {
           $element['file_' . $delta]['fid_token'] = array(
             '#type' => 'hidden',
diff --git a/core/modules/file/src/Tests/FileFieldAnonymousSubmissionTest.php b/core/modules/file/src/Tests/FileFieldAnonymousSubmissionTest.php
index 84683ec..2286473 100644
--- a/core/modules/file/src/Tests/FileFieldAnonymousSubmissionTest.php
+++ b/core/modules/file/src/Tests/FileFieldAnonymousSubmissionTest.php
@@ -95,7 +95,7 @@ public function testAnonymousNodeWithFile() {
   /**
    * Tests file submission for an anonymous visitor with a missing node title.
    */
-  function testAnonymousNodeWithFileWithoutTitle() {
+  public function testAnonymousNodeWithFileWithoutTitle() {
     $this->drupalLogout();
     $this->doTestNodeWithFileWithoutTitle();
   }
@@ -103,7 +103,7 @@ function testAnonymousNodeWithFileWithoutTitle() {
   /**
    * Tests file submission for an authenticated user with a missing node title.
    */
-  function testAuthenticatedNodeWithFileWithoutTitle() {
+  public function testAuthenticatedNodeWithFileWithoutTitle() {
     $admin_user = $this->drupalCreateUser(array(
       'bypass node access',
       'access content overview',

Minor nits fixed on commit.

#22 Works really well for me !!!! :)

Patch in #22 worked for me in Drupal 7. Before the fix we had users facing errors when attempting to upload a file via Webform.

Any chance this patch can be backported to 7.x?

i think that it has been: project/drupal - 7.x line so should be out with next 7.44 update.

Awesome, thanks!

I think patch #22 didn't make it into 7.44.

7.44 is 7.43 plus exactly 1 commit with the security update.

@criscom The 7.44 released this week is a security release only. Any features or bugfixes since 7.43 would be contained in a future release.

Just to be clear, the fix is not in 7.44 because that only contained security fixes, no other changes were included. This is currently slated for the forthcoming 7.50 release, per David_Rothstein's comment in #62 above.

Sorry I didn't see that part of the thread, my bad. I got burnt but fixed now.

+++ b/core/modules/file/src/Element/ManagedFile.php
@@ -91,19 +93,32 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
+                  $token = NestedArray::getValue($form_state->getUserInput(), array_merge($element['#parents'], array('file_' . $file->id(), 'fid_token')));
+                  if ($token !== Crypt::hmacBase64('file-' . $file->id(), \Drupal::service('private_key')->get() . Settings::getHashSalt())) {

@@ -309,6 +324,17 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
+          $element['file_' . $delta]['fid_token'] = array(
+            '#type' => 'hidden',
+            '#value' => Crypt::hmacBase64('file-' . $delta, \Drupal::service('private_key')->get() . Settings::getHashSalt()),

The check uses a fid, while the token uses a delta.

That can't work! (unless I am missing something really obvious)

It only works by accident in the test, because likely fid == 0 and delta == 0 for the first upload.

Re-opening - we looked at this for 7.x backport.

I was wrong, this is a change in D8 and just the naming is 'strange':

    $element['#files'] = !empty($fids) ? File::loadMultiple($fids) : FALSE;