From 8213ea9a7182df48ac9c12bb7b74098b5ad665a4 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 25 Jul 2020 11:18:40 +0100
Subject: [PATCH] Fixed issue where URL params in image names would cause
 loading failure

Updated file name handling to route through str:slug to be cleaned up
a little.
Added testing to cover.

Fixes #2161
---
 app/Uploads/ImageService.php | 36 ++++++++++++++++++++++++------------
 tests/Uploads/ImageTest.php  | 32 ++++++++++++++++++++++++++++++++
 tests/Uploads/UsesImages.php |  5 +----
 3 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php
index 78458dc39..89744386d 100644
--- a/app/Uploads/ImageService.php
+++ b/app/Uploads/ImageService.php
@@ -124,29 +124,24 @@ class ImageService extends UploadService
     }
 
     /**
-     * Saves a new image
-     * @param string $imageName
-     * @param string $imageData
-     * @param string $type
-     * @param int $uploadedTo
-     * @return Image
+     * Save a new image into storage.
      * @throws ImageUploadException
      */
-    private function saveNew($imageName, $imageData, $type, $uploadedTo = 0)
+    private function saveNew(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image
     {
         $storage = $this->getStorage($type);
         $secureUploads = setting('app-secure-images');
-        $imageName = str_replace(' ', '-', $imageName);
+        $fileName = $this->cleanImageFileName($imageName);
 
         $imagePath = '/uploads/images/' . $type . '/' . Date('Y-m') . '/';
 
-        while ($storage->exists($imagePath . $imageName)) {
-            $imageName = Str::random(3) . $imageName;
+        while ($storage->exists($imagePath . $fileName)) {
+            $fileName = Str::random(3) . $fileName;
         }
 
-        $fullPath = $imagePath . $imageName;
+        $fullPath = $imagePath . $fileName;
         if ($secureUploads) {
-            $fullPath = $imagePath . Str::random(16) . '-' . $imageName;
+            $fullPath = $imagePath . Str::random(16) . '-' . $fileName;
         }
 
         try {
@@ -175,6 +170,23 @@ class ImageService extends UploadService
         return $image;
     }
 
+    /**
+     * Clean up an image file name to be both URL and storage safe.
+     */
+    protected function cleanImageFileName(string $name): string
+    {
+        $name = str_replace(' ', '-', $name);
+        $nameParts = explode('.', $name);
+        $extension = array_pop($nameParts);
+        $name = implode('.', $nameParts);
+        $name = Str::slug($name);
+
+        if (strlen($name) === 0) {
+            $name = Str::random(10);
+        }
+
+        return  $name . '.' . $extension;
+    }
 
     /**
      * Checks if the image is a gif. Returns true if it is, else false.
diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php
index 0de94158b..08ac63326 100644
--- a/tests/Uploads/ImageTest.php
+++ b/tests/Uploads/ImageTest.php
@@ -182,6 +182,38 @@ class ImageTest extends TestCase
         $this->assertFalse(file_exists(public_path($relPath)), 'Uploaded double extension file was uploaded but should have been stopped');
     }
 
+    public function test_url_entities_removed_from_filenames()
+    {
+        $this->asEditor();
+        $badNames = [
+            "bad-char-#-image.png",
+            "bad-char-?-image.png",
+            "?#.png",
+            "?.png",
+            "#.png",
+        ];
+        foreach ($badNames as $name) {
+            $galleryFile = $this->getTestImage($name);
+            $page = Page::first();
+            $badPath = $this->getTestImagePath('gallery', $name);
+            $this->deleteImage($badPath);
+
+            $upload = $this->call('POST', '/images/gallery', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []);
+            $upload->assertStatus(200);
+
+            $lastImage = Image::query()->latest('id')->first();
+            $newFileName = explode('.', basename($lastImage->path))[0];
+
+            $this->assertEquals($lastImage->name, $name);
+            $this->assertFalse(strpos($lastImage->path, $name), 'Path contains original image name');
+            $this->assertFalse(file_exists(public_path($badPath)), 'Uploaded image file name was not stripped of url entities');
+
+            $this->assertTrue(strlen($newFileName) > 0, 'File name was reduced to nothing');
+
+            $this->deleteImage($lastImage->path);
+        }
+    }
+
     public function test_secure_images_uploads_to_correct_place()
     {
         config()->set('filesystems.images', 'local_secure');
diff --git a/tests/Uploads/UsesImages.php b/tests/Uploads/UsesImages.php
index 251a61c9f..f5d1032ad 100644
--- a/tests/Uploads/UsesImages.php
+++ b/tests/Uploads/UsesImages.php
@@ -39,11 +39,8 @@ trait UsesImages
 
     /**
      * Get the path for a test image.
-     * @param $type
-     * @param $fileName
-     * @return string
      */
-    protected function getTestImagePath($type, $fileName)
+    protected function getTestImagePath(string $type, string $fileName): string
     {
         return '/uploads/images/' . $type . '/' . Date('Y-m') . '/' . $fileName;
     }