From aaa28186bc0dff12c38f4627dac2034fa9ddb63d Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 18 Nov 2025 14:19:46 +0000 Subject: [PATCH] Exports: Updated perm checking for images in ZIP exports For #5885 Adds to, uses and cleans-up central permission checking in ImageService to mirror that which would be experienced by users in the UI to result in the same image access conditions. Adds testing to cover. --- .../ZipExports/ZipExportReferences.php | 16 ++++-- app/Uploads/Image.php | 16 +++--- app/Uploads/ImageService.php | 50 +++++++++++++------ app/Uploads/ImageStorage.php | 9 ++++ tests/Exports/ZipExportTest.php | 48 ++++++++++++++++++ 5 files changed, 113 insertions(+), 26 deletions(-) diff --git a/app/Exports/ZipExports/ZipExportReferences.php b/app/Exports/ZipExports/ZipExportReferences.php index 64107cf21aa..a79988d44fe 100644 --- a/app/Exports/ZipExports/ZipExportReferences.php +++ b/app/Exports/ZipExports/ZipExportReferences.php @@ -15,6 +15,7 @@ use BookStack\Permissions\Permission; use BookStack\Uploads\Attachment; use BookStack\Uploads\Image; +use BookStack\Uploads\ImageService; class ZipExportReferences { @@ -33,6 +34,7 @@ class ZipExportReferences public function __construct( protected ZipReferenceParser $parser, + protected ImageService $imageService, ) { } @@ -133,10 +135,17 @@ protected function handleModelReference(Model $model, ZipExportModel $exportMode return "[[bsexport:image:{$model->id}]]"; } - // Find and include images if in visibility + // Get the page which we'll reference this image upon $page = $model->getPage(); - $pageExportModel = $this->pages[$page->id] ?? ($exportModel instanceof ZipExportPage ? $exportModel : null); - if (isset($this->images[$model->id]) || ($page && $pageExportModel && userCan(Permission::PageView, $page))) { + $pageExportModel = null; + if ($page && isset($this->pages[$page->id])) { + $pageExportModel = $this->pages[$page->id]; + } elseif ($exportModel instanceof ZipExportPage) { + $pageExportModel = $exportModel; + } + + // Add the image to the export if it's accessible or just return the existing reference if already added + if (isset($this->images[$model->id]) || ($pageExportModel && $this->imageService->imageAccessible($model))) { if (!isset($this->images[$model->id])) { $exportImage = ZipExportImage::fromModel($model, $files); $this->images[$model->id] = $exportImage; @@ -144,6 +153,7 @@ protected function handleModelReference(Model $model, ZipExportModel $exportMode } return "[[bsexport:image:{$model->id}]]"; } + return null; } diff --git a/app/Uploads/Image.php b/app/Uploads/Image.php index 20def9de655..81b6db6fd22 100644 --- a/app/Uploads/Image.php +++ b/app/Uploads/Image.php @@ -13,14 +13,14 @@ use Illuminate\Database\Eloquent\Relations\HasMany; /** - * @property int $id - * @property string $name - * @property string $url - * @property string $path - * @property string $type - * @property int $uploaded_to - * @property int $created_by - * @property int $updated_by + * @property int $id + * @property string $name + * @property string $url + * @property string $path + * @property string $type + * @property int|null $uploaded_to + * @property int $created_by + * @property int $updated_by */ class Image extends Model implements OwnableInterface { diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index fadafc8e527..a26a04ac5c6 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -148,7 +148,7 @@ public function getImageStream(Image $image): mixed } /** - * Destroy an image along with its revisions, thumbnails and remaining folders. + * Destroy an image along with its revisions, thumbnails, and remaining folders. * * @throws Exception */ @@ -252,33 +252,48 @@ public function pathAccessibleInLocalSecure(string $imagePath): bool { $disk = $this->storage->getDisk('gallery'); + return $disk->usingSecureImages() && $this->pathAccessible($imagePath); + } + + /** + * Check if the given path exists and is accessible depending on the current settings. + */ + public function pathAccessible(string $imagePath): bool + { if ($this->storage->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImageAtPath($imagePath)) { return false; } - // Check local_secure is active - return $disk->usingSecureImages() - // Check the image file exists - && $disk->exists($imagePath) - // Check the file is likely an image file - && str_starts_with($disk->mimeType($imagePath), 'image/'); + if ($this->storage->usingSecureImages() && user()->isGuest()) { + return false; + } + + return $this->imageFileExists($imagePath, 'gallery'); } /** - * Check if the given path exists and is accessible depending on the current settings. + * Check if the given image should be accessible to the current user. */ - public function pathAccessible(string $imagePath): bool + public function imageAccessible(Image $image): bool { - $disk = $this->storage->getDisk('gallery'); + if ($this->storage->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImage($image)) { + return false; + } - if ($this->storage->usingSecureRestrictedImages() && !$this->checkUserHasAccessToRelationOfImageAtPath($imagePath)) { + if ($this->storage->usingSecureImages() && user()->isGuest()) { return false; } - // Check local_secure is active - return $disk->exists($imagePath) - // Check the file is likely an image file - && str_starts_with($disk->mimeType($imagePath), 'image/'); + return $this->imageFileExists($image->path, $image->type); + } + + /** + * Check if the given image path exists for the given image type and that it is likely an image file. + */ + protected function imageFileExists(string $imagePath, string $imageType): bool + { + $disk = $this->storage->getDisk($imageType); + return $disk->exists($imagePath) && str_starts_with($disk->mimeType($imagePath), 'image/'); } /** @@ -307,6 +322,11 @@ protected function checkUserHasAccessToRelationOfImageAtPath(string $path): bool return false; } + return $this->checkUserHasAccessToRelationOfImage($image); + } + + protected function checkUserHasAccessToRelationOfImage(Image $image): bool + { $imageType = $image->type; // Allow user or system (logo) images diff --git a/app/Uploads/ImageStorage.php b/app/Uploads/ImageStorage.php index ddaa26a9400..38a22e3b498 100644 --- a/app/Uploads/ImageStorage.php +++ b/app/Uploads/ImageStorage.php @@ -34,6 +34,15 @@ public function usingSecureRestrictedImages(): bool return config('filesystems.images') === 'local_secure_restricted'; } + /** + * Check if "local secure" (Fetched behind auth, either with or without permissions enforced) + * is currently active in the instance. + */ + public function usingSecureImages(): bool + { + return config('filesystems.images') === 'local_secure' || $this->usingSecureRestrictedImages(); + } + /** * Clean up an image file name to be both URL and storage safe. */ diff --git a/tests/Exports/ZipExportTest.php b/tests/Exports/ZipExportTest.php index 692a5910f3c..063f6d9425a 100644 --- a/tests/Exports/ZipExportTest.php +++ b/tests/Exports/ZipExportTest.php @@ -374,6 +374,54 @@ public function test_image_links_are_handled_when_using_external_storage_url() $this->assertStringContainsString("Original URLStorage URL", $pageData['html']); } + public function test_orphaned_images_can_be_used_on_default_local_storage() + { + $this->asEditor(); + $page = $this->entities->page(); + $result = $this->files->uploadGalleryImageToPage($this, $page); + $displayThumb = $result['response']->thumbs->gallery ?? ''; + $page->html = '

My image

'; + $page->save(); + + $image = Image::findOrFail($result['response']->id); + $image->uploaded_to = null; + $image->save(); + + $zipResp = $this->asEditor()->get($page->getUrl("/export/zip")); + $zipResp->assertOk(); + $zip = ZipTestHelper::extractFromZipResponse($zipResp); + $pageData = $zip->data['page']; + + $this->assertCount(1, $pageData['images']); + $imageData = $pageData['images'][0]; + $this->assertEquals($image->id, $imageData['id']); + + $this->assertEquals('

My image

', $pageData['html']); + } + + public function test_orphaned_images_cannot_be_used_on_local_secure_restricted() + { + config()->set('filesystems.images', 'local_secure_restricted'); + + $this->asEditor(); + $page = $this->entities->page(); + $result = $this->files->uploadGalleryImageToPage($this, $page); + $displayThumb = $result['response']->thumbs->gallery ?? ''; + $page->html = '

My image

'; + $page->save(); + + $image = Image::findOrFail($result['response']->id); + $image->uploaded_to = null; + $image->save(); + + $zipResp = $this->asEditor()->get($page->getUrl("/export/zip")); + $zipResp->assertOk(); + $zip = ZipTestHelper::extractFromZipResponse($zipResp); + $pageData = $zip->data['page']; + + $this->assertCount(0, $pageData['images']); + } + public function test_cross_reference_links_external_to_export_are_not_converted() { $page = $this->entities->page();