Use consistent logic to validate uploaded filenames

2 files changed, 53 insertions, 10 deletions

diff --git a/app/Http/RequestHandlers/UploadMediaAction.php b/app/Http/RequestHandlers/UploadMediaAction.php
index 8785511b57..0d4cef04da 100644
--- a/app/Http/RequestHandlers/UploadMediaAction.php
+++ b/app/Http/RequestHandlers/UploadMediaAction.php
@@ -66,7 +66,7 @@ final class UploadMediaAction implements RequestHandlerInterface
                 throw new FileUploadException($uploaded_file);
             }
 
-            $key      = substr($key, 9);
+            $key      = substr($key, 9); // "mediafile1", "mediafile2", ...
             $folder   = Validator::parsedBody($request)->string('folder' . $key);
             $filename = Validator::parsedBody($request)->string('filename' . $key);
 
@@ -84,15 +84,19 @@ final class UploadMediaAction implements RequestHandlerInterface
             $filename = str_replace('\\', '/', $filename);
             $filename = trim($filename, '/');
 
-            if (preg_match('/([:])/', $filename, $match)) {
-                // Local media files cannot contain certain special characters, especially on MS Windows
-                FlashMessages::addMessage(I18N::translate('Filenames are not allowed to contain the character “%s”.', $match[1]));
+            $tmp = strpbrk($filename, MediaFileService::BLOCKED_CHARACTERS);
+
+            if ($tmp !== false) {
+                $message = I18N::translate('Filenames are not allowed to contain the character “%s”.', $tmp[0]);
+                FlashMessages::addMessage($message);
                 continue;
             }
 
-            if (preg_match('/(\.(php|pl|cgi|bash|sh|bat|exe|com|htm|html|shtml))$/i', $filename, $match)) {
-                // Do not allow obvious script files.
-                FlashMessages::addMessage(I18N::translate('Filenames are not allowed to have the extension “%s”.', $match[1]));
+            $extension = pathinfo($filename, PATHINFO_EXTENSION);
+
+            if (in_array(strtolower($extension), MediaFileService::BLOCKED_EXTENSIONS, true)) {
+                $message = I18N::translate('Filenames are not allowed to have the extension “%s”.', $extension);
+                FlashMessages::addMessage($message);
                 continue;
             }
 
diff --git a/app/Services/MediaFileService.php b/app/Services/MediaFileService.php
index a4e6ba3c0a..7d6f1b30b2 100644
--- a/app/Services/MediaFileService.php
+++ b/app/Services/MediaFileService.php
@@ -41,12 +41,15 @@ use function array_diff;
 use function array_intersect;
 use function dirname;
 use function explode;
+use function in_array;
 use function intdiv;
 use function min;
 use function pathinfo;
 use function sha1;
 use function sort;
 use function str_contains;
+use function strpbrk;
+use function strtolower;
 use function strtoupper;
 use function strtr;
 use function trim;
@@ -59,6 +62,25 @@ use const UPLOAD_ERR_OK;
  */
 class MediaFileService
 {
+    // Characters that are not allowed in media filenames.
+    public const string BLOCKED_CHARACTERS = ':';
+
+    // Media files that are images are displayed.  Others (pdf, xls, txt, etc.) are downloaded.
+    // Block files with obvious executable extensions.
+    public const array BLOCKED_EXTENSIONS = [
+        'bash',
+        'bat',
+        'cgi',
+        'com',
+        'exe',
+        'htm',
+        'html',
+        'php',
+        'pl',
+        'sh',
+        'shtml',
+    ];
+
     private const array IGNORE_FOLDERS = [
         // Old versions of webtrees
         'thumbs',
@@ -173,11 +195,28 @@ class MediaFileService
                     $folder .= '/';
                 }
 
+                $tmp = strpbrk($folder . $file, self::BLOCKED_CHARACTERS);
+
+                if ($tmp !== false) {
+                    $message = I18N::translate('Filenames are not allowed to contain the character “%s”.', $tmp[0]);
+                    FlashMessages::addMessage($message);
+
+                    return '';
+                }
+
+                $extension = pathinfo($file, PATHINFO_EXTENSION);
+
+                if (in_array(strtolower($extension), self::BLOCKED_EXTENSIONS, true)) {
+                    $message = I18N::translate('Filenames are not allowed to have the extension “%s”.', $extension);
+                    FlashMessages::addMessage($message);
+
+                    return '';
+                }
+
                 // Generate a unique name for the file?
                 if ($auto === '1' || $tree->mediaFilesystem()->fileExists($folder . $file)) {
-                    $folder    = '';
-                    $extension = pathinfo($uploaded_file->getClientFilename(), PATHINFO_EXTENSION);
-                    $file      = sha1((string) $uploaded_file->getStream()) . '.' . $extension;
+                    $folder = '';
+                    $file   = sha1((string) $uploaded_file->getStream()) . '.' . $extension;
                 }
 
                 try {