diff --git a/app/Entities/Models/Bookshelf.php b/app/Entities/Models/Bookshelf.php index b2dab252a..cdc6648f9 100644 --- a/app/Entities/Models/Bookshelf.php +++ b/app/Entities/Models/Bookshelf.php @@ -86,7 +86,7 @@ class Bookshelf extends Entity implements HasCoverImage */ public function coverImageTypeKey(): string { - return 'cover_shelf'; + return 'cover_bookshelf'; } /** diff --git a/app/Entities/Repos/BaseRepo.php b/app/Entities/Repos/BaseRepo.php index 747d1b176..da939e102 100644 --- a/app/Entities/Repos/BaseRepo.php +++ b/app/Entities/Repos/BaseRepo.php @@ -86,8 +86,9 @@ class BaseRepo public function updateCoverImage($entity, ?UploadedFile $coverImage, bool $removeImage = false) { if ($coverImage) { + $imageType = $entity->coverImageTypeKey(); $this->imageRepo->destroyImage($entity->cover); - $image = $this->imageRepo->saveNew($coverImage, 'cover_book', $entity->id, 512, 512, true); + $image = $this->imageRepo->saveNew($coverImage, $imageType, $entity->id, 512, 512, true); $entity->cover()->associate($image); $entity->save(); } diff --git a/app/Entities/Tools/ExportFormatter.php b/app/Entities/Tools/ExportFormatter.php index 97902db82..9e4d63cf7 100644 --- a/app/Entities/Tools/ExportFormatter.php +++ b/app/Entities/Tools/ExportFormatter.php @@ -235,7 +235,7 @@ class ExportFormatter $linksOutput = []; preg_match_all("/\/i", $htmlContent, $linksOutput); - // Replace image src with base64 encoded image strings + // Update relative links to be absolute, with instance url if (isset($linksOutput[0]) && count($linksOutput[0]) > 0) { foreach ($linksOutput[0] as $index => $linkMatch) { $oldLinkString = $linkMatch; @@ -248,7 +248,6 @@ class ExportFormatter } } - // Replace any relative links with system domain return $htmlContent; } diff --git a/app/Http/Controllers/Images/ImageController.php b/app/Http/Controllers/Images/ImageController.php index 21ed58553..b5bc840a1 100644 --- a/app/Http/Controllers/Images/ImageController.php +++ b/app/Http/Controllers/Images/ImageController.php @@ -33,7 +33,7 @@ class ImageController extends Controller */ public function showImage(string $path) { - if (!$this->imageService->pathExistsInLocalSecure($path)) { + if (!$this->imageService->pathAccessibleInLocalSecure($path)) { throw (new NotFoundException(trans('errors.image_not_found'))) ->setSubtitle(trans('errors.image_not_found_subtitle')) ->setDetails(trans('errors.image_not_found_details')); diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index 6a92cb5a5..88bb41efb 100644 --- a/app/Uploads/AttachmentService.php +++ b/app/Uploads/AttachmentService.php @@ -41,7 +41,7 @@ class AttachmentService // Change to our secure-attachment disk if any of the local options // are used to prevent escaping that location. - if ($storageType === 'local' || $storageType === 'local_secure') { + if ($storageType === 'local' || $storageType === 'local_secure' || $storageType === 'local_secure_restricted') { $storageType = 'local_secure_attachments'; } diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index ca0db997b..ec2f6da54 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -2,6 +2,9 @@ namespace BookStack\Uploads; +use BookStack\Entities\Models\Book; +use BookStack\Entities\Models\Bookshelf; +use BookStack\Entities\Models\Page; use BookStack\Exceptions\ImageUploadException; use ErrorException; use Exception; @@ -24,20 +27,15 @@ use Symfony\Component\HttpFoundation\StreamedResponse; class ImageService { - protected $imageTool; - protected $cache; + protected ImageManager $imageTool; + protected Cache $cache; protected $storageUrl; - protected $image; - protected $fileSystem; + protected FilesystemManager $fileSystem; protected static $supportedExtensions = ['jpg', 'jpeg', 'png', 'gif', 'webp']; - /** - * ImageService constructor. - */ - public function __construct(Image $image, ImageManager $imageTool, FilesystemManager $fileSystem, Cache $cache) + public function __construct(ImageManager $imageTool, FilesystemManager $fileSystem, Cache $cache) { - $this->image = $image; $this->imageTool = $imageTool; $this->fileSystem = $fileSystem; $this->cache = $cache; @@ -55,9 +53,18 @@ class ImageService * Check if local secure image storage (Fetched behind authentication) * is currently active in the instance. */ - protected function usingSecureImages(): bool + protected function usingSecureImages(string $imageType = 'gallery'): bool { - return $this->getStorageDiskName('gallery') === 'local_secure_images'; + return $this->getStorageDiskName($imageType) === 'local_secure_images'; + } + + /** + * Check if "local secure restricted" (Fetched behind auth, with permissions enforced) + * is currently active in the instance. + */ + protected function usingSecureRestrictedImages() + { + return config('filesystems.images') === 'local_secure_restricted'; } /** @@ -68,7 +75,7 @@ class ImageService { $path = Util::normalizePath(str_replace('uploads/images/', '', $path)); - if ($this->getStorageDiskName($imageType) === 'local_secure_images') { + if ($this->usingSecureImages($imageType)) { return $path; } @@ -87,7 +94,9 @@ class ImageService $storageType = 'local'; } - if ($storageType === 'local_secure') { + // Rename local_secure options to get our image specific storage driver which + // is scoped to the relevant image directories. + if ($storageType === 'local_secure' || $storageType === 'local_secure_restricted') { $storageType = 'local_secure_images'; } @@ -179,8 +188,8 @@ class ImageService $imageDetails['updated_by'] = $userId; } - $image = $this->image->newInstance(); - $image->forceFill($imageDetails)->save(); + $image = (new Image())->forceFill($imageDetails); + $image->save(); return $image; } @@ -451,7 +460,7 @@ class ImageService $types = ['gallery', 'drawio']; $deletedPaths = []; - $this->image->newQuery()->whereIn('type', $types) + Image::query()->whereIn('type', $types) ->chunk(1000, function ($images) use ($checkRevisions, &$deletedPaths, $dryRun) { foreach ($images as $image) { $searchQuery = '%' . basename($image->path) . '%'; @@ -492,6 +501,14 @@ class ImageService } $storagePath = $this->adjustPathForStorageDisk($storagePath); + + // Apply access control when local_secure_restricted images are active + if ($this->usingSecureRestrictedImages()) { + if (!$this->checkUserHasAccessToRelationOfImageAtPath($storagePath)) { + return null; + } + } + $storage = $this->getStorageDisk(); $imageData = null; if ($storage->exists($storagePath)) { @@ -511,14 +528,19 @@ class ImageService } /** - * Check if the given path exists in the local secure image system. - * Returns false if local_secure is not in use. + * Check if the given path exists and is accessible in the local secure image system. + * Returns false if local_secure is not in use, if the file does not exist, if the + * file is likely not a valid image, or if permission does not allow access. */ - public function pathExistsInLocalSecure(string $imagePath): bool + public function pathAccessibleInLocalSecure(string $imagePath): bool { /** @var FilesystemAdapter $disk */ $disk = $this->getStorageDisk('gallery'); + if ($this->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImageAtPath($imagePath)) { + return false; + } + // Check local_secure is active return $this->usingSecureImages() && $disk instanceof FilesystemAdapter @@ -528,6 +550,54 @@ class ImageService && strpos($disk->getMimetype($imagePath), 'image/') === 0; } + /** + * Check that the current user has access to the relation + * of the image at the given path. + */ + protected function checkUserHasAccessToRelationOfImageAtPath(string $path): bool + { + if (strpos($path, '/uploads/images/') === 0) { + $path = substr($path, 15); + } + + // Strip thumbnail element from path if existing + $originalPathSplit = array_filter(explode('/', $path), function(string $part) { + $resizedDir = (strpos($part, 'thumbs-') === 0 || strpos($part, 'scaled-') === 0); + $missingExtension = strpos($part, '.') === false; + return !($resizedDir && $missingExtension); + }); + + // Build a database-format image path and search for the image entry + $fullPath = '/uploads/images/' . ltrim(implode('/', $originalPathSplit), '/'); + $image = Image::query()->where('path', '=', $fullPath)->first(); + + if (is_null($image)) { + return false; + } + + $imageType = $image->type; + + // Allow user or system (logo) images + // (No specific relation control but may still have access controlled by auth) + if ($imageType === 'user' || $imageType === 'system') { + return true; + } + + if ($imageType === 'gallery' || $imageType === 'drawio') { + return Page::visible()->where('id', '=', $image->uploaded_to)->exists(); + } + + if ($imageType === 'cover_book') { + return Book::visible()->where('id', '=', $image->uploaded_to)->exists(); + } + + if ($imageType === 'cover_bookshelf') { + return Bookshelf::visible()->where('id', '=', $image->uploaded_to)->exists(); + } + + return false; + } + /** * For the given path, if existing, provide a response that will stream the image contents. */ diff --git a/database/migrations/2022_09_02_082910_fix_shelf_cover_image_types.php b/database/migrations/2022_09_02_082910_fix_shelf_cover_image_types.php new file mode 100644 index 000000000..837bdfe24 --- /dev/null +++ b/database/migrations/2022_09_02_082910_fix_shelf_cover_image_types.php @@ -0,0 +1,42 @@ +whereNotNull('image_id') + ->pluck('image_id') + ->values()->all(); + + DB::table('images') + ->where('type', '=', 'cover_book') + ->whereIn('id', $shelfImageIds) + ->update(['type' => 'cover_bookshelf']); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + DB::table('images') + ->where('type', '=', 'cover_bookshelf') + ->update(['type' => 'cover_book']); + } +} diff --git a/tests/Entity/BookShelfTest.php b/tests/Entity/BookShelfTest.php index 5a7107ff0..44d30a548 100644 --- a/tests/Entity/BookShelfTest.php +++ b/tests/Entity/BookShelfTest.php @@ -125,6 +125,7 @@ class BookShelfTest extends TestCase 'image_id' => $lastImage->id, ]); $this->assertEquals($lastImage->id, $shelf->cover->id); + $this->assertEquals('cover_bookshelf', $lastImage->type); } public function test_shelf_view() diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index 27a23bcae..e71adf70b 100644 --- a/tests/Uploads/AttachmentTest.php +++ b/tests/Uploads/AttachmentTest.php @@ -341,4 +341,19 @@ class AttachmentTest extends TestCase $this->deleteUploads(); } + + public function test_file_upload_works_when_local_secure_restricted_is_in_use() + { + config()->set('filesystems.attachments', 'local_secure_restricted'); + + $page = Page::query()->first(); + $fileName = 'upload_test_file.txt'; + + $upload = $this->asAdmin()->uploadFile($fileName, $page->id); + $upload->assertStatus(200); + + $attachment = Attachment::query()->orderBy('id', 'desc')->where('uploaded_to', '=', $page->id)->first(); + $this->assertFileExists(storage_path($attachment->path)); + $this->deleteUploads(); + } } diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index c006f9612..2a3023a9e 100644 --- a/tests/Uploads/ImageTest.php +++ b/tests/Uploads/ImageTest.php @@ -327,6 +327,89 @@ class ImageTest extends TestCase } } + public function test_secure_restricted_images_inaccessible_without_relation_permission() + { + config()->set('filesystems.images', 'local_secure_restricted'); + $this->asEditor(); + $galleryFile = $this->getTestImage('my-secure-restricted-test-upload.png'); + /** @var Page $page */ + $page = Page::query()->first(); + + $upload = $this->call('POST', '/images/gallery', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []); + $upload->assertStatus(200); + $expectedUrl = url('uploads/images/gallery/' . date('Y-m') . '/my-secure-restricted-test-upload.png'); + $expectedPath = storage_path('uploads/images/gallery/' . date('Y-m') . '/my-secure-restricted-test-upload.png'); + + $this->get($expectedUrl)->assertOk(); + + $this->setEntityRestrictions($page, [], []); + + $resp = $this->get($expectedUrl); + $resp->assertNotFound(); + + if (file_exists($expectedPath)) { + unlink($expectedPath); + } + } + + public function test_thumbnail_path_handled_by_secure_restricted_images() + { + config()->set('filesystems.images', 'local_secure_restricted'); + $this->asEditor(); + $galleryFile = $this->getTestImage('my-secure-restricted-thumb-test-test.png'); + /** @var Page $page */ + $page = Page::query()->first(); + + $upload = $this->call('POST', '/images/gallery', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []); + $upload->assertStatus(200); + $expectedUrl = url('uploads/images/gallery/' . date('Y-m') . '/thumbs-150-150/my-secure-restricted-thumb-test-test.png'); + $expectedPath = storage_path('uploads/images/gallery/' . date('Y-m') . '/my-secure-restricted-thumb-test-test.png'); + + $this->get($expectedUrl)->assertOk(); + + $this->setEntityRestrictions($page, [], []); + + $resp = $this->get($expectedUrl); + $resp->assertNotFound(); + + if (file_exists($expectedPath)) { + unlink($expectedPath); + } + } + + public function test_secure_restricted_image_access_controlled_in_exports() + { + config()->set('filesystems.images', 'local_secure_restricted'); + $this->asEditor(); + $galleryFile = $this->getTestImage('my-secure-restricted-export-test.png'); + + /** @var Page $pageA */ + /** @var Page $pageB */ + $pageA = Page::query()->first(); + $pageB = Page::query()->where('id', '!=', $pageA->id)->first(); + $expectedPath = storage_path('uploads/images/gallery/' . date('Y-m') . '/my-secure-restricted-export-test.png'); + + $upload = $this->asEditor()->call('POST', '/images/gallery', ['uploaded_to' => $pageA->id], [], ['file' => $galleryFile], []); + $upload->assertOk(); + + $imageUrl = json_decode($upload->getContent(), true)['url']; + $pageB->html .= ""; + $pageB->save(); + + $encodedImageContent = base64_encode(file_get_contents($expectedPath)); + $export = $this->get($pageB->getUrl('/export/html')); + $this->assertStringContainsString($encodedImageContent, $export->getContent()); + + $this->setEntityRestrictions($pageA, [], []); + + $export = $this->get($pageB->getUrl('/export/html')); + $this->assertStringNotContainsString($encodedImageContent, $export->getContent()); + + if (file_exists($expectedPath)) { + unlink($expectedPath); + } + } + public function test_image_delete() { $page = Page::query()->first();