Input WYSIWYG: Updated tests, Added simple html limiting

diff --git a/app/Entities/Repos/BaseRepo.php b/app/Entities/Repos/BaseRepo.php
index 3d3d16732a6057b89b41084b9ba9f7502555c9f9..b52f3974ef80208cb639f34b1c92d4e4d15138e0 100644 (file)
--- a/app/Entities/Repos/BaseRepo.php
+++ b/app/Entities/Repos/BaseRepo.php
@@ -10,6 +10,7 @@ use BookStack\Exceptions\ImageUploadException;
 use BookStack\References\ReferenceStore;
 use BookStack\References\ReferenceUpdater;
 use BookStack\Uploads\ImageRepo;
 use BookStack\References\ReferenceStore;
 use BookStack\References\ReferenceUpdater;
 use BookStack\Uploads\ImageRepo;

+use BookStack\Util\HtmlDescriptionFilter;

 use Illuminate\Http\UploadedFile;
 
 class BaseRepo
 use Illuminate\Http\UploadedFile;
 
 class BaseRepo


@@ -111,7 +112,7 @@ class BaseRepo
 
         /** @var HasHtmlDescription $entity */
         if (isset($input['description_html'])) {
 
         /** @var HasHtmlDescription $entity */
         if (isset($input['description_html'])) {

-            $entity->description_html = $input['description_html'];
+            $entity->description_html = HtmlDescriptionFilter::filterFromString($input['description_html']);

             $entity->description = html_entity_decode(strip_tags($input['description_html']));
         } else if (isset($input['description'])) {
             $entity->description = $input['description'];
             $entity->description = html_entity_decode(strip_tags($input['description_html']));
         } else if (isset($input['description'])) {
             $entity->description = $input['description'];

diff --git a/app/Util/HtmlDescriptionFilter.php b/app/Util/HtmlDescriptionFilter.php
new file mode 100644 (file)
index 0000000..4494a73
--- /dev/null
+++ b/app/Util/HtmlDescriptionFilter.php
@@ -0,0 +1,75 @@
+<?php
+
+namespace BookStack\Util;
+
+use DOMAttr;
+use DOMElement;
+use DOMNamedNodeMap;
+use DOMNode;
+
+/**
+ * Filter to ensure HTML input for description content remains simple and
+ * to a limited allow-list of elements and attributes.
+ * More for consistency and to prevent nuisance rather than for security
+ * (which would be done via a separate content filter and CSP).
+ */
+class HtmlDescriptionFilter
+{
+    /**
+     * @var array<string, string[]>
+     */
+    protected static array $allowedAttrsByElements = [
+        'p' => [],
+        'a' => ['href', 'title'],
+        'ol' => [],
+        'ul' => [],
+        'li' => [],
+        'strong' => [],
+        'em' => [],
+        'br' => [],
+    ];
+
+    public static function filterFromString(string $html): string
+    {
+        $doc = new HtmlDocument($html);
+
+        $topLevel = [...$doc->getBodyChildren()];
+        foreach ($topLevel as $child) {
+            /** @var DOMNode $child */
+            if ($child instanceof DOMElement) {
+                static::filterElement($child);
+            } else {
+                $child->parentNode->removeChild($child);
+            }
+        }
+
+        return $doc->getBodyInnerHtml();
+    }
+
+    protected static function filterElement(DOMElement $element): void
+    {
+        $elType = strtolower($element->tagName);
+        $allowedAttrs = static::$allowedAttrsByElements[$elType] ?? null;
+        if (is_null($allowedAttrs)) {
+            $element->remove();
+            return;
+        }
+
+        /** @var DOMNamedNodeMap $attrs */
+        $attrs = $element->attributes;
+        for ($i = $attrs->length - 1; $i >= 0; $i--) {
+            /** @var DOMAttr $attr */
+            $attr = $attrs->item($i);
+            $name = strtolower($attr->name);
+            if (!in_array($name, $allowedAttrs)) {
+                $element->removeAttribute($attr->name);
+            }
+        }
+
+        foreach ($element->childNodes as $child) {
+            if ($child instanceof DOMElement) {
+                static::filterElement($child);
+            }
+        }
+    }
+}

diff --git a/database/factories/Entities/Models/BookFactory.php b/database/factories/Entities/Models/BookFactory.php
index 3bf15778670387554fd7f655147610a200103a32..9cb8e971c6e14e16542ea4d203da45a4d3511ff5 100644 (file)
--- a/database/factories/Entities/Models/BookFactory.php
+++ b/database/factories/Entities/Models/BookFactory.php
@@ -21,10 +21,12 @@ class BookFactory extends Factory
      */
     public function definition()
     {
      */
     public function definition()
     {

+        $description = $this->faker->paragraph();

         return [
             'name'        => $this->faker->sentence(),
             'slug'        => Str::random(10),
         return [
             'name'        => $this->faker->sentence(),
             'slug'        => Str::random(10),

-            'description' => $this->faker->paragraph(),
+            'description' => $description,
+            'description_html' => '<p>' . e($description) . '</p>'

         ];
     }
 }
         ];
     }
 }

diff --git a/database/factories/Entities/Models/BookshelfFactory.php b/database/factories/Entities/Models/BookshelfFactory.php
index 66dd1c111851419004761b24dc9377e2cf8c0dd6..edbefc3e797b721c50ebd90b38a7acfc088f1209 100644 (file)
--- a/database/factories/Entities/Models/BookshelfFactory.php
+++ b/database/factories/Entities/Models/BookshelfFactory.php
@@ -21,10 +21,12 @@ class BookshelfFactory extends Factory
      */
     public function definition()
     {
      */
     public function definition()
     {

+        $description = $this->faker->paragraph();

         return [
             'name'        => $this->faker->sentence,
             'slug'        => Str::random(10),
         return [
             'name'        => $this->faker->sentence,
             'slug'        => Str::random(10),

-            'description' => $this->faker->paragraph,
+            'description' => $description,
+            'description_html' => '<p>' . e($description) . '</p>'

         ];
     }
 }
         ];
     }
 }

diff --git a/database/factories/Entities/Models/ChapterFactory.php b/database/factories/Entities/Models/ChapterFactory.php
index 36379866ed85bfafe7822413ed99510ba751ab4b..1fc49933ef766adcf5ff37639d9fd3a7424759b5 100644 (file)
--- a/database/factories/Entities/Models/ChapterFactory.php
+++ b/database/factories/Entities/Models/ChapterFactory.php
@@ -21,10 +21,12 @@ class ChapterFactory extends Factory
      */
     public function definition()
     {
      */
     public function definition()
     {

+        $description = $this->faker->paragraph();

         return [
             'name'        => $this->faker->sentence(),
             'slug'        => Str::random(10),
         return [
             'name'        => $this->faker->sentence(),
             'slug'        => Str::random(10),

-            'description' => $this->faker->paragraph(),
+            'description' => $description,
+            'description_html' => '<p>' . e($description) . '</p>'

         ];
     }
 }
         ];
     }
 }

diff --git a/resources/js/wysiwyg/config.js b/resources/js/wysiwyg/config.js
index 2bd4b630b038a35eecf9de0e301079ebdd85b4fa..f669849ade879821160509ca4a0bbe374b5108b5 100644 (file)
--- a/resources/js/wysiwyg/config.js
+++ b/resources/js/wysiwyg/config.js
@@ -329,7 +329,7 @@ export function buildForInput(options) {
         menubar: false,
         plugins: 'link autolink lists',
         contextmenu: false,
         menubar: false,
         plugins: 'link autolink lists',
         contextmenu: false,

-        toolbar: 'bold italic underline link bullist numlist',
+        toolbar: 'bold italic link bullist numlist',

         content_style: getContentStyle(options),
         color_map: colorMap,
         file_picker_types: 'file',
         content_style: getContentStyle(options),
         color_map: colorMap,
         file_picker_types: 'file',

diff --git a/tests/Entity/BookShelfTest.php b/tests/Entity/BookShelfTest.php
index c1842c175a791c335810179a7111c5c49ae31a5e..7f6542d5c710d4f4abfc5d61666fcfcf0edabeaa 100644 (file)
--- a/tests/Entity/BookShelfTest.php
+++ b/tests/Entity/BookShelfTest.php
@@ -77,8 +77,8 @@ class BookShelfTest extends TestCase
     {
         $booksToInclude = Book::take(2)->get();
         $shelfInfo = [
     {
         $booksToInclude = Book::take(2)->get();
         $shelfInfo = [

-            'name'        => 'My test book' . Str::random(4),
-            'description' => 'Test book description ' . Str::random(10),
+            'name'             => 'My test shelf' . Str::random(4),
+            'description_html' => '<p>Test book description ' . Str::random(10) . '</p>',

         ];
         $resp = $this->asEditor()->post('/shelves', array_merge($shelfInfo, [
             'books' => $booksToInclude->implode('id', ','),
         ];
         $resp = $this->asEditor()->post('/shelves', array_merge($shelfInfo, [
             'books' => $booksToInclude->implode('id', ','),


@@ -96,7 +96,7 @@ class BookShelfTest extends TestCase
         $shelf = Bookshelf::where('name', '=', $shelfInfo['name'])->first();
         $shelfPage = $this->get($shelf->getUrl());
         $shelfPage->assertSee($shelfInfo['name']);
         $shelf = Bookshelf::where('name', '=', $shelfInfo['name'])->first();
         $shelfPage = $this->get($shelf->getUrl());
         $shelfPage->assertSee($shelfInfo['name']);

-        $shelfPage->assertSee($shelfInfo['description']);
+        $shelfPage->assertSee($shelfInfo['description_html'], false);

         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Category');
         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Tag Value');
 
         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Category');
         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Tag Value');
 


@@ -107,8 +107,8 @@ class BookShelfTest extends TestCase
     public function test_shelves_create_sets_cover_image()
     {
         $shelfInfo = [
     public function test_shelves_create_sets_cover_image()
     {
         $shelfInfo = [

-            'name'        => 'My test book' . Str::random(4),
-            'description' => 'Test book description ' . Str::random(10),
+            'name'             => 'My test shelf' . Str::random(4),
+            'description_html' => '<p>Test book description ' . Str::random(10) . '</p>',

         ];
 
         $imageFile = $this->files->uploadedImage('shelf-test.png');
         ];
 
         $imageFile = $this->files->uploadedImage('shelf-test.png');


@@ -174,7 +174,7 @@ class BookShelfTest extends TestCase
         // Set book ordering
         $this->asAdmin()->put($shelf->getUrl(), [
             'books' => $books->implode('id', ','),
         // Set book ordering
         $this->asAdmin()->put($shelf->getUrl(), [
             'books' => $books->implode('id', ','),

-            'tags'  => [], 'description' => 'abc', 'name' => 'abc',
+            'tags'  => [], 'description_html' => 'abc', 'name' => 'abc',

         ]);
         $this->assertEquals(3, $shelf->books()->count());
         $shelf->refresh();
         ]);
         $this->assertEquals(3, $shelf->books()->count());
         $shelf->refresh();


@@ -207,7 +207,7 @@ class BookShelfTest extends TestCase
         // Set book ordering
         $this->asAdmin()->put($shelf->getUrl(), [
             'books' => $books->implode('id', ','),
         // Set book ordering
         $this->asAdmin()->put($shelf->getUrl(), [
             'books' => $books->implode('id', ','),

-            'tags'  => [], 'description' => 'abc', 'name' => 'abc',
+            'tags'  => [], 'description_html' => 'abc', 'name' => 'abc',

         ]);
         $this->assertEquals(3, $shelf->books()->count());
         $shelf->refresh();
         ]);
         $this->assertEquals(3, $shelf->books()->count());
         $shelf->refresh();


@@ -229,8 +229,8 @@ class BookShelfTest extends TestCase
 
         $booksToInclude = Book::take(2)->get();
         $shelfInfo = [
 
         $booksToInclude = Book::take(2)->get();
         $shelfInfo = [

-            'name'        => 'My test book' . Str::random(4),
-            'description' => 'Test book description ' . Str::random(10),
+            'name'             => 'My test shelf' . Str::random(4),
+            'description_html' => '<p>Test book description ' . Str::random(10) . '</p>',

         ];
 
         $resp = $this->asEditor()->put($shelf->getUrl(), array_merge($shelfInfo, [
         ];
 
         $resp = $this->asEditor()->put($shelf->getUrl(), array_merge($shelfInfo, [


@@ -251,7 +251,7 @@ class BookShelfTest extends TestCase
 
         $shelfPage = $this->get($shelf->getUrl());
         $shelfPage->assertSee($shelfInfo['name']);
 
         $shelfPage = $this->get($shelf->getUrl());
         $shelfPage->assertSee($shelfInfo['name']);

-        $shelfPage->assertSee($shelfInfo['description']);
+        $shelfPage->assertSee($shelfInfo['description_html'], false);

         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Category');
         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Tag Value');
 
         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Category');
         $this->withHtml($shelfPage)->assertElementContains('.tag-item', 'Test Tag Value');
 


@@ -270,8 +270,8 @@ class BookShelfTest extends TestCase
         $testName = 'Test Book in Shelf Name';
 
         $createBookResp = $this->asEditor()->post($shelf->getUrl('/create-book'), [
         $testName = 'Test Book in Shelf Name';
 
         $createBookResp = $this->asEditor()->post($shelf->getUrl('/create-book'), [

-            'name'        => $testName,
-            'description' => 'Book in shelf description',
+            'name'             => $testName,
+            'description_html' => 'Book in shelf description',

         ]);
         $createBookResp->assertRedirect();
 
         ]);
         $createBookResp->assertRedirect();
 


@@ -372,8 +372,8 @@ class BookShelfTest extends TestCase
     {
         // Create shelf
         $shelfInfo = [
     {
         // Create shelf
         $shelfInfo = [

-            'name'        => 'My test shelf' . Str::random(4),
-            'description' => 'Test shelf description ' . Str::random(10),
+            'name'             => 'My test shelf' . Str::random(4),
+            'description_html' => '<p>Test shelf description ' . Str::random(10) . '</p>',

         ];
 
         $this->asEditor()->post('/shelves', $shelfInfo);
         ];
 
         $this->asEditor()->post('/shelves', $shelfInfo);


@@ -381,8 +381,8 @@ class BookShelfTest extends TestCase
 
         // Create book and add to shelf
         $this->asEditor()->post($shelf->getUrl('/create-book'), [
 
         // Create book and add to shelf
         $this->asEditor()->post($shelf->getUrl('/create-book'), [

-            'name'        => 'Test book name',
-            'description' => 'Book in shelf description',
+            'name'             => 'Test book name',
+            'description_html' => '<p>Book in shelf description</p>',

         ]);
 
         $newBook = Book::query()->orderBy('id', 'desc')->first();
         ]);
 
         $newBook = Book::query()->orderBy('id', 'desc')->first();

diff --git a/tests/Entity/BookTest.php b/tests/Entity/BookTest.php
index 833cabaae93c112d95e27dc516ccc0d9c2744f36..3e37e61f7d7b394659ab93c14e2ed1451debcd54 100644 (file)
--- a/tests/Entity/BookTest.php
+++ b/tests/Entity/BookTest.php
@@ -22,7 +22,7 @@ class BookTest extends TestCase
         $resp = $this->get('/create-book');
         $this->withHtml($resp)->assertElementContains('form[action="' . url('/books') . '"][method="POST"]', 'Save Book');
 
         $resp = $this->get('/create-book');
         $this->withHtml($resp)->assertElementContains('form[action="' . url('/books') . '"][method="POST"]', 'Save Book');
 

-        $resp = $this->post('/books', $book->only('name', 'description'));
+        $resp = $this->post('/books', $book->only('name', 'description_html'));

         $resp->assertRedirect('/books/my-first-book');
 
         $resp = $this->get('/books/my-first-book');
         $resp->assertRedirect('/books/my-first-book');
 
         $resp = $this->get('/books/my-first-book');


@@ -36,8 +36,8 @@ class BookTest extends TestCase
             'name' => 'My First Book',
         ]);
 
             'name' => 'My First Book',
         ]);
 

-        $this->asEditor()->post('/books', $book->only('name', 'description'));
-        $this->asEditor()->post('/books', $book->only('name', 'description'));
+        $this->asEditor()->post('/books', $book->only('name', 'description_html'));
+        $this->asEditor()->post('/books', $book->only('name', 'description_html'));

 
         $books = Book::query()->where('name', '=', $book->name)
             ->orderBy('id', 'desc')
 
         $books = Book::query()->where('name', '=', $book->name)
             ->orderBy('id', 'desc')


@@ -52,9 +52,9 @@ class BookTest extends TestCase
     {
         // Cheeky initial update to refresh slug
         $this->asEditor()->post('books', [
     {
         // Cheeky initial update to refresh slug
         $this->asEditor()->post('books', [

-            'name'        => 'My book with tags',
-            'description' => 'A book with tags',
-            'tags'        => [
+            'name'             => 'My book with tags',
+            'description_html' => '<p>A book with tags</p>',
+            'tags'             => [

                 [
                     'name'  => 'Category',
                     'value' => 'Donkey Content',
                 [
                     'name'  => 'Category',
                     'value' => 'Donkey Content',


@@ -79,23 +79,23 @@ class BookTest extends TestCase
     {
         $book = $this->entities->book();
         // Cheeky initial update to refresh slug
     {
         $book = $this->entities->book();
         // Cheeky initial update to refresh slug

-        $this->asEditor()->put($book->getUrl(), ['name' => $book->name . '5', 'description' => $book->description]);
+        $this->asEditor()->put($book->getUrl(), ['name' => $book->name . '5', 'description_html' => $book->description_html]);

         $book->refresh();
 
         $newName = $book->name . ' Updated';
         $book->refresh();
 
         $newName = $book->name . ' Updated';

-        $newDesc = $book->description . ' with more content';
+        $newDesc = $book->description_html . '<p>with more content</p>';

 
         $resp = $this->get($book->getUrl('/edit'));
         $resp->assertSee($book->name);
 
         $resp = $this->get($book->getUrl('/edit'));
         $resp->assertSee($book->name);

-        $resp->assertSee($book->description);
+        $resp->assertSee($book->description_html);

         $this->withHtml($resp)->assertElementContains('form[action="' . $book->getUrl() . '"]', 'Save Book');
 
         $this->withHtml($resp)->assertElementContains('form[action="' . $book->getUrl() . '"]', 'Save Book');
 

-        $resp = $this->put($book->getUrl(), ['name' => $newName, 'description' => $newDesc]);
+        $resp = $this->put($book->getUrl(), ['name' => $newName, 'description_html' => $newDesc]);

         $resp->assertRedirect($book->getUrl() . '-updated');
 
         $resp = $this->get($book->getUrl() . '-updated');
         $resp->assertSee($newName);
         $resp->assertRedirect($book->getUrl() . '-updated');
 
         $resp = $this->get($book->getUrl() . '-updated');
         $resp->assertSee($newName);

-        $resp->assertSee($newDesc);
+        $resp->assertSee($newDesc, false);

     }
 
     public function test_update_sets_tags()
     }
 
     public function test_update_sets_tags()


@@ -184,7 +184,7 @@ class BookTest extends TestCase
 
     public function test_recently_viewed_books_updates_as_expected()
     {
 
     public function test_recently_viewed_books_updates_as_expected()
     {

-        $books = Book::all()->take(2);
+        $books = Book::take(2)->get();

 
         $resp = $this->asAdmin()->get('/books');
         $this->withHtml($resp)->assertElementNotContains('#recents', $books[0]->name)
 
         $resp = $this->asAdmin()->get('/books');
         $this->withHtml($resp)->assertElementNotContains('#recents', $books[0]->name)


@@ -200,7 +200,7 @@ class BookTest extends TestCase
 
     public function test_popular_books_updates_upon_visits()
     {
 
     public function test_popular_books_updates_upon_visits()
     {

-        $books = Book::all()->take(2);
+        $books = Book::take(2)->get();

 
         $resp = $this->asAdmin()->get('/books');
         $this->withHtml($resp)->assertElementNotContains('#popular', $books[0]->name)
 
         $resp = $this->asAdmin()->get('/books');
         $this->withHtml($resp)->assertElementNotContains('#popular', $books[0]->name)


@@ -262,6 +262,22 @@ class BookTest extends TestCase
         $this->assertEquals('parta-partb-partc', $book->slug);
     }
 
         $this->assertEquals('parta-partb-partc', $book->slug);
     }
 

+    public function test_description_limited_to_specific_html()
+    {
+        $book = $this->entities->book();
+
+        $input = '<h1>Test</h1><p id="abc" href="beans">Content<a href="#cat" data-a="b">a</a><section>Hello</section></p>';
+        $expected = '<p>Content<a href="#cat">a</a></p>';
+
+        $this->asEditor()->put($book->getUrl(), [
+            'name' => $book->name,
+            'description_html' => $input
+        ]);
+
+        $book->refresh();
+        $this->assertEquals($expected, $book->description_html);
+    }
+

     public function test_show_view_has_copy_button()
     {
         $book = $this->entities->book();
     public function test_show_view_has_copy_button()
     {
         $book = $this->entities->book();

diff --git a/tests/Entity/ChapterTest.php b/tests/Entity/ChapterTest.php
index 7fa32c252675a655748369ee3fab793a57850aa8..a057d91b5603eab598add61c85881a97caccb9df 100644 (file)
--- a/tests/Entity/ChapterTest.php
+++ b/tests/Entity/ChapterTest.php
@@ -23,12 +23,12 @@ class ChapterTest extends TestCase
         $resp = $this->get($book->getUrl('/create-chapter'));
         $this->withHtml($resp)->assertElementContains('form[action="' . $book->getUrl('/create-chapter') . '"][method="POST"]', 'Save Chapter');
 
         $resp = $this->get($book->getUrl('/create-chapter'));
         $this->withHtml($resp)->assertElementContains('form[action="' . $book->getUrl('/create-chapter') . '"][method="POST"]', 'Save Chapter');
 

-        $resp = $this->post($book->getUrl('/create-chapter'), $chapter->only('name', 'description'));
+        $resp = $this->post($book->getUrl('/create-chapter'), $chapter->only('name', 'description_html'));

         $resp->assertRedirect($book->getUrl('/chapter/my-first-chapter'));
 
         $resp = $this->get($book->getUrl('/chapter/my-first-chapter'));
         $resp->assertSee($chapter->name);
         $resp->assertRedirect($book->getUrl('/chapter/my-first-chapter'));
 
         $resp = $this->get($book->getUrl('/chapter/my-first-chapter'));
         $resp->assertSee($chapter->name);

-        $resp->assertSee($chapter->description);
+        $resp->assertSee($chapter->description_html, false);

     }
 
     public function test_delete()
     }
 
     public function test_delete()