Skip to content

Commit b5caaa7

Browse files
ssddanbrownssddanbrown
committed
Fixed content parsing break with line html comment
Fixes issues thrown in custom HMTL head & page content filtering when the content is comprised of only a single HTML comment. Adds tests to cover. For #2804
1 parent a8471b2 commit b5caaa7

File tree

4 files changed

+73
-42
lines changed

4 files changed

+73
-42
lines changed

‎app/Entities/Tools/PageContent.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ protected function parsePageIncludes(string $html): string
332332
protected function fetchSectionOfPage(Page $page, string $sectionId): string
333333
{
334334
$topLevelTags = ['table', 'ul', 'ol'];
335-
$doc = $this->loadDocumentFromHtml('<body>' . $page->html . '</body>');
335+
$doc = $this->loadDocumentFromHtml($page->html);
336336

337337
// Search included content for the id given and blank out if not exists.
338338
$matchingElem = $doc->getElementById($sectionId);
@@ -363,6 +363,7 @@ protected function loadDocumentFromHtml(string $html): DOMDocument
363363
{
364364
libxml_use_internal_errors(true);
365365
$doc = new DOMDocument();
366+
$html = '<body>' . $html . '</body>';
366367
$doc->loadHTML(mb_convert_encoding($html, 'HTML-ENTITIES', 'UTF-8'));
367368
return $doc;
368369
}

‎app/Util/HtmlContentFilter.php

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
<?php namespace BookStack\Util;
22

33
use DOMDocument;
4-
use DOMNode;
54
use DOMNodeList;
65
use DOMXPath;
76

@@ -16,6 +15,7 @@ public static function removeScripts(string $html): string
1615
return $html;
1716
}
1817

18+
$html = '<body>' . $html . '</body>';
1919
libxml_use_internal_errors(true);
2020
$doc = new DOMDocument();
2121
$doc->loadHTML(mb_convert_encoding($html, 'HTML-ENTITIES', 'UTF-8'));
@@ -61,11 +61,10 @@ public static function removeScripts(string $html): string
6161
/**
6262
* Removed all of the given DOMNodes.
6363
*/
64-
static protected function removeNodes(DOMNodeList $nodes): void
64+
protected static function removeNodes(DOMNodeList $nodes): void
6565
{
6666
foreach ($nodes as $node) {
6767
$node->parentNode->removeChild($node);
6868
}
6969
}
70-
71-
}
70+
}

‎tests/Entity/ExportTest.php

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ class ExportTest extends TestCase
1212

1313
public function test_page_text_export()
1414
{
15-
$page = Page::first();
15+
$page = Page::query()->first();
1616
$this->asEditor();
1717

1818
$resp = $this->get($page->getUrl('/export/plaintext'));
@@ -23,7 +23,7 @@ public function test_page_text_export()
2323

2424
public function test_page_pdf_export()
2525
{
26-
$page = Page::first();
26+
$page = Page::query()->first();
2727
$this->asEditor();
2828

2929
$resp = $this->get($page->getUrl('/export/pdf'));
@@ -33,7 +33,7 @@ public function test_page_pdf_export()
3333

3434
public function test_page_html_export()
3535
{
36-
$page = Page::first();
36+
$page = Page::query()->first();
3737
$this->asEditor();
3838

3939
$resp = $this->get($page->getUrl('/export/html'));
@@ -44,7 +44,7 @@ public function test_page_html_export()
4444

4545
public function test_book_text_export()
4646
{
47-
$page = Page::first();
47+
$page = Page::query()->first();
4848
$book = $page->book;
4949
$this->asEditor();
5050

@@ -57,7 +57,7 @@ public function test_book_text_export()
5757

5858
public function test_book_pdf_export()
5959
{
60-
$page = Page::first();
60+
$page = Page::query()->first();
6161
$book = $page->book;
6262
$this->asEditor();
6363

@@ -68,7 +68,7 @@ public function test_book_pdf_export()
6868

6969
public function test_book_html_export()
7070
{
71-
$page = Page::first();
71+
$page = Page::query()->first();
7272
$book = $page->book;
7373
$this->asEditor();
7474

@@ -95,7 +95,7 @@ public function test_book_html_export_shows_chapter_descriptions()
9595

9696
public function test_chapter_text_export()
9797
{
98-
$chapter = Chapter::first();
98+
$chapter = Chapter::query()->first();
9999
$page = $chapter->pages[0];
100100
$this->asEditor();
101101

@@ -108,7 +108,7 @@ public function test_chapter_text_export()
108108

109109
public function test_chapter_pdf_export()
110110
{
111-
$chapter = Chapter::first();
111+
$chapter = Chapter::query()->first();
112112
$this->asEditor();
113113

114114
$resp = $this->get($chapter->getUrl('/export/pdf'));
@@ -118,7 +118,7 @@ public function test_chapter_pdf_export()
118118

119119
public function test_chapter_html_export()
120120
{
121-
$chapter = Chapter::first();
121+
$chapter = Chapter::query()->first();
122122
$page = $chapter->pages[0];
123123
$this->asEditor();
124124

@@ -131,7 +131,7 @@ public function test_chapter_html_export()
131131

132132
public function test_page_html_export_contains_custom_head_if_set()
133133
{
134-
$page = Page::first();
134+
$page = Page::query()->first();
135135

136136
$customHeadContent = "<style>p{color: red;}</style>";
137137
$this->setSettings(['app-custom-head' => $customHeadContent]);
@@ -140,9 +140,21 @@ public function test_page_html_export_contains_custom_head_if_set()
140140
$resp->assertSee($customHeadContent);
141141
}
142142

143+
public function test_page_html_export_does_not_break_with_only_comments_in_custom_head()
144+
{
145+
$page = Page::query()->first();
146+
147+
$customHeadContent = "<!-- A comment -->";
148+
$this->setSettings(['app-custom-head' => $customHeadContent]);
149+
150+
$resp = $this->asEditor()->get($page->getUrl('/export/html'));
151+
$resp->assertStatus(200);
152+
$resp->assertSee($customHeadContent);
153+
}
154+
143155
public function test_page_html_export_use_absolute_dates()
144156
{
145-
$page = Page::first();
157+
$page = Page::query()->first();
146158

147159
$resp = $this->asEditor()->get($page->getUrl('/export/html'));
148160
$resp->assertSee($page->created_at->formatLocalized('%e %B %Y %H:%M:%S'));
@@ -153,7 +165,7 @@ public function test_page_html_export_use_absolute_dates()
153165

154166
public function test_page_export_does_not_include_user_or_revision_links()
155167
{
156-
$page = Page::first();
168+
$page = Page::query()->first();
157169

158170
$resp = $this->asEditor()->get($page->getUrl('/export/html'));
159171
$resp->assertDontSee($page->getUrl('/revisions'));
@@ -163,7 +175,7 @@ public function test_page_export_does_not_include_user_or_revision_links()
163175

164176
public function test_page_export_sets_right_data_type_for_svg_embeds()
165177
{
166-
$page = Page::first();
178+
$page = Page::query()->first();
167179
Storage::disk('local')->makeDirectory('uploads/images/gallery');
168180
Storage::disk('local')->put('uploads/images/gallery/svg_test.svg', '<svg></svg>');
169181
$page->html = '<img src="http://localhost/uploads/images/gallery/svg_test.svg">';
@@ -179,7 +191,7 @@ public function test_page_export_sets_right_data_type_for_svg_embeds()
179191

180192
public function test_page_image_containment_works_on_multiple_images_within_a_single_line()
181193
{
182-
$page = Page::first();
194+
$page = Page::query()->first();
183195
Storage::disk('local')->makeDirectory('uploads/images/gallery');
184196
Storage::disk('local')->put('uploads/images/gallery/svg_test.svg', '<svg></svg>');
185197
Storage::disk('local')->put('uploads/images/gallery/svg_test2.svg', '<svg></svg>');
@@ -195,7 +207,7 @@ public function test_page_image_containment_works_on_multiple_images_within_a_si
195207

196208
public function test_page_export_contained_html_image_fetches_only_run_when_url_points_to_image_upload_folder()
197209
{
198-
$page = Page::first();
210+
$page = Page::query()->first();
199211
$page->html = '<img src="http://localhost/uploads/images/gallery/svg_test.svg"/>'
200212
.'<img src="http://localhost/uploads/svg_test.svg"/>'
201213
.'<img src="/uploads/svg_test.svg"/>';
@@ -233,7 +245,7 @@ public function test_exports_removes_scripts_from_custom_head()
233245
public function test_page_export_with_deleted_creator_and_updater()
234246
{
235247
$user = $this->getViewer(['name' => 'ExportWizardTheFifth']);
236-
$page = Page::first();
248+
$page = Page::query()->first();
237249
$page->created_by = $user->id;
238250
$page->updated_by = $user->id;
239251
$page->save();

‎tests/Entity/PageContentTest.php

Lines changed: 40 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ class PageContentTest extends TestCase
1313

1414
public function test_page_includes()
1515
{
16-
$page = Page::first();
17-
$secondPage = Page::where('id', '!=', $page->id)->first();
16+
$page = Page::query()->first();
17+
$secondPage = Page::query()->where('id', '!=', $page->id)->first();
1818

1919
$secondPage->html = "<p id='section1'>Hello, This is a test</p><p id='section2'>This is a second block of content</p>";
2020
$secondPage->save();
@@ -42,8 +42,8 @@ public function test_page_includes()
4242

4343
public function test_saving_page_with_includes()
4444
{
45-
$page = Page::first();
46-
$secondPage = Page::where('id', '!=', $page->id)->first();
45+
$page = Page::query()->first();
46+
$secondPage = Page::query()->where('id', '!=', $page->id)->first();
4747

4848
$this->asEditor();
4949
$includeTag = '{{@' . $secondPage->id . '}}';
@@ -60,8 +60,8 @@ public function test_saving_page_with_includes()
6060

6161
public function test_page_includes_do_not_break_tables()
6262
{
63-
$page = Page::first();
64-
$secondPage = Page::where('id', '!=', $page->id)->first();
63+
$page = Page::query()->first();
64+
$secondPage = Page::query()->where('id', '!=', $page->id)->first();
6565

6666
$content = '<table id="table"><tbody><tr><td>test</td></tr></tbody></table>';
6767
$secondPage->html = $content;
@@ -97,7 +97,7 @@ public function test_page_includes_rendered_on_book_export()
9797
public function test_page_content_scripts_removed_by_default()
9898
{
9999
$this->asEditor();
100-
$page = Page::first();
100+
$page = Page::query()->first();
101101
$script = 'abc123<script>console.log("hello-test")</script>abc123';
102102
$page->html = "escape {$script}";
103103
$page->save();
@@ -120,7 +120,7 @@ public function test_more_complex_content_script_escaping_scenarios()
120120
];
121121

122122
$this->asEditor();
123-
$page = Page::first();
123+
$page = Page::query()->first();
124124

125125
foreach ($checks as $check) {
126126
$page->html = $check;
@@ -145,7 +145,7 @@ public function test_iframe_js_and_base64_urls_are_removed()
145145
];
146146

147147
$this->asEditor();
148-
$page = Page::first();
148+
$page = Page::query()->first();
149149

150150
foreach ($checks as $check) {
151151
$page->html = $check;
@@ -171,7 +171,7 @@ public function test_javascript_uri_links_are_removed()
171171
];
172172

173173
$this->asEditor();
174-
$page = Page::first();
174+
$page = Page::query()->first();
175175

176176
foreach ($checks as $check) {
177177
$page->html = $check;
@@ -192,7 +192,7 @@ public function test_form_actions_with_javascript_are_removed()
192192
];
193193

194194
$this->asEditor();
195-
$page = Page::first();
195+
$page = Page::query()->first();
196196

197197
foreach ($checks as $check) {
198198
$page->html = $check;
@@ -215,7 +215,7 @@ public function test_metadata_redirects_are_removed()
215215
];
216216

217217
$this->asEditor();
218-
$page = Page::first();
218+
$page = Page::query()->first();
219219

220220
foreach ($checks as $check) {
221221
$page->html = $check;
@@ -232,7 +232,7 @@ public function test_metadata_redirects_are_removed()
232232
public function test_page_inline_on_attributes_removed_by_default()
233233
{
234234
$this->asEditor();
235-
$page = Page::first();
235+
$page = Page::query()->first();
236236
$script = '<p onmouseenter="console.log(\'test\')">Hello</p>';
237237
$page->html = "escape {$script}";
238238
$page->save();
@@ -255,7 +255,7 @@ public function test_more_complex_inline_on_attributes_escaping_scenarios()
255255
];
256256

257257
$this->asEditor();
258-
$page = Page::first();
258+
$page = Page::query()->first();
259259

260260
foreach ($checks as $check) {
261261
$page->html = $check;
@@ -271,7 +271,7 @@ public function test_more_complex_inline_on_attributes_escaping_scenarios()
271271
public function test_page_content_scripts_show_when_configured()
272272
{
273273
$this->asEditor();
274-
$page = Page::first();
274+
$page = Page::query()->first();
275275
config()->push('app.allow_content_scripts', 'true');
276276

277277
$script = 'abc123<script>console.log("hello-test")</script>abc123';
@@ -286,7 +286,7 @@ public function test_page_content_scripts_show_when_configured()
286286
public function test_page_inline_on_attributes_show_if_configured()
287287
{
288288
$this->asEditor();
289-
$page = Page::first();
289+
$page = Page::query()->first();
290290
config()->push('app.allow_content_scripts', 'true');
291291

292292
$script = '<p onmouseenter="console.log(\'test\')">Hello</p>';
@@ -301,7 +301,7 @@ public function test_page_inline_on_attributes_show_if_configured()
301301
public function test_duplicate_ids_does_not_break_page_render()
302302
{
303303
$this->asEditor();
304-
$pageA = Page::first();
304+
$pageA = Page::query()->first();
305305
$pageB = Page::query()->where('id', '!=', $pageA->id)->first();
306306

307307
$content = '<ul id="bkmrk-xxx-%28"></ul> <ul id="bkmrk-xxx-%28"></ul>';
@@ -318,7 +318,7 @@ public function test_duplicate_ids_does_not_break_page_render()
318318
public function test_duplicate_ids_fixed_on_page_save()
319319
{
320320
$this->asEditor();
321-
$page = Page::first();
321+
$page = Page::query()->first();
322322

323323
$content = '<ul id="bkmrk-test"><li>test a</li><li><ul id="bkmrk-test"><li>test b</li></ul></li></ul>';
324324
$pageSave = $this->put($page->getUrl(), [
@@ -328,14 +328,14 @@ public function test_duplicate_ids_fixed_on_page_save()
328328
]);
329329
$pageSave->assertRedirect();
330330

331-
$updatedPage = Page::where('id', '=', $page->id)->first();
331+
$updatedPage = Page::query()->where('id', '=', $page->id)->first();
332332
$this->assertEquals(substr_count($updatedPage->html, "bkmrk-test\""), 1);
333333
}
334334

335335
public function test_anchors_referencing_non_bkmrk_ids_rewritten_after_save()
336336
{
337337
$this->asEditor();
338-
$page = Page::first();
338+
$page = Page::query()->first();
339339

340340
$content = '<h1 id="non-standard-id">test</h1><p><a href="#non-standard-id">link</a></p>';
341341
$this->put($page->getUrl(), [
@@ -344,7 +344,7 @@ public function test_anchors_referencing_non_bkmrk_ids_rewritten_after_save()
344344
'summary' => ''
345345
]);
346346

347-
$updatedPage = Page::where('id', '=', $page->id)->first();
347+
$updatedPage = Page::query()->where('id', '=', $page->id)->first();
348348
$this->assertStringContainsString('id="bkmrk-test"', $updatedPage->html);
349349
$this->assertStringContainsString('href="#bkmrk-test"', $updatedPage->html);
350350
}
@@ -484,6 +484,25 @@ public function test_page_markdown_strikethrough_rendering()
484484
$pageView->assertElementExists('.page-content p > s');
485485
}
486486

487+
public function test_page_markdown_single_html_comment_saving()
488+
{
489+
$this->asEditor();
490+
$page = Page::query()->first();
491+
492+
$content = '<!-- Test Comment -->';
493+
$this->put($page->getUrl(), [
494+
'name' => $page->name, 'markdown' => $content,
495+
'html' => '', 'summary' => ''
496+
]);
497+
498+
$page->refresh();
499+
$this->assertStringMatchesFormat($content, $page->html);
500+
501+
$pageView = $this->get($page->getUrl());
502+
$pageView->assertStatus(200);
503+
$pageView->assertSee($content);
504+
}
505+
487506
public function test_base64_images_get_extracted_from_page_content()
488507
{
489508
$this->asEditor();

0 commit comments

Comments
 (0)