From 0e0a17cc30fc4be84b09ab1e30d7689839ff1bae Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 5 Jan 2019 17:18:40 +0000 Subject: [PATCH] Prevented page text content includes Avoids possible permission issues where included content shown in search or preview where the user would not normally have permission to view the included content. Closes #1178 --- app/Entities/Repos/EntityRepo.php | 59 +++++++++++++++++++++---------- app/Entities/Repos/PageRepo.php | 4 +-- tests/Entity/PageContentTest.php | 7 ++-- 3 files changed, 47 insertions(+), 23 deletions(-) diff --git a/app/Entities/Repos/EntityRepo.php b/app/Entities/Repos/EntityRepo.php index 44fb9ad12..576ed70f0 100644 --- a/app/Entities/Repos/EntityRepo.php +++ b/app/Entities/Repos/EntityRepo.php @@ -599,24 +599,48 @@ class EntityRepo } /** - * Render the page for viewing, Parsing and performing features such as page transclusion. + * Render the page for viewing * @param Page $page - * @param bool $ignorePermissions - * @return mixed|string + * @param bool $blankIncludes + * @return string */ - public function renderPage(Page $page, $ignorePermissions = false) + public function renderPage(Page $page, bool $blankIncludes = false) : string { $content = $page->html; + if (!config('app.allow_content_scripts')) { $content = $this->escapeScripts($content); } - $matches = []; - preg_match_all("/{{@\s?([0-9].*?)}}/", $content, $matches); - if (count($matches[0]) === 0) { - return $content; + if ($blankIncludes) { + $content = $this->blankPageIncludes($content); + } else { + $content = $this->parsePageIncludes($content); } + return $content; + } + + /** + * Remove any page include tags within the given HTML. + * @param string $html + * @return string + */ + protected function blankPageIncludes(string $html) : string + { + return preg_replace("/{{@\s?([0-9].*?)}}/", '', $html); + } + + /** + * Parse any include tags "{{@#section}}" to be part of the page. + * @param string $html + * @return mixed|string + */ + protected function parsePageIncludes(string $html) : string + { + $matches = []; + preg_match_all("/{{@\s?([0-9].*?)}}/", $html, $matches); + $topLevelTags = ['table', 'ul', 'ol']; foreach ($matches[1] as $index => $includeId) { $splitInclude = explode('#', $includeId, 2); @@ -625,14 +649,14 @@ class EntityRepo continue; } - $matchedPage = $this->getById('page', $pageId, false, $ignorePermissions); + $matchedPage = $this->getById('page', $pageId); if ($matchedPage === null) { - $content = str_replace($matches[0][$index], '', $content); + $html = str_replace($matches[0][$index], '', $html); continue; } if (count($splitInclude) === 1) { - $content = str_replace($matches[0][$index], $matchedPage->html, $content); + $html = str_replace($matches[0][$index], $matchedPage->html, $html); continue; } @@ -640,7 +664,7 @@ class EntityRepo $doc->loadHTML(mb_convert_encoding(''.$matchedPage->html.'', 'HTML-ENTITIES', 'UTF-8')); $matchingElem = $doc->getElementById($splitInclude[1]); if ($matchingElem === null) { - $content = str_replace($matches[0][$index], '', $content); + $html = str_replace($matches[0][$index], '', $html); continue; } $innerContent = ''; @@ -652,25 +676,22 @@ class EntityRepo $innerContent .= $doc->saveHTML($childNode); } } - $content = str_replace($matches[0][$index], trim($innerContent), $content); + $html = str_replace($matches[0][$index], trim($innerContent), $html); } - return $content; + return $html; } /** * Escape script tags within HTML content. * @param string $html - * @return mixed + * @return string */ - protected function escapeScripts(string $html) + protected function escapeScripts(string $html) : string { $scriptSearchRegex = '/.*?<\/script>/ms'; $matches = []; preg_match_all($scriptSearchRegex, $html, $matches); - if (count($matches) === 0) { - return $html; - } foreach ($matches[0] as $match) { $html = str_replace($match, htmlentities($match), $html); diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php index d9f9c2720..3558b29b3 100644 --- a/app/Entities/Repos/PageRepo.php +++ b/app/Entities/Repos/PageRepo.php @@ -194,9 +194,9 @@ class PageRepo extends EntityRepo * @param \BookStack\Entities\Page $page * @return string */ - public function pageToPlainText(Page $page) + protected function pageToPlainText(Page $page) : string { - $html = $this->renderPage($page); + $html = $this->renderPage($page, true); return strip_tags($html); } diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php index 6a7112bcb..86abadf14 100644 --- a/tests/Entity/PageContentTest.php +++ b/tests/Entity/PageContentTest.php @@ -40,15 +40,18 @@ class PageContentTest extends TestCase { $page = Page::first(); $secondPage = Page::where('id', '!=', $page->id)->first(); + $this->asEditor(); - $page->html = "

{{@$secondPage->id}}

"; + $includeTag = '{{@' . $secondPage->id . '}}'; + $page->html = '

' . $includeTag . '

'; $resp = $this->put($page->getUrl(), ['name' => $page->name, 'html' => $page->html, 'summary' => '']); $resp->assertStatus(302); $page = Page::find($page->id); - $this->assertContains("{{@$secondPage->id}}", $page->html); + $this->assertContains($includeTag, $page->html); + $this->assertEquals('', $page->text); } public function test_page_includes_do_not_break_tables()