Images: Prevented base64 extraction without permission

Also added content sniffing as an extra check.
Added tests to cover.

diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php
index 61a1db63e397dbdfa944d7a2818a37d5f2b9d296..dbd4a47d243e23f3b0f5870e9d14de42ec83a705 100644 (file)
--- a/app/Entities/Repos/PageRepo.php
+++ b/app/Entities/Repos/PageRepo.php
@@ -211,13 +211,13 @@ class PageRepo
         $inputEmpty = empty($input['markdown']) && empty($input['html']);
 
         if ($haveInput && $inputEmpty) {
         $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';
         } 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';
         } elseif (isset($input['html'])) {
             $newEditor = 'wysiwyg';

-            $pageContent->setNewHTML($input['html']);
+            $pageContent->setNewHTML($input['html'], user());

         }
 
         if ($newEditor !== $currentEditor && userCan('editor-change')) {
         }
 
         if ($newEditor !== $currentEditor && userCan('editor-change')) {


@@ -284,9 +284,9 @@ class PageRepo
         $content = new PageContent($page);
 
         if (!empty($revision->markdown)) {
         $content = new PageContent($page);
 
         if (!empty($revision->markdown)) {

-            $content->setNewMarkdown($revision->markdown);
+            $content->setNewMarkdown($revision->markdown, user());

         } else {
         } else {

-            $content->setNewHTML($revision->html);
+            $content->setNewHTML($revision->html, user());

         }
 
         $page->updated_by = user()->id;
         }
 
         $page->updated_by = user()->id;

diff --git a/app/Entities/Tools/PageContent.php b/app/Entities/Tools/PageContent.php
index 949816eff577cc2fc67e0a36c8507bc4d2efe8d1..1bde7b6ce16c4485d2d37028b1726690ad5584c8 100644 (file)
--- a/app/Entities/Tools/PageContent.php
+++ b/app/Entities/Tools/PageContent.php
@@ -9,7 +9,9 @@ use BookStack\Facades\Theme;
 use BookStack\Theming\ThemeEvents;
 use BookStack\Uploads\ImageRepo;
 use BookStack\Uploads\ImageService;
 use BookStack\Theming\ThemeEvents;
 use BookStack\Uploads\ImageRepo;
 use BookStack\Uploads\ImageService;

+use BookStack\Users\Models\User;

 use BookStack\Util\HtmlContentFilter;
 use BookStack\Util\HtmlContentFilter;

+use BookStack\Util\WebSafeMimeSniffer;

 use DOMDocument;
 use DOMElement;
 use DOMNode;
 use DOMDocument;
 use DOMElement;
 use DOMNode;


@@ -27,9 +29,9 @@ class PageContent
     /**
      * Update the content of the page with new provided HTML.
      */
     /**
      * 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 = '';
         $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.
      */
     /**
      * 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);
         $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.
      */
     /**
      * 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;
     {
         if (empty($htmlText) || !str_contains($htmlText, 'data:image')) {
             return $htmlText;


@@ -66,7 +68,7 @@ class PageContent
         $imageNodes = $xPath->query('//img[contains(@src, \'data:image\')]');
         foreach ($imageNodes as $imageNode) {
             $imageSrc = $imageNode->getAttribute('src');
         $imageNodes = $xPath->query('//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);
         }
 
             $imageNode->setAttribute('src', $newUrl);
         }
 


@@ -86,7 +88,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.
      */
      * 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);
     {
         $matches = [];
         $contentLength = strlen($markdown);


@@ -104,7 +106,7 @@ class PageContent
                 $dataUri .= $char;
             }
 
                 $dataUri .= $char;
             }
 

-            $newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri);
+            $newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri, $updater);

             $replacements[] = [$dataUri, $newUrl];
         }
 
             $replacements[] = [$dataUri, $newUrl];
         }
 


@@ -119,16 +121,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.
      */
      * 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);
 
     {
         $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 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) {
         // Validate that the content is not over our upload limit
         $uploadLimitBytes = (config('app.upload_limit') * 1000000);
         if (strlen($imageInfo['data']) > $uploadLimitBytes) {

diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php
index 5bd308ae87707cd425f6ba21b42748cc5a4392b7..3797e7cb0279d27cd88902443d9aafd393857178 100644 (file)
--- a/app/Users/Models/User.php
+++ b/app/Users/Models/User.php
@@ -160,10 +160,6 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon
      */
     public function can(string $permissionName): bool
     {
      */
     public function can(string $permissionName): bool
     {

-        if ($this->email === 'guest') {
-            return false;
-        }
-

         return $this->permissions()->contains($permissionName);
     }
 
         return $this->permissions()->contains($permissionName);
     }
 

diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php
index d8845fe127662d8008b1c88262761f6476810d71..78b739512ba1115d6b800d4a05d47fb4a87e7bf1 100644 (file)
--- a/tests/Entity/PageContentTest.php
+++ b/tests/Entity/PageContentTest.php
@@ -8,7 +8,7 @@ use Tests\TestCase;
 
 class PageContentTest extends TestCase
 {
 
 class PageContentTest extends TestCase
 {

-    protected $base64Jpeg = '/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=';
+    protected string $base64Jpeg = '/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=';

 
     public function test_page_includes()
     {
 
     public function test_page_includes()
     {


@@ -649,6 +649,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();
     public function test_base64_images_get_extracted_from_markdown_page_content()
     {
         $this->asEditor();


@@ -682,7 +711,7 @@ class PageContentTest extends TestCase
         ini_set('pcre.backtrack_limit', '500');
         ini_set('pcre.recursion_limit', '500');
 
         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(), [
         $base64Content = base64_encode($content);
 
         $this->put($page->getUrl(), [


@@ -716,6 +745,34 @@ class PageContentTest extends TestCase
         $this->assertStringContainsString('<img src=""', $page->refresh()->html);
     }
 
         $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();
     public function test_nested_headers_gets_assigned_an_id()
     {
         $page = $this->entities->page();

diff --git a/tests/ThemeTest.php b/tests/ThemeTest.php
index 53361e35194dad7b8fdec639373cc1a673bdfdd4..f6706595bf273015c663ba637fc5f43cdb8f42a2 100644 (file)
--- a/tests/ThemeTest.php
+++ b/tests/ThemeTest.php
@@ -78,7 +78,7 @@ class ThemeTest extends TestCase
 
         $page = $this->entities->page();
         $content = new PageContent($page);
 
         $page = $this->entities->page();
         $content = new PageContent($page);

-        $content->setNewMarkdown('# test');
+        $content->setNewMarkdown('# test', $this->users->editor());

 
         $this->assertTrue($callbackCalled);
     }
 
         $this->assertTrue($callbackCalled);
     }