From c77a0fdff361b816db59f96315978bf1aa636b17 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 29 Jan 2026 14:54:08 +0000 Subject: [PATCH] Page Content: Added form elements to filtering Added and updated tests to cover. Also updated API auth to a narrower focus of existing session instead of also existing user auth. This is mainly for tests, to ensure they're following the session process we'd see for activity in the UI. --- app/Activity/Models/Comment.php | 2 +- app/Entities/Tools/EntityHtmlDescription.php | 2 +- app/Entities/Tools/PageContent.php | 2 +- app/Http/Middleware/ApiAuthenticate.php | 8 +- app/Theming/CustomHtmlHeadContentProvider.php | 2 +- app/Util/HtmlContentFilter.php | 62 ++++++++-- tests/Api/ApiAuthTest.php | 5 +- tests/Api/ChaptersApiTest.php | 2 +- tests/Api/RecycleBinApiTest.php | 10 +- tests/Api/UsersApiTest.php | 2 +- tests/Entity/PageContentTest.php | 110 ++++++++++++++++-- 11 files changed, 176 insertions(+), 31 deletions(-) diff --git a/app/Activity/Models/Comment.php b/app/Activity/Models/Comment.php index 0f8e5d785..ce05e3df3 100644 --- a/app/Activity/Models/Comment.php +++ b/app/Activity/Models/Comment.php @@ -82,7 +82,7 @@ class Comment extends Model implements Loggable, OwnableInterface public function safeHtml(): string { - return HtmlContentFilter::removeScriptsFromHtmlString($this->html ?? ''); + return HtmlContentFilter::removeActiveContentFromHtmlString($this->html ?? ''); } public function jointPermissions(): HasMany diff --git a/app/Entities/Tools/EntityHtmlDescription.php b/app/Entities/Tools/EntityHtmlDescription.php index 335703c36..b14deb257 100644 --- a/app/Entities/Tools/EntityHtmlDescription.php +++ b/app/Entities/Tools/EntityHtmlDescription.php @@ -50,7 +50,7 @@ class EntityHtmlDescription return $html; } - return HtmlContentFilter::removeScriptsFromHtmlString($html); + return HtmlContentFilter::removeActiveContentFromHtmlString($html); } public function getPlain(): string diff --git a/app/Entities/Tools/PageContent.php b/app/Entities/Tools/PageContent.php index c7a59216a..5358e8f0c 100644 --- a/app/Entities/Tools/PageContent.php +++ b/app/Entities/Tools/PageContent.php @@ -318,7 +318,7 @@ class PageContent } if (!config('app.allow_content_scripts')) { - HtmlContentFilter::removeScriptsFromDocument($doc); + HtmlContentFilter::removeActiveContentFromDocument($doc); } return $doc->getBodyInnerHtml(); diff --git a/app/Http/Middleware/ApiAuthenticate.php b/app/Http/Middleware/ApiAuthenticate.php index 4123385c0..bfee87f6c 100644 --- a/app/Http/Middleware/ApiAuthenticate.php +++ b/app/Http/Middleware/ApiAuthenticate.php @@ -30,9 +30,9 @@ class ApiAuthenticate */ protected function ensureAuthorizedBySessionOrToken(Request $request): void { - // Return if the user is already found to be signed in via session-based auth. - // This is to make it easy to browse the API via browser when exploring endpoints via the UI. - if (!user()->isGuest() || session()->isStarted()) { + // Use the active user session already exists. + // This is to make it easy to explore API endpoints via the UI. + if (session()->isStarted()) { // Ensure the user has API access permission if (!$this->sessionUserHasApiAccess()) { throw new ApiAuthException(trans('errors.api_user_no_api_permission'), 403); @@ -49,7 +49,7 @@ class ApiAuthenticate // Set our api guard to be the default for this request lifecycle. auth()->shouldUse('api'); - // Validate the token and it's users API access + // Validate the token and its users API access auth()->authenticate(); } diff --git a/app/Theming/CustomHtmlHeadContentProvider.php b/app/Theming/CustomHtmlHeadContentProvider.php index 95d9ff5ad..e0cf5b3b5 100644 --- a/app/Theming/CustomHtmlHeadContentProvider.php +++ b/app/Theming/CustomHtmlHeadContentProvider.php @@ -50,7 +50,7 @@ class CustomHtmlHeadContentProvider $hash = md5($content); return $this->cache->remember('custom-head-export:' . $hash, 86400, function () use ($content) { - return HtmlContentFilter::removeScriptsFromHtmlString($content); + return HtmlContentFilter::removeActiveContentFromHtmlString($content); }); } diff --git a/app/Util/HtmlContentFilter.php b/app/Util/HtmlContentFilter.php index 758591729..ad5bf8c5f 100644 --- a/app/Util/HtmlContentFilter.php +++ b/app/Util/HtmlContentFilter.php @@ -9,9 +9,11 @@ use DOMNodeList; class HtmlContentFilter { /** - * Remove all the script elements from the given HTML document. + * Remove all active content from the given HTML document. + * This aims to cover anything which can dynamically deal with, or send, data + * like any JavaScript actions or form content. */ - public static function removeScriptsFromDocument(HtmlDocument $doc) + public static function removeActiveContentFromDocument(HtmlDocument $doc): void { // Remove standard script tags $scriptElems = $doc->queryXPath('//script'); @@ -21,7 +23,7 @@ class HtmlContentFilter $badLinks = $doc->queryXPath('//*[' . static::xpathContains('@href', 'javascript:') . ']'); static::removeNodes($badLinks); - // Remove forms with calls to JavaScript URI + // Remove elements with form-like attributes with calls to JavaScript URI $badForms = $doc->queryXPath('//*[' . static::xpathContains('@action', 'javascript:') . '] | //*[' . static::xpathContains('@formaction', 'javascript:') . ']'); static::removeNodes($badForms); @@ -47,25 +49,71 @@ class HtmlContentFilter // Remove 'on*' attributes $onAttributes = $doc->queryXPath('//@*[starts-with(name(), \'on\')]'); static::removeAttributes($onAttributes); + + // Remove form elements + $formElements = ['form', 'fieldset', 'button', 'textarea', 'select']; + foreach ($formElements as $formElement) { + $matchingFormElements = $doc->queryXPath('//' . $formElement); + static::removeNodes($matchingFormElements); + } + + // Remove non-checkbox inputs + $inputsToRemove = $doc->queryXPath('//input'); + /** @var DOMElement $input */ + foreach ($inputsToRemove as $input) { + $type = strtolower($input->getAttribute('type')); + if ($type !== 'checkbox') { + $input->parentNode->removeChild($input); + } + } + + // Remove form attributes + $formAttrs = ['form', 'formaction', 'formmethod', 'formtarget']; + foreach ($formAttrs as $formAttr) { + $matchingFormAttrs = $doc->queryXPath('//@' . $formAttr); + static::removeAttributes($matchingFormAttrs); + } } /** - * Remove scripts from the given HTML string. + * Remove active content from the given HTML string. + * This aims to cover anything which can dynamically deal with, or send, data + * like any JavaScript actions or form content. */ - public static function removeScriptsFromHtmlString(string $html): string + public static function removeActiveContentFromHtmlString(string $html): string { if (empty($html)) { return $html; } $doc = new HtmlDocument($html); - static::removeScriptsFromDocument($doc); + static::removeActiveContentFromDocument($doc); return $doc->getBodyInnerHtml(); } /** - * Create a xpath contains statement with a translation automatically built within + * Alias using the old method name to avoid potential compatibility breaks during patch release. + * To remove in future feature release. + * @deprecated Use removeActiveContentFromDocument instead. + */ + public static function removeScriptsFromDocument(HtmlDocument $doc): void + { + static::removeActiveContentFromDocument($doc); + } + + /** + * Alias using the old method name to avoid potential compatibility breaks during patch release. + * To remove in future feature release. + * @deprecated Use removeActiveContentFromHtmlString instead. + */ + public static function removeScriptsFromHtmlString(string $html): string + { + return static::removeActiveContentFromHtmlString($html); + } + + /** + * Create an x-path 'contains' statement with a translation automatically built within * to affectively search in a cases-insensitive manner. */ protected static function xpathContains(string $property, string $value): string diff --git a/tests/Api/ApiAuthTest.php b/tests/Api/ApiAuthTest.php index c8472ce7f..76c2c9ce9 100644 --- a/tests/Api/ApiAuthTest.php +++ b/tests/Api/ApiAuthTest.php @@ -24,7 +24,8 @@ class ApiAuthTest extends TestCase $this->actingAs($viewer, 'standard'); - $resp = $this->get($this->endpoint); + $this->startSession(); + $resp = $this->withCredentials()->get($this->endpoint); $resp->assertStatus(200); } @@ -75,6 +76,7 @@ class ApiAuthTest extends TestCase { $editor = $this->users->editor(); $this->actingAs($editor, 'standard'); + $this->startSession(); $resp = $this->get($this->endpoint); $resp->assertStatus(200); @@ -116,6 +118,7 @@ class ApiAuthTest extends TestCase { $user = $this->users->admin(); $this->actingAs($user, 'standard'); + $this->startSession(); $uriByMethods = [ 'POST' => '/books', diff --git a/tests/Api/ChaptersApiTest.php b/tests/Api/ChaptersApiTest.php index 194140a56..953b3a0f5 100644 --- a/tests/Api/ChaptersApiTest.php +++ b/tests/Api/ChaptersApiTest.php @@ -252,7 +252,7 @@ class ChaptersApiTest extends TestCase { $editor = $this->users->editor(); $this->permissions->removeUserRolePermissions($editor, ['chapter-delete-all', 'chapter-delete-own']); - $this->actingAs($editor); + $this->actingAsForApi($editor); $chapter = $this->entities->chapterHasPages(); $newBook = Book::query()->where('id', '!=', $chapter->book_id)->first(); diff --git a/tests/Api/RecycleBinApiTest.php b/tests/Api/RecycleBinApiTest.php index 6ccc69c35..9e645fe21 100644 --- a/tests/Api/RecycleBinApiTest.php +++ b/tests/Api/RecycleBinApiTest.php @@ -23,7 +23,7 @@ class RecycleBinApiTest extends TestCase { $editor = $this->users->editor(); $this->permissions->grantUserRolePermissions($editor, ['settings-manage']); - $this->actingAs($editor); + $this->actingAsForApi($editor); foreach ($this->endpointMap as [$method, $uri]) { $resp = $this->json($method, $uri); @@ -36,7 +36,7 @@ class RecycleBinApiTest extends TestCase { $editor = $this->users->editor(); $this->permissions->grantUserRolePermissions($editor, ['restrictions-manage-all']); - $this->actingAs($editor); + $this->actingAsForApi($editor); foreach ($this->endpointMap as [$method, $uri]) { $resp = $this->json($method, $uri); @@ -53,6 +53,7 @@ class RecycleBinApiTest extends TestCase $book = $this->entities->book(); $this->actingAs($admin)->delete($page->getUrl()); $this->delete($book->getUrl()); + $this->actingAsForApi($admin); $deletions = Deletion::query()->orderBy('id')->get(); @@ -89,7 +90,7 @@ class RecycleBinApiTest extends TestCase $deletion = Deletion::query()->orderBy('id')->first(); - $resp = $this->getJson($this->baseEndpoint); + $resp = $this->actingAsForApi($admin)->getJson($this->baseEndpoint); $expectedData = [ [ @@ -115,6 +116,7 @@ class RecycleBinApiTest extends TestCase $this->actingAs($admin)->delete($page->getUrl()); $deletion = Deletion::query()->orderBy('id')->first(); + $this->actingAsForApi($admin); $resp = $this->getJson($this->baseEndpoint); $expectedData = [ @@ -141,6 +143,7 @@ class RecycleBinApiTest extends TestCase $page = $this->entities->page(); $this->asAdmin()->delete($page->getUrl()); $page->refresh(); + $this->actingAsApiAdmin(); $deletion = Deletion::query()->orderBy('id')->first(); @@ -165,6 +168,7 @@ class RecycleBinApiTest extends TestCase $page = $this->entities->page(); $this->asAdmin()->delete($page->getUrl()); $page->refresh(); + $this->actingAsApiAdmin(); $deletion = Deletion::query()->orderBy('id')->first(); diff --git a/tests/Api/UsersApiTest.php b/tests/Api/UsersApiTest.php index a0c67d0d2..e7b9df6aa 100644 --- a/tests/Api/UsersApiTest.php +++ b/tests/Api/UsersApiTest.php @@ -80,7 +80,7 @@ class UsersApiTest extends TestCase /** @var ActivityModel $activity */ $activity = ActivityModel::query()->where('user_id', '=', $user->id)->latest()->first(); - $resp = $this->asAdmin()->getJson($this->baseEndpoint . '?filter[id]=3'); + $resp = $this->actingAsApiAdmin()->getJson($this->baseEndpoint . '?filter[id]=3'); $resp->assertJson(['data' => [ [ 'id' => $user->id, diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php index 23a38b573..770261130 100644 --- a/tests/Entity/PageContentTest.php +++ b/tests/Entity/PageContentTest.php @@ -208,11 +208,11 @@ class PageContentTest extends TestCase public function test_form_actions_with_javascript_are_removed() { $checks = [ - '
', - '
', - '
', - '
', - '
', + '', + 'Click me', + 'Click me', + '', + '', ]; $this->asEditor(); @@ -224,11 +224,101 @@ class PageContentTest extends TestCase $pageView = $this->get($page->getUrl()); $pageView->assertStatus(200); - $this->withHtml($pageView)->assertElementNotContains('.page-content', '', + '

thisisacattofind

', + <<<'TESTCASE' + + + + +

thisisacattofind

+
+

thisdogshouldnotbefound

+
+ + + + + + +TESTCASE + + ]; + + $this->asEditor(); + $page = $this->entities->page(); + + foreach ($checks as $check) { + $page->html = $check; + $page->save(); + + $pageView = $this->get($page->getUrl()); + $pageView->assertStatus(200); + $pageView->assertSee('thisisacattofind'); + $pageView->assertDontSee('thisdogshouldnotbefound'); + } + } + + public function test_form_attributes_are_removed() + { + $withinSvgSample = <<<'TESTCASE' + + + + +

thisisacattofind

+

thisisacattofind

+ + + + +TESTCASE; + + $checks = [ + 'formaction' => '

thisisacattofind

', + 'form' => '

thisisacattofind

', + 'formmethod' => '

thisisacattofind

', + 'formtarget' => '

thisisacattofind

', + 'FORMTARGET' => '

thisisacattofind

', + ]; + + $this->asEditor(); + $page = $this->entities->page(); + + foreach ($checks as $attribute => $check) { + $page->html = $check; + $page->save(); + + $pageView = $this->get($page->getUrl()); + $pageView->assertStatus(200); + $pageView->assertSee('thisisacattofind'); + $this->withHtml($pageView)->assertElementNotExists(".page-content [{$attribute}]"); + } + + $page->html = $withinSvgSample; + $page->save(); + $pageView = $this->get($page->getUrl()); + $pageView->assertStatus(200); + $html = $this->withHtml($pageView); + foreach ($checks as $attribute => $check) { + $pageView->assertSee('thisisacattofind'); + $html->assertElementNotExists(".page-content [{$attribute}]"); } }