Mirrors/BookStack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2024-11-23 19:32:29 +01:00
Code Issues Releases Wiki Activity

Fixed issue where URL params in image names would cause loading failure

Browse Source 
Updated file name handling to route through str:slug to be cleaned up
a little.
Added testing to cover.

Fixes #2161
This commit is contained in:
Dan Brown 2020-07-25 11:18:40 +01:00
parent 03211ebea6
commit 8213ea9a71
No known key found for this signature in database
GPG Key ID: 46D9F943C24A2EF9
3 changed files with 57 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
app/Uploads/ImageService.php
View File

@ -124,29 +124,24 @@ class ImageService extends UploadService
}
/**
* Saves a new image
* @param string $imageName
* @param string $imageData
* @param string $type
* @param int $uploadedTo
* @return Image
* Save a new image into storage.
* @throws ImageUploadException
*/
private function saveNew($imageName, $imageData, $type, $uploadedTo = 0)
private function saveNew(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image
{
$storage = $this->getStorage($type);
$secureUploads = setting('app-secure-images');
$imageName = str_replace(' ', '-', $imageName);
$fileName = $this->cleanImageFileName($imageName);
$imagePath = '/uploads/images/' . $type . '/' . Date('Y-m') . '/';
while ($storage->exists($imagePath . $imageName)) {
$imageName = Str::random(3) . $imageName;
while ($storage->exists($imagePath . $fileName)) {
$fileName = Str::random(3) . $fileName;
}
$fullPath = $imagePath . $imageName;
$fullPath = $imagePath . $fileName;
if ($secureUploads) {
$fullPath = $imagePath . Str::random(16) . '-' . $imageName;
$fullPath = $imagePath . Str::random(16) . '-' . $fileName;
}
try {
@ -175,6 +170,23 @@ class ImageService extends UploadService
return $image;
}
/**
* Clean up an image file name to be both URL and storage safe.
*/
protected function cleanImageFileName(string $name): string
{
$name = str_replace(' ', '-', $name);
$nameParts = explode('.', $name);
$extension = array_pop($nameParts);
$name = implode('.', $nameParts);
$name = Str::slug($name);
if (strlen($name) === 0) {
$name = Str::random(10);
}
return $name . '.' . $extension;
}
/**
* Checks if the image is a gif. Returns true if it is, else false.

32
tests/Uploads/ImageTest.php
View File

@ -182,6 +182,38 @@ class ImageTest extends TestCase
$this->assertFalse(file_exists(public_path($relPath)), 'Uploaded double extension file was uploaded but should have been stopped');
}
public function test_url_entities_removed_from_filenames()
{
$this->asEditor();
$badNames = [
"bad-char-#-image.png",
"bad-char-?-image.png",
"?#.png",
"?.png",
"#.png",
];
foreach ($badNames as $name) {
$galleryFile = $this->getTestImage($name);
$page = Page::first();
$badPath = $this->getTestImagePath('gallery', $name);
$this->deleteImage($badPath);
$upload = $this->call('POST', '/images/gallery', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []);
$upload->assertStatus(200);
$lastImage = Image::query()->latest('id')->first();
$newFileName = explode('.', basename($lastImage->path))[0];
$this->assertEquals($lastImage->name, $name);
$this->assertFalse(strpos($lastImage->path, $name), 'Path contains original image name');
$this->assertFalse(file_exists(public_path($badPath)), 'Uploaded image file name was not stripped of url entities');
$this->assertTrue(strlen($newFileName) > 0, 'File name was reduced to nothing');
$this->deleteImage($lastImage->path);
}
}
public function test_secure_images_uploads_to_correct_place()
{
config()->set('filesystems.images', 'local_secure');

5
tests/Uploads/UsesImages.php
View File

@ -39,11 +39,8 @@ trait UsesImages
/**
* Get the path for a test image.
* @param $type
* @param $fileName
* @return string
*/
protected function getTestImagePath($type, $fileName)
protected function getTestImagePath(string $type, string $fileName): string
{
return '/uploads/images/' . $type . '/' . Date('Y-m') . '/' . $fileName;
}