diff --git a/app/Http/Controllers/Images/ImageController.php b/app/Http/Controllers/Images/ImageController.php index 4070a0e2fe6..66ccadc5e3f 100644 --- a/app/Http/Controllers/Images/ImageController.php +++ b/app/Http/Controllers/Images/ImageController.php @@ -7,25 +7,31 @@ 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 @@ public function __construct(Image $image, File $file, ImageRepo $imageRepo) */ 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); } /** diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index f7a0918c609..c9cd99b389b 100644 --- a/app/Uploads/AttachmentService.php +++ b/app/Uploads/AttachmentService.php @@ -27,7 +27,7 @@ public function __construct(FileSystem $fileSystem) /** * 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 @@ protected function adjustPathForStorageDisk(string $path): string */ 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 @@ public function deleteFile(Attachment $attachment) */ 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 @@ protected function putFileInStorage(UploadedFile $uploadedFile): string { $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(); diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php index d6c74c751c7..dc18783c5d5 100644 --- a/app/Uploads/ImageService.php +++ b/app/Uploads/ImageService.php @@ -16,6 +16,7 @@ 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 @@ public function __construct(Image $image, ImageManager $imageTool, FileSystem $f /** * 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 @@ public function saveNewFromBase64Uri(string $base64Uri, string $name, string $ty */ 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 @@ public function getThumbnail(Image $image, $width = 220, $height = 220, $keepRat 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 @@ protected function resizeImage(string $imageData, $width = 220, $height = null, */ 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 @@ public function destroy(Image $image) 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 @@ public function imageUriToBase64(string $uri): ?string } $storagePath = $this->adjustPathForStorageDisk($storagePath); - $storage = $this->getStorage(); + $storage = $this->getStorageDisk(); $imageData = null; if ($storage->exists($storagePath)) { $imageData = $storage->get($storagePath); @@ -435,6 +445,31 @@ public function imageUriToBase64(string $uri): ?string 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". diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index 69b6dc90e96..296e4d1878a 100644 --- a/tests/Uploads/ImageTest.php +++ b/tests/Uploads/ImageTest.php @@ -241,6 +241,36 @@ public function test_secure_images_uploads_to_correct_place() } } + 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');