Input WYSIWYG: Updated tests, Added simple html limiting

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\Util\HtmlDescriptionFilter;
 use Illuminate\Http\UploadedFile;
 
 class BaseRepo
@@ -111,7 +112,7 @@ class BaseRepo
 
         /** @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'];

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);
+            }
+        }
+    }
+}

database/factories/Entities/Models/BookFactory.php

@@ -21,10 +21,12 @@ class BookFactory extends Factory
      */
     public function definition()
     {
+        $description = $this->faker->paragraph();
         return [
             'name'        => $this->faker->sentence(),
             'slug'        => Str::random(10),
-            'description' => $this->faker->paragraph(),
+            'description' => $description,
+            'description_html' => '<p>' . e($description) . '</p>'
         ];
     }
 }

database/factories/Entities/Models/BookshelfFactory.php

@@ -21,10 +21,12 @@ class BookshelfFactory extends Factory
      */
     public function definition()
     {
+        $description = $this->faker->paragraph();
         return [
             'name'        => $this->faker->sentence,
             'slug'        => Str::random(10),
-            'description' => $this->faker->paragraph,
+            'description' => $description,
+            'description_html' => '<p>' . e($description) . '</p>'
         ];
     }
 }

database/factories/Entities/Models/ChapterFactory.php

@@ -21,10 +21,12 @@ class ChapterFactory extends Factory
      */
     public function definition()
     {
+        $description = $this->faker->paragraph();
         return [
             'name'        => $this->faker->sentence(),
             'slug'        => Str::random(10),
-            'description' => $this->faker->paragraph(),
+            'description' => $description,
+            'description_html' => '<p>' . e($description) . '</p>'
         ];
     }
 }

resources/js/wysiwyg/config.js

@@ -329,7 +329,7 @@ export function buildForInput(options) {
         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',

tests/Entity/BookShelfTest.php

@@ -77,8 +77,8 @@ class BookShelfTest extends TestCase
     {
         $booksToInclude = Book::take(2)->get();
         $shelfInfo = [
-            'name'        => 'My test book' . Str::random(4),
+            'name'             => 'My test shelf' . Str::random(4),
-            'description' => 'Test book description ' . Str::random(10),
+            'description_html' => '<p>Test book description ' . Str::random(10) . '</p>',
         ];
         $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']);
-        $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');
 
@@ -107,8 +107,8 @@ class BookShelfTest extends TestCase
     public function test_shelves_create_sets_cover_image()
     {
         $shelfInfo = [
-            'name'        => 'My test book' . Str::random(4),
+            'name'             => 'My test shelf' . Str::random(4),
-            'description' => 'Test book description ' . Str::random(10),
+            'description_html' => '<p>Test book description ' . Str::random(10) . '</p>',
         ];
 
         $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', ','),
-            'tags'  => [], 'description' => 'abc', 'name' => 'abc',
+            'tags'  => [], 'description_html' => 'abc', 'name' => 'abc',
         ]);
         $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', ','),
-            'tags'  => [], 'description' => 'abc', 'name' => 'abc',
+            'tags'  => [], 'description_html' => 'abc', 'name' => 'abc',
         ]);
         $this->assertEquals(3, $shelf->books()->count());
         $shelf->refresh();
@@ -229,8 +229,8 @@ class BookShelfTest extends TestCase
 
         $booksToInclude = Book::take(2)->get();
         $shelfInfo = [
-            'name'        => 'My test book' . Str::random(4),
+            'name'             => 'My test shelf' . Str::random(4),
-            'description' => 'Test book description ' . Str::random(10),
+            'description_html' => '<p>Test book description ' . Str::random(10) . '</p>',
         ];
 
         $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->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');
 
@@ -270,8 +270,8 @@ class BookShelfTest extends TestCase
         $testName = 'Test Book in Shelf Name';
 
         $createBookResp = $this->asEditor()->post($shelf->getUrl('/create-book'), [
-            'name'        => $testName,
+            'name'             => $testName,
-            'description' => 'Book in shelf description',
+            'description_html' => 'Book in shelf description',
         ]);
         $createBookResp->assertRedirect();
 
@@ -372,8 +372,8 @@ class BookShelfTest extends TestCase
     {
         // Create shelf
         $shelfInfo = [
-            'name'        => 'My test shelf' . Str::random(4),
+            'name'             => 'My test shelf' . Str::random(4),
-            'description' => 'Test shelf description ' . Str::random(10),
+            'description_html' => '<p>Test shelf description ' . Str::random(10) . '</p>',
         ];
 
         $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'), [
-            'name'        => 'Test book name',
+            'name'             => 'Test book name',
-            'description' => 'Book in shelf description',
+            'description_html' => '<p>Book in shelf description</p>',
         ]);
 
         $newBook = Book::query()->orderBy('id', 'desc')->first();

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->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');
@@ -36,8 +36,8 @@ class BookTest extends TestCase
             'name' => 'My First Book',
         ]);
 
-        $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'));
+        $this->asEditor()->post('/books', $book->only('name', 'description_html'));
 
         $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', [
-            'name'        => 'My book with tags',
+            'name'             => 'My book with tags',
-            'description' => 'A book with tags',
+            'description_html' => '<p>A book with tags</p>',
-            'tags'        => [
+            'tags'             => [
                 [
                     'name'  => 'Category',
                     'value' => 'Donkey Content',
@@ -79,23 +79,23 @@ class BookTest extends TestCase
     {
         $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';
-        $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->assertSee($book->description);
+        $resp->assertSee($book->description_html);
         $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->assertSee($newDesc);
+        $resp->assertSee($newDesc, false);
     }
 
     public function test_update_sets_tags()
@@ -184,7 +184,7 @@ class BookTest extends TestCase
 
     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)
@@ -200,7 +200,7 @@ class BookTest extends TestCase
 
     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)
@@ -262,6 +262,22 @@ class BookTest extends TestCase
         $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();

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->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->assertSee($chapter->description);
+        $resp->assertSee($chapter->description_html, false);
     }
 
     public function test_delete()