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.

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'])],
         ];
 

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
      */

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
+                ]);
+            }
+        }
     }
 }

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;

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])) {

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.',
 

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);
     }

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']);
+    }
 }

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);
+    }
 }