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 000000000..dd15f6e83
Binary files /dev/null and b/tests/test-data/bad.phtml.png differ