From 193e2ffebe920234438faff812f731de7b36ca9f Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 24 Mar 2019 19:07:18 +0000 Subject: [PATCH] Prevent dbl exts. on img upload, Randomized attachment upload names --- app/Http/Controllers/ImageController.php | 2 +- app/Providers/AppServiceProvider.php | 5 ++++ app/Uploads/AttachmentService.php | 9 +++--- tests/Uploads/AttachmentTest.php | 35 ++++++++++++++--------- tests/Uploads/ImageTest.php | 20 ++++++++++--- tests/test-data/bad.phtml.png | Bin 0 -> 158 bytes 6 files changed, 48 insertions(+), 23 deletions(-) create mode 100644 tests/test-data/bad.phtml.png diff --git a/app/Http/Controllers/ImageController.php b/app/Http/Controllers/ImageController.php index fdcb3aba4..4d6f759b3 100644 --- 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, [ - '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)) { diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 1d9a8736e..3ca59dcb3 100644 --- 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); }); + 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) { diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index 7bafb6c0a..e613642c4 100644 --- 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(); - $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; } diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index 373d9eb5a..35ffda821 100644 --- 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], []); } - /** - * 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', [ diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index 4091bec57..8373a809c 100644 --- 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'); + } - $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() diff --git a/tests/test-data/bad.phtml.png b/tests/test-data/bad.phtml.png new file mode 100644 index 0000000000000000000000000000000000000000..dd15f6e8376f13b59c0298149b2aae80ff6239c3 GIT binary patch literal 158 zcmeAS@N?(olHy`uVBq!ia0vp^tRT$61SFYwH*Nw_oCO|{#S9GG!XV7ZFl&wkP>{XE z)7O>#5u*%`mStyi&M%;lY-UJAiF1B#Zfaf$kjuc}T$GwvlA5AWo>`Ki;O^-gkfN8$ v4ip#hba4#fxSqVF=l-b%mKhQOj4Nvx(rz(^FIjp|2&B=|)z4*}Q$iB}%ws0s literal 0 HcmV?d00001