Register symfony's mime guessers if they are supported

duplicate key in the core.services.yml file.

Looks perfect from my perspective

+++ b/core/tests/Drupal/Tests/Core/File/MimeTypeGuesserTest.php
@@ -14,11 +14,40 @@
+    $guesser_1 = $this->getMock('Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface');
+    $guesser_1->expects($this->once())
+      ->method('guess')
+      ->with('file.txt')
+      ->willReturn('text/plain');
+    $mime_guesser_service->addGuesser($guesser_1);
+    $this->assertEquals('text/plain', $mime_guesser_service->guess('file.txt'));
+    $guesser_2 = $this->getMock('Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface');
+    $guesser_2->expects($this->once())
+      ->method('guess')
+      ->with('file.txt')
+      ->willReturn('text/x-diff');
+    $mime_guesser_service->addGuesser($guesser_2, 10);
+    $this->assertEquals('text/x-diff', $mime_guesser_service->guess('file.txt'));

In an ideal world, we would ensure here that we care about the order. http://docs.mockery.io/en/latest/reference/expectations.html allows this really nicely to do.

I'm trying to reroll.
Please review

+++ b/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php
@@ -865,40 +865,9 @@ class ExtensionMimeTypeGuesser implements MimeTypeGuesserInterface {
-      $this->moduleHandler->alter('file_mimetype_mapping', $mapping);

i agree with this, but it needs a change record

other than that its good to go

Ah, seems #22 misses ExtensionMimeTypeGuesserTest from #12

Sorry, I missed an untracked file.
ExtensionMimeTypeGuesserTest returned in a patch.
And rerolled core.services.yml after #2379741: Add Renderer::getCacheableRenderArray() to encapsulate which data is needed for caching a render array and have views use it

@adci_contributor
An interdiff between #22 and #25 would make it more likely you will get a review faster.
For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff

I rerolled #22 and returned ExtensionMimeTypeGuesserTest in the same patch, so i could not make interdiff between #22(which doesn't apply) and #25.
I guess we can just ignore #22, because #25 is actually a correct reroll of #12. Sorry again for this confusion.

I updated https://www.drupal.org/node/2258015

just a reroll

Committed/pushed to 8.0.x, thanks!

This is missing a change notice for how to replace hook_file_mimetype_mapping_alter() for all conditions of adding, removing, and altering existing extension mappings. I don't agree with the removal of that hook either because it was also used to *fix* invalid mappings in core when it happened (which it did in Drupal 7's lifetime).

Ok I see the change record mentions the hook was removed (which I don't think all use cases were fully justified in this issue before it was thrown out), but the examples need to be expanded on how to cover what hook_file_mimetype_mapping_alter() was used for before, rather than incredibly generic examples.

I don't see at all how supporting the alter hook makes it impossible to use the better and more secure services, especially after reading the patch that was committed.

My assumption is that the alter hook only affects data for that specific service, not that it would automatically be used in favor of the other services. I say that as the person who's written the only module that implements this hook in D7.

My 2 cents - can we think of adding something similar in #2311679: Separate MIME type mapping from ExtensionMimeTypeGuesser, but as a method of the MimeTypeMapper service rather than a hook? AFAICS the point here is not about determining the mime type of a 'real' file (the extension based guessing is becoming vestigial after this issue, there are two more prioritised guessers that take precedence), but rather about determining the mime type of a 'virtual' file based on its extension? And the mapping extension<->mimetype built-in in Drupal is a valuable asset that can be used via MimeTypeMapper methods, not guessers, introduced by the other issue.

(Setting back to needs work because of major security vulnerability)

I'm very unclear why analyzing a file to determine its mimetype is considered "secure". To me, it's always been safer to assign the mime type based on file extension. The site will whitelist file extensions for a file upload field, and because each extension maps to a mime type, it's also whitelisting mime types. So it could easily prevent everything from malware to HTML from being uploaded.

Currently in Drupal 8, if I create a file field and allow .txt files to be uploaded, then create a file that starts with <?php and upload it, Drupal saves this as a text/x-php file. That seems sketchy, but maybe not easy to exploit.

Let's try another one, if I create a file that starts with <html> and upload it, Drupal saves it as a text/html file. When downloaded from the private filesystem, this file is downloaded as text/html and interpreted by the browser as HTML, which could contain javascript - this is a nice fat XSS vulnerability!

Aside from these simple examples, it's well-known that malicious files can be constructed that could be interpreted as a valid file of multiple different types. I didn't yet try these w/ Drupal 8.

BTW, see https://www.drupal.org/project/filemime for a drupal 7 contrib module that allows altering the built-in mime/extension mapping. I was planning to upgrade this to d8 at some point.

Thanks mfb. I have a vague memory of us discussing this before a long time ago, and it may have been one of the original reasons we stuck to extension-based mime type guessing for so long. Would have been better to have remembered this before committing the patch...

Rolled this back for now.

We should:

1. Have a test for the file uploads/file downloads issue to avoid regressing.

2. Decide whether this problem is specific to file uploads (where we could possibly restrict to the extension guesser) or whether generally it's better not to do this.

I see three things to consider:

1. A way to whitelist allowed mime types. This could involve lots of permutations of mime types, due to multiple valid mime types for a file format or the preferred mime type changing over time.
2. How do you deal with files that are not detected as one of the allowed mime types? You may want to reject the file, or you may want to forcibly set its mime type to one of the allowed mime types.
3. A file could be saved by Drupal with one mime type, but served by the webserver with another mime type, because the webserver typically uses /etc/mime.types to determine the file's mime type.

Re-roll for 8.3.x with additional tests.

Forgot to add new files.

Also, as @mfb, a long time ago I've made a separate module - https://www.drupal.org/project/mimeinfo - which meets the needs. In case of 7.x it allows optionally override stream wrapper for checking the MIME type, 8.x - defines couple guessers from Symfony package and overrides addGuesser method of file.mime_type.guesser service to check that guesser supported (most part of last patch is there, in 8.x).

Wow, this is an old issue.
But it got relevant to me today because I have to guess the mimetype of files that don't have extensions so I can judge whether that file is allowed or not.
Thanks for sharing your knowledge.

The patch from comment #54 still applies. I tried it on Drupal 8.9.x and it does what the issue description says.
But it does also break regular file upload functionality like with a file field on a node edit form.

I could cover my use case without that patch, but directly using the Symfony Guesser class and applying #3156672-6: ExtensionMimeTypeGuesser breaks other mime_type_guesser services to let the guesser based on file extension fall back to the other guessers.

Extending PieterDC's thoughts in #67

But it does also break regular file upload functionality like with a file field on a node edit form.

It also breaks file upload via JSONAPI - this is how I found this ancient one... - because also passes file name only here when \Symfony\Component\Mime\FileinfoMimeTypeGuesser::guessMimeType() needs a valid path. So the root cause of the issue is pretty much the same. (See also \Drupal\file\Upload\FileUploadHandler::handleFileUpload()