Multiple file upload does not validate extensions correctly

1. Reproducing. I have two files created via touch: dummy.txt and dummy.ftw.
2. Adding a file field to the article content type, allowing unlimited uploads and.. hmm. The only extension listed is txt. The default list mentioned in the issue summary isn't there. Oh well, continuing the test as best I can. Adding "ftw" to the list of allowed extensions.
3. Adding an article and attempting to add dummy.txt and dummy.ftw, in that order: succeeded.

Unable to reproduce. Please provide step-by-step instructions for reproducing this issue.

I have tested and confirmed the patch from #7.

I am using the contrib module allow_all_file_extensions to enable the UI to specify a file field that allows files with any extension to be uploaded. There is no official D8 release yet, but a tarball containing working D8 code is available from https://www.drupal.org/project/allow_all_file_extensions/issues/2974935

My suggestion to improve the patch would be to put ALL of the processing of the $validators['file_validate_extensions'] value above the "foreach file" loop rather than just the portion that checks whether the list is present but empty. The logic of the block can then be left unchanged from the original.

Cheers

Test and fix looks good. Already tester this in the other issue as well.

One nitpick

+  // If 'file_validate_extensions' is set and the list is empty then the caller
+  // wants to allow any extension. In this case we have to remove the validator
+  // or else it will reject all extensions.
+  $is_not_validate_extensions = isset($validators['file_validate_extensions']) && !isset($validators['file_validate_extensions'][0]);
+
   foreach ($uploaded_files as $i => $file_info

Why not test for empty($validators['file_validate_extensions']) instead of first element.

Does what the comment says then, so a bit more readable imo.

I think we should refactor to prevent these type of errors. The problem is the loop. Let's refactor everything in the loop into a function. This means this type of issue is far less likely to occur.

The interdiff attached is not easy to read.

+++ b/core/modules/system/tests/themes/test_theme_settings/config/schema/test_theme_settings.schema.yml
@@ -10,3 +10,9 @@ test_theme_settings.settings:
         label: 'fids'
+    multi_file:
+      type: sequence
+      label: 'Multiple file field with all file extensions'
+      sequence:
+        type: integer
+        label: 'fids'

+++ b/core/modules/system/tests/themes/test_theme_settings/theme-settings.php
@@ -24,6 +24,17 @@ function test_theme_settings_form_system_theme_settings_alter(&$form, FormStateI
+  $form['multi_file'] = [
+    '#type' => 'managed_file',
+    '#title' => t('Multiple file field with all file extensions'),
+    '#multiple' => TRUE,
+    '#default_value' => theme_get_setting('multi_file'),
+    '#upload_location' => 'public://test',
+    '#upload_validators'  => [
+      'file_validate_extensions' => [],
+    ],
+  ];

I'm not sure that this is the right place for this test code. Can we not do something only in the file module?

Here's a test that is only part of the file module and its existing test infra.

@alexpott. I like the refactoring you have done, I converted your patch to allow it to apply to 8.5.4 and have tested it. I can confirm that it works correctly with my environment (D8.5.4 and contrib module "allow_all_file_extensions"). Converted patch attached. leaving on "Needs review" as I have not tested your patch in an 8.6 environment.

Patch for D8.5.4 rerolled and marked as do-not-test.

@jlscott thanks for backporting to 8.5.x - the patch in #17 doesn't have the new test. I'm uploading the 8.6.x patch as we have to get that done first and it helps the committers and reviewers when the most recent patch is the one to review / commit.

@jlscott perhaps you can review the patch with a view to rtbc?

Crediting @mallezie for work on the duplicate issue. Crediting @kkonuhov for creating the issue.

I can confirm that the patch in #18 works correctly on my system (D8.6.0-dev plus allow_all_file_extensions).

Just took a look at the new patch. I really like the separate function, makes things more readable. Only found some very minor nitpicks on the comments.

+++ b/core/modules/file/file.module
@@ -887,182 +886,208 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
+ *   (optional) A string containing the URI that the file should be copied to.

the Uri where the file should be copied to.

+ *   The created file entity or FALSE if the uploaded file not saved.

If the uploaded file was not saved.

+  // See http://php.net/manual/features.file-upload.errors.php.

See lowercase

+  // If file_destination() returns FALSE then $replace === FILE_EXISTS_ERROR and
+  // there's an existing file so we need to bail.

Not sure this comment makes much sense.

@mallezie all of this documentation is copied from the existing documentation is copied apart from The created file entity or FALSE if the uploaded file not saved. and I think you could have was, is or even nothing here are it is all still correct :)

Then just an extra RTBC from my side. Did not want to change status on those nitpicks, so thanks a lot for explaining them.

The failed test run had nothing to do with this issue.

Putting back to RTBC here. The test fail was the selenium which hang on the testbot. See: https://dispatcher.drupalci.org/job/drupal_patches/74739/console

1. +++ b/core/modules/file/file.module
   @@ -887,182 +886,208 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
   + * @param \SplFileInfo $file_info
   

   nit: this needs a param comment

2. +++ b/core/modules/file/file.module
   @@ -887,182 +886,208 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
   +function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $validators = [], $destination = FALSE, $replace = FILE_EXISTS_RENAME) {
   

   are we certain we want to add a new procedural function?

3. +++ b/core/modules/file/tests/src/Functional/MultipleFileUploadTest.php
   @@ -0,0 +1,59 @@
   + * @group system
   

   should this be file?

Thanks for the review @larowlan
1. Fixed
2. Yes because it makes these type of errors impossible to have in this function. It's much easier to understand the responsibilities when _file_save_upload_single() is in a different scope than file_save_upload()
3. Fixed.

+++ b/core/modules/file/file.module
@@ -887,182 +886,208 @@ function file_save_upload() ...

+  $file->setFileUri($file->destination);
  +  if (!drupal_move_uploaded_file($file_info->getRealPath(), $file->getFileUri())) {
  +    \Drupal::messenger()->addError(t('File upload error. Could not move uploaded file.'));
  +    \Drupal::logger('file')->notice('Upload error. Could not move uploaded file %file to destination %destination.', ['%file' => $file->getFilename(), '%destination' => $file->getFileUri()]);
  +    return FALSE;
  +  }

One little Nitpicking: When commenting the setting of permission we should comment consistently that we set FileUri too before that, isn't?

Apart from that RTBC from me, since simpletest.me test with patch applied run successfully and multiple files upload test with different file types worked as expected without flaws. Additionally tested with experimental module media library: https://dmb66.ply.st/node/1

Do we need a backport for 8.6.x ?

@diqidoq this code is not introduced on changed by this patch. This patch moves everything from inside a foreach into it's own function. Also the patch applies cleanly to 8.6.x so it's upto the committer as to whether it should be cherry-picked. There's no need for a separate backport patch.

@alexpott: Thanks for taking the time to clarify. Great to see progress on it. Thanks to all who have worked on it. Maybe we get some more testers for RTBC?

Quite right @diqidoq. Where are my manners. I tested this patch on Friday and it worked beautifully. Attached multiple files, no validation errors!

Rerolled. Conflicted with #2989734: PHP 7.3 compatibility

Committed and pushed 2b94ed3c25 to 8.7.x and e297dace3f to 8.6.x. Thanks!

1+ @catch & @alexpott! Awesome!

+++ b/core/modules/file/tests/src/Functional/MultipleFileUploadTest.php
@@ -0,0 +1,60 @@
+    $assert->pageTextContains('Only files with the following extensions are allowed');
+    $assert->pageTextNotContains('The file ids are 1,2.');
+    $assert->pageTextContains('The specified file file1.wtf could not be uploaded.');
+    $assert->pageTextContains('The specified file file2.wtf could not be uploaded.');

Shame these assertions that @alexpott added in #13 got taken out in #14, now we have no positive assertions for the error messages, only a negative assertion for the top level error.

Hello. I'm comparing this and the patch in #2869855: Race condition in file_save_upload causes data loss - do the changes to validation here make the latter unnecessary now? (and if not, might someone be able to help with re-rolling it?)

What I noticed was the chunk of identical code in file.module:1039 (building the render array for the error message, which is the primary part of the race condition patch).