Merge branch 'v23-10' into development

app/Config/app.php

@@ -141,7 +141,6 @@ return [
         // Third party service providers
         Barryvdh\DomPDF\ServiceProvider::class,
         Barryvdh\Snappy\ServiceProvider::class,
-        Intervention\Image\ImageServiceProvider::class,
         SocialiteProviders\Manager\ServiceProvider::class,
 
         // BookStack custom service providers
@@ -161,9 +160,6 @@ return [
         // Laravel Packages
         'Socialite'    => Laravel\Socialite\Facades\Socialite::class,
 
-        // Third Party
-        'ImageTool' => Intervention\Image\Facades\Image::class,
-
         // Custom BookStack
         'Activity'    => BookStack\Facades\Activity::class,
         'Theme'       => BookStack\Facades\Theme::class,

app/Entities/Repos/PageRepo.php

@@ -211,13 +211,13 @@ class PageRepo
         $inputEmpty = empty($input['markdown']) && empty($input['html']);
 
         if ($haveInput && $inputEmpty) {
-            $pageContent->setNewHTML('');
+            $pageContent->setNewHTML('', user());
         } elseif (!empty($input['markdown']) && is_string($input['markdown'])) {
             $newEditor = 'markdown';
-            $pageContent->setNewMarkdown($input['markdown']);
+            $pageContent->setNewMarkdown($input['markdown'], user());
         } elseif (isset($input['html'])) {
             $newEditor = 'wysiwyg';
-            $pageContent->setNewHTML($input['html']);
+            $pageContent->setNewHTML($input['html'], user());
         }
 
         if ($newEditor !== $currentEditor && userCan('editor-change')) {
@@ -284,9 +284,9 @@ class PageRepo
         $content = new PageContent($page);
 
         if (!empty($revision->markdown)) {
-            $content->setNewMarkdown($revision->markdown);
+            $content->setNewMarkdown($revision->markdown, user());
         } else {
-            $content->setNewHTML($revision->html);
+            $content->setNewHTML($revision->html, user());
         }
 
         $page->updated_by = user()->id;

app/Entities/Tools/PageContent.php

@@ -9,8 +9,10 @@ use BookStack\Facades\Theme;
 use BookStack\Theming\ThemeEvents;
 use BookStack\Uploads\ImageRepo;
 use BookStack\Uploads\ImageService;
+use BookStack\Users\Models\User;
 use BookStack\Util\HtmlContentFilter;
 use BookStack\Util\HtmlDocument;
+use BookStack\Util\WebSafeMimeSniffer;
 use Closure;
 use DOMElement;
 use DOMNode;
@@ -27,9 +29,9 @@ class PageContent
     /**
      * Update the content of the page with new provided HTML.
      */
-    public function setNewHTML(string $html): void
+    public function setNewHTML(string $html, User $updater): void
     {
-        $html = $this->extractBase64ImagesFromHtml($html);
+        $html = $this->extractBase64ImagesFromHtml($html, $updater);
         $this->page->html = $this->formatHtml($html);
         $this->page->text = $this->toPlainText();
         $this->page->markdown = '';
@@ -38,9 +40,9 @@ class PageContent
     /**
      * Update the content of the page with new provided Markdown content.
      */
-    public function setNewMarkdown(string $markdown): void
+    public function setNewMarkdown(string $markdown, User $updater): void
     {
-        $markdown = $this->extractBase64ImagesFromMarkdown($markdown);
+        $markdown = $this->extractBase64ImagesFromMarkdown($markdown, $updater);
         $this->page->markdown = $markdown;
         $html = (new MarkdownToHtml($markdown))->convert();
         $this->page->html = $this->formatHtml($html);
@@ -50,7 +52,7 @@ class PageContent
     /**
      * Convert all base64 image data to saved images.
      */
-    protected function extractBase64ImagesFromHtml(string $htmlText): string
+    protected function extractBase64ImagesFromHtml(string $htmlText, User $updater): string
     {
         if (empty($htmlText) || !str_contains($htmlText, 'data:image')) {
             return $htmlText;
@@ -62,7 +64,7 @@ class PageContent
         $imageNodes = $doc->queryXPath('//img[contains(@src, \'data:image\')]');
         foreach ($imageNodes as $imageNode) {
             $imageSrc = $imageNode->getAttribute('src');
-            $newUrl = $this->base64ImageUriToUploadedImageUrl($imageSrc);
+            $newUrl = $this->base64ImageUriToUploadedImageUrl($imageSrc, $updater);
             $imageNode->setAttribute('src', $newUrl);
         }
 
@@ -76,7 +78,7 @@ class PageContent
      * Attempting to capture the whole data uri using regex can cause PHP
      * PCRE limits to be hit with larger, multi-MB, files.
      */
-    protected function extractBase64ImagesFromMarkdown(string $markdown): string
+    protected function extractBase64ImagesFromMarkdown(string $markdown, User $updater): string
     {
         $matches = [];
         $contentLength = strlen($markdown);
@@ -94,7 +96,7 @@ class PageContent
                 $dataUri .= $char;
             }
 
-            $newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri);
+            $newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri, $updater);
             $replacements[] = [$dataUri, $newUrl];
         }
 
@@ -109,16 +111,28 @@ class PageContent
      * Parse the given base64 image URI and return the URL to the created image instance.
      * Returns an empty string if the parsed URI is invalid or causes an error upon upload.
      */
-    protected function base64ImageUriToUploadedImageUrl(string $uri): string
+    protected function base64ImageUriToUploadedImageUrl(string $uri, User $updater): string
     {
         $imageRepo = app()->make(ImageRepo::class);
         $imageInfo = $this->parseBase64ImageUri($uri);
 
+        // Validate user has permission to create images
+        if (!$updater->can('image-create-all')) {
+            return '';
+        }
+
         // Validate extension and content
         if (empty($imageInfo['data']) || !ImageService::isExtensionSupported($imageInfo['extension'])) {
             return '';
         }
 
+        // Validate content looks like an image via sniffing mime type
+        $mimeSniffer = new WebSafeMimeSniffer();
+        $mime = $mimeSniffer->sniff($imageInfo['data']);
+        if (!str_starts_with($mime, 'image/')) {
+            return '';
+        }
+
         // Validate that the content is not over our upload limit
         $uploadLimitBytes = (config('app.upload_limit') * 1000000);
         if (strlen($imageInfo['data']) > $uploadLimitBytes) {

app/Uploads/FaviconHandler.php

@@ -3,14 +3,13 @@
 namespace BookStack\Uploads;
 
 use Illuminate\Http\UploadedFile;
-use Intervention\Image\ImageManager;
 
 class FaviconHandler
 {
     protected string $path;
 
     public function __construct(
-        protected ImageManager $imageTool
+        protected ImageResizer $imageResizer,
     ) {
         $this->path = public_path('favicon.ico');
     }
@@ -25,10 +24,8 @@ class FaviconHandler
         }
 
         $imageData = file_get_contents($file->getRealPath());
-        $image = $this->imageTool->make($imageData);
+        $pngData = $this->imageResizer->resizeImageData($imageData, 32, 32, false, 'png');
-        $image->resize(32, 32);
+        $icoData = $this->pngToIco($pngData, 32, 32);
-        $bmpData = $image->encode('png');
-        $icoData = $this->pngToIco($bmpData, 32, 32);
 
         file_put_contents($this->path, $icoData);
     }
@@ -81,7 +78,7 @@ class FaviconHandler
      * Built following the file format info from Wikipedia:
      * https://en.wikipedia.org/wiki/ICO_(file_format)
      */
-    protected function pngToIco(string $bmpData, int $width, int $height): string
+    protected function pngToIco(string $pngData, int $width, int $height): string
     {
         // ICO header
         $header = pack('v', 0x00); // Reserved. Must always be 0
@@ -100,11 +97,11 @@ class FaviconHandler
         // via intervention from png typically provides this as 24.
         $entry .= pack('v', 0x00);
         // Size of the image data in bytes
-        $entry .= pack('V', strlen($bmpData));
+        $entry .= pack('V', strlen($pngData));
         // Offset of the bmp data from file start
         $entry .= pack('V', strlen($header) + strlen($entry) + 4);
 
         // Join & return the combined parts of the ICO image data
-        return $header . $entry . $bmpData;
+        return $header . $entry . $pngData;
     }
 }

app/Uploads/ImageResizer.php

@@ -6,15 +6,14 @@ use BookStack\Exceptions\ImageUploadException;
 use Exception;
 use GuzzleHttp\Psr7\Utils;
 use Illuminate\Support\Facades\Cache;
+use Intervention\Image\Gd\Driver;
 use Intervention\Image\Image as InterventionImage;
-use Intervention\Image\ImageManager;
 
 class ImageResizer
 {
     protected const THUMBNAIL_CACHE_TIME = 604_800; // 1 week
 
     public function __construct(
-        protected ImageManager $intervention,
         protected ImageStorage $storage,
     ) {
     }
@@ -105,13 +104,19 @@ class ImageResizer
 
     /**
      * Resize the image of given data to the specified size, and return the new image data.
+     * Format will remain the same as the input format, unless specified.
      *
      * @throws ImageUploadException
      */
-    public function resizeImageData(string $imageData, ?int $width, ?int $height, bool $keepRatio): string
+    public function resizeImageData(
-    {
+        string $imageData,
+        ?int $width,
+        ?int $height,
+        bool $keepRatio,
+        ?string $format = null,
+    ): string {
         try {
-            $thumb = $this->intervention->make($imageData);
+            $thumb = $this->interventionFromImageData($imageData);
         } catch (Exception $e) {
             throw new ImageUploadException(trans('errors.cannot_create_thumbs'));
         }
@@ -127,7 +132,7 @@ class ImageResizer
             $thumb->fit($width, $height);
         }
 
-        $thumbData = (string) $thumb->encode();
+        $thumbData = (string) $thumb->encode($format);
 
         // Use original image data if we're keeping the ratio
         // and the resizing does not save any space.
@@ -138,6 +143,17 @@ class ImageResizer
         return $thumbData;
     }
 
+    /**
+     * Create an intervention image instance from the given image data.
+     * Performs some manual library usage to ensure image is specifically loaded
+     * from given binary data instead of data being misinterpreted.
+     */
+    protected function interventionFromImageData(string $imageData): InterventionImage
+    {
+        $driver = new Driver();
+        return $driver->decoder->initFromBinary($imageData);
+    }
+
     /**
      * Orientate the given intervention image based upon the given original image data.
      * Intervention does have an `orientate` method but the exif data it needs is lost before it

app/Users/Models/User.php

@@ -160,10 +160,6 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon
      */
     public function can(string $permissionName): bool
     {
-        if ($this->email === 'guest') {
-            return false;
-        }
-
         return $this->permissions()->contains($permissionName);
     }
 

tests/Entity/PageContentTest.php

@@ -630,6 +630,35 @@ class PageContentTest extends TestCase
         }
     }
 
+    public function test_base64_images_within_html_blanked_if_no_image_create_permission()
+    {
+        $editor = $this->users->editor();
+        $page = $this->entities->page();
+        $this->permissions->removeUserRolePermissions($editor, ['image-create-all']);
+
+        $this->actingAs($editor)->put($page->getUrl(), [
+            'name' => $page->name,
+            'html' => '<p>test<img src="data:image/jpeg;base64,' . $this->base64Jpeg . '"/></p>',
+        ]);
+
+        $page->refresh();
+        $this->assertStringMatchesFormat('%A<p%A>test<img src="">%A</p>%A', $page->html);
+    }
+
+    public function test_base64_images_within_html_blanked_if_content_does_not_appear_like_an_image()
+    {
+        $page = $this->entities->page();
+
+        $imgContent = base64_encode('file://test/a/b/c');
+        $this->asEditor()->put($page->getUrl(), [
+            'name' => $page->name,
+            'html' => '<p>test<img src="data:image/jpeg;base64,' . $imgContent . '"/></p>',
+        ]);
+
+        $page->refresh();
+        $this->assertStringMatchesFormat('%A<p%A>test<img src="">%A</p>%A', $page->html);
+    }
+
     public function test_base64_images_get_extracted_from_markdown_page_content()
     {
         $this->asEditor();
@@ -663,7 +692,7 @@ class PageContentTest extends TestCase
         ini_set('pcre.backtrack_limit', '500');
         ini_set('pcre.recursion_limit', '500');
 
-        $content = str_repeat('a', 5000);
+        $content = str_repeat(base64_decode($this->base64Jpeg), 50);
         $base64Content = base64_encode($content);
 
         $this->put($page->getUrl(), [
@@ -697,6 +726,34 @@ class PageContentTest extends TestCase
         $this->assertStringContainsString('<img src=""', $page->refresh()->html);
     }
 
+    public function test_base64_images_within_markdown_blanked_if_no_image_create_permission()
+    {
+        $editor = $this->users->editor();
+        $page = $this->entities->page();
+        $this->permissions->removeUserRolePermissions($editor, ['image-create-all']);
+
+        $this->actingAs($editor)->put($page->getUrl(), [
+            'name' => $page->name,
+            'markdown' => 'test ![test](data:image/jpeg;base64,' . $this->base64Jpeg . ')',
+        ]);
+
+        $this->assertStringContainsString('<img src=""', $page->refresh()->html);
+    }
+
+    public function test_base64_images_within_markdown_blanked_if_content_does_not_appear_like_an_image()
+    {
+        $page = $this->entities->page();
+
+        $imgContent = base64_encode('file://test/a/b/c');
+        $this->asEditor()->put($page->getUrl(), [
+            'name' => $page->name,
+            'markdown' => 'test ![test](data:image/jpeg;base64,' . $imgContent . ')',
+        ]);
+
+        $page->refresh();
+        $this->assertStringContainsString('<img src=""', $page->refresh()->html);
+    }
+
     public function test_nested_headers_gets_assigned_an_id()
     {
         $page = $this->entities->page();

tests/ThemeTest.php

@@ -78,7 +78,7 @@ class ThemeTest extends TestCase
 
         $page = $this->entities->page();
         $content = new PageContent($page);
-        $content->setNewMarkdown('# test');
+        $content->setNewMarkdown('# test', $this->users->editor());
 
         $this->assertTrue($callbackCalled);
     }