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.

diff --git a/app/Exports/ZipExports/Models/ZipExportImage.php b/app/Exports/ZipExports/Models/ZipExportImage.php
index 89083b15be116bbe1e97e2833559131265177cc6..e0e7d11986d82c58324e95845ad47a1bde4095e3 100644 (file)
--- 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 ebc2fbbc9e1d9fb0aebe1584979d0bf4f5ccc12a..6b88ef61c33f45fbe966bae68c3d95dde274e1af 100644 (file)
--- 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 7d6c829cf037c78242bc79725c1ead3f82f7d054..90e78c060b03a8d2f97b4d5579b4c936ff444195 100644 (file)
--- 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 27d859e5915ea4c4ef37907cfa001af243cf667c..d25a1621f6ed17bdd935b26ff772b1193dfdf715 100644 (file)
--- 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 7659c228bcdedca5d780b5ee903c33bee9652ece..fd9cd7844729adfc7b6c8fe7a151975d6fa0ad2b 100644 (file)
--- 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 fdfc3d9a9b9ec9f12888bbac50b45a3db7debc26..d9b982d1e23e2eb2ad98ae4815e8b94de20f11dc 100644 (file)
--- 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 ac07b33aef5f7c5567ea3afb2f8ff7938937a5b2..12531239ff3c39cf888cb46becaa797312de4955 100644 (file)
--- 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 4cacea95ec65a49e60d5f5cf7e64be78cd98e9cc..c453ef294d460831e6ae576ee0b604c494c52fbd 100644 (file)
--- 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 c833fadda96d3d6ef4075992c89667c4addeccdc..d3af6df76a604b9e4aa56f06b85d7bd6ace810d2 100644 (file)
--- 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' => '<p>hello</p>',
+                '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);
+    }
 }