Problem/Motivation

When uploading a file with "0" (zero) as a part of its name, the file is marked incorrectly as application/octet-stream instead of its real mimetype.

This is because when the file name contains a "0" (zero) between dots, guessMimeType() from the ExtensionMimeTypeGuesser class returns NULL prematurely.

Steps to reproduce

  1. Have a file media entity.
  2. Upload a file named foo.0.zip. (No need to save the media entity.)
  3. Go to the Content > Files list.
  4. The uploaded file is marked as application/octet-stream instead of application/zip.

Proposed resolution

Fix the "while" condition in guessMimeType to exit only if array_shift returns NULL.

Remaining tasks

  1. Review the patch.
  2. Merge.
  3. Apply the patch to the latest core versions.

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

Issue fork drupal-3487488

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dakwamine created an issue. See original summary.

dakwamine opened merge request !10187

dakwamine’s picture

dakwamine
Primary language French
commented
Issue summary: View changes
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Version: 10.3.x-dev » 11.x-dev
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

Thank you for reporting

So fixes actually need to land in the development branch first (11.x).
If still an issue on 11.x then it's still valid.
Next step then would be to add test coverage

Thanks

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

This may be resolved with #3477346: ExtensionMimeTypeGuesser::guessMimeType returns less accurate MIME type when file extensions have multiple parts Are you able to test on 10.3.x?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Ah looks like you did test on 10.3.x already. We will need a test to show the before and after with the fix.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review

Rebased on 11.x, simplified the change, and added a test.

Confirmed the test-only pipeline failed:

There was 1 failure:
1) Drupal\KernelTests\Core\File\MimeTypeTest::testFileMimeTypeDetection
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'application/zip'
+'application/octet-stream'
mondrake’s picture

mondrake
Location 🇮🇹
commented

couple of comments inline

kim.pepper opened merge request !10195

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Created a new MR because I don't know how to properly rebase via the gitlab UI. 😞

mondrake’s picture

mondrake
Location 🇮🇹
commented

one more nitpicking comment

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Looks good. Test coverage added.

mondrake changed the visibility of the branch 3487488-extensionmimetypeguesserguessmimetype-must-support to hidden.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Changed the comment since my earlier suggestion duplicated the word 'because' and was not compliant to the 80 chars line limit.

I think I can leave that to RTBC.

dakwamine’s picture

dakwamine
Primary language French
commented

Hello. :) The updated fix looks good for me. Much simpler indeed. :D Many thanks for the hard work.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Priority: Normal » Critical

I think this should go in the upcoming point releases - it fixes a regression bug introduced by a MR not released yet in 10.4 and 11.1, and the bug leads to data corruption.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: ExtensionMimeTypeGuesser::guessMimeType must support file names with "0" (zero) like foo.0.zip » ExtensionMimeTypeGuesser::guessMimeType must support file names with "0" (zero) in the extension parts like foo.0.zip

  • longwave committed 0475cecc on 10.3.x 
    Issue #3487488 by kim.pepper, mondrake, dakwamine, smustgrave:...

  • longwave committed e539d9bf on 10.4.x 
    Issue #3487488 by kim.pepper, mondrake, dakwamine, smustgrave:...

  • longwave committed 5c12effc on 10.5.x 
    Issue #3487488 by kim.pepper, mondrake, dakwamine, smustgrave:...

  • longwave committed bc5e491c on 11.0.x 
    Issue #3487488 by kim.pepper, mondrake, dakwamine, smustgrave:...

  • longwave committed d83ba02d on 11.1.x 
    Issue #3487488 by kim.pepper, mondrake, dakwamine, smustgrave:...

  • longwave committed 393baa76 on 11.x 
    Issue #3487488 by kim.pepper, mondrake, dakwamine, smustgrave:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Great find, thanks for the fix and test. The previous change was committed to 10.3 and 11.0 so backporting this all the way down to there as an eligible bug fix.

Committed and pushed 393baa761cc to 11.x and d83ba02ddbb to 11.1.x and bc5e491c885 to 11.0.x and 5c12effc9e3 to 10.5.x and e539d9bf68d to 10.4.x and 0475ceccefc to 10.3.x. Thanks!

longwave closed merge request !10195

Status: Fixed » Closed (fixed)

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