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.
2026-02-05 00:29:48 +03:00 · 2026-01-29 14:54:08 +00:00
parent 6a63b38bb3
commit c77a0fdff3
11 changed files with 176 additions and 31 deletions
--- 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
--- 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
--- 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();
--- 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();
    }

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

--- 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
--- 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',
--- 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();

--- 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();

--- 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,
--- 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 = [
-            '<form><input id="xss" type=submit formaction=javascript:alert(document.domain) value=Submit><input></form>',
-            '<form ><button id="xss" formaction="JaVaScRiPt:alert(document.domain)">Click me</button></form>',
-            '<form ><button id="xss" formaction=javascript:alert(document.domain)>Click me</button></form>',
-            '<form id="xss" action=javascript:alert(document.domain)><input type=submit value=Submit></form>',
-            '<form id="xss" action="JaVaScRiPt:alert(document.domain)"><input type=submit value=Submit></form>',
+            '<customform><custominput id="xss" type=submit formaction=javascript:alert(document.domain) value=Submit><custominput></customform>',
+            '<customform ><custombutton id="xss" formaction="JaVaScRiPt:alert(document.domain)">Click me</custombutton></customform>',
+            '<customform ><custombutton id="xss" formaction=javascript:alert(document.domain)>Click me</custombutton></customform>',
+            '<customform id="xss" action=javascript:alert(document.domain)><input type=submit value=Submit></customform>',
+            '<customform id="xss" action="JaVaScRiPt:alert(document.domain)"><input type=submit value=Submit></customform>',
        ];

        $this->asEditor();
@@ -224,11 +224,101 @@ class PageContentTest extends TestCase

            $pageView = $this->get($page->getUrl());
            $pageView->assertStatus(200);
-            $this->withHtml($pageView)->assertElementNotContains('.page-content', '<button id="xss"');
-            $this->withHtml($pageView)->assertElementNotContains('.page-content', '<input id="xss"');
-            $this->withHtml($pageView)->assertElementNotContains('.page-content', '<form id="xss"');
-            $this->withHtml($pageView)->assertElementNotContains('.page-content', 'action=javascript:');
-            $this->withHtml($pageView)->assertElementNotContains('.page-content', 'formaction=javascript:');
+            $pageView->assertDontSee('id="xss"', false);
+            $pageView->assertDontSee('action=javascript:', false);
+            $pageView->assertDontSee('action=JaVaScRiPt:', false);
+            $pageView->assertDontSee('formaction=javascript:', false);
+            $pageView->assertDontSee('formaction=JaVaScRiPt:', false);
+        }
+    }
+
+    public function test_form_elements_are_removed()
+    {
+        $checks = [
+            '<p>thisisacattofind</p><form>thisdogshouldnotbefound</form>',
+            '<p>thisisacattofind</p><input type="text" value="thisdogshouldnotbefound">',
+            '<p>thisisacattofind</p><select><option>thisdogshouldnotbefound</option></select>',
+            '<p>thisisacattofind</p><textarea>thisdogshouldnotbefound</textarea>',
+            '<p>thisisacattofind</p><fieldset>thisdogshouldnotbefound</fieldset>',
+            '<p>thisisacattofind</p><button>thisdogshouldnotbefound</button>',
+            '<p>thisisacattofind</p><BUTTON>thisdogshouldnotbefound</BUTTON>',
+            <<<'TESTCASE'
+<svg width="200" height="100" xmlns="http://www.w3.org/2000/svg">
+  <foreignObject width="100%" height="100%">
+    
+    <body xmlns="http://www.w3.org/1999/xhtml">
+    <p>thisisacattofind</p>
+      <form>
+        <p>thisdogshouldnotbefound</p>
+      </form>
+      <input type="text" placeholder="thisdogshouldnotbefound" />
+      <button type="submit">thisdogshouldnotbefound</button>
+    </body>
+
+  </foreignObject>
+</svg>
+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'
+<svg width="200" height="100" xmlns="http://www.w3.org/2000/svg">
+  <foreignObject width="100%" height="100%">
+    
+    <body xmlns="http://www.w3.org/1999/xhtml">
+    <p formaction="a">thisisacattofind</p>
+    <p formaction="a">thisisacattofind</p>
+    </body>
+
+  </foreignObject>
+</svg>
+TESTCASE;
+
+        $checks = [
+            'formaction' => '<p formaction="a">thisisacattofind</p>',
+            'form' => '<p form="a">thisisacattofind</p>',
+            'formmethod' => '<p formmethod="a">thisisacattofind</p>',
+            'formtarget' => '<p formtarget="a">thisisacattofind</p>',
+            'FORMTARGET' => '<p FORMTARGET="a">thisisacattofind</p>',
+        ];
+
+        $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}]");
        }
    }