Do not allow files with "php|pl|py|cgi|asp|js" extensions to be renamed to *.txt and be uploaded if *.txt is not allowed by the field widget

I hope this title is clearer.

+++ b/includes/file.inc
@@ -1531,8 +1531,12 @@ function file_save_upload($form_field_name, $validators = array(), $destination
+      //$validators['file_validate_extensions'][0] .= ' txt';

Why those lines are commented?

After uploading a patch you must change issue status to Needs review so Drupal I can apply it and run tests.

This is present in Drupal 8 too afaics and so will need to be fixed there first. It's also present in Drupal 6 so it might be worth searching the issue queue for prior discussions of this issue.

Test-only patch. This is supposed to fail.

Please test & review the following patch hopefully it'll solve the problem.

diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index a6fc3f6..cb95687 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -813,15 +813,18 @@ function file_save_upload($form_field_name, $validators = [], $destination = FAL
// filename.php.txt, respectively). Don't rename if 'allow_insecure_uploads'
// evaluates to TRUE.
if (!\Drupal::config('system.file')->get('allow_insecure_uploads') && preg_match('/\.(php|pl|py|cgi|asp|js)(\.|$)/i', $file->getFilename()) && (substr($file->getFilename(), -4) != '.txt')) {
- $file->setMimeType('text/plain');
- // The destination filename will also later be used to create the URI.
- $file->setFilename($file->getFilename() . '.txt');
- // The .txt extension may not be in the allowed list of extensions. We have
- // to add it here or else the file upload will fail.
- if (!empty($extensions)) {
- $validators['file_validate_extensions'][0] .= ' txt';
- drupal_set_message(t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]));
- }
+ //check whether file extension contains txt or not
+ if (strpos($validators['file_validate_extensions'][0], 'txt') !== false) {
+ $file->setMimeType('text/plain');
+ // The destination filename will also later be used to create the URI.
+ $file->setFilename($file->getFilename() . '.txt');
+ // The .txt extension may not be in the allowed list of extensions. We have
+ // to add it here or else the file upload will fail.
+ if (!empty($extensions)) {
+ $validators['file_validate_extensions'][0] .= ' txt';
+ drupal_set_message(t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]));
+ }
+ }
}

// If the destination is not provided, use the temporary directory.

I have edited the latest patch and changed the inline comment. Also replaced the array with another variable that is available throughout the function.

I would raise the discussion though if this is a user friendly implementation?

Scenario of a user:
- I allow the extensions 'php js' (or not)
- Now I am able to upload a .php file
- The file is renamed to .txt (although not allowed either)

Scenario of a user (with patch):
- I allow the extensions 'php js'
- Now I am still NOT able to upload a .php file
- I add the extension 'txt'
- Now I am able to upload the .php file, but my .php file is renamed to .txt

As seen above, both scenarios are not optimal. As it both does not restrict what has been set by the user.

Possible solutions:
1: Add a description to the upload file field, that .txt needs to be required due to security purposes in case unsafe extensions are detected?
2: Add an option to the upload file field that allows unsafe file uploads?
3: ?

Last patch, re-rolled for 8.6.3

The fix here is not quite right because the rename takes place even if the field does allow .php files if you have not allowed insecure uploads. So we need to fix and test for that.

We need to address #20

Here's a rewrite of the patch to account for #24. Still need to account for the usability considerations. I.e we should warn people to have php without txt in the file config form if insecure uploads are not allowed.

So still to do - add an update path to detect a file field which allows potentially a malicious extensions but not txt. We need to add txt for them.
And we need to do the UI stuff.

I.e we should warn people to have php without txt in the file config form if insecure uploads are not allowed.

Is there any usecase for allowing insecure file extensions such as .php while disallowing insecure uploads but then not allowing them to be renamed to .txt either? I think not.
So if insecure uploads are disallowed, we might want to automatically (though probably not silently) add .txt to the list of allowed extensions, or probably better, let automatically renamed files pass the allowed extensions test. Finally, the sitebuilder might not want end users to upload .txt files directly.

@Pancho

let automatically renamed files pass the allowed extensions test

is the current behaviour.

I think the current behaviour is wrong. Not allowing insecure updates and allowing php without txt should cause the users to see an error if they try to configure a field that way. The automatic rename and silently allowing .txt when the users has apparently chosen to not allow txt is weird.

Like, trying to upload blah.php will work but trying to upload blah.php.txt won't but uploading blah.php means you'll end up with a file called blah.php.txt. That is very confusing for a site administrator and user.

let automatically renamed files pass the allowed extensions test

is the current behaviour.

True, so in the end, maybe current behavior turns out to be not so wrong at all? Let me investigate this a bit.

@Pancho sure investigate. But please ensure that the following is covered:

Like, trying to upload blah.php will work but trying to upload blah.php.txt won't but uploading blah.php means you'll end up with a file called blah.php.txt. That is very confusing for a site administrator and user.

Here's yet-another-reason we the current behaviour is wrong. On a modern browser if you download the file and try to re-upload it it will not allow you because it will prevent non-allowed extensions at the browser level before they even hit Drupal.

Indeed, the current behavior is clearly wrong.

Obviously, we should only accept .php files if .php extension is allowed. However, if the extension is allowed, we have two options how to implement this in a way every sitebuilder would expect it to be:

1. the .php file might be accepted no matter whether in the end it is stored with its original extension or whether it is renamed to .txt for security reasons.
2. the .php file might even bypass the security check, as in the end, the sitebuilder voluntarily decided to allow .php files and might have a proper usecase for exactly this.

Obviously, the second option might be risky and should be accompanied by a security warning, but in the end, if a sitebuilder wants .php files to be allowed, they might know what they're doing.

Still, as long as the latter usecase has not been substantiated, I'd tend to stick with the inherently safer variant 1.

trying to upload blah.php will work but trying to upload blah.php.txt won't but uploading blah.php means you'll end up with a file called blah.php.txt. That is very confusing for a site administrator and user.

Don't think so. If the sitebuilder wants to configure a particular file field just for .php files, they don't want people to upload all kinds of .txt files, just because .php are internally renamed. If the sitebuilder wants to accept both, they may add both extensions.

The intention of the rename is to cope with situations where a user has configured a file field to allow all extensions. I.e. where allowed extensions equals an empty string. In this instance we do not want insecure files to be uploaded - hence the reasonable strategy to rename to add a .txt extension.

There is a 0.1% use-case where a site builder configures a file field to allow php files and does not enable allow_insecure_uploads. We should not bend over backwards for this use-case. We should inform site builders that they need to add txt as an allowed extension because this is the extension the uploaded file will have. This new error also has the added benefit of telling a site builder that what they are intending to do is weird and to be honest not recommended.

Still need to add UI tests for the new error and update path to take care of existing fields.

Whoops

Here's a UI test. Now we need an update path test.

Nice - adding the post update found #3033494: SiteConfigureForm can install the file module without the field module

Here I fix the drupal-8.6.0.bare.testing.php.gz to have field module installed.

There is a 0.1% use-case where a site builder configures a file field to allow php files and does not enable allow_insecure_uploads.

True. However, this doesn’t answer my question why the admin needs to accept .txt files in the front end, just because internally, .php files are renamed .php.txt.

[edit: plus allowing renamed files to bypass the extension check would mean no string addition, so fixable in 8.6.x.]

However, I really don’t want to wreck anyone’s nerves arguing over this. Feel free to go ahead. Your approach does fix the reported bug, and that might be just enough.

@Pancho I answered

However, this doesn’t answer my question why the admin needs to accept .txt files in the front end, just because internally, .php files are renamed .php.txt.

Already here was my answer:

Here's yet-another-reason we the current behaviour is wrong. On a modern browser if you download the file and try to re-upload it it will not allow you because it will prevent non-allowed extensions at the browser level before they even hit Drupal.

You disagreed with my reasoning.

From my point of view, users can download files directly from the node edit form using a right click - they would then not be able to reupload this file. Further more when uploading to a multi value field they see a list of already uploaded files and the allowed extensions and files which don't match that rule. But if they try to upload a file with a match extension they won't be able to.

Also

internally, .php files are renamed .php.txt.

- it's not an internal rename - it is renamed file, the file entity has the new filename and google / the world sees the file as a txt file.

@alexpott: You have a valid point (allowing to re-upload the file), so do I (disallowing .txt files unrelated to security renames).
In the end, both approaches mean the original bug gets fixed, and that's probably more important than anything else. So I'm really fine with focussing on that! :)

Does the drupal-8.6.0.bare.testing.php.gz belong here or in #3033494: SiteConfigureForm can install the file module without the field module? In the latter case, the other issue would be a blocker (plus major for blocking a major bug).

The changes to drupal-8.6.0.bare.testing.php.gz can be here or there. We can just re-roll after whichever one gets in first. This change proves the change to drupal-8.6.0.bare.testing.php.gz works as expected. The change already has testing of its specific bug.

Blockers are in.

Updated the issue summary.

We have test coverage of the UI changes and the update path.

I'm not entirely sure that we need a change record for this change as existing sites function without change because we provide an update path and the user is informed about how to configure the fields in the the UI.

I can't find anything to nitpick about 😮. There's explicit test coverage that looks great, even for FileUploadRestResource 👌

Mostly looks great. I agree with the direction this is going. The current behavior is definitely problematic and weird.

However, I think the UI text needs a bit more help before this is RTBC. A few other nits:

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -224,14 +224,24 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   +            $form_state->setError($element, t('The extension %extension is insecure. In order to allow it to be uploaded allow the "txt" extension.', ['%extension' => $extension]));
   

   UX concern: I think this message needs the word "also". As it is, it's not clear we're saying you can put "js" in this list as long as you also include "txt", etc. I think this would be more clear:

   "The extension %extension is insecure. In order to allow it to be uploaded, also allow the "txt" extension."

   However, even that's still not great, since we don't say anywhere else that we rename "insecure" files to .txt. At the risk of being too verbose, perhaps this:

   "The extension %extension is insecure, and will be renamed to @extension_.txt. In order to allow %extension files to be uploaded, also allow the "txt" extension."

   (or something)?

   Finally, it's a little weird that %extension is an italicized placeholder while "txt" is quoted. Maybe we can/should be consistent about that and always format references to extensions in this message the same way?

2. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -474,13 +474,13 @@ protected function prepareFilename($filename, array &$validators) {
   diff --git a/core/modules/file/src/Tests/FileFieldWidgetTest.php b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   
   diff --git a/core/modules/file/src/Tests/FileFieldWidgetTest.php b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   index 0c18eaf80b..994a318a8f 100644
   
   index 0c18eaf80b..994a318a8f 100644
   --- a/core/modules/file/src/Tests/FileFieldWidgetTest.php
   
   --- a/core/modules/file/src/Tests/FileFieldWidgetTest.php
   +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   
   +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   @@ -336,6 +336,37 @@ public function testPrivateFileSetting() {
   

   Ugh to be adding code to one of the last deprecated WebTestBase test classes. I guess we'll just move all this again when that's finally killed.

3. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   @@ -336,6 +336,37 @@ public function testPrivateFileSetting() {
   +    $this->assertText('The extension php is insecure. In order to allow it to be uploaded allow the "txt" extension.');
   

   If we address point #1, this spot also needs an update.

4. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   @@ -336,6 +336,37 @@ public function testPrivateFileSetting() {
   +    $this->drupalPostForm("admin/structure/types/manage/$type_name/fields/$field_id", $edit, t('Save settings'));
   +    $this->assertText('The extension php is insecure. In order to allow it to be uploaded allow the "txt" extension.');
   

   Shouldn't we also assert the page response here, too?

5. +++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
   @@ -336,6 +336,37 @@ public function testPrivateFileSetting() {
   +    $this->assertText('Saved ' . $field_name . ' configuration.');
   +
   ...
   +    $this->assertText('Saved ' . $field_name . ' configuration.');
   +  }
   

   Assert page responses at these spots, too?

6. +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
   @@ -496,6 +496,19 @@ public function testFileUploadMaliciousExtension() {
   +    $this->field->setSetting('file_extensions', 'php')
   +      ->save();
   

   Minor nit: Why is this split onto 2 lines when it'd easily fit in 80 chars? Do we consider the newline ->save(); more readable for some reason?

7. +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
   @@ -496,6 +496,19 @@ public function testFileUploadMaliciousExtension() {
        $this->field->setSetting('file_extensions', 'doc')->save();
   

   cuz, we don't do that consistently if that's what we intend...

@dww thanks for the review - especially looking at the UI text.
1. I've addressed this by being more explicit - The extension php is insecure. In order to allow files with this extension to be uploaded add the txt extension as well. -- I've also placeholdered the txt since this is not translatable.
2. Yep stuff takes time - this is one of the last.
3. Fixed
4. I don't think so. It's going to be a 200 - if we had to assert that everywhere then we'd be adding 100s of these - if it was not a 200 then I'd agree. I think as long as we have 1 assertion about the expected output then we're good. Is there a reason you think different?
5. See answer to 4.
6. Dunno maybe PHPStorm being clever - fixed.
7. Indeed.

"The extension php is insecure. In order to allow files with this extension to be uploaded add the txt extension as well." is definitely better, thanks.

I still kinda wish we said why. Either here, or perhaps a link to a d.o handbook page about it?

I think we should tag this for UX review and get some more eyes on this issue.

p.s. re #50.4 and 5: I agree with you. I've just seen assert 200's everywhere in core tests, and assumed it was standard practice to basically always do that before asserting anything else after a new request. *shrug*.

Refreshing the summary a bit to make it easier for the UX review.

Also, from an "API-First Initiative" standpoint, if you can create a file field through the API, not just the field UI, don't we need the validation that txt is also in the allowed list if anything insecure is at some lower level than $form_state->setError()? Admitting my ignorance here about the actual API-First-ness of core. ;) Not sure if/how that works.

Thanks,
-Derek

I also think we could consider adding txt when adding a malicious file extension via UI. And telling the user we've done this rather than erroring.

Re. API first creation of fields. Well config validation is something we're working on but is not all there atm. Field config validation should be attempted altogether - I don't think this issue should add the first piece.

Re: #54.A: Auto-adding .txt seems weird, but I'll let the UX team respond to that part. ;)

Re: #54.B and API-first creation of fields: Duly noted. Makes sense. I had the thought, so I wanted to raise it, but I'm totally satisfied that we can't solve it here.

I pinged #ux in Slack to try to get a review of this on the weekly UX call (which is today). Let's see what happens.

Thanks!
-Derek

Further updates to the summary to make the UI text changes totally explicit for reviewers without having to read the patch.

Well config validation is something we're working on but is not all there atm. Field config validation should be attempted altogether - I don't think this issue should add the first piece.

For @dww's information: see "full config entity support in #3002188: REST: top priorities for Drupal 8.7.x — specifically #2869792: [meta] Add constraints to all config entity types.

Usability review

We discussed this at the end of the weekly Drupal usability meeting today (2019-02-26).

1. When "txt" and one or more unsafe extensions have been added to the allowed extensions, we think there should be a message explaining how "unsafe" files will be added.
2. We felt that the text in the warning message is still not entirely clear.
3. We did not discuss it at the meeting, but I think you should add a few lines to hook_help() to be displayed on /admin/help/file.
4. Personally, I am annoyed by error messages that give me new rules. But maybe adding unsafe extensions is uncommon, so it is not worth always explaining this behavior.

I will also add a few screenshots. You might add them to the IS to help further review.

Regarding (1): let's start with the text shown when someone tries to upload an "unsafe" file: "For security reasons, your upload has been renamed to index.php.txt." Can we add to the help text for the "Allowed file extensions" setting, something like "For security reasons, '.txt' will be added to any file ending in '.php'."? Ideally, figure out which of the allowed extensions are unsafe; my suggestion assumes just 'php'.

It is OK if the additional text only appears when the settings page is reloaded: save settings, then edit them again and see the extended text.

As for (2), how about changing "add" to "allow"? Also, add a comma and be consistent with "the extension ..." instead of "the ... extension": "The extension php is insecure. In order to allow files with this extension to be uploaded, allow the extension txt as well."

I notice that if I try to allow "cgi" as well as "php", only the first is mentioned in the warning message. I guess that is OK, especially if you address (1).

For (3), there is already a section on "Allowing file extensions", so it is clear where to add further details.

For (4), my typical annoyance is with password forms. Either I am not using a password manager and I choose something too short or else I am using a password manager and it fills in something with characters that are not allowed. Either way, I get annoyed that the first time I am told about the rule is in an error message.

I guess you do not have to worry about (4) as long as you address (3).

I will leave the "Needs usability review" tag until these points are addressed.

@benjifisher thanks for the review. Should have a patch up to address most of the points. However re the "allow" wording...

In order to allow files with this extension to be uploaded, allow the extension txt as well.

The double allow doesn't really work for me. Also in order to allow text files you need to add the txt extension so it is telling you what to do. Which seems to be good UX to me.

@alexpott:

Here are some more possibilities: In order to allow files with this extension to be uploaded, ...

1. ... add the txt extension as well.
2. ... allow the extension txt as well.
3. ... add txt to the list of allowed file extensions.
4. ... add txt to the allowed file extensions.
5. ... also allow the extension txt.
6. ... also allow files with the extension txt.
7. ... include txt as an Allowed file extension.
8. ... add txt as an Allowed file extension.

The first is the one I do not like, and the second is the one you do not like. I think the longest is the most explicit, but it is also the longest. ;) Does the second "allow" look any better after "also"?

@benjifisher I think we need to tell the user what to do. Saying allow does not do that.

Going back to the whole message. How about:

For security reasons you must add txt to the list of allowed extensions to upload files with a php extension.

It's on point, explains exactly what to do and why you are being asked.

> The upload has been renamed to index.php.txt for security reasons

Maybe remove "reasons":
The upload has been renamed to index.php.txt for security.

(because "reasons" in the abstract does not really add information. "because reasons" is a kind of hand-wavy expression these days, no?)

Riffing on #61:

Add txt to the list of allowed extensions to securely upload files with a php extension.

with added explanation:

Add txt to the list of allowed extensions to securely upload files with a php extension. The txt extension will then be added automatically.

I wouldn't mind the more verbose version here as these are tricky things to consider.

How is this issue affected by https://www.drupal.org/sa-core-2020-012?

Re-rolled for 9.2.x. Re #68 - that SA did not change this behaviour.

Still need to account for the UX feedback. The new patch adds less strings because it fallbacks back to the security behaviour in file_validate()... that exists in HEAD...

  // Ensure the file does not contain a malicious extension. At this point
  // _file_save_upload_single() will have munged the file so it does not contain
  // a malicious extension. Contributed and custom code that calls this method
  // needs to take similar steps if they need to permit files with malicious
  // extensions to be uploaded.
  if (empty($errors) && !\Drupal::config('system.file')->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $file->getFilename())) {
    $errors[] = t('For security reasons, your upload has been rejected.');
  }
  return $errors;

So the whole "security reasons" debate is out-of-scope here - sure we can open a follow-up to improve said text but it's not really part of this change.

Still need to address the #61 part of the UX feedback.

No interdiff because nearly 2 years since last patch.

Changing the UI test to @yoroy's suggestion in #67

On reviewing this I've realised that we can remove some dead code and make some code a bit clearer.

1. +++ b/core/modules/file/file.module
   @@ -1003,7 +1003,7 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +      if (!isset($validators['file_validate_extensions']) || (preg_match('/(^|\s)txt(\s|$)/', $extensions) && empty(file_validate_extensions($file, $extensions)))) {
   
   +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -485,24 +485,24 @@ protected function prepareFilename($filename, array &$validators) {
   +          $passes_validation = $passes_validation && preg_match('/(^|\s)txt(\s|$)/', $validators['file_validate_extensions'][0]);
   
   +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -403,24 +403,24 @@ protected function prepareFilename($filename, array &$validators) {
   +          $passes_validation = $passes_validation && preg_match('/(^|\s)txt(\s|$)/', $validators['file_validate_extensions'][0]);
   

   Could we use the simpler /\btxt\b here instead?

   https://www.phpliveregex.com/p/yJT

2. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,42 @@
   + * Adds txt to allowed extensions for all fields that allow uploads of insecure files.
   

   nit: > 80

3. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,42 @@
   +      $allowed_extensions_string = $field->getSetting('file_extensions');
   ...
   +          $allowed_extensions_string .= ' txt';
   

   should we trim() this here to make sure we don't end up with two spaces when we're appending ' txt'

4. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,42 @@
   +          // insecure is allowed.
   

   nit: missing a word after insecure here?
   perhaps insecure (one|extension) is allowed

5. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -225,14 +225,24 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   +      if (!in_array('txt', $extension_array) && !\Drupal::config('system.file')->get('allow_insecure_uploads')) {
   

   these are strings so should we be using TRUE as the third argument to in_array

6. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -225,14 +225,24 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   +            $form_state->setError($element, t('Add %txt_extension to the list of allowed extensions to securely upload files with a %extension extension. The %txt_extension extension will then be added automatically.', ['%extension' => $extension, '%txt_extension' => 'txt']));
   

   because this is in a foreach loop, we may ending up setting multiple errors with essentially the same text for the one field. Should we instead collect all insecure extensions and then print the message once with the list if the said list is not empty?

   And if so, would array_filter be a better language construct to use than foreach

addressed #74.

1. +++ b/core/modules/file/file.post_update.php
   @@ -10,7 +10,8 @@
   - * Adds txt to allowed extensions for all fields that allow uploads of insecure files.
   + * Adds txt to allowed extensions for all fields that allow uploads of insecure
   + * files.
   

   This is test is allowed to be longer than 80 - it is used in the UI for update descriptions. Also it should be "Add" and not "Adds".

2. +++ b/core/modules/file/file.post_update.php
   @@ -29,7 +30,7 @@
   +          // insecure (one|extension) is allowed.
   

   :) this needs to be either one OR extension. Let's go for extension.

3. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -236,11 +236,17 @@
   +        $unique_extensions = [];
   +        // Filter out definitions that are not intended to be placed by the UI.
   +        $unique_extensions = array_filter($extension_array, function (string $extension) {
   +          if (!in_array($extension, $unique_extensions) && preg_match(FILE_INSECURE_EXTENSION_REGEX, 'test.' . $extension)) {
   +            return $extension;
              }
   +        });
   +        if (!empty($unique_extensions)) {
   +          $unique_extensions = implode(' or ', $unique_extensions);
   +          $form_state->setError($element, t('Add %txt_extension to the list of allowed extensions to securely upload files with a %extension extension. The %txt_extension extension will then be added automatically.', ['%extension' => $unique_extensions, '%txt_extension' => 'txt']));
            }
   

   This doesn't look correct or easy to read. I think that what was here before was easier to understand and read. You don't get multiple errors. Each form element can only have one error.

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -236,17 +236,11 @@
   +      if (!in_array('txt', $extension_array) && !\Drupal::config('system.file')->get('allow_insecure_uploads')) {
   

   Unfortunately removed the TRUE - which was a good change see #73.5

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -236,17 +236,11 @@
   +        foreach ($extension_array as $extension) {
   +          if (preg_match(FILE_INSECURE_EXTENSION_REGEX, 'test.' . $extension)) {
   +            $form_state->setError($element, t('Add %txt_extension to the list of allowed extensions to securely upload files with a %extension extension. The %txt_extension extension will then be added automatically.', ['%extension' => $extension, '%txt_extension' => 'txt']));
   ...
   -        });
   -        if (!empty($unique_extensions)) {
   -          $unique_extensions = implode(' or ', $unique_extensions);
   -          $form_state->setError($element, t('Add %txt_extension to the list of allowed extensions to securely upload files with a %extension extension. The %txt_extension extension will then be added automatically.', ['%extension' => $unique_extensions, '%txt_extension' => 'txt']));
            }
   

   One problem this code has is what if a user add both php and js to the list of allowed extensions. We'll only raise an error on one of the extensions.

   I think this is okay because adding the txt extension will fix it. So what this points out is that we can do:

           foreach ($extension_array as $extension) {
             if (preg_match(FILE_INSECURE_EXTENSION_REGEX, 'test.' . $extension)) {
               $form_state->setError($element, t('Add %txt_extension to the list of allowed extensions to securely upload files with a %extension extension. The %txt_extension extension will then be added automatically.', ['%extension' => $extension, '%txt_extension' => 'txt']));
               break;
             }
           }
   

   Adding the break; will prevent unnecessary looping/calls to $form_state->setError(). And it's a bit more explicit about what matters.

Mostly looking great, thanks for all the work in here! The actual changes are very small and an improvement in behavior. Test coverage is solid and clear. Only a handful of cosmetic nits about comments and code style.

Is it worth a new test-only patch upload?

After a few minor edits, the issue summary is mostly in good shape. Also, crossed off UX review from remaining tasks per previous comments. Do we need screenshots for the UI changes section?

Is it fair to say there are no API changes when we're changing the behavior of the JSONAPI + REST endpoints? Files that used to be allowed would be rejected after the change, unless the site is reconfigured. Sort of a gray area for "API changes".

Seems this needs a change record, especially since it can impact JSONAPI/REST clients. That's the main reason I'm NW'ing this. None of the points below are really blockers. Otherwise I'd RTBC this.

1. +++ b/core/modules/file/file.module
   @@ -25,6 +25,8 @@
   +// cspell:ignore btxt
   
   +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -32,6 +32,8 @@
   +// cspell:ignore btxt
   
   +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -27,6 +27,8 @@
   +// cspell:ignore btxt
   

   Wouldn't it be better to add these to the global dictionary instead of each file we use it?

   Edit: Guess not. ;) I grep'ed and see we're doing it in a few spots. Note: we mostly (but not entirely) use "cSpell:ignore" for these...

2. +++ b/core/modules/file/file.module
   @@ -1003,7 +1005,7 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +      if (!isset($validators['file_validate_extensions']) || (preg_match('/\btxt\b/', $extensions) && empty(file_validate_extensions($file, $extensions)))) {
   

   We're removing the old comment about .txt handling. It'd be nice to explain the preg_match() right above this if().

   It's already a little unfortunate that the existing comment explains the non-existent "else" case, not what the code below it is actually doing.

   Perhaps:

   If there is no file extension validation at all, or the site allows 'txt' extensions and the file would otherwise pass validation, rename it. If the file will be rejected anyway due to...

   (re-wrapped to 80 chars).

3. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,42 @@
   +function file_post_update_update_allowed_file_extensions_if_insecure(&$sandbox = NULL) {
   

   Wondering if this function name is what we want to commit. Maybe:

   file_post_update_add_txt_extension_if_allows_insecure()

   ?

4. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,42 @@
   +    // Determine if this is field uses a item definition that extends FileItem.
   

   Nits:
   "Determine if this is field uses an item definition that extends FileItem."

5. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,42 @@
   +        // .txt is specifically allowed there's nothing to do.
   

   Hyper nit: could use a comma after "allowed".

6. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,42 @@
   +          // Add the txt extension to the list of allowed extensions if an
   +          // insecure extension is allowed.
   

   a) Standards are to document outside the if.

   b) This is a bit wordy, and doesn't really add much beyond the code. How about:

   // If any insecure extension is allowed, add the 'txt' extension.

7. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -225,14 +225,25 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   -      $extensions = array_filter(explode(' ', $extensions));
   -      $extensions = implode(' ', array_unique($extensions));
   +      $extension_array = array_filter(explode(' ', $extensions));
   +      $extensions = implode(' ', array_unique($extension_array));
   

   Although this is a bit clearer, it isn't needed, is it? Out of scope?

8. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -225,14 +225,25 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   +      // If insecure uploads are not allowed then error if txt is not an allowed
   +      // extension.
   

   How about?

   // Set an error if neither 'txt' extensions nor insecure uploads are allowed.

9. +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
   @@ -470,6 +470,39 @@ public function testTemporaryFileRemovalExploitAnonymous() {
   +    // By default allowing PHP files without TXT is not permitted.
   ...
   +    // Test allowing PHP and TXT.
   ...
   +    // If the system is configured to allow insecure uploads, TXT is not
   

   Hyper nit: 'PHP' is an acronym (a recursive one, no less!), while 'txt' is an abbreviation for 'text'. So we don't need caps 'TXT'.

10. +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
    @@ -470,6 +470,39 @@ public function testTemporaryFileRemovalExploitAnonymous() {
    +    $this->assertSession()->pageTextContains('Add txt to the list of allowed extensions to securely upload files with a php extension. The txt extension will then be added automatically.');
    

    Should we also assert there's no 'Saved $field_name configuration' at this point?

11. +++ b/core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php
    @@ -0,0 +1,110 @@
    +    $connection = Database::getConnection();
    ...
    +    // Do direct database updates to avoid dependencies.
    

    The comment from below might be helpful above, too. I did a double-take when I first saw this...

12. +++ b/core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php
    @@ -0,0 +1,110 @@
    +   * @param $allowed_extensions
    

    Doesn't this need a type and an explanation?

13. +++ b/core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php
    --- a/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
    +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
    

    Off topic, but I wonder how long this "temporary" class is going to be part of core. ;)

14. +++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
    @@ -628,9 +628,8 @@ public function testFileUploadMaliciousExtension() {
         // Add php as an allowed format. Allow insecure uploads still being FALSE
    

    Maybe need to say:

    Add 'php' and 'txt' as allowed extensions. Allow insecure...

15. +++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
    @@ -628,9 +628,8 @@ public function testFileUploadMaliciousExtension() {
    +    $this->field->setSetting('file_extensions', 'php txt')
           ->save();
    
    @@ -698,6 +697,18 @@ public function testFileUploadMaliciousExtension() {
    +    $this->field->setSetting('file_extensions', 'php')->save();
    

    Hyper nit: As below, the ->save() would easily fit on the previous line. Maybe that'd be out of scope. ;)

    But if we don't, maybe we need to split the ->save() onto a newline later to be consistent?

Thanks!
-Derek

Thanks for the review @dww.

Re #80:

1. Yeah I agree using cSpell:ignore
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Fixed
7. We use $extension_array later.
8. The new comment suggested here is incorrect. I've tried to improve the comment.
9. I've changed this to be .php and .txt and that's more correct and explicit.
10. I don't think so - we're asserting form error text.
11. Sure - this is not that irregular in a update test
12. Sure - this is a test so /shrug
13. This is a blocker to adding the sanitise event which in turn is step along the way to centralising all of this logic.
14. Fixed... fwiw

Seems this needs a change record, especially since it can impact JSONAPI/REST clients.

@dww what do you think a change record needs to say? We've got an update function and allowing insecure extensions is super super uncommon.

Thanks for the fixes and improvements! Almost there. A few final minor nits:

Not going to NW over these, since other reviews are still welcome and none of these should block RTBC:

1. +++ b/core/modules/file/file.module
   @@ -1000,10 +1002,12 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +      // If there is no file extension validation at all, or allows the 'txt'
   

   a) "or allows" here is still clumsy. "..., or the site allows..."?

   b) should we use "... allows the .txt extension" for consistency with the rest of the patch?

2. +++ b/core/modules/file/file.post_update.php
   @@ -0,0 +1,41 @@
   +        // If any insecure extension is allowed, add the 'txt' extension.
   

   s/'txt'/.txt/ ?

3. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -225,14 +225,25 @@ public static function validateDirectory($element, FormStateInterface $form_stat
   +      // If insecure uploads are not allowed and txt is not in the list of
   +      // allowed extensions, ensure that no insecure extensions are allowed.
   

   .txt

   Otherwise, 👍. This is more clear than before, and more correct than what I proposed. 😉 Thanks!

4. +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
   @@ -470,6 +470,39 @@ public function testTemporaryFileRemovalExploitAnonymous() {
   +    // By default allowing .php files without txt is not permitted.
   

   Hyper nit: .txt

5. +++ b/core/modules/file/tests/src/Functional/Update/FileFieldFileExtensionsUpdateTest.php
   @@ -0,0 +1,112 @@
   +
   

   If we're rerolling, maybe remove this newline?

6. +++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
   @@ -698,6 +696,18 @@ public function testFileUploadMaliciousExtension() {
   +    // Add php as an allowed format without txt. Allow insecure uploads still
   
   +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
   @@ -523,11 +523,9 @@ public function testFileUploadMaliciousExtension() {
   +    $this->field->setSetting('file_extensions', 'php txt')->save();
   
   @@ -594,6 +592,18 @@ public function testFileUploadMaliciousExtension() {
   +    // Add php as an allowed format without txt. Allow insecure uploads still
   

   .php and .txt here?

Re: CR, here's what I had in mind: https://www.drupal.org/node/3194107
Needs version info filled in, and to be published (if we agree it's worth keeping).

Thanks again!
-Derek

@dww thanks for the review.
1. The site might have some form element that do allow and some that don't ... the site is not really a context we can talk about. It feels funny to be re-writing this so much. It's all going to be removed in #3032390: Add an event to sanitize filenames during upload - anyhow re-wrote it again. Re... 'txt' vs .txt /shrug.
2. Sure
3. Here I think this is correct the list does not have a . in it.
4. Fixed
5. That's against coding standards...

------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------
 110 | ERROR | [x] Expected 1 blank line after function; 0 found
 111 | ERROR | [x] The closing brace for the class must have an empty line before it
------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------

6. Re-worked the entire thing so it is easier to read.

Re the CR

and the site decides to not use the new configuration.

If someone updates a field and tries to remove the added txt extension they will get an error because the insecure extension will be in the list. Still not really getting the point of a CR. Once the update has occurred a site's behaviour doesn't change - unless they try to remove the txt extension from the list - but given they were allowing an insecure extension they were already getting .txt files so why would you do that and how does the CR help more than the form error?

Thanks, @alexpott!

Sorry for being this pedantic. I've just had so many issues held up for similarly trivial pedantic nitpicks over comments that I thought that's what we "need" for something to be committed to core, and that I'm trying to find all the "problems" before this is RTBC.

1. 👍
5. Sorry, dreditor gave crappy context. That wasn't the newline I meant. Doesn't matter.
6. 👍

Re: CR: I don't know. Again, I was err'ing on the side of "caution" and providing one. I think it doesn't hurt to call out this behavior change with a CR. But you're probably right that it doesn't really matter and folks will find out via the admin UI. It's true that behavior won't change once the DB update runs. It's also true that sites can change their file config at any time and potentially "break" JSONAPI clients that used to be able to upload files that are now rejected.

Anyway, thanks again for taking care of everything, and sorry for spending energy on relatively pointless comment cleanup. To that end, you left a double-word in #83:

+++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
@@ -594,6 +592,18 @@ public function testFileUploadMaliciousExtension() {
+    // Add .php as an allowed extension without .txt. Since insecure uploads are
+    // are not allowed, .php files will be rejected.

'are' repeated.

Seems cruel to NW for that. So I'm just fixing it here and self-RTBC'ing. Hopefully I won't be struck down by lightning for breaking the rules. 😉

At least it's a comment in a test that will survive the rewrite from #3032390: Add an event to sanitize filenames during upload, so it's worth fixing.

Adding review/discussion credits

💬 Fixed on commit:

diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index b4a48d7402..925facef01 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -1004,7 +1004,7 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
     if (preg_match(FILE_INSECURE_EXTENSION_REGEX, $file->getFilename())) {
       // If there is no file extension validation at all, or .txt is considered
       // a valid extension and the file would otherwise pass validation, rename
-      // it. If the file will be rejected due extension validation, it should
+      // it. If the file will be rejected due to extension validation, it should
       // not be renamed; rather, let file_validate_extensions() reject it below.
       if (!isset($validators['file_validate_extensions']) || (preg_match('/\btxt\b/', $extensions) && empty(file_validate_extensions($file, $extensions)))) {
         $file->setMimeType('text/plain');

✅ Committed fd1d675 and pushed to 9.2.x. Thanks!

⏬ Because of the update hook, I think this would be minor eligible only, as much as I'd like to backport this to 9.1 for the sake of enhanced security.

➡️ Unpostponed the blocked #3032390: Add an event to sanitize filenames during upload
➡️ Published the change-record
➡️ Tagged for release notes and confirmed there's a release note snippet

🎶 This commit brought to you by Viet Nam by Minutemen, Pinch by CAN and Whatcha Drinkin' by Hüsker Dü