chore: add tests for malicious uploads (#429)

* chore: add tests for malicious uploads

* Apply fixes from StyleCI

* chore: remove debug code

* test zip/apk, include additional polyglot

* Apply fixes from StyleCI

* refine tests further

* Apply fixes from StyleCI

* chore: add migration to update previous insecure image regex

* Apply fixes from StyleCI

* chore: update README

---------

Co-authored-by: StyleCI Bot <bot@styleci.io>

README.md

@@ -37,12 +37,24 @@ php flarum cache:clear
 
 Enable the extension, a new tab will appear on the left hand side. This separate settings page allows you to further configure the extension.
 
+On new installations, a pre-defined regex will be inserted for you that enables image uploads, restricted to safe image types. We now include SVG as safe, due to our SVG sanitization method. Default image types allowed are:
+
+- JPEG
+- PNG
+- GIF
+- WebP
+- AVIF
+- BMP
+- TIFF
+- SVG
+
+The regex for these types is `^image\/(jpeg|png|gif|webp|avif|bmp|tiff|svg\+xml)$`, and can be modified as required. We **STRONGLY** discourage the use of a wildcard such as `^image\/.*`, as this could introduce vulnerabilities in the uploaded files. Versions of `fof/upload` prior to `1.8.0` used this as default, and is considered insecure.
+
 Make sure you configure the upload permission on the permissions page as well.
 
 ### Mimetype regular expression
 
-Regular expressions allow you a lot of freedom, but they are also very difficult to understand. Here are some pointers, but feel free to ask
-for help on the official Flarum forums.
+Regular expressions allow you a lot of freedom, but they are also very difficult to understand. Here are some pointers, but feel free to ask for help on the official Flarum forums, or check out [regex101.com](https://regex101.com/) where you can interactively build and test your regex pattern.
 
 In case you want to allow all regular file types including video, music, compressed files and images, use this:
 
@@ -121,6 +133,30 @@ The following (to resume) will happen when this command is put into a recurring
 - the command will go over all uploads to discover in which posts they have been used
 - delete those files that have been uploaded "last year" that have not been found in posts
 
+## Testing and Security Measures
+
+FoF Upload includes **automated tests** to ensure:
+
+✅ Valid files upload successfully
+✅ Restricted files are blocked
+✅ SVG sanitization removes potential XSS risks
+
+### 🔍 Security Tests for Malicious Files
+We specifically test against:
+- HTML Injection (`.html` disguised as an image)
+- MIME Spoofing (e.g., `.png` containing a script)
+- Polygot Files (Files that act as two different formats)
+- SVG Sanitization (`<script>`, `<foreignObject>`, event handlers, external styles, etc)
+- ZIP & APK Handling (Ensuring APKs are valid and ZIPs are not misclassified)
+
+### Submitting Additional Test Cases
+We welcome community contributes in all our extensions! Especially where security is concerned. If you find a new edge case or a file format that bypasses validation, please:
+- Open an issue on [GitHub](https://github.com/FriendsOfFlarum/upload/issues)
+- Submit a test case as a PR under `tests/`
+- Describe the expected behaviour (Should the file be accepted? Should it be sanitized?)
+
+🚀 These tests ensure FoF Upload remains secure and reliable for all Flarum users! 🚀
+
 ## FAQ
 
 -  __AWS S3__: read the [AWS S3 configuration page](https://github.com/FriendsOfFlarum/upload/wiki/aws-s3).

extend.php

@@ -112,7 +112,9 @@
         ->render(Formatter\TextPreview\FormatTextPreview::class),
 
     (new SvgSanitizer())
-        ->allowTag('animate'),
+        ->allowTag('animate')
+        ->removeTag('image')
+        ->removeTag('style'),
 
     (new Extend\ErrorHandling())
         ->handler(InvalidUploadException::class, ExceptionHandler::class),

migrations/2025_02_06_000000_alter_default_image_mime_expression.php

@@ -0,0 +1,81 @@
+<?php
+
+/*
+ * This file is part of fof/upload.
+ *
+ * Copyright (c) FriendsOfFlarum.
+ * Copyright (c) Flagrow.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+use Illuminate\Database\Schema\Builder;
+
+return [
+    'up' => function (Builder $schema) {
+        $db = $schema->getConnection();
+
+        $oldRegex = '^image\/.*';
+        $newRegex = '^image\/(jpeg|png|gif|webp|avif|bmp|tiff|svg\+xml)$';
+
+        $setting = $db->table('settings')->where('key', 'fof-upload.mimeTypes')->first();
+
+        if ($setting && isset($setting->value)) {
+            $decodedValue = json_decode($setting->value, true);
+
+            if (is_array($decodedValue)) {
+                $updatedValue = [];
+
+                foreach ($decodedValue as $mime => $config) {
+                    if ($mime === $oldRegex) {
+                        // Replace the old regex with the new one
+                        $updatedValue[$newRegex] = $config;
+                    } else {
+                        // Preserve all other settings
+                        $updatedValue[$mime] = $config;
+                    }
+                }
+
+                $jsonUpdated = json_encode($updatedValue, JSON_UNESCAPED_SLASHES);
+
+                if ($jsonUpdated !== $setting->value) {
+                    $db->table('settings')->where('key', 'fof-upload.mimeTypes')->update(['value' => $jsonUpdated]);
+                }
+            }
+        }
+    },
+
+    'down' => function (Builder $schema) {
+        $db = $schema->getConnection();
+
+        $oldRegex = '^image\/(jpeg|png|gif|webp|avif|bmp|tiff|svg\+xml)$';
+        $newRegex = '^image\/.*';
+
+        $setting = $db->table('settings')->where('key', 'fof-upload.mimeTypes')->first();
+
+        if ($setting && isset($setting->value)) {
+            $decodedValue = json_decode($setting->value, true);
+
+            if (is_array($decodedValue)) {
+                $revertedValue = [];
+
+                foreach ($decodedValue as $mime => $config) {
+                    if ($mime === $newRegex) {
+                        // Revert back to the old regex
+                        $revertedValue[$oldRegex] = $config;
+                    } else {
+                        // Preserve all other settings
+                        $revertedValue[$mime] = $config;
+                    }
+                }
+
+                $jsonReverted = json_encode($revertedValue, JSON_UNESCAPED_SLASHES);
+
+                if ($jsonReverted !== $setting->value) {
+                    $db->table('settings')->where('key', 'fof-upload.mimeTypes')->update(['value' => $jsonReverted]);
+                }
+            }
+        }
+    },
+];

src/Helpers/Util.php

@@ -89,7 +89,7 @@ public function defaultMimeTypes(): Collection
         $adapters = $this->getAvailableUploadMethods();
 
         return collect([
-            '^image\/.*' => [
+            '^image\/(jpeg|png|gif|webp|avif|bmp|tiff|svg\+xml)$' => [
                 'adapter'  => $adapters->flip()->last(),
                 'template' => 'image-preview',
             ],

src/Mime/MimeTypeDetector.php

@@ -63,14 +63,35 @@ public function getMimeType(): ?string
         }
 
         try {
-            $type = $this->getMimeInternally();
+            // Get MIME from php-mime-detector
+            $detectorMime = $this->getMimeInternally();
 
-            // If mime_content_type returns application/zip or empty, perform magic byte detection
-            if ($type === 'application/zip' || empty($type)) {
-                return $this->detectUsingMagicBytes();
+            // Get MIME from PHP Fileinfo
+            $fileinfoMime = mime_content_type($this->filePath);
+
+            // Special handling for APKs (before mismatch rejection)
+            if ($detectorMime === 'application/zip' || $fileinfoMime === 'application/zip') {
+                if ($this->isApk($this->filePath)) {
+                    return 'application/vnd.android.package-archive';
+                }
+            }
+
+            // Reject if MIME mismatch occurs (AFTER checking for APKs)
+            if ($detectorMime !== $fileinfoMime) {
+                $message = "MIME type mismatch detected: $detectorMime vs $fileinfoMime";
+                resolve('log')->error("[fof/upload] $message");
+
+                // Check if the file exists, if it does, delete it.
+                if (file_exists($this->filePath)) {
+                    unlink($this->filePath);
+                }
+
+                throw new ValidationException([
+                    'upload' => $message,
+                ]);
             }
 
-            return $type;
+            return $detectorMime;
         } catch (\Exception $e) {
             throw new ValidationException(['upload' => 'Could not detect MIME type.']);
         }
@@ -90,37 +111,6 @@ private function getMimeInternally(): bool|string
         return $mime;
     }
 
-    /**
-     * Detect MIME type using file magic bytes.
-     *
-     * @return string|null
-     */
-    private function detectUsingMagicBytes(): ?string
-    {
-        $handle = fopen($this->filePath, 'rb');
-        if (!$handle) {
-            return null;
-        }
-
-        $magicBytes = fread($handle, 4); // Read the first 4 bytes
-        fclose($handle);
-
-        foreach ($this->getMappings() as $mapping) {
-            foreach ($mapping['magicBytes'] as $bytes) {
-                if ($magicBytes === $bytes) {
-                    // Additional checks for APK-specific files
-                    if ($mapping['extension'] === 'apk' && !$this->isApk($this->filePath)) {
-                        continue; // Not an APK, fallback to other mappings
-                    }
-
-                    return $mapping['mime'];
-                }
-            }
-        }
-
-        return null;
-    }
-
     /**
      * Check if the file is a valid APK by inspecting its contents.
      *
@@ -132,18 +122,19 @@ private function isApk(string $filePath): bool
     {
         $zip = new \ZipArchive();
         if ($zip->open($filePath) === true) {
-            // APKs should contain "AndroidManifest.xml" and "classes.dex"
             $requiredFiles = ['AndroidManifest.xml', 'classes.dex'];
+
             foreach ($requiredFiles as $file) {
                 if ($zip->locateName($file) === false) {
                     $zip->close();
 
-                    return false; // Required APK-specific file not found
+                    return false; // Required file not found, reject as non-APK
                 }
             }
+
             $zip->close();
 
-            return true; // All required files found
+            return true; // Valid APK structure detected
         }
 
         return false; // Not a valid ZIP file

src/Repositories/FileRepository.php

@@ -173,7 +173,12 @@ protected function handleUploadError($code): void
 
     public function removeFromTemp(Upload $file): bool
     {
-        return $this->getTempFilesystem($file->getPath())->delete($file->getBasename());
+        $filesystem = $this->getTempFilesystem($file->getPath());
+        if ($filesystem->has($file->getBasename())) {
+            return $filesystem->delete($file->getBasename());
+        }
+
+        return true;
     }
 
     protected function getTempFilesystem(string $path): Filesystem

tests/fixtures/Malicious.html

@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>Malicious Upload</title>
+</head>
+<body>
+    <script>alert("XSS Attack!");</script>
+    <p>This is a test file.</p>
+</body>
+</html>

tests/fixtures/Polyglot.flif

@@ -0,0 +1,7 @@
+FLIF<?xml foo="bar"?>
+<!DOCTYPE html><html>
+<script>
+alert("XSS Attack!");
+</script>
+<p>Blaklis!</p>
+</html>