(Tests followup) Valid file extensions in file names are not properly enforced when uploading files

The same issue persists with drupal 7.26, the following will correct the issue.

Working on this issue locally

Changes both whitelist and filename_part to lowercase before comparing them

Looks like patch applies cleanly, triggering testbot

Looks like @dgroene made the same mistake creating the patch I did checking the patch. It applies cleanly against 8.x :) Needs 7 port.

Restesting patch by #3, @aalamaki which applies cleanly to 7.26 as he stated! It appears to have failed due to the version number set at the time of submission. This didn't need reroll AFAICT.

3: 2112247.patch queued for re-testing.

This is still an issue in D8. See the attached image. It will have to be fixed for D8 first.

Rerolled (offset -1 lines) patch from #9 for the test bot now that the correct version is active. Seems to work.
Added before and after images.

Setting as Needs Work because it still needs tests.

David, Could you please share what other test cases are required that need to be reviewed. As #18 patch look fine, tested manually.

Whenever we fix an issue like this we also add an automated test, which helps to ensure that future changes do not revert our hard work and cause the bug to reoccur. Here are the instructions for writing tests.

Working on the tests.

I can't do a proper review of the patch right now, but I like it so far. The only thing I'm wondering about at this time is whether we should include a comment about why we have to normalize the extensions to lowercase.

Patch #23 fixed the problem for Drupal 8.x-dev using Firefox 30.0 on Ubuntu. Drupal no longer renames approved extensions in upper or lower case.

screen shot of acceptable file.

+++ b/core/modules/file/src/Tests/ValidatorTest.php
@@ -45,6 +45,15 @@ function testFileValidateExtensions() {
+  function testFileMungeFilename() {
+    $this->assertNotEqual('file.foo.txt', file_munge_filename('file.foo.txt', 'asdf txt pork', FALSE), 'Altered invalid extra extension.', 'File');
+    $this->assertEqual('file.asdf.txt', file_munge_filename('file.asdf.txt', 'asdf txt pork', FALSE), 'Accepted valid same case extra extension.', 'File');
+    $this->assertEqual('file.Asdf.txt', file_munge_filename('file.Asdf.txt', 'asdf txt pork', FALSE), 'Accepted valid different case extra extension.', 'File');
+  }

This test belongs in NameMungingTest and the first assertNotEqual is not necessary as it is already tested. And actually so is the second test. You could adapt testMungeIgnoreWhitelisted() to test this to.

@alexpott: Thanks for the feedback, I'll do the changes.

+++ b/core/modules/system/src/Tests/File/NameMungingTest.php
@@ -60,8 +60,8 @@ function testMungeIgnoreInsecure() {
+    // Declare our extension as whitelisted. The declared extensions should be case insensitive so test using one with a different case.

This comment needs to wrap at 80 characters.

I think we need to test this differently. Right now you're making the allowed extension uppercase, not the extension in the file name. It may not make any difference, but I think we ought to test against potential user input instead. A new $name with the uppercase extension should be created in the method to test.

Of course, another valid argument is that the allowed file extension is also user input and we should test to make certain it's normalized too. In which case your assertion should stay and my suggestion should be added as a second assertion. The only reason I know of why we might not care is that core file fields already normalize allowed extensions to lowercase when their settings are saved to the database, but that might not be the case forever or with custom file fields.

Thanks, I'll just add another assertion.

Here are the updated patches.

I followed the steps in the issue summary both before and after applying the patch in #34, and this patch looks to solve the stated problem. Marking as RTBC.

Committed 1ddfac8 and pushed to 8.x. Thanks!

I'm working on the backport.

Thank you, @mitsuroseba!

#40 is RTBC. Rather than making @mitsuroseba create a tests-only patch, I just ran the tests myself without the fix. The results are shown in the image below. As you can see, the new assertions failed.

After applying the patch a valid file extension with a different case did not have the underscore added.

Committed to 7.x - thanks!

However, I don't understand why this patch removed one of the existing tests?

Before the patch it tested an example like this:

file_munge_filename('something.php.txt', 'php');

After the patch it tested two examples like this:

file_munge_filename('something.PHP.txt', 'php');
file_munge_filename('something.php.txt', 'PHP');

I think we should have a quick followup to add the first test back in addition to the new ones, especially since it's the most common case.

tagging as needs tests to add the test back in.

I have added the testcase requested in #58 and tested in 8.0.0-beta11.

@KimNyholm, thanks for adding the test back in.

Made some minor changes to the displayed text and moved the re-added test after the upper case test, because the test results were easier to follow (at least for me).

A test-only change to 8.1.x is allowable.

The patch doesn't apply though.

Rerolled #65 with auto-merge

+++ b/core/modules/system/src/Tests/File/NameMungingTest.php
@@ -66,10 +66,14 @@ function testMungeIgnoreInsecure() {
-    // Declare our extension as whitelisted. The declared extensions should
-    // be case insensitive so test using one with a different case.
+    // Declare our extension as whitelisted.
+    // The declared extensions should be case insensitive.
+    // Test using one with a different case.

Thank you for the patches!

Unfortunately, this change to the comment is unrelated to the issue and should be removed. If it really enhanced the clarity of what's going on in this section of code then I might not care, but it doesn't in my opinion. Furthermore, it violates our documentation standards. Sentences within a single comment should be separated by spaces, not put on new lines.

Fixed the comment.

The work here is to add a test of the file name munging. The test case being added is for a filename like random.foo.txt with allowed extensions of foo</code'. The resulting filename is <code>random.foo_.txt. AFAICT, a test for that exists in \Drupal\Tests\system\Unit\Event\SecurityFileUploadEventSubscriberTest::testSanitizeName where the filename is foo.phar.png.php.jpg with allowed extensions jpg png and a resulting filename of foo.phar_.png_.php_.jpg.

Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!