Updated showImage file serving to not be traversable

For #3030

app/Http/Controllers/Images/ImageController.php

@@ -7,25 +7,31 @@ use BookStack\Exceptions\NotFoundException;
 use BookStack\Http\Controllers\Controller;
 use BookStack\Uploads\Image;
 use BookStack\Uploads\ImageRepo;
+use BookStack\Uploads\ImageService;
 use Exception;
 use Illuminate\Filesystem\Filesystem as File;
+use Illuminate\Filesystem\FilesystemAdapter;
 use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Storage;
 use Illuminate\Validation\ValidationException;
+use League\Flysystem\Util;
 
 class ImageController extends Controller
 {
     protected $image;
     protected $file;
     protected $imageRepo;
+    protected $imageService;
 
     /**
      * ImageController constructor.
      */
-    public function __construct(Image $image, File $file, ImageRepo $imageRepo)
+    public function __construct(Image $image, File $file, ImageRepo $imageRepo, ImageService $imageService)
     {
         $this->image = $image;
         $this->file = $file;
         $this->imageRepo = $imageRepo;
+        $this->imageService = $imageService;
     }
 
     /**
@@ -35,14 +41,13 @@ class ImageController extends Controller
      */
     public function showImage(string $path)
     {
-        $path = storage_path('uploads/images/' . $path);
-        if (!file_exists($path)) {
+        if (!$this->imageService->pathExistsInLocalSecure($path)) {
             throw (new NotFoundException(trans('errors.image_not_found')))
                 ->setSubtitle(trans('errors.image_not_found_subtitle'))
                 ->setDetails(trans('errors.image_not_found_details'));
         }
 
-        return response()->file($path);
+        return $this->imageService->streamImageFromStorageResponse('gallery', $path);
     }
 
     /**

app/Uploads/AttachmentService.php

@@ -27,7 +27,7 @@ class AttachmentService
     /**
      * Get the storage that will be used for storing files.
      */
-    protected function getStorage(): FileSystemInstance
+    protected function getStorageDisk(): FileSystemInstance
     {
         return $this->fileSystem->disk($this->getStorageDiskName());
     }
@@ -70,7 +70,7 @@ class AttachmentService
      */
     public function getAttachmentFromStorage(Attachment $attachment): string
     {
-        return $this->getStorage()->get($this->adjustPathForStorageDisk($attachment->path));
+        return $this->getStorageDisk()->get($this->adjustPathForStorageDisk($attachment->path));
     }
 
     /**
@@ -195,7 +195,7 @@ class AttachmentService
      */
     protected function deleteFileInStorage(Attachment $attachment)
     {
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
         $dirPath = $this->adjustPathForStorageDisk(dirname($attachment->path));
 
         $storage->delete($this->adjustPathForStorageDisk($attachment->path));
@@ -213,7 +213,7 @@ class AttachmentService
     {
         $attachmentData = file_get_contents($uploadedFile->getRealPath());
 
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
         $basePath = 'uploads/files/' . date('Y-m-M') . '/';
 
         $uploadFileName = Str::random(16) . '.' . $uploadedFile->getClientOriginalExtension();

app/Uploads/ImageService.php

@@ -16,6 +16,7 @@ use Intervention\Image\Exception\NotSupportedException;
 use Intervention\Image\ImageManager;
 use League\Flysystem\Util;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Symfony\Component\HttpFoundation\StreamedResponse;
 
 class ImageService
 {
@@ -39,11 +40,20 @@ class ImageService
     /**
      * Get the storage that will be used for storing images.
      */
-    protected function getStorage(string $imageType = ''): FileSystemInstance
+    protected function getStorageDisk(string $imageType = ''): FileSystemInstance
     {
         return $this->fileSystem->disk($this->getStorageDiskName($imageType));
     }
 
+    /**
+     * Check if local secure image storage (Fetched behind authentication)
+     * is currently active in the instance.
+     */
+    protected function usingSecureImages(): bool
+    {
+        return $this->getStorageDiskName('gallery') === 'local_secure_images';
+    }
+
     /**
      * Change the originally provided path to fit any disk-specific requirements.
      * This also ensures the path is kept to the expected root folders.
@@ -126,7 +136,7 @@ class ImageService
      */
     public function saveNew(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image
     {
-        $storage = $this->getStorage($type);
+        $storage = $this->getStorageDisk($type);
         $secureUploads = setting('app-secure-images');
         $fileName = $this->cleanImageFileName($imageName);
 
@@ -243,7 +253,7 @@ class ImageService
             return $this->getPublicUrl($thumbFilePath);
         }
 
-        $storage = $this->getStorage($image->type);
+        $storage = $this->getStorageDisk($image->type);
         if ($storage->exists($this->adjustPathForStorageDisk($thumbFilePath, $image->type))) {
             return $this->getPublicUrl($thumbFilePath);
         }
@@ -307,7 +317,7 @@ class ImageService
      */
     public function getImageData(Image $image): string
     {
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
 
         return $storage->get($this->adjustPathForStorageDisk($image->path, $image->type));
     }
@@ -330,7 +340,7 @@ class ImageService
     protected function destroyImagesFromPath(string $path, string $imageType): bool
     {
         $path = $this->adjustPathForStorageDisk($path, $imageType);
-        $storage = $this->getStorage($imageType);
+        $storage = $this->getStorageDisk($imageType);
 
         $imageFolder = dirname($path);
         $imageFileName = basename($path);
@@ -417,7 +427,7 @@ class ImageService
         }
 
         $storagePath = $this->adjustPathForStorageDisk($storagePath);
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
         $imageData = null;
         if ($storage->exists($storagePath)) {
             $imageData = $storage->get($storagePath);
@@ -435,6 +445,31 @@ class ImageService
         return 'data:image/' . $extension . ';base64,' . base64_encode($imageData);
     }
 
+    /**
+     * Check if the given path exists in the local secure image system.
+     * Returns false if local_secure is not in use.
+     */
+    public function pathExistsInLocalSecure(string $imagePath): bool
+    {
+        $disk = $this->getStorageDisk('gallery');
+
+        // Check local_secure is active
+        return $this->usingSecureImages()
+            // Check the image file exists
+            && $disk->exists($imagePath)
+            // Check the file is likely an image file
+            && strpos($disk->getMimetype($imagePath), 'image/') === 0;
+    }
+
+    /**
+     * For the given path, if existing, provide a response that will stream the image contents.
+     */
+    public function streamImageFromStorageResponse(string $imageType, string $path): StreamedResponse
+    {
+        $disk = $this->getStorageDisk($imageType);
+        return $disk->response($path);
+    }
+
     /**
      * Get a storage path for the given image URL.
      * Ensures the path will start with "uploads/images".

tests/Uploads/ImageTest.php

@@ -241,6 +241,36 @@ class ImageTest extends TestCase
         }
     }
 
+    public function test_secure_image_paths_traversal_causes_500()
+    {
+        config()->set('filesystems.images', 'local_secure');
+        $this->asEditor();
+
+        $resp = $this->get('/uploads/images/../../logs/laravel.log');
+        $resp->assertStatus(500);
+    }
+
+    public function test_secure_image_paths_traversal_on_non_secure_images_causes_404()
+    {
+        config()->set('filesystems.images', 'local');
+        $this->asEditor();
+
+        $resp = $this->get('/uploads/images/../../logs/laravel.log');
+        $resp->assertStatus(404);
+    }
+
+    public function test_secure_image_paths_dont_serve_non_images()
+    {
+        config()->set('filesystems.images', 'local_secure');
+        $this->asEditor();
+
+        $testFilePath = storage_path('/uploads/images/testing.txt');
+        file_put_contents($testFilePath, 'hello from test_secure_image_paths_dont_serve_non_images');
+
+        $resp = $this->get('/uploads/images/testing.txt');
+        $resp->assertStatus(404);
+    }
+
     public function test_secure_images_included_in_exports()
     {
         config()->set('filesystems.images', 'local_secure');