Skip to content

Commit 59cfc08

Browse files
ssddanbrownssddanbrown
committed
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.
1 parent e2f6e50 commit 59cfc08

File tree

9 files changed

+92
-10
lines changed

9 files changed

+92
-10
lines changed

‎app/Exports/ZipExports/Models/ZipExportImage.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,11 @@ public function metadataOnly(): void
3232

3333
public static function validate(ZipValidationHelper $context, array $data): array
3434
{
35+
$acceptedImageTypes = ['image/png', 'image/jpeg', 'image/gif', 'image/webp'];
3536
$rules = [
3637
'id' => ['nullable', 'int', $context->uniqueIdRule('image')],
3738
'name' => ['required', 'string', 'min:1'],
38-
'file' => ['required', 'string', $context->fileReferenceRule()],
39+
'file' => ['required', 'string', $context->fileReferenceRule($acceptedImageTypes)],
3940
'type' => ['required', 'string', Rule::in(['gallery', 'drawio'])],
4041
];
4142

‎app/Exports/ZipExports/ZipExportReader.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use BookStack\Exports\ZipExports\Models\ZipExportChapter;
88
use BookStack\Exports\ZipExports\Models\ZipExportModel;
99
use BookStack\Exports\ZipExports\Models\ZipExportPage;
10+
use BookStack\Util\WebSafeMimeSniffer;
1011
use ZipArchive;
1112

1213
class ZipExportReader
@@ -81,6 +82,17 @@ public function streamFile(string $fileName)
8182
return $this->zip->getStream("files/{$fileName}");
8283
}
8384

85+
/**
86+
* Sniff the mime type from the file of given name.
87+
*/
88+
public function sniffFileMime(string $fileName): string
89+
{
90+
$stream = $this->streamFile($fileName);
91+
$sniffContent = fread($stream, 2000);
92+
93+
return (new WebSafeMimeSniffer())->sniff($sniffContent);
94+
}
95+
8496
/**
8597
* @throws ZipExportException
8698
*/

‎app/Exports/ZipExports/ZipFileReferenceRule.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ class ZipFileReferenceRule implements ValidationRule
99
{
1010
public function __construct(
1111
protected ZipValidationHelper $context,
12+
protected array $acceptedMimes,
1213
) {
1314
}
1415

@@ -21,5 +22,16 @@ public function validate(string $attribute, mixed $value, Closure $fail): void
2122
if (!$this->context->zipReader->fileExists($value)) {
2223
$fail('validation.zip_file')->translate();
2324
}
25+
26+
if (!empty($this->acceptedMimes)) {
27+
$fileMime = $this->context->zipReader->sniffFileMime($value);
28+
if (!in_array($fileMime, $this->acceptedMimes)) {
29+
$fail('validation.zip_file_mime')->translate([
30+
'attribute' => $attribute,
31+
'validTypes' => implode(',', $this->acceptedMimes),
32+
'foundType' => $fileMime
33+
]);
34+
}
35+
}
2436
}
2537
}

‎app/Exports/ZipExports/ZipImportRunner.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,9 @@ protected function importAttachment(ZipExportAttachment $exportAttachment, Page
228228

229229
protected function importImage(ZipExportImage $exportImage, Page $page, ZipExportReader $reader): Image
230230
{
231+
$mime = $reader->sniffFileMime($exportImage->file);
232+
$extension = explode('/', $mime)[1];
233+
231234
$file = $this->zipFileToUploadedFile($exportImage->file, $reader);
232235
$image = $this->imageService->saveNewFromUpload(
233236
$file,
@@ -236,9 +239,12 @@ protected function importImage(ZipExportImage $exportImage, Page $page, ZipExpor
236239
null,
237240
null,
238241
true,
239-
$exportImage->name,
242+
$exportImage->name . '.' . $extension,
240243
);
241244

245+
$image->name = $exportImage->name;
246+
$image->save();
247+
242248
$this->references->addImage($image, $exportImage->id);
243249

244250
return $image;

‎app/Exports/ZipExports/ZipValidationHelper.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,17 @@ public function validateData(array $data, array $rules): array
3333
return $messages;
3434
}
3535

36-
public function fileReferenceRule(): ZipFileReferenceRule
36+
public function fileReferenceRule(array $acceptedMimes = []): ZipFileReferenceRule
3737
{
38-
return new ZipFileReferenceRule($this);
38+
return new ZipFileReferenceRule($this, $acceptedMimes);
3939
}
4040

4141
public function uniqueIdRule(string $type): ZipUniqueIdRule
4242
{
4343
return new ZipUniqueIdRule($this, $type);
4444
}
4545

46-
public function hasIdBeenUsed(string $type, int $id): bool
46+
public function hasIdBeenUsed(string $type, mixed $id): bool
4747
{
4848
$key = $type . ':' . $id;
4949
if (isset($this->validatedIds[$key])) {

‎lang/en/validation.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@
106106
'uploaded' => 'The file could not be uploaded. The server may not accept files of this size.',
107107

108108
'zip_file' => 'The :attribute needs to reference a file within the ZIP.',
109+
'zip_file_mime' => 'The :attribute needs to reference a file of type :validTypes, found :foundType.',
109110
'zip_model_expected' => 'Data object expected but ":type" found.',
110111
'zip_unique' => 'The :attribute must be unique for the object type within the ZIP.',
111112

‎tests/Exports/ZipExportTest.php

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -107,12 +107,10 @@ public function test_page_export_with_tags()
107107
[
108108
'name' => 'Exporty',
109109
'value' => 'Content',
110-
'order' => 1,
111110
],
112111
[
113112
'name' => 'Another',
114113
'value' => '',
115-
'order' => 2,
116114
]
117115
], $pageData['tags']);
118116
}
@@ -162,7 +160,6 @@ public function test_page_export_file_attachments()
162160
$attachmentData = $pageData['attachments'][0];
163161
$this->assertEquals('PageAttachmentExport.txt', $attachmentData['name']);
164162
$this->assertEquals($attachment->id, $attachmentData['id']);
165-
$this->assertEquals(1, $attachmentData['order']);
166163
$this->assertArrayNotHasKey('link', $attachmentData);
167164
$this->assertNotEmpty($attachmentData['file']);
168165

@@ -193,7 +190,6 @@ public function test_page_export_link_attachments()
193190
$attachmentData = $pageData['attachments'][0];
194191
$this->assertEquals('My link attachment for export', $attachmentData['name']);
195192
$this->assertEquals($attachment->id, $attachmentData['id']);
196-
$this->assertEquals(1, $attachmentData['order']);
197193
$this->assertEquals('https://example.com/cats', $attachmentData['link']);
198194
$this->assertArrayNotHasKey('file', $attachmentData);
199195
}

‎tests/Exports/ZipExportValidatorTests.php renamed to ‎tests/Exports/ZipExportValidatorTest.php

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
use BookStack\Uploads\Image;
1212
use Tests\TestCase;
1313

14-
class ZipExportValidatorTests extends TestCase
14+
class ZipExportValidatorTest extends TestCase
1515
{
1616
protected array $filesToRemove = [];
1717

@@ -71,4 +71,23 @@ public function test_ids_have_to_be_unique()
7171
$this->assertEquals($expectedMessage, $results['book.pages.1.id']);
7272
$this->assertEquals($expectedMessage, $results['book.chapters.1.id']);
7373
}
74+
75+
public function test_image_files_need_to_be_a_valid_detected_image_file()
76+
{
77+
$validator = $this->getValidatorForData([
78+
'page' => [
79+
'id' => 4,
80+
'name' => 'My page',
81+
'markdown' => 'hello',
82+
'images' => [
83+
['id' => 4, 'name' => 'Image A', 'type' => 'gallery', 'file' => 'cat'],
84+
],
85+
]
86+
], ['cat' => $this->files->testFilePath('test-file.txt')]);
87+
88+
$results = $validator->validate();
89+
$this->assertCount(1, $results);
90+
91+
$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']);
92+
}
7493
}

‎tests/Exports/ZipImportRunnerTest.php

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,4 +358,39 @@ public function test_revert_cleans_up_uploaded_files()
358358

359359
ZipTestHelper::deleteZipForImport($import);
360360
}
361+
362+
public function test_imported_images_have_their_detected_extension_added()
363+
{
364+
$testImagePath = $this->files->testFilePath('test-image.png');
365+
$parent = $this->entities->chapter();
366+
367+
$import = ZipTestHelper::importFromData([], [
368+
'page' => [
369+
'name' => 'Page A',
370+
'html' => '<p>hello</p>',
371+
'images' => [
372+
[
373+
'id' => 2,
374+
'name' => 'Cat',
375+
'type' => 'gallery',
376+
'file' => 'cat_image'
377+
]
378+
],
379+
],
380+
], [
381+
'cat_image' => $testImagePath,
382+
]);
383+
384+
$this->asAdmin();
385+
/** @var Page $page */
386+
$page = $this->runner->run($import, $parent);
387+
388+
$pageImages = Image::where('uploaded_to', '=', $page->id)->whereIn('type', ['gallery', 'drawio'])->get();
389+
390+
$this->assertCount(1, $pageImages);
391+
$this->assertStringEndsWith('.png', $pageImages[0]->url);
392+
$this->assertStringEndsWith('.png', $pageImages[0]->path);
393+
394+
ZipTestHelper::deleteZipForImport($import);
395+
}
361396
}

0 commit comments

Comments
 (0)