Add an event to sanitize filenames during upload

+++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
@@ -374,7 +392,7 @@ protected function validateAndParseContentDispositionHeader(Request $request) {
     // Make sure only the filename component is returned. Path information is
     // stripped as per https://tools.ietf.org/html/rfc6266#section-4.3.
-    return basename($filename);
+    return $this->fileSystem->basename($filename);

@@ -458,29 +476,14 @@ protected function validate(FileInterface $file, array $validators) {
+    $event = new FileUploadSanitizeNameEvent($filename, $extensions);

Does this means we need to add explicit test coverage to \Drupal\Tests\file\Functional\FileUploadJsonBasicAuthTest for this?

Or are we at a point where we don't want to add explicit REST test coverage because we're making that convergence happen?

Nice to see lots of my code from #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) has survived, and thanks for all the improvements! Found lots of things, mostly nits, a few are (slightly) more substantial.

1. +++ b/core/includes/file.inc
   @@ -702,8 +702,14 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
   + *   a \Drupal\Core\File\Event\FileEvents::SANITIZE event instead.
   

   Shouldn't this say \Drupal\Core\File\Event\FileUploadSanitizeNameEvent instead?

2. +++ b/core/includes/file.inc
   @@ -749,8 +755,14 @@ function file_munge_filename($filename, $extensions, $alerts = TRUE) {
   + * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use
   + *   str_replace() instead.
   

   Hah! Makes you wonder why this function exists. ;)

3. +++ b/core/lib/Drupal/Core/File/Event/ExtensionLockException.php
   @@ -0,0 +1,13 @@
   + * @see \Drupal\Core\File\Event\FileUploadSanitizeNameEvent::setExtension
   

   Nit: ::setExtension() (with parens)

4. +++ b/core/lib/Drupal/Core/File/Event/FileEvents.php
   @@ -0,0 +1,22 @@
   +final class FileEvents {
   +
   +  /**
   +   * Name of the event fired when uploading a file to sanitize the filename.
   +   *
   +   * @Event
   +   *
   +   * @see \_file_save_upload_single()
   +   * @see \Drupal\Core\File\Event\FileUploadSanitizeNameEvent
   +   *
   +   * @var string
   +   */
   +  const SANITIZE = 'file.upload.sanitize.name';
   +
   +}
   

   Why are we doing this? I thought you were supposed to have the name constant live with the event. This seems weird.

5. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,199 @@
   + * PATHINFO_FILENAME). The original file extension is passed as context, and can
   + * only be modified before if the event subscriber has a priority higher then
   + * 1024.
   

   This wording is now clumsy. "can only be modified before if ..." - huh? Also s/then/than/

   Maybe: "The file extension can be modified via self::setExtension(), but only before self::lockExtension() is called. Core locks the extension (for security reasons) with an event subscriber using priority 1024, so if you must alter the extension, your subscriber must have a higher priority than that."

   Or something. This whole thing seems a bit awkward and clumsy. I don't love the new exception being thrown and all the additional complication for this. *shrug*

6. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,199 @@
   + * @see \Drupal\Core\File\FileSystemInterface::basename()
   

   We no longer use FileSystemInterface::basename() in here. Can probably remove this @see.

7. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,199 @@
   +   * The (read-only) extension of the file being uploaded.
   

   Not really "read-only" anymore. Stale comment from my previous version of this event.

8. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,199 @@
   +  /**
   +   * A list of allowed extensions.
   +   *
   +   * @var string
   +   */
   +  protected $allowedExtensions;
   

   I'm kinda sad that the new version of this event now cares about this.

   And now that I've made it to the end of this event, I don't see it actually being used anywhere, other than to update it in makeTextFile(). What gives? ;) Why is this here?

9. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,199 @@
   +  protected $securityRename = FALSE;
   

   maybe $isSecurityRename to self-document it's a bool?

10. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
    @@ -0,0 +1,199 @@
    +  protected $extensionLock = FALSE;
    

    $extensionLocked ?

11. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
    @@ -0,0 +1,199 @@
    +   * Constructs a file upload event object.
    

    "... file upload sanitize name event object."

12. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
    @@ -0,0 +1,199 @@
    +   *   The filename to use for the uploaded file.
    

    Maybe: "The filename (without extension) to use..." ?

13. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
    @@ -0,0 +1,199 @@
    +  /**
    +   * Converts the file to use the .txt extension.
    +   *
    +   * @return $this
    +   */
    +  public function makeTextFile() {
    +    $this->filename = $this->getFilenameWithExtension();
    +    // Empty means everything is allowed.
    +    if ($this->allowedExtensions) {
    +      $this->allowedExtensions .= ' txt';
    +    }
    +    $this->extension = 'txt';
    +    return $this;
    +  }
    

    foo.php.txt is okay? don't we need to make it foo._php.txt or something? Not totally clear why this function is here, frankly.

14. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
    @@ -0,0 +1,199 @@
    +  public function setSecurityRename() {
    

    Does this want to take a bool and set the flag to whatever we tell it? Everything else in here called "set" lets you set what you need. Seems a bit inconsistent that this particular one forces the flag to TRUE.

15. +++ b/core/modules/file/file.module
    @@ -1015,12 +995,34 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
    +  if (isset($validators['file_validate_extensions'][0])) {
    +    // @see \Drupal\Core\File\Event\FileUploadSanitizeNameEvent::makeTextFile()
    +    $validators['file_validate_extensions'][0] = $event->getAllowedExtensions();
    +  }
    

    Weird. :/ This seems fairly convoluted. I don't have a better proposal just yet, but spidey-sense says yuck. ;)

16. +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
    @@ -221,7 +222,7 @@ public function testHandleDangerousFile() {
    -    $this->assertRaw(t('File name is @filename', ['@filename' => $this->phpfile->filename]), 'Dangerous file was not renamed when insecure uploads is TRUE.');
    +    $this->assertSession()->pageTextContains('File name is php-2.php.');
    

    can we pass a message to pageTextContains()? Would love to keep the more helpful message from the assertRaw() we're removing.

17. +++ b/core/modules/system/tests/src/Unit/Event/FileEventSubscriberTest.php
    @@ -0,0 +1,79 @@
    +    // running security event the extension is locked.
    

    // running FileEventSubscriber::security() on the event that the extension is locked.

18. +++ b/core/modules/system/tests/src/Unit/Event/FileEventSubscriberTest.php
    @@ -0,0 +1,79 @@
    +   * Provides data for testSanitizeFilename().
    

    s/testSanitizeFilename/testSecurity/

19. +++ b/core/modules/system/tests/src/Unit/Event/FileEventSubscriberTest.php
    @@ -0,0 +1,79 @@
    +   *   insecure uploads ane allowed and the expected security rename flag.
    

    s/ane/are/

20. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,107 @@
    +   *   Arrays with original name, expected name, and sanitization options.
    

    s/sanitization options/expected extensions (as a space-delimited string)/

21. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,107 @@
    +      'Unicode filename with lots of dots' => [
    +        'dotty.....txt',
    +        'dotty....',
    +        'txt',
    +      ],
    

    Doesn't look like Unicode to me. Should this move up to the ASCII section above and get a better name?

This fixes the trivial / obvious nits:
1-3
6,7
9-12 (for 10, I also renamed the exception class to ExtensionLockedException to match).
17-21

@TODO from #9:
4-5
8
13-16

Still NW, but a bit closer. ;)

Cheers,
-Derek

p.s. Fixed the API changes section of the summary and some other updates/fixes. Also, the summary mentions language, but that's not addressed anywhere in the patch.

p.p.s. Queued for testing, even though it's NW, to make sure I didn't accidentally break something. ;)

Thanks for the new patch and the replies.
#11.4: cool, duly noted.

11.5: cool. ;)

11.8: Okay, fair enough.

11.13: I also notice the existing functionality (removed from _file_save_upload_single()) does this: $file->setMimeType('text/plain');. Not totally clear how that happens in the new flow. I guess we assume this gets it right:
+ $file->setMimeType(\Drupal::service('file.mime_type.guesser')->guess($file->destination));

11.14: Ok.

11.15: Ok, I can live with it for now.

11.16: Fair enough. Yes, I agree the filename in the message is good. I just meant it'd be nice for the assert message to say the functionality that fails, if possible. But if not, I'm okay with this as-is. Totally agree on the fullstop and stopping false positives, good catch!

Review of patch #11:

1. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,189 @@
   +  /**
   +   * Gets the full filename with extension (if any).
   +   *
   +   * @return string
   +   *   The full filename to use.
   +   */
   +  public function getFilenameWithExtension() {
   +    return $this->filename . ($this->extension !== '' ? ('.' . $this->extension) : '');
   +  }
   

   Possible can of worms: Now that we're letting any event subscriber munge the extension, I wonder if all this complication is needed anymore. Originally, we split extension from filename to ensure that subscribers couldn't touch the extension. Now, subscribers munge the extension by design. Maybe we should just have (get|set)Filename() and it operates on the full name, and if you need to munge it, you do whatever you want.

2. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,189 @@
   +  /**
   +   * Converts the file to use the .txt extension.
   +   *
   +   * @return $this
   +   */
   +  public function makeTextFile() {
   +    $this->filename = $this->getFilenameWithExtension();
   +    // Empty means everything is allowed.
   +    if ($this->allowedExtensions) {
   +      $this->allowedExtensions .= ' txt';
   +    }
   +    $this->extension = 'txt';
   +    return $this;
   +  }
   

   You removed the place in FileEventSubscriber::security() that calls this, and the test coverage, but the function is still here. Seems we should remove it entirely now.

3. +++ b/core/modules/file/file.module
   @@ -1015,12 +995,34 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +    // @see \Drupal\Core\File\Event\FileUploadSanitizeNameEvent::makeTextFile()
   

   stale @see

4. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -458,29 +476,14 @@ protected function validate(FileInterface $file, array $validators) {
   +      // @see \Drupal\Core\File\Event\FileUploadSanitizeNameEvent::makeTextFile()
   

   Another stale @see

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockPrivateFilesTest.php
   --- /dev/null
   +++ b/core/modules/system/src/EventSubscriber/FileEventSubscriber.php
   

   I asked in Slack about this, but for the record... it seemed weird to me that this event listener lives in system. @alexpott replied:

   It’s because file module is not required but this is file security functionality that other can leverage. i.e it’s replacing file_munge_filename

   Still slightly weird, but okay. I guess in theory you could have rest.module enabled without file.module (?), and you'd still definitely need this subscriber to fire. Perhaps we should add a comment about this to the PHPdoc for this class?

6. +++ b/core/modules/system/src/EventSubscriber/FileEventSubscriber.php
   @@ -0,0 +1,108 @@
   +    $filename = str_replace(chr(0), '', $event->getFilename());
   +    $extension = str_replace(chr(0), '', $event->getExtension());
   

   Yeah, see, we'd only have to do a single str_replace() here if we had a single filename instead of name vs. extension.

7. +++ b/core/modules/system/tests/src/Unit/Event/FileEventSubscriberTest.php
   @@ -0,0 +1,73 @@
   +   *   Whether insecure uploads are allowed/
   

   Nit: allowed.

Thanks again!
-Derek

I think splitting this issue off and getting the security stuff into the event is proving its worth.

So true. The other issue was all to convoluted, so this way we'll get it all done one after the other. :)

I also want to say sorry to @alexpott that in the other issue I overreacted quite a bit. Think I was busy and invested more time with that issue than at that moment I could afford, but that's my problem not yours. Peace?

So I think the big issue is whether we want to address the odd auto-allow of .txt files in a separate issue that blocks this one.

Yep. I think #2863652: 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 needs to be fixed first.

Re: #10.ps I see you mentioned that in #2. I just did some digging into file.module. I can't see anything that's doing anything fancy with a language provided with the upload. REST doesn't do that, either. It seems in all cases, we end up just getting the default language. So yeah, I don't think the transliterate case cares, and we don't have to mess with that in this event. Removed that from the summary.

Meanwhile, re: #14: Yay. :) Agreed.

Re #15: almost there. ;) Sorry I missed some of these earlier. Some are new because of recent changes.

1. +++ b/core/lib/Drupal/Core/File/Event/FileEvents.php
   @@ -0,0 +1,22 @@
   +  const SANITIZE = 'file.upload.sanitize.name';
   

   Now that this is in a generic FileEvents class, probably SANITIZE isn't enough. Perhaps at least SANITIZE_NAME or something would be better.

2. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,119 @@
   +    // @todo should we do something to ensure that this is a basename?
   

   Good question. What do you propose? It seems we don't want this event also responsible for changing destinations, so yeah, probably a good idea to make sure there's no path info in here. We could potentially strip it for them, or perhaps throw an Exception if a full path is passed in?

3. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,119 @@
   +  /**
   +   * Gets the filename (without extension).
   +   *
   +   * @return string
   +   *   The filename (without extension) to use.
   

   s/(without extension)//

4. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,119 @@
   +   *   The filename (without extension) to use for the uploaded file.
   

   same

5. +++ b/core/modules/file/file.module
   @@ -1015,12 +995,34 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  $event_dispatcher->dispatch(FileEvents::SANITIZE, $event);
   

   Yeah, at this call site, it's not totally self-evident what's being sanitized. The class name a few lines above is good, but if I missed that, I might get the wrong idea here...

6. +++ b/core/modules/file/file.module
   @@ -1015,12 +995,34 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +    'filename' => $file_info->getClientOriginalName(),
   

   we already have this as $original_filename -- shall we reuse that instead of calling getClientOriginalName() again?

7. +++ b/core/modules/file/file.module
   @@ -1059,6 +1061,21 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  // Update the filename with any changes as a result of the FileUploadEvent
   +  // (sanitization) or renaming due to an existing file.
   

   s/FileUploadEvent (sanitization)/FileUploadSanitizeNameEvent/

8. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -164,6 +176,11 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +      @trigger_error('The event_dispatcher service must be passed to FileUploadResource::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2972665.', E_USER_DEPRECATED);
   

   Presumably we need to link this to https://www.drupal.org/node/3032541 now, not 2972665.

9. +++ b/core/modules/system/src/EventSubscriber/FileEventSubscriber.php
   @@ -0,0 +1,107 @@
   +  public function security(FileUploadSanitizeNameEvent $event) {
   

   I'm not sure I love "security" as a function name here. ;) Don't we at least want a verb? enforceFilenameSecuritySettings() or something?

10. +++ b/core/modules/system/src/EventSubscriber/FileEventSubscriber.php
    @@ -0,0 +1,107 @@
    +    // Allow potentially insecure uploads for very savvy users and admin
    

    s/and admin/and admins./

11. +++ b/core/modules/system/tests/src/Unit/Event/FileEventSubscriberTest.php
    @@ -0,0 +1,73 @@
    +      // @todo this is an existing logic bug that is harder to reveal.
    +      ['foo.php.php', 'jpg', 'foo.php_.php.txt', FALSE, TRUE],
    

    Do we want a follow-up for this?

12. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,75 @@
    +  /**
    +   * Test event construction.
    +   *
    +   * @dataProvider provideFilenames
    +   * @covers ::__construct
    +   * @covers ::getFilename
    +   */
    +  public function testEventFilenameFunctions($filename) {
    +    $event = new FileUploadSanitizeNameEvent($filename, '');
    +    $this->assertSame($filename, $event->getFilename());
    +  }
    

    For completeness of this specific unit test, shouldn't we also try to create a 2nd event with some allowed extensions and see what happens?

13. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,75 @@
    +  /**
    +   * Provides data for testEvent().
    +   *
    +   * @return array
    +   *   Arrays with original name, expected name and allowed extensions (as a
    +   *   space-delimited string).
    +   */
    +  public function provideFilenames() {
    

    Hrm, now I'm a bit confused how this is passing at all. This claims it provides data for testEvent(), but that function no longer exists. It's marked as the provider for testEventFilenameFunctions(), but the function signature of that doesn't match what this provider is returning. WTF? ;) I think this provider needs help.

@Pancho re: #17: Yay! Nice to have you back. :)

1.

+    // Remove any null bytes. See
+    // http://php.net/manual/security.filesystem.nullbytes.php

@see ?

2.

+    // Build a default non-munged safe list for
+    // \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::security().

+ // @see \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::security()
2x "sanitizeName()" ?

3.

+  /**
+   * Tests file name sanitization.
+   *
+   * @param string $original
+   *   The original filename.
+   * @param string $extensions
+   *   The allowed extensions.
+   * @param string $expected
+   *   The expected filename.
+   * @param bool $allow_insecure_uploads
+   *   Whether insecure uploads are allowed.
+   * @param bool $expected_security_rename
+   *   The expected security rename flag.
+   *
+   * @dataProvider provideFilenames
+   *
+   * @covers ::sanitizeName
+   */
+  public function testSecurity($original, $extensions, $expected, $allow_insecure_uploads, $expected_security_rename) {

a.)
Just a test, but how about having the three input parameters first and the two test parameters last?

b.)
Furthermore a bit more verbose and consistent:

$original => $original_name
$extensions => $allowed_extensions
$expected => $expected_name

c.)
And for a flag, this is quite confusing. Propose renaming, too:
$expected_security_rename => $expect_security_rename

d.)
public function testSecurity(...)
testSanitizeFilename(...) ?

4.

+      // @todo this is an existing logic bug that is harder to reveal.
+      ['foo.php.php', 'jpg', 'foo.php_.php.txt', FALSE, TRUE],

Question: you are referring to this being renamed to 'php_.php.txt', so this is not about #2863652: 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, right?

1. No, @see must only be used as a standalone reference to other things at the bottom of a docblock, inline is "See" correct.

Re: #20:

2. Good catch.

3. Agreed on all 4 subpoints.

4. Right. See #9.13: I found it weird that the final "rename *.php to *.php.txt" doesn't use the same logic as file_munge_filename() does. Seems like if we rename "foo.php.bar" to "foo.php_.bar" (from filename munging) that if we're renaming "foo.php" to be "more secure" we should use the same logic and rename it to "foo.php_.txt", not just "foo.php.txt". This is a separate bug from the #2863652: 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 weirdness. AFAIK, there's no open issue about this particular inconsistency.

1.) Further nitpicks in random order:

a.)
+ * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Dispatch
81 chars.

b.)

+    // Munge the filename to protect against possible malicious extension hiding
+    // within an unknown file type (ie: filename.html.foo).

81 chars + "i.e."

c.)

+      // Split the filename up by periods. The first part becomes the basename
+      // the last part the final extension.

=> "Split up the filename", also comma missing after basename.

d.)

+   * @return array
+   *   Arrays with original name, allowed extensions, expected name, whether
+   *   insecure uploads are allowed and the expected security rename flag.

Should match the new parameter order:

+   * @return array
+   *   Arrays with original name, allowed extensions, whether insecure uploads
+   *   are allowed, expected name and the expected security rename flag.

e.)

+      // Test to prove adding .txt does not munge the dangerous extension it
+      // replaces. This occurs because Apache always recognises the txt
+      // extension.

May be slightly shortened:

+      // Test to prove adding .txt does not munge the dangerous extension it
+      // replaces. This is because Apache always recognises the .txt extension.

2.)
Another considerably bigger aspect, even if possibly out of scope here:
+ const INSECURE_EXTENSION_REGEX = '/\.(phar|php|pl|py|cgi|asp|js)(\.|$)/i';

This is nice, however what about all other extensions considered potentially sensitive by our default .htaccess, which are therefore just as well disallowed in a default install:

# Protect files and directories from prying eyes.
<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock))$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$">
  <IfModule mod_authz_core.c>
    Require all denied
  </IfModule>

Tested this with a *.module file which didn't end up being security renamed, but not visible either.

3.)

+      // Loop through the middle parts of the name and add an underscore to the
+      // end of each section that could be a file extension but isn't in the
+      // list of allowed extensions.
+      foreach ($filename_parts as $filename_part) {
+        $filename .= '.' . $filename_part;
+        if (!in_array(strtolower($filename_part), $whitelist) && preg_match("/^[a-zA-Z]{2,5}\d?$/", $filename_part)) {
+          $filename .= '_';
+        }
+      }

If adding underscores is necessary (still not sure why), then matching possible extensions with 2 to 5 characters might not be enough. Some are longer. Propose we figure out (and document) why this is necessary, what exactly is to be avoided, and then figure out which possible extensions we want to match.

Re: #24.1: Sure, all seems fine.

#24.2: A. Out of scope. ;) B. I disagree. The .htaccess stuff is preventing those files from being seen. This dangerous extension renaming stuff is to prevent code execution. foo.module isn't executable by apache, so it doesn't need to be renamed to foo.module_. Why our .htaccess is preventing all *.module files (to stick with this example) from being seen, not just the ones provided by core, is a separate issue.

#24.3: git show 5524445b ;) @see SA-2006-006.

Sadly, that was back in the days of security-by-obscurity in the commit message, so Gerhard (@killes) just said "fixing stuff" (on the dubious assumption that hackers would be less interested in the commit and less quick to write exploits). There are some code comments, but not too much help. But in short, if you upload foo.php.bar and Apache doesn't know what to do with ".bar" files, it will fall back and treat that as a .php file, and happily execute it for you. I just tried this again. Much to my disbelief, this Apache behavior hasn't changed in 13+ years and it's still executing the PHP code, not just displaying it. :(

TL;DR: We do not want to change this behavior in any way. This issue is simply moving around the code. If you change how it works, you risk re-introducing an arbitrary code execution vulnerability.

Thanks,
-Derek

This means 2. is out of scope, while 3. works as designed. So we're just fixing the few nitpicks and might be done? Nice.

Fixed #24/1 (a-e).

Also tried to document the Apache issue in a few words, so it gets a bit clearer why we implemented that and how to find a tiny bit of further information about it.

-    // Munge the filename to protect against possible malicious extension hiding
-    // within an unknown file type (ie: filename.html.foo).
+    // Munge the filename to protect against possible malicious extension
+    // hiding within an unknown file type (i.e. filename.html.foo).
+    // This was introduced as part of SA-2006-006 to fix Apache's risky
+    // fallback behaviour.

Certainly you'll find a better wording, possibly a better reference.

We do not want to change this behavior in any way.

Totally agree now. In this case, we might want to move these monolithic lines of code to a separate internal function, so it is a bit separated from the rest of sanitizeName().

I'm running out of things to complain about in here. ;) But I found one:

+++ b/core/includes/file.inc
@@ -702,8 +702,14 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
+  @trigger_error('file_munge_filename() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Dispatch a \Drupal\Core\File\Event\FileEvents::SANITIZE event instead. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED);

#9.1 again: s/FileEvents::SANITIZE/FileUploadSanitizeNameEvent/

But, makes me wonder: what if I just want to "munge" a filename, but not necessarily be subjected to lowercasing, transliteration, whitespace replacement, and any other thing contrib might dream up that could happen if I dispatch this event? Am I a sad panda? Perhaps this isn't a fair deprecation. Perhaps the replacement for file_munge_filename() should continue to live as a stand-alone function somewhere (along the lines of @Pancho's concern at the end of #27)? If so, perhaps the FileSystem service makes more sense than a public static method of system's event listener class?

Meanwhile, if this is truly blocked on those other two issues, then let's call this postponed...

Thanks,
-Derek

1.)

Am I a sad panda?

You shouldn't be... :)

If so, perhaps the FileSystem service makes more sense than a public static method of system's event listener class

Yep, that's even better.

2.)
Do I get it wrong or is both removing Null bytes and munging the filename only necessary if we're not adding a .txt extension, i.e. if after all our sanitizing it isn't a SecurityRename? Why are we doing both in a row? And wouldn't we want to remove Null bytes and munge even if 'allow_insecure_uploads' is TRUE, so while we're helping the panda get his otherwise insecure uploads we'd still be avoiding totally unexpected behaviour?

3.)
I found that the PHP Null byte bug has been fixed for file_exists() in PHP 5.3, see http://php.net/manual/en/security.filesystem.nullbytes.php and https://bugs.php.net/bug.php?id=39863. While it remains for include/require, this shouldn't be relevant here. In that case we might be able to get rid of it altogether in another followup.

Don't think we should change any of this in this issue, we should keep it in mind when reorganizing the actual code to the FileSystem service.

=>
So for now, prepareFilename() should probably basically keep doing what _file_save_upload_single did, except it also dispatches the new event first:

1. $this->eventDispatcher->dispatch(FileEvents::SANITIZE_NAME, $event);
2. remove Null Bytes
3. if (!allow_insecure_uploads)
   \Drupal\Core\File\FileSystem::mungeFilename()
4. if (!allow_insecure_uploads)
   \Drupal\Core\File\FileSystem::ensureNotExecutable()

So everything is in place and in a followup we can rethink the logic. Also, if the event subscribers already alter the filename in a way that ensures nothing needs to be munged or ensuredNotExecutable, then we're done. And the other way around, if the event subscribers should possibly do something that makes removing Null Bytes, munging or ensuringNotExecutable necessary, while insecure uploads are still not wished, then it's not to late to do so.

Re: moving the security related filename changes out of the event; doing file name munging on your own
Mhm, well, nevermind.

So for example pointing to a .htaccess and saying this is not necessary anymore does not feel correct.

Just to make sure I'm not strawmanned: Nowhere did I say the additional line of defence wasn't necessary anymore. I just pointed to the inconsistency and the fact that we're allowing files being uploaded that in the end aren't allowed to be displayed by our default .htaccess directive.

However, we agreed that this is way out of scope and might be a followup.

For example the null byte removal doesn't just make sense on security grounds it also makes sense on a usability perspective.

Out of scope here. Let's discuss in a followup.

#7 + #8: awesome, thanks a lot for #3032620: \Drupal\file\Plugin\rest\resource\FileUploadResource uses basename() when it needs to use the Drupal version!
#9.5 + #11.5: yep, this is Symfony's event system not being designed for pluggable/extensible systems like Drupal. Explicit before and after relationships to denote ordering instead of randomly picked numbers would solve this. But alas, we can't solve that here.
#16: kudos for creating #2863652: 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 too 👏
#17: ❤️

1. +++ b/core/includes/file.inc
   @@ -702,8 +702,14 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
   +  @trigger_error('file_munge_filename() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Dispatch a \Drupal\Core\File\Event\FileEvents::SANITIZE event instead. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED);
   

   🐛 s/SANITIZE/SANITIZE_NAME/

2. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,119 @@
   +   * The filename being uploaded.
   

   Nit: The name of the file being uploaded., because you can't upload a filename :)

3. +++ b/core/modules/file/file.module
   @@ -949,17 +955,9 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  // @todo it'd be great to not have this here.
   

   🤔 What's the plan for this? Do we really want to tackle this in this issue?

4. +++ b/core/modules/file/file.module
   @@ -975,34 +973,13 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   -  // Rename potentially executable files, to help prevent exploits (i.e. will
   -  // rename filename.php.foo and filename.php to filename.php.foo.txt and
   -  // 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(FILE_INSECURE_EXTENSION_REGEX, $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::messenger()->addStatus(t('For security reasons, your upload has been renamed to %filename.', ['%filename' => $file->getFilename()]));
   -    }
   -  }
   
   +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -458,29 +476,14 @@ protected function validate(FileInterface $file, array $validators) {
   -    // Rename potentially executable files, to help prevent exploits (i.e. will
   -    // rename filename.php.foo and filename.php to filename.php.foo.txt and
   -    // filename.php.txt, respectively). Don't rename if 'allow_insecure_uploads'
   -    // evaluates to TRUE.
   -    if (!$this->systemFileConfig->get('allow_insecure_uploads') && preg_match(FILE_INSECURE_EXTENSION_REGEX, $filename) && (substr($filename, -4) != '.txt')) {
   -      // The destination filename will also later be used to create the URI.
   -      $filename .= '.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($validators['file_validate_extensions'][0])) {
   -        $validators['file_validate_extensions'][0] .= ' txt';
   -      }
   -    }
   

   👍 This is removed in both code paths (REST and non-REST) in favor of the event. Great! Convergence!

5. +++ b/core/modules/file/file.module
   @@ -1015,12 +992,37 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  $values = [
   +    'uid' => $user->id(),
   +    'status' => 0,
   +    // This will be replaced later with a filename based on the destination.
   +    'filename' => $original_filename,
   +    'uri' => $file_info->getRealPath(),
   +    'filesize' => $file_info->getSize(),
   +  ];
   

   🤔 Wouldn't it be better to use the setters on FileInterface instead?

6. +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
   @@ -0,0 +1,105 @@
   +/**
   + * Class SecurityFileUploadEventSubscriber.
   + */
   

   Nit: still needs meaningful description.

7. +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
   @@ -0,0 +1,105 @@
   +   * Constructs a new file event listener.
   

   This too.

8. +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
   @@ -0,0 +1,105 @@
   +      $filename .= '.txt';
   +      $event
   +        ->setFilename($filename)
   

   Nit: why not ->setFileName($filename . '.txt')?

9. +++ b/core/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php
   @@ -0,0 +1,75 @@
   +      ['foo.txt', '', FALSE, 'foo.txt', FALSE],
   +      ['foo.txt', '', TRUE, 'foo.txt', FALSE],
   +      ['foo.php', '', FALSE, 'foo.php.txt', TRUE],
   +      ['foo.php', '', TRUE, 'foo.php', FALSE],
   +      ['foo.phar.png.php.jpg', 'jpg png', FALSE, 'foo.phar_.png.php_.jpg', TRUE],
   +      ['foo.phar.png.php.jpg', 'jpg png', TRUE, 'foo.phar.png.php.jpg', FALSE],
   +      // Test to prove adding .txt does not munge the dangerous extension it
   +      // replaces. This is because Apache always recognises the .txt extension.
   +      ['foo.php.php', 'jpg', FALSE, 'foo.php_.php.txt', TRUE],
   +      ['foo' . chr(0) . '.txt' . chr(0), '', FALSE, 'foo.txt', TRUE],
   +      ['foo' . chr(0) . '.txt' . chr(0), '', TRUE, 'foo' . chr(0) . '.txt' . chr(0), FALSE],
   

   Would be good to have non-auto-numerical array keys here that provide useful descriptions. Makes it much easier for future maintainers (and current reviewers, like me) to understand what exactly these permutations are doing.

10. +++ b/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php
    @@ -33,6 +34,8 @@ protected function setUp() {
    +   * @expectedDeprecation file_munge_filename() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Dispatch a \Drupal\Core\File\Event\FileEvents::SANITIZE event instead. See https://www.drupal.org/node/3032541
    

    Same as the first remark.

11. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,82 @@
    +      'ASCII filename with extension' => [
    +        'example.txt',
    +      ],
    +      'ASCII filename with complex extension' => [
    +        'example.html.twig',
    +      ],
    +      'ASCII filename with lots of dots' => [
    +        'dotty.....txt',
    +      ],
    +      'Unicode filename with extension' => [
    +        'Ä Ö Ü Å Ø äöüåøhello.txt',
    +      ],
    +      'Unicode filename without extension' => [
    +        'Ä Ö Ü Å Ø äöüåøhello',
    +      ],
    

    👍 Great test case labels here :)

Agree with almost all of #36.

However, for #36.8:

Nit: why not ->setFileName($filename . '.txt')?

Because the whole rest of this function is operating on and building up $filename. Arguably, this part:

+    if ($filename !== $event->getFilename()) {
+      $event->setFilename($filename)->setSecurityRename();
+    }
+
+    // Rename potentially executable files, to help prevent exploits (i.e. will
+    // rename filename.php.foo and filename.php to filename.php.foo.txt and
+    // filename.php.txt, respectively).
+    if (preg_match(static::INSECURE_EXTENSION_REGEX, $filename) && substr($filename, -4) !== '.txt') {
+      $filename .= '.txt';
+      $event
+        ->setFilename($filename)
+        ->addAllowedExtension('txt')
+        ->setSecurityRename();
+    }
+  }

should/could be simplified to:

  // Rename potentially executable files, to help prevent exploits (i.e. will
  // rename filename.php.foo and filename.php to filename.php.foo.txt and
  // filename.php.txt, respectively).
  if (preg_match(static::INSECURE_EXTENSION_REGEX, $filename) && substr($filename, -4) !== '.txt') {
    $event->addAllowedExtension('txt');  // Evil: remove when #2863652 is done.
    $filename .= '.txt';
  }
  if ($filename !== $event->getFilename()) {
    $event->setFilename($filename)->setSecurityRename();
  }

No need to call setFilename() and setSecurityRename() twice. Might as well put the setFilename() at the very end of the function, to cover all possible code paths.

p.s. re: #36.1 and .10: I prefer #9.1 and #31. Don't reference the FileEvents::SANITIZE_NAME constant (which isn't an event you can dispatch, right?). Reference the class of the event you need to use: \Drupal\Core\File\Event\FileUploadSanitizeNameEvent.

p.p.s. That makes me wonder, I don't see SANITIZE_NAME anywhere in FileUploadSanitizeNameEvent.php. Don't we want that at least in the class's docblock comment? FileEvents does an @see to point to FileUploadSanitizeNameEvent. Seems we want to have a pointer in the other direction, too.

Copying over some tags from #3021652: Deprecate upload-related functions file.inc and move to a file upload service which has been superseded by this issue.

I guess as 2863652 no longer public - it makes sense to proceed

@alexpott Is there an updated patch that could be posted here for others to help with?

1. +++ b/core/includes/file.inc
   @@ -176,8 +176,14 @@ function file_build_uri($path) {
   + *   a \Drupal\Core\File\Event\FileUploadSanitizeNameEvent event instead.
   ...
   +  @trigger_error('file_munge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Dispatch a \Drupal\Core\File\Event\FileEvents::SANITIZE_NAME event instead. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED);
   

   Should be \Drupal\Core\File\Event\FileEvents::SANITIZE_NAME, I guess

2. +++ b/core/includes/file.inc
   @@ -176,8 +176,14 @@ function file_build_uri($path) {
   + * @see https://www.drupal.org/node/3032541
   

   CR needs to be updated

3. +++ b/core/modules/file/file.module
   @@ -1036,22 +995,55 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +    // Use the result of the sanitization event to as the destination name.
   

   Sounds like the 'to' shouldn't be there

Looks much nicer, just a couple of nits and one question

1. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,122 @@
   +    // @todo should we do something to ensure that this is a basename?
   

   yep, I think we should throw an invalid argument exception

2. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,122 @@
   +  public function setFilename(string $filename): self {
   ...
   +  public function addAllowedExtension(string $extension):self {
   ...
   +  public function setSecurityRename():self {
   

   nit: some consistency issues here w.r.t whitespace

3. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,122 @@
   +    $this->filename = $filename;
   

   I guess the todo regarding ensuring this is a basename applies here too

4. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,122 @@
   +      $this->allowedExtensions .= ' ' . $extension;
   ...
   +      $this->allowedExtensions = $extension;
   

   now that we have a value object, should we work with an array of extensions internally?

5. +++ b/core/modules/file/file.module
   @@ -1036,22 +995,55 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +    // Use the result of the sanitization event to as the destination name.
   

   is this 'to' needed? if so, should it be 'too'

6. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -166,6 +178,11 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +      @trigger_error('The event_dispatcher service must be passed to FileUploadResource::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED);
   

   this needs to be updated now

7. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -114,13 +124,17 @@ class TemporaryJsonapiFileFieldUploader {
   +      $event_dispatcher = \Drupal::service('event_dispatcher');
   

   should there be a trigger error here, or is it omitted because internal

8. +++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
   @@ -680,13 +680,13 @@ public function testFileUploadMaliciousExtension() {
   -    $expected = $this->getExpectedDocument(5, 'example_5.php_.png_.txt', TRUE);
   +    $expected = $this->getExpectedDocument(5, 'example_5.php_.png', TRUE);
   ...
   -    $expected['data']['attributes']['filemime'] = 'text/plain';
   +    $expected['data']['attributes']['filemime'] = 'image/png';
   

   why is this changing now?

I had a go at writing the change record.

I think it is worth considering how can we make this event and the file_validate work better together.

If that's the next step, should we work towards the API we would want in the future rather than what we have now?

Yeah 100% this is a huge improvement. I think it's early enough in 9.2 cycle that if we have to refine this as we get to the next steps then there's time to do so

The blocker is in

Fantastic, thanks! Updating title, tags and status to match. Pretty sure NW is correct, to take advantage of #2863652 now being in...

Cheers,
-Derek

Looks great!

1. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,108 @@
   +      throw new \InvalidArgumentException(sprintf('$filename must be a filename with no path information, "%s" provided', $filename));
   

   Do we need to add this to the docblock?

2. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -116,13 +124,17 @@ class TemporaryJsonapiFileFieldUploader {
   +    if (!$event_dispatcher) {
   

   Do we need to trigger a deprecation here?

This is looking fantastic, thanks!

1. +++ b/core/includes/file.inc
   @@ -229,8 +235,14 @@ function file_munge_filename($filename, $extensions, $alerts = TRUE) {
   + *
   + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use
   + *   str_replace() instead.
   + *
   + * @see https://www.drupal.org/node/3032541
     */
    function file_unmunge_filename($filename) {
   +  @trigger_error('file_unmunge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use str_replace() instead. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED);
   

   Should we expand the CR to talk about how to replace file_unmunge_filename() with str_replace(), too?

2. +++ b/core/lib/Drupal/Core/File/Event/FileEvents.php
   @@ -0,0 +1,22 @@
   +   * @see \_file_save_upload_single()
   
   +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,111 @@
   + * @see _file_save_upload_single()
   

   Seems like @see _file... would be the preferred version (based on grepping the rest of core). Probably these should be the same.

3. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,111 @@
   +   *   A list of allowed extensions. If empty all extensions are allowed.
   

   Wants a comma after empty.

4. +++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php
   @@ -0,0 +1,111 @@
   +   * Sets the security rename flag.
   

   "... to TRUE." ?

5. +++ b/core/modules/file/file.api.php
   @@ -5,6 +5,52 @@
   + * @section file_security Uploading files and security considerations
   

   This section / summary is fantastic, thanks for adding it!

6. +++ b/core/modules/file/file.module
   @@ -25,12 +28,15 @@
   + *
   + * @deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Use
   + *   \Drupal\system\EventSubscriber\SecurityFileUploadEventSubscriber::INSECURE_EXTENSION_REGEX.
   + *
   + * @see https://www.drupal.org/node/3032541
     */
   -define('FILE_INSECURE_EXTENSION_REGEX', '/\.(phar|php|pl|py|cgi|asp|js)(\.|$)/i');
   +define('FILE_INSECURE_EXTENSION_REGEX', SecurityFileUploadEventSubscriber::INSECURE_EXTENSION_REGEX);
   

   Should we expand the CR to mention this change, too?

7. +++ b/core/modules/file/file.module
   @@ -1034,22 +996,50 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  $event_dispatcher = \Drupal::service('event_dispatcher');
   +  $event_dispatcher->dispatch($event, FileEvents::SANITIZE_NAME);
   

   Why have $event_dispatcher as a local var instead of just \Drupal::service('event_dispatcher')->dispatch(...); ?

8. +++ b/core/modules/file/file.module
   @@ -1034,22 +996,50 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  // Begin building file entity.
   

   "...building the file..."

9. +++ b/core/modules/file/file.module
   @@ -1034,22 +996,50 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
   +  // If file_destination() returns FALSE then $replace === FILE_EXISTS_ERROR and
   

   There's no file_destination(). The original "If the destination is FALSE..." seems better for this.

10. +++ b/core/modules/file/file.module
    @@ -1086,6 +1076,21 @@ function _file_save_upload_single(\SplFileInfo $file_info, $form_field_name, $va
    +    \Drupal::messenger()->addStatus($message);
    

    Out of scope, but I wonder if adding this status message is wise via this API. We're maintaining previous behavior, so we should keep it, but I wonder if we want a follow-up to reconsider this.

11. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -470,46 +486,10 @@ protected function validate(FileInterface $file, array $validators) {
    +    $extensions = $validators['file_validate_extensions'][0] ?? '';
    +    $event = new FileUploadSanitizeNameEvent($filename, $extensions);
    +    $this->eventDispatcher->dispatch($event, FileEvents::SANITIZE_NAME);
    +    return $event->getFilename();
    

    Sooooo much cleaner now! 🎉 Yay.

12. +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
    @@ -337,6 +337,65 @@ public function testHandleDangerousFile() {
    +    // Allow the .php extension and make sure it gets munged and given a .txt
    +    // extension for safety. Also check to make sure its MIME type was changed.
    

    Are these stray comments from another issue?

13. +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
    @@ -422,7 +481,7 @@ public function testHandleFileMunge() {
    -    $this->assertRaw(t('File name is @filename', ['@filename' => 'image-test.png.php_.png']));
    +    $this->assertRaw(t('File name is @filename', ['@filename' => 'image-test.png_.php_.png']));
    

    Note to self and other reviewers: This is the correct behavior now -- each extension before the last is now being munged:

    SecurityFileUploadEventSubscriber::sanitizeName() contains:

    +    // If there are any insecure extensions in the filename munge all the
    +    // internal extensions.
    +    $munge_everything = !empty(array_intersect(array_map('strtolower', $filename_parts), self::INSECURE_EXTENSIONS));
    ...
    

14. +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
    @@ -440,7 +499,7 @@ public function testHandleFileMunge() {
    -    $this->assertRaw(t('File name is @filename.', ['@filename' => 'image-test.png_.php_.png_.txt']));
    +    $this->assertRaw(t('File name is @filename.', ['@filename' => 'image-test.png_.php__0.png']));
    

    Given the above, this is needed, since the same original filename is now being munged the same way twice, resulting in a duplicate "image-test.png_.php_.png" filename, which then gets the "_0" appended to avoid collisions. The automatic .txt rename is no longer happening.

15. +++ b/core/modules/jsonapi/tests/src/Functional/FileUploadTest.php
    @@ -678,13 +678,13 @@ public function testFileUploadMaliciousExtension() {
         // The file mime should also now be text.
    -    $expected['data']['attributes']['filemime'] = 'text/plain';
    +    $expected['data']['attributes']['filemime'] = 'image/png';
    
    +++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
    @@ -574,13 +574,13 @@ public function testFileUploadMaliciousExtension() {
         // The file mime should also now be text.
    -    $expected['filemime'][0]['value'] = 'text/plain';
    +    $expected['filemime'][0]['value'] = 'image/png';
    

    Comment no longer matches the code.

16. +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
    @@ -0,0 +1,131 @@
    +    // This event must be run last to ensure the filename obeys the security
    +    // rules.
    +    $events[FileEvents::SANITIZE_NAME][] = ['sanitizeName', -1024];
    

    Still wish there was a better way to ensure this always runs, and runs last.

17. +++ b/core/modules/system/tests/src/Unit/Event/SecurityFileUploadEventSubscriberTest.php
    @@ -0,0 +1,150 @@
    +   * Tests file name sanitization.
    

    "... without file munging."

18. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,103 @@
    +   * @covers ::setFilename
    

    I don't think this is true. Copy/paste error?

19. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,103 @@
    +  public function testEventFilenameFunctions(string $filename) {
    ...
    +   * Provides data for testEvent().
    

    Comment doesn't match.

20. +++ b/core/tests/Drupal/Tests/Core/File/FileUploadSanitizeNameEventTest.php
    @@ -0,0 +1,103 @@
    +   *   Arrays with original name, expected name and allowed extensions (as a
    +   *   space-delimited string).
    

    No longer true.

Attached patch fixes: 2, 3, 4, 8, 9, 12, 15, 17, 18, 19, 20.

TODO: 1, 6, 7, 16?

Nothing need: 5, 10, 11, 13, 14.

We have a CR, removing that tag. However, I think it needs edits (1 + 6), so tagging for that.

Didn't do a detailed review, but the change overall looks great. Only thing I was wondering is if it would be worth having a "better" API than having to fire the event yourself. We still have a wrapper method now in those (tempary) API related things now. Based on a quick scan through the comments, this wasn't discussed yet. The API used to be more complicated, but there is still the security flag argument that needs to be passed back, so I guess a single-method API for this would still require a value object to be returned (could be the event class) so I guess it's not really worth it?

Should the security rename flag be mentioned in the change record and what you're expected to do with it? The API related usages aren't using that atm, but I guess we have no mechanism there to return information/warning messages there anyway and it would be out of scope for sure as it wasn't done before.

+++ b/core/lib/Drupal/Core/File/Event/FileEvents.php
@@ -0,0 +1,22 @@
+/**
+ * Defines events for the file system.
+ */
+final class FileEvents {
+
+  /**
+   * Name of the event fired when uploading a file to sanitize the filename.
+   *
+   * @Event
+   *
+   * @see _file_save_upload_single()
+   * @see \Drupal\Core\File\Event\FileUploadSanitizeNameEvent
+   *
+   * @var string
+   */
+  const SANITIZE_NAME = 'file.upload.sanitize.name';
+

I've not followed recent developments too closely, but from what I understand, Symfony has been moving away from named events in many cases in favor of just using the event class name as the identifier per https://symfony.com/blog/new-in-symfony-4-3-simpler-event-dispatching. Has there been any discussion on whether we should do the same, at least for new events?

Re: #68: Okay, fair enough. I disagree on some of your reverts, but it's not worth delaying this to further discuss them. ;)

Re: #71: Cool about not needing the event name, and the class that's just there to hold the event name constant. Yay. Agreed that looks more nice.

The patch is excellent. I'm nearly out of concerns. ;) Just one tiny cosmetic thing for the doc comments:

+++ b/core/includes/file.inc
@@ -176,8 +176,14 @@ function file_build_uri($path) {
+ *   \Drupal\Core\File\Event\FileUploadSanitizeNameEvent::class event instead.
...
+  @trigger_error('file_munge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Dispatch a \Drupal\Core\File\Event\FileUploadSanitizeNameEvent::class event instead. See https://www.drupal.org/node/3032541', E_USER_DEPRECATED);

+++ b/core/modules/file/file.api.php
@@ -5,6 +5,52 @@
+ * - File names are sanitized by the FileUploadSanitizeNameEvent::class event.
...
+ * - Use file_save_upload() to trigger the FileUploadSanitizeNameEvent::class

+++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
@@ -0,0 +1,130 @@
+    $events[FileUploadSanitizeNameEvent::class][] = ['sanitizeName', -1024];

+++ b/core/tests/Drupal/KernelTests/Core/File/NameMungingTest.php
@@ -60,6 +61,7 @@ protected function setUp(): void {
+    $this->expectDeprecation('file_munge_filename() is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Dispatch a \Drupal\Core\File\Event\FileUploadSanitizeNameEvent::class event instead. See https://www.drupal.org/node/3032541');

Do we want the "::class" part in these doc comments? I know we need it for the actual call sites like $events[FileUploadSanitizeNameEvent::class][] = ['sanitizeName', -1024];, but in a comment, "Dispatch a \Drupal\Core\File\Event\FileUploadSanitizeNameEvent event instead." seems sufficient (and more legible). Attached here in patch / interdiff form.

Re: #72: I re-read the CR. I added a missing comma, but otherwise it looks great.

Also, just re-checked the summary. It was mostly accurate, but it mentioned 9.0.0 where it now needs to say 10.0.0 for the constructor deprecation stuff. Fixed.

Thanks, @alexpott, for driving this one this far! Almost home. ;)

Cheers,
-Derek

Re-reviewed the patch, feedback has been addressed, and this all looks good now.

1. +++ b/core/modules/file/file.post_update.php
   @@ -8,6 +8,7 @@
   @@ -28,7 +29,7 @@ function file_post_update_add_txt_if_allows_insecure_extensions(&$sandbox = NULL
   
   @@ -28,7 +29,7 @@ function file_post_update_add_txt_if_allows_insecure_extensions(&$sandbox = NULL
          }
          foreach ($allowed_extensions as $extension) {
            // Allow .txt if an insecure extension is allowed.
   -        if (preg_match(FILE_INSECURE_EXTENSION_REGEX, 'test.' . $extension)) {
   +        if (preg_match(SecurityFileUploadEventSubscriber::INSECURE_EXTENSION_REGEX, 'test.' . $extension)) {
              $allowed_extensions_string .= ' txt';
              $field->setSetting('file_extensions', $allowed_extensions_string);
   

   Seems weird to use a public constant from an event subscriber here. If INSECURE_EXTENSION_REGEX needs to be public API it should be outside something that is @internal.

2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -238,7 +239,7 @@ public static function validateExtensions($element, FormStateInterface $form_sta
            foreach ($extension_array as $extension) {
   -          if (preg_match(FILE_INSECURE_EXTENSION_REGEX, 'test.' . $extension)) {
   +          if (preg_match(SecurityFileUploadEventSubscriber::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']));
   

   Here too.

3. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -470,46 +485,10 @@ protected function validate(FileInterface $file, array $validators) {
   -        // reject it below.
   -        if (!empty($validators['file_validate_extensions'][0])) {
   -          $file = File::create([]);
   -          $file->setFilename($filename);
   -          $passes_validation = empty(file_validate_extensions($file, $validators['file_validate_extensions'][0]));
   -          // Only allow upload and rename to .txt if .txt files are allowed.
   -          $passes_validation = $passes_validation && preg_match('/\btxt\b/', $validators['file_validate_extensions'][0]);
   

   I can see a call to file_validate_extensions() removed from here, but I don't immediately see where it's added back.

4. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -388,46 +399,10 @@ protected function validate(FileInterface $file, array $validators) {
   -          $file = File::create([]);
   -          $file->setFilename($filename);
   -          $passes_validation = empty(file_validate_extensions($file, $validators['file_validate_extensions'][0]));
   -          // Only allow upload and rename to .txt if .txt files are allowed.
   -          $passes_validation = $passes_validation && preg_match('/\btxt\b/', $validators['file_validate_extensions'][0]);
   -        }
   

   And also here.

5. +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
   @@ -0,0 +1,130 @@
   +    $extensions = $event->getAllowedExtensions();
   +    if (!empty($extensions) && !in_array(strtolower($final_extension), $extensions, TRUE)) {
   +      // This upload will be rejected file_validate_extension() anyway so do not
   +      // make any alterations to the filename.
   +      return;
   

   Missing a 'by'. And should this by file_validate_extensions()? And per above comments where does that file_validate_extensions() actually happen? I think if we're relying on it being called, we should point out where (for the various code paths in core from where the event can be triggered).

The rename event subscriber is set to 1024 to run last - hopefully that works. However there is also the case with the event dispatcher that a previous listener could call Event::stopPropagation, which will stop it from running altogether. Do we then need to check $event->isPropagationStopped() in the calling code to detect this and throw an exception or warning? This is a disadvantage of moving to an event (or a hook since hook_module_implements_alter() would also let you prevent the logic being called). Or is the intention to let modules swap this out if they really, really want to?

What about making the secure-rename flag a tri-state? NULL/TRUE/FALSE, and at least one event subscriber must set it to TRUE or FALSE to be accepted? Then you can skip/replace if you really want to but it must be a deliberate decision? However, then we definitely need an API wrapper around invoking the event so we can verify that.

+++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
@@ -79,12 +66,15 @@
-      // make any alterations to the filename.
+      // This upload will be rejected by file_validate_extension() anyway so do
+      // not make any alterations to the filename. It is the responsibility of
+      // the function that dispatched the event to ensure file_validate() is
+      // called with file_validate_extension() as a validator if the list of
+      // allowed extensions is not empty.
       return;
     }
 

Isn't this file_validate_extensions()?

Stopping propagation altogether seems good.

@alexpott there's one more file_validate_extension a line bellow the one fixed (could be fixed on commiit)

@catch: Great points, thanks for the careful review, and apologies we didn't deal with some of those points sooner.

@alexpott: Great work on the fixes. +1000 to moving the constants to FileSystemInterface.

Agreed on blowing up in the face of anyone that tries to call stopPropagation(). Nice solution that doesn't further complicate this or expand the API surface. Do we "need" to document the @throws, though? It's all the function does. ;) But trying to preemptively cover any final concerns.

@andypost: Good catch on that last reference to "file_validate_extension()". Fixed here.

And here's a @throws comment, just in case. ;)

What about this TODO in file_munge_filename()?

    // Remove unsafe extensions from the allowed list of extensions.
    // @todo https://www.drupal.org/project/drupal/issues/3032390 Make the list
    //   of unsafe extensions a constant. The list is copied from
    //   FILE_INSECURE_EXTENSION_REGEX.
    $allowed_extensions = array_diff($allowed_extensions, explode('|', 'phar|php|pl|py|cgi|asp|js'));

Should it be removed or can it stay because the function is deprecated anyway now?

Looking really good. Nice improvements. Back to RTBC

1. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
   @@ -470,46 +485,10 @@ protected function validate(FileInterface $file, array $validators) {
   -    return $filename;
   +    $extensions = $validators['file_validate_extensions'][0] ?? '';
   +    $event = new FileUploadSanitizeNameEvent($filename, $extensions);
   +    $this->eventDispatcher->dispatch($event);
   +    return $event->getFilename();
      }
   

   Can we add a comment here referring to where the validation happens? Also it's a bit weird that we're passing all validators into here then only use file_validate_extensions (I realise this is pre-existing in the code this replaces too).

2. +++ b/core/modules/jsonapi/src/Controller/TemporaryJsonapiFileFieldUploader.php
   @@ -388,46 +399,10 @@ protected function validate(FileInterface $file, array $validators) {
   -
   -    return $filename;
   +    $extensions = $validators['file_validate_extensions'][0] ?? '';
   +    $event = new FileUploadSanitizeNameEvent($filename, $extensions);
   +    $this->eventDispatcher->dispatch($event);
   +    return $event->getFilename();
      }
    
   

   Same thing here.

3. +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
   @@ -0,0 +1,120 @@
   +   */
   +  public static function getSubscribedEvents() {
   +    // This event must be run last to ensure the filename obeys the security
   +    // rules.
   +    $events[FileUploadSanitizeNameEvent::class][] = ['sanitizeName', -1024];
   +    return $events;
   +  }
   

   What happens if someone adds an event with priority -1025, do we just assume they know what they're doing?

4. +++ b/core/modules/system/src/EventSubscriber/SecurityFileUploadEventSubscriber.php
   @@ -0,0 +1,120 @@
   +
   +    $extensions = $event->getAllowedExtensions();
   +    if (!empty($extensions) && !in_array(strtolower($final_extension), $extensions, TRUE)) {
   +      // This upload will be rejected by file_validate_extensions() anyway so do
   +      // not make any alterations to the filename. It is the responsibility of
   +      // the function that dispatched the event to ensure file_validate() is
   +      // called with 'file_validate_extensions' in the list of validators if
   +      // $extensions is not empty.
   +      return;
   +    }
   +
   

   To me it's a bit odd that we don't munge the filename if validation would already fail. If validation is going to fail, what harm does it do to munge the filename? If validation doesn't run, then why not do the thing that the event claims it does - feels like it would compound an existing issue?

@alexpott
This line looks a little odd.

This prevents a file named 'example.php' being renamed to 'example.php_.txt' and uploaded if the .txt extension is allowed but .php is not.

Maybe we can put it like this.

This prevents a file named 'example.php' from being renamed to 'example.php_.txt' and hence prevents file upload if the .txt extension is allowed and .PHP is not.

Or you can suggest something better.

hmm, it's not a very easy thing to explain, but also I don't think the suggestion in #90 is an improvement - for example the double 'prevents'. I can't think of another way to word this either, so think we should go ahead with the current wording in the patch.

Committed 69a0a83 and pushed to 9.2.x. Thanks!

Thank you