From 59cfc087e12c8752b4a9f1760db71a13ad6c121c Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 18 Nov 2024 17:42:49 +0000 Subject: [PATCH] ZIP Imports: Added image type validation/handling Images were missing their extension after import since it was (potentially) not part of the import data. This adds validation via mime sniffing (to match normal image upload checks) and also uses the same logic to sniff out a correct extension. Added tests to cover. Also fixed some existing tests around zip functionality. --- .../ZipExports/Models/ZipExportImage.php | 3 +- app/Exports/ZipExports/ZipExportReader.php | 12 +++++++ .../ZipExports/ZipFileReferenceRule.php | 12 +++++++ app/Exports/ZipExports/ZipImportRunner.php | 8 ++++- .../ZipExports/ZipValidationHelper.php | 6 ++-- lang/en/validation.php | 1 + tests/Exports/ZipExportTest.php | 4 --- ...orTests.php => ZipExportValidatorTest.php} | 21 ++++++++++- tests/Exports/ZipImportRunnerTest.php | 35 +++++++++++++++++++ 9 files changed, 92 insertions(+), 10 deletions(-) rename tests/Exports/{ZipExportValidatorTests.php => ZipExportValidatorTest.php} (77%) diff --git a/app/Exports/ZipExports/Models/ZipExportImage.php b/app/Exports/ZipExports/Models/ZipExportImage.php index 89083b15b..e0e7d1198 100644 --- a/app/Exports/ZipExports/Models/ZipExportImage.php +++ b/app/Exports/ZipExports/Models/ZipExportImage.php @@ -32,10 +32,11 @@ class ZipExportImage extends ZipExportModel public static function validate(ZipValidationHelper $context, array $data): array { + $acceptedImageTypes = ['image/png', 'image/jpeg', 'image/gif', 'image/webp']; $rules = [ 'id' => ['nullable', 'int', $context->uniqueIdRule('image')], 'name' => ['required', 'string', 'min:1'], - 'file' => ['required', 'string', $context->fileReferenceRule()], + 'file' => ['required', 'string', $context->fileReferenceRule($acceptedImageTypes)], 'type' => ['required', 'string', Rule::in(['gallery', 'drawio'])], ]; diff --git a/app/Exports/ZipExports/ZipExportReader.php b/app/Exports/ZipExports/ZipExportReader.php index ebc2fbbc9..6b88ef61c 100644 --- a/app/Exports/ZipExports/ZipExportReader.php +++ b/app/Exports/ZipExports/ZipExportReader.php @@ -7,6 +7,7 @@ use BookStack\Exports\ZipExports\Models\ZipExportBook; use BookStack\Exports\ZipExports\Models\ZipExportChapter; use BookStack\Exports\ZipExports\Models\ZipExportModel; use BookStack\Exports\ZipExports\Models\ZipExportPage; +use BookStack\Util\WebSafeMimeSniffer; use ZipArchive; class ZipExportReader @@ -81,6 +82,17 @@ class ZipExportReader return $this->zip->getStream("files/{$fileName}"); } + /** + * Sniff the mime type from the file of given name. + */ + public function sniffFileMime(string $fileName): string + { + $stream = $this->streamFile($fileName); + $sniffContent = fread($stream, 2000); + + return (new WebSafeMimeSniffer())->sniff($sniffContent); + } + /** * @throws ZipExportException */ diff --git a/app/Exports/ZipExports/ZipFileReferenceRule.php b/app/Exports/ZipExports/ZipFileReferenceRule.php index 7d6c829cf..90e78c060 100644 --- a/app/Exports/ZipExports/ZipFileReferenceRule.php +++ b/app/Exports/ZipExports/ZipFileReferenceRule.php @@ -9,6 +9,7 @@ class ZipFileReferenceRule implements ValidationRule { public function __construct( protected ZipValidationHelper $context, + protected array $acceptedMimes, ) { } @@ -21,5 +22,16 @@ class ZipFileReferenceRule implements ValidationRule if (!$this->context->zipReader->fileExists($value)) { $fail('validation.zip_file')->translate(); } + + if (!empty($this->acceptedMimes)) { + $fileMime = $this->context->zipReader->sniffFileMime($value); + if (!in_array($fileMime, $this->acceptedMimes)) { + $fail('validation.zip_file_mime')->translate([ + 'attribute' => $attribute, + 'validTypes' => implode(',', $this->acceptedMimes), + 'foundType' => $fileMime + ]); + } + } } } diff --git a/app/Exports/ZipExports/ZipImportRunner.php b/app/Exports/ZipExports/ZipImportRunner.php index 27d859e59..d25a1621f 100644 --- a/app/Exports/ZipExports/ZipImportRunner.php +++ b/app/Exports/ZipExports/ZipImportRunner.php @@ -228,6 +228,9 @@ class ZipImportRunner protected function importImage(ZipExportImage $exportImage, Page $page, ZipExportReader $reader): Image { + $mime = $reader->sniffFileMime($exportImage->file); + $extension = explode('/', $mime)[1]; + $file = $this->zipFileToUploadedFile($exportImage->file, $reader); $image = $this->imageService->saveNewFromUpload( $file, @@ -236,9 +239,12 @@ class ZipImportRunner null, null, true, - $exportImage->name, + $exportImage->name . '.' . $extension, ); + $image->name = $exportImage->name; + $image->save(); + $this->references->addImage($image, $exportImage->id); return $image; diff --git a/app/Exports/ZipExports/ZipValidationHelper.php b/app/Exports/ZipExports/ZipValidationHelper.php index 7659c228b..fd9cd7844 100644 --- a/app/Exports/ZipExports/ZipValidationHelper.php +++ b/app/Exports/ZipExports/ZipValidationHelper.php @@ -33,9 +33,9 @@ class ZipValidationHelper return $messages; } - public function fileReferenceRule(): ZipFileReferenceRule + public function fileReferenceRule(array $acceptedMimes = []): ZipFileReferenceRule { - return new ZipFileReferenceRule($this); + return new ZipFileReferenceRule($this, $acceptedMimes); } public function uniqueIdRule(string $type): ZipUniqueIdRule @@ -43,7 +43,7 @@ class ZipValidationHelper return new ZipUniqueIdRule($this, $type); } - public function hasIdBeenUsed(string $type, int $id): bool + public function hasIdBeenUsed(string $type, mixed $id): bool { $key = $type . ':' . $id; if (isset($this->validatedIds[$key])) { diff --git a/lang/en/validation.php b/lang/en/validation.php index fdfc3d9a9..d9b982d1e 100644 --- a/lang/en/validation.php +++ b/lang/en/validation.php @@ -106,6 +106,7 @@ return [ 'uploaded' => 'The file could not be uploaded. The server may not accept files of this size.', 'zip_file' => 'The :attribute needs to reference a file within the ZIP.', + 'zip_file_mime' => 'The :attribute needs to reference a file of type :validTypes, found :foundType.', 'zip_model_expected' => 'Data object expected but ":type" found.', 'zip_unique' => 'The :attribute must be unique for the object type within the ZIP.', diff --git a/tests/Exports/ZipExportTest.php b/tests/Exports/ZipExportTest.php index ac07b33ae..12531239f 100644 --- a/tests/Exports/ZipExportTest.php +++ b/tests/Exports/ZipExportTest.php @@ -107,12 +107,10 @@ class ZipExportTest extends TestCase [ 'name' => 'Exporty', 'value' => 'Content', - 'order' => 1, ], [ 'name' => 'Another', 'value' => '', - 'order' => 2, ] ], $pageData['tags']); } @@ -162,7 +160,6 @@ class ZipExportTest extends TestCase $attachmentData = $pageData['attachments'][0]; $this->assertEquals('PageAttachmentExport.txt', $attachmentData['name']); $this->assertEquals($attachment->id, $attachmentData['id']); - $this->assertEquals(1, $attachmentData['order']); $this->assertArrayNotHasKey('link', $attachmentData); $this->assertNotEmpty($attachmentData['file']); @@ -193,7 +190,6 @@ class ZipExportTest extends TestCase $attachmentData = $pageData['attachments'][0]; $this->assertEquals('My link attachment for export', $attachmentData['name']); $this->assertEquals($attachment->id, $attachmentData['id']); - $this->assertEquals(1, $attachmentData['order']); $this->assertEquals('https://example.com/cats', $attachmentData['link']); $this->assertArrayNotHasKey('file', $attachmentData); } diff --git a/tests/Exports/ZipExportValidatorTests.php b/tests/Exports/ZipExportValidatorTest.php similarity index 77% rename from tests/Exports/ZipExportValidatorTests.php rename to tests/Exports/ZipExportValidatorTest.php index 4cacea95e..c453ef294 100644 --- a/tests/Exports/ZipExportValidatorTests.php +++ b/tests/Exports/ZipExportValidatorTest.php @@ -11,7 +11,7 @@ use BookStack\Exports\ZipExports\ZipImportRunner; use BookStack\Uploads\Image; use Tests\TestCase; -class ZipExportValidatorTests extends TestCase +class ZipExportValidatorTest extends TestCase { protected array $filesToRemove = []; @@ -71,4 +71,23 @@ class ZipExportValidatorTests extends TestCase $this->assertEquals($expectedMessage, $results['book.pages.1.id']); $this->assertEquals($expectedMessage, $results['book.chapters.1.id']); } + + public function test_image_files_need_to_be_a_valid_detected_image_file() + { + $validator = $this->getValidatorForData([ + 'page' => [ + 'id' => 4, + 'name' => 'My page', + 'markdown' => 'hello', + 'images' => [ + ['id' => 4, 'name' => 'Image A', 'type' => 'gallery', 'file' => 'cat'], + ], + ] + ], ['cat' => $this->files->testFilePath('test-file.txt')]); + + $results = $validator->validate(); + $this->assertCount(1, $results); + + $this->assertEquals('The file needs to reference a file of type image/png,image/jpeg,image/gif,image/webp, found text/plain.', $results['page.images.0.file']); + } } diff --git a/tests/Exports/ZipImportRunnerTest.php b/tests/Exports/ZipImportRunnerTest.php index c833fadda..d3af6df76 100644 --- a/tests/Exports/ZipImportRunnerTest.php +++ b/tests/Exports/ZipImportRunnerTest.php @@ -358,4 +358,39 @@ class ZipImportRunnerTest extends TestCase ZipTestHelper::deleteZipForImport($import); } + + public function test_imported_images_have_their_detected_extension_added() + { + $testImagePath = $this->files->testFilePath('test-image.png'); + $parent = $this->entities->chapter(); + + $import = ZipTestHelper::importFromData([], [ + 'page' => [ + 'name' => 'Page A', + 'html' => '

hello

', + 'images' => [ + [ + 'id' => 2, + 'name' => 'Cat', + 'type' => 'gallery', + 'file' => 'cat_image' + ] + ], + ], + ], [ + 'cat_image' => $testImagePath, + ]); + + $this->asAdmin(); + /** @var Page $page */ + $page = $this->runner->run($import, $parent); + + $pageImages = Image::where('uploaded_to', '=', $page->id)->whereIn('type', ['gallery', 'drawio'])->get(); + + $this->assertCount(1, $pageImages); + $this->assertStringEndsWith('.png', $pageImages[0]->url); + $this->assertStringEndsWith('.png', $pageImages[0]->path); + + ZipTestHelper::deleteZipForImport($import); + } }