Problem/Motivation

file_validate_extensions() incorrectly assumes that $file->filename is always the name of the file stored on the web server. This is not always the case. For example, the Media module allows users to change $file->filename, which has no affect on the actual file stored on the web server.

Example

  1. User uploads syllabus.pdf with Media module. $file->filename is "syllabus.pdf". The file name on the web server is "syllabus.pdf".
  2. User renames the file entity from "syllabus.pdf" to "2016 Fall Syllabus". $file->filename is "2016 Fall Syllabus". The file name on the web server is "syllabus.pdf".
  3. User references the file entity using a file field and FileField Sources. When file_validate_extensions() evaluates the $file object, the file will fail because $file->filename is incorrectly assumed to contain the file's extension.

Proposed resolution

For managed files, get the filename from the URI, which contains the actual file name from the web server.

For unmanaged files, the functionality is unchanged. (It's possible contrib modules could call this function to evaluate an unmanaged file.)

Remaining tasks

Create patch and maintainer review.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#26 interdiff-2834958-22-26.txt4.64 KBhuzooka
#26 core-file_validate_extensions-2834958-26--complete.patch4.67 KBhuzooka
#26 core-file_validate_extensions-2834958-26--test-only.patch3.59 KBhuzooka
#22 2834958-22.patch2.97 KBLendude
#20 core-file_validate_extensions-bug--20.patch3.1 KBalfaguru
#18 core-file_validate_extension_bug-2834958-18--complete.patch3.09 KBWim Leers
#15 core-file_validate_extension_bug-2834958-15--complete.patch3.06 KBhuzooka
#15 core-file_validate_extension_bug-2834958-15--test-only.patch1.98 KBhuzooka
#13 core-file_validate_extension_bug-2834958-13--complete.patch1.73 KBhuzooka
#13 core-file_validate_extension_bug-2834958-13--test-only.patch1.12 KBhuzooka
#2 drupal-file_validate_extensions_check_against_uri-2834958-2.patch1.08 KBChris Burge
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chris Burge created an issue. See original summary.

Chris Burge’s picture

Chris Burge CreditAttribution: Chris Burge commented
FileSize
drupal-file_validate_extensions_check_against_uri-2834958-2.patch1.08 KB

Attached is a patch for D7. I'm currently working on a D7 project but would be happy to write a patch for D8 if this issue gets some attention.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Active » Needs review
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Issue tags: +Needs tests, +Needs reroll

Patch in #2 is outdated and cannot be applied anymore.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Version: 8.9.x-dev » 9.0.x-dev
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs reroll +Needs issue summary update
FileSize
core-file_validate_extension_bug-2834958-13--test-only.patch1.12 KB
core-file_validate_extension_bug-2834958-13--complete.patch1.73 KB

Status: Needs review » Needs work

The last submitted patch, 13: core-file_validate_extension_bug-2834958-13--complete.patch, failed testing. View results

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
core-file_validate_extension_bug-2834958-15--test-only.patch1.98 KB
core-file_validate_extension_bug-2834958-15--complete.patch3.06 KB

The last submitted patch, 15: core-file_validate_extension_bug-2834958-15--test-only.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 9.0.x-dev » 8.8.x-dev

Thanks!

This would need to be fixed in D8 as well. D8 and D9 probably require separate patches. We should file the issue against the lowest branch that will receive the fix (which is 8.8.x for now and 8.9.x after next week) and just be sure to queue each patch's tests against the correct branch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
core-file_validate_extension_bug-2834958-18--complete.patch3.09 KB

Rebased #15.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

alfaguru’s picture

alfaguru CreditAttribution: alfaguru at Figure W Limited commented
FileSize
core-file_validate_extensions-bug--20.patch3.1 KB

Reroll of #18 for 8.8.

alison’s picture

alison
she/her
CreditAttribution: alison as a volunteer commented

#18 actually applies smoothly for me on 8.9.x. Also, I think the directories/paths in #20 are one level off?

Anyway, at this point (with dropping support for 8.8 anyway, IIRC?), I think #20 could be hidden, and we can go with #18?

-------
MEANWHILE: #18 tested on 8.9.16 -- success!
In my case, I was able to save my media entity "edit" form, making a test change to the media "name" field, and the form let me save just fine. AND, it still blocked me from putting a JPG file on this media entity that only allows pdf/doc/docx/txt/xls/xlsx/etc. file extensions.

I'll leave it "Needs review" in case y'all want to wait for someone else to review, too, but meanwhile, "yay" and thank you all!

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at iO commented
Version: 8.9.x-dev » 9.2.x-dev
Issue tags: -Needs issue summary update
FileSize
2834958-22.patch2.97 KB

Needed a reroll for 9.x, this won't go into 8.9.x anymore

Cleaned up the test a bit to match 9.x. Also gave its own method so we don't change any existing tests.

idebr’s picture

idebr CreditAttribution: idebr at iO commented
Status: Needs review » Reviewed & tested by the community

Patch work as expected, and includes test coverage so setting to RTBC. Thanks!

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented

Many-many thanks for pushing this forwards!

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented

Also gave its own method so we don't change any existing tests.

Then we should use dataprovider (imho).

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
FileSize
core-file_validate_extensions-2834958-26--test-only.patch3.59 KB
core-file_validate_extensions-2834958-26--complete.patch4.67 KB
interdiff-2834958-22-26.txt4.64 KB

Addressed #25, the new test method now uses data provider.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
CreditAttribution: huzooka at Cheppers for Acquia commented
Status: Reviewed & tested by the community » Needs review

The last submitted patch, 26: core-file_validate_extensions-2834958-26--test-only.patch, failed testing. View results

idebr’s picture

idebr CreditAttribution: idebr at iO commented
Status: Needs review » Reviewed & tested by the community

Patch work as expected, and includes different test coverage so setting to RTBC. Thanks!

  • larowlan committed 5e80229 on 9.2.x 
    Issue #2834958 by huzooka, Chris Burge, alfaguru, Wim Leers, Lendude:...
  • larowlan committed b27e6a8 on 9.3.x
    Issue #2834958 by huzooka, Chris Burge, alfaguru, Wim Leers, Lendude:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Fixed on commit

diff --git a/core/modules/file/tests/src/Kernel/ValidatorTest.php b/core/modules/file/tests/src/Kernel/ValidatorTest.php
index 4e85269600..27ad4b9df1 100644
--- a/core/modules/file/tests/src/Kernel/ValidatorTest.php
+++ b/core/modules/file/tests/src/Kernel/ValidatorTest.php
@@ -79,7 +79,7 @@ public function testFileValidateExtensionsOnUri(array $file_properties, array $e
    * @return array[][]
    *   The test cases.
    */
-  public function providerTestFileValidateExtensionsOnUri() {
+  public function providerTestFileValidateExtensionsOnUri(): array {
     $temporary_txt_file_properties = [
       'filename' => 'asdf.txt',
       'uri' => 'temporary://asdf',

Committed b27e6a8 and pushed to 9.3.x. Thanks!
As there is little risk of disruption here, backported to 9.2.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.