Merge pull request #3693 from BookStackApp/local_secure_restricted

Addition of a `local_secure_restricted` image storage option

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';
     }
 
     /**

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

app/Entities/Tools/ExportFormatter.php

@@ -235,7 +235,7 @@ class ExportFormatter
         $linksOutput = [];
         preg_match_all("/\<a.*href\=(\'|\")(.*?)(\'|\").*?\>/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;
     }
 

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

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

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

database/migrations/2022_09_02_082910_fix_shelf_cover_image_types.php

@@ -0,0 +1,42 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Support\Facades\DB;
+
+class FixShelfCoverImageTypes extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        // This updates the 'type' field for images, uploaded as shelf cover images,
+        // to be cover_bookshelf instead of cover_book.
+        // This does not fix their paths, since fixing that requires a more complicated operation,
+        // but their path does not affect functionality at time of this fix.
+
+        $shelfImageIds = DB::table('bookshelves')
+            ->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']);
+    }
+}

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()

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

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 .= "<img src=\"{$imageUrl}\">";
+        $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();