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

diff --git a/app/Http/Controllers/ImageController.php b/app/Http/Controllers/ImageController.php
index fdcb3aba49fbd0380cc878c9953b232e26cedf61..4d6f759b336eddb97e0516c6ddb69eb8034c9639 100644 (file)
--- a/app/Http/Controllers/ImageController.php
+++ b/app/Http/Controllers/ImageController.php
@@ -119,7 +119,7 @@ class ImageController extends Controller
     {
         $this->checkPermission('image-create-all');
         $this->validate($request, [
     {
         $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)) {
         ]);
 
         if (!$this->imageRepo->isValidType($type)) {

diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php
index 1d9a8736e6f66a1c5e4305a6a5869af08468f86a..3ca59dcb33453a2bb7cf71dd9b1b20690ec4526a 100644 (file)
--- a/app/Providers/AppServiceProvider.php
+++ b/app/Providers/AppServiceProvider.php
@@ -28,6 +28,11 @@ class AppServiceProvider extends ServiceProvider
             return in_array(strtolower($value->getClientOriginalExtension()), $validImageExtensions);
         });
 
             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) {
 
         // Custom blade view directives
         Blade::directive('icon', function ($expression) {

diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php
index 7bafb6c0a26c050daab630a7f9ff692044708725..e613642c4f3dd8699a7c3f0c37b2c5394e468700 100644 (file)
--- a/app/Uploads/AttachmentService.php
+++ b/app/Uploads/AttachmentService.php
@@ -44,7 +44,7 @@ class AttachmentService extends UploadService
     public function saveNewUpload(UploadedFile $uploadedFile, $page_id)
     {
         $attachmentName = $uploadedFile->getClientOriginalName();
     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([
         $largestExistingOrder = Attachment::where('uploaded_to', '=', $page_id)->max('order');
 
         $attachment = Attachment::forceCreate([


@@ -75,7 +75,7 @@ class AttachmentService extends UploadService
         }
 
         $attachmentName = $uploadedFile->getClientOriginalName();
         }
 
         $attachmentName = $uploadedFile->getClientOriginalName();

-        $attachmentPath = $this->putFileInStorage($attachmentName, $uploadedFile);
+        $attachmentPath = $this->putFileInStorage($uploadedFile);

 
         $attachment->name = $attachmentName;
         $attachment->path = $attachmentPath;
 
         $attachment->name = $attachmentName;
         $attachment->path = $attachmentPath;


@@ -174,19 +174,18 @@ class AttachmentService extends UploadService
 
     /**
      * Store a file in storage with the given filename
 
     /**
      * Store a file in storage with the given filename

-     * @param $attachmentName

      * @param UploadedFile $uploadedFile
      * @return string
      * @throws FileUploadException
      */
      * @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') . '/';
 
     {
         $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;
         }
         while ($storage->exists($basePath . $uploadFileName)) {
             $uploadFileName = str_random(3) . $uploadFileName;
         }

diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php
index 373d9eb5a9c9ce99655c407dee51664e5b9f43eb..35ffda821ef35262f1b135fcde4c8cbc90c7a519 100644 (file)
--- a/tests/Uploads/AttachmentTest.php
+++ b/tests/Uploads/AttachmentTest.php
@@ -28,16 +28,6 @@ class AttachmentTest extends TestCase
         return $this->call('POST', '/attachments/upload', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []);
     }
 
         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.
     /**
      * 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,
             'order' => 1,
             'created_by' => $admin->id,
             'updated_by' => $admin->id,

-            'path' => $this->getUploadPath($fileName)

         ];
 
         $upload = $this->uploadFile($fileName, $page->id);
         $upload->assertStatus(200);
         ];
 
         $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();
     }
 
         $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();
     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);
 
         $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();
         $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);
 
         $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', [
 
         $this->assertTrue(file_exists($filePath), 'File at path ' . $filePath . ' does not exist');
         $this->assertDatabaseHas('attachments', [

diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php
index 4091bec570abd1d588599d619996d41129360ce3..8373a809c2962738a0895301aa64ed3ad4e5198f 100644 (file)
--- a/tests/Uploads/ImageTest.php
+++ b/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');
         $upload->assertStatus(302);
 
         $this->assertFalse(file_exists(public_path($relPath)), 'Uploaded php file was uploaded but should have been stopped');

+    }

 
 

-        $this->assertDatabaseMissing('images', [
-            'type' => 'gallery',
-            'name' => $fileName
-        ]);
+    public function test_files_with_double_extensions_cannot_be_uploaded()
+    {
+        $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()
     }
 
     public function test_secure_images_uploads_to_correct_place()

diff --git a/tests/test-data/bad.phtml.png b/tests/test-data/bad.phtml.png
new file mode 100644 (file)
index 0000000..dd15f6e
Binary files /dev/null and b/tests/test-data/bad.phtml.png differ