Prevent dbl exts. on img upload, Randomized attachment upload names

app/Http/Controllers/ImageController.php

@@ -119,7 +119,7 @@ class ImageController extends Controller
     {
         $this->checkPermission('image-create-all');
         $this->validate($request, [
-            'file' => 'image_extension|mimes:jpeg,png,gif,bmp,webp,tiff'
+            'file' => 'image_extension|no_double_extension|mimes:jpeg,png,gif,bmp,webp,tiff'
         ]);
 
         if (!$this->imageRepo->isValidType($type)) {

app/Providers/AppServiceProvider.php

@@ -28,6 +28,11 @@ class AppServiceProvider extends ServiceProvider
             return in_array(strtolower($value->getClientOriginalExtension()), $validImageExtensions);
         });
 
+        Validator::extend('no_double_extension', function ($attribute, $value, $parameters, $validator) {
+            $uploadName = $value->getClientOriginalName();
+            return substr_count($uploadName, '.') < 2;
+        });
+
 
         // Custom blade view directives
         Blade::directive('icon', function ($expression) {

app/Uploads/AttachmentService.php

@@ -44,7 +44,7 @@ class AttachmentService extends UploadService
     public function saveNewUpload(UploadedFile $uploadedFile, $page_id)
     {
         $attachmentName = $uploadedFile->getClientOriginalName();
-        $attachmentPath = $this->putFileInStorage($attachmentName, $uploadedFile);
+        $attachmentPath = $this->putFileInStorage($uploadedFile);
         $largestExistingOrder = Attachment::where('uploaded_to', '=', $page_id)->max('order');
 
         $attachment = Attachment::forceCreate([
@@ -75,7 +75,7 @@ class AttachmentService extends UploadService
         }
 
         $attachmentName = $uploadedFile->getClientOriginalName();
-        $attachmentPath = $this->putFileInStorage($attachmentName, $uploadedFile);
+        $attachmentPath = $this->putFileInStorage($uploadedFile);
 
         $attachment->name = $attachmentName;
         $attachment->path = $attachmentPath;
@@ -174,19 +174,18 @@ class AttachmentService extends UploadService
 
     /**
      * Store a file in storage with the given filename
-     * @param $attachmentName
      * @param UploadedFile $uploadedFile
      * @return string
      * @throws FileUploadException
      */
-    protected function putFileInStorage($attachmentName, UploadedFile $uploadedFile)
+    protected function putFileInStorage(UploadedFile $uploadedFile)
     {
         $attachmentData = file_get_contents($uploadedFile->getRealPath());
 
         $storage = $this->getStorage();
         $basePath = 'uploads/files/' . Date('Y-m-M') . '/';
 
-        $uploadFileName = $attachmentName;
+        $uploadFileName = str_random(16) . '.' . $uploadedFile->getClientOriginalExtension();
         while ($storage->exists($basePath . $uploadFileName)) {
             $uploadFileName = str_random(3) . $uploadFileName;
         }

tests/Uploads/AttachmentTest.php

@@ -28,16 +28,6 @@ class AttachmentTest extends TestCase
         return $this->call('POST', '/attachments/upload', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []);
     }
 
-    /**
-     * Get the expected upload path for a file.
-     * @param $fileName
-     * @return string
-     */
-    protected function getUploadPath($fileName)
-    {
-        return 'uploads/files/' . Date('Y-m-M') . '/' . $fileName;
-    }
-
     /**
      * Delete all uploaded files.
      * To assist with cleanup.
@@ -64,17 +54,34 @@ class AttachmentTest extends TestCase
             'order' => 1,
             'created_by' => $admin->id,
             'updated_by' => $admin->id,
-            'path' => $this->getUploadPath($fileName)
         ];
 
         $upload = $this->uploadFile($fileName, $page->id);
         $upload->assertStatus(200);
+
+        $attachment = Attachment::query()->orderBy('id', 'desc')->first();
+        $expectedResp['path'] = $attachment->path;
+
         $upload->assertJson($expectedResp);
         $this->assertDatabaseHas('attachments', $expectedResp);
 
         $this->deleteUploads();
     }
 
+    public function test_file_upload_does_not_use_filename()
+    {
+        $page = Page::first();
+        $fileName = 'upload_test_file.txt';
+
+
+        $upload = $this->asAdmin()->uploadFile($fileName, $page->id);
+        $upload->assertStatus(200);
+
+        $attachment = Attachment::query()->orderBy('id', 'desc')->first();
+        $this->assertNotContains($fileName, $attachment->path);
+        $this->assertStringEndsWith('.txt', $attachment->path);
+    }
+
     public function test_file_display_and_access()
     {
         $page = Page::first();
@@ -172,7 +179,8 @@ class AttachmentTest extends TestCase
         $fileName = 'deletion_test.txt';
         $this->uploadFile($fileName, $page->id);
 
-        $filePath = base_path('storage/' . $this->getUploadPath($fileName));
+        $attachment = Attachment::query()->orderBy('id', 'desc')->first();
+        $filePath = storage_path($attachment->path);
         $this->assertTrue(file_exists($filePath), 'File at path ' . $filePath . ' does not exist');
 
         $attachment = \BookStack\Uploads\Attachment::first();
@@ -193,7 +201,8 @@ class AttachmentTest extends TestCase
         $fileName = 'deletion_test.txt';
         $this->uploadFile($fileName, $page->id);
 
-        $filePath = base_path('storage/' . $this->getUploadPath($fileName));
+        $attachment = Attachment::query()->orderBy('id', 'desc')->first();
+        $filePath = storage_path($attachment->path);
 
         $this->assertTrue(file_exists($filePath), 'File at path ' . $filePath . ' does not exist');
         $this->assertDatabaseHas('attachments', [

tests/Uploads/ImageTest.php

@@ -76,11 +76,23 @@ class ImageTest extends TestCase
         $upload->assertStatus(302);
 
         $this->assertFalse(file_exists(public_path($relPath)), 'Uploaded php file was uploaded but should have been stopped');
+    }
 
-        $this->assertDatabaseMissing('images', [
+    public function test_files_with_double_extensions_cannot_be_uploaded()
-            'type' => 'gallery',
+    {
-            'name' => $fileName
+        $page = Page::first();
-        ]);
+        $admin = $this->getAdmin();
+        $this->actingAs($admin);
+
+        $fileName = 'bad.phtml.png';
+        $relPath = $this->getTestImagePath('gallery', $fileName);
+        $this->deleteImage($relPath);
+
+        $file = $this->getTestImage($fileName);
+        $upload = $this->withHeader('Content-Type', 'image/png')->call('POST', '/images/gallery/upload', ['uploaded_to' => $page->id], [], ['file' => $file], []);
+        $upload->assertStatus(302);
+
+        $this->assertFalse(file_exists(public_path($relPath)), 'Uploaded double extension file was uploaded but should have been stopped');
     }
 
     public function test_secure_images_uploads_to_correct_place()