[3.x] Form handler refactor to allow more flexible and accurate status reporting in the manager UI

_build/test/Tests/Utilities/modStringSanitizerTest.php

@@ -0,0 +1,211 @@
+<?php
+
+/*
+ * This file is part of the MODX Revolution package.
+ *
+ * Copyright (c) MODX, LLC
+ *
+ * For complete copyright and license information, see the COPYRIGHT and LICENSE
+ * files found in the top-level directory of this distribution.
+ *
+ * @package modx-test
+ */
+
+/** @phpcs:disable Squiz.Classes.ValidClassName.NotCamelCaps */
+
+namespace MODX\Revolution\Tests\Utilities\Sanitizers;
+
+use MODX\Revolution\MODxTestCase;
+use MODX\Revolution\Utilities\Sanitizers\modStringSanitizer;
+
+class modStringSanitizerTest extends MODxTestCase
+{
+    /** @var modStringSanitizer $sanitizers */
+    public $sanitizers;
+
+    /**
+     * Setup fixtures before each test.
+     *
+     * @before
+     */
+    public function setUpFixtures()
+    {
+        parent::setUpFixtures();
+        $this->sanitizers = $this->modx->services->get(modStringSanitizer::class);
+    }
+
+    /**
+     * @param string $expected
+     * @param string $htmlSource The html string to clean
+     * @param ?string|array $allowedTags An array or comma-separated list of tag names to allow
+     * @param ?string|array $allowedAttr An array or comma-separated list of tag attribute names to allow
+     * @param bool $allowScripts Whether to allow javascript in html source passed to this method
+     * @param bool $allowComments Whether to allow comments in the final output
+     * @dataProvider providerStripHTML
+     */
+    public function testStripHTML(
+        $expected,
+        string $htmlSource,
+        string|array|null $allowedTags = '',
+        string|array|null $allowedAttr = '',
+        bool $allowScripts = false,
+        bool $allowComments = false
+    ) {
+
+        $allowedTags = $allowedTags ?? '';
+        $allowedAttr = $allowedAttr ?? '';
+
+        $result = $this->sanitizers->stripHTML($htmlSource, $allowedTags, $allowedAttr, $allowScripts, $allowComments);
+        $this->assertEquals($expected, $result);
+    }
+
+    public function providerStripHTML(): array
+    {
+        $nullParams = [null, null];
+        // String list configs (including odd spacing)
+        $parmSet1 = ['a, strong  , em', 'href,  title,id'];
+        // Array list configs (including odd spacing)
+        $parmSet2 = [['p', 'a', ' strong', 'em'], ['href  ', 'class', 'style', 'onclick', 'title']];
+        // Custom/non-existing
+        $parmSet3 = ['p, notatag', 'notanattr'];
+        // Data attr and allowing scripts
+        $parmSet4 = ['div,img, script', 'data, src', true];
+
+        // Have to hack this to keep parser from interpreting as actual opening short tag in the tests below
+        $shortOpenTag = <<<TAG
+        <? 
+        TAG;
+
+        return [
+            // Full strip, nothing passed in for allowed params
+            'Should remove all tags and attrs' => [
+                'My great string',
+                '<p class="gone">My <em>great</em> string</p>'
+            ],
+            'Should remove all tags and attrs (when null is passed to allowed params)' => [
+                'My great string',
+                '<p class="gone">My <em>great</em> string</p>',
+                ...$nullParams
+            ],
+            'Should remove script tag and its contents' => [
+                'This would , but we fixed it.',
+                'This would <script>alert("be bad");</script>, but we fixed it.'
+            ],
+            'Should remove broken script tag and remaining contents (no closing)' => [
+                'This would ',
+                'This would <script>alert("be bad");, but we fixed it.'
+            ],
+            'Should remove broken script tag (no opening)' => [
+                'This would alert("be bad");',
+                'This would alert("be bad");</script>'
+            ],
+            'Should remove php (long tag)' => [
+                '',
+                '<?php echo "Also not great!"; ?>'
+            ],
+            'Should remove php (long incomplete tag)' => [
+                '',
+                '<?php echo "Again, not great!";'
+            ],
+            'Should remove php (short tag)' => [
+                '',
+                trim($shortOpenTag) . ' echo "Still not great!"; ?>'
+            ],
+            'Should remove php (short incomplete tag)' => [
+                '',
+                trim($shortOpenTag) . ' echo "You know ... not great!";'
+            ],
+            /*
+                paramSet1 rules, allowed:
+                    tags -- a, strong, em
+                    attr -- href, title, id
+            */
+            'Should auto complete broken em (incorrect closing tags)' => [
+                'A <em>jazzy<em> caption</em></em>',
+                'A <em>jazzy<em> caption',
+                ...$parmSet1
+            ],
+            'Should handle removals in nested structures' => [
+                'A <em>jazzy</em> caption <a id="myId">more</a>',
+                'A <b><em><span>jazzy</span></em></b> caption <span><a id="myId" style="color: red;"><b>more</b></a></span>',
+                ...$parmSet1
+            ],
+            /*
+                paramSet2 rules, allowed, given in array instead of string:
+                    tags -- ['p', 'a', 'strong', 'em']
+                    attr -- ['href', 'class', 'style', 'onclick', 'title'] {1}
+
+                    {1} note that event handlers should always be removed, even when scripts
+                    are allowed as it is bad practice mixing javascript directly in html
+            */
+            'Should remove non-standard tag and others not in list' => [
+                '<p>A jazzy caption</p>',
+                '<div><p>A <notatag>jazzy</notatag> caption</p></div>',
+                ...$parmSet2
+            ],
+            'Should remove event handlers' => [
+                '<p class="myClass">This element does <strong>all</strong> these things</p>',
+                '<p class="myClass" data-someprop="hello">This <b>element</b> does <strong onclick="javascript:alert(hello);">all</strong> these things</p>',
+                ...$parmSet2
+            ],
+            'Should replace javascript in all attrs' => [
+                '<p class="myClass">This element does <strong title="#js-not-allowed#">all</strong> these things</p>',
+                '<p class="myClass" data-someprop="hello">This <b>element</b> does <strong title="javascript:alert(hello);">all</strong> these things</p>',
+                ...$parmSet2
+            ],
+            /*
+                paramSet3 rules, allowed:
+                    tags -- p, notatag
+                    attr -- notanattr
+            */
+            'Should retain non-standard and other allowed tags' => [
+                '<p>A <notatag>jazzy</notatag> caption</p>',
+                '<div><p>A <notatag>jazzy</notatag> caption</p></div>',
+                ...$parmSet3
+            ],
+            'Should retain non-standard and other allowed attributes' => [
+                '<p notanattr="technically ok, but not advisable">A <notatag>jazzy</notatag> caption</p>',
+                '<div><p notanattr="technically ok, but not advisable">A <notatag>jazzy</notatag> caption</p></div>',
+                ...$parmSet3
+            ],
+            /*
+                paramSet4 rules, allowed:
+                    tags -- div, img, script
+                    attr -- data, src
+            */
+            'Should retain data attributes' => [
+                '<div data-someprop="hello" data-otherprop="world">A jazzy caption</div>',
+                '<div data-someprop="hello" data-otherprop="world"><p>A <notatag>jazzy</notatag> caption</p></div>',
+                ...$parmSet4
+            ],
+            'Should retain script tag and contents' => [
+                '<div>As long as you are sure, </div><script>alert("you can do this");</script>',
+                '<div>As long as you are sure, </div><script>alert("you can do this");</script>',
+                ...$parmSet4
+            ],
+            'Should handle retention and removals in multiline html' => [
+                <<<EXP
+                    <div>
+                        These tags will disappear
+                        <img src="/some/path/to.png">
+                    </div>
+                    <script>
+                        let x = 1;
+                        console.log('X is ', x);
+                    </script>
+                EXP,
+                <<<SRC
+                    <div>
+                        <p>These tags will <span>disappear</span></p>
+                        <img src="/some/path/to.png" alt="should have this but not in list">
+                    </div>
+                    <script>
+                        let x = 1;
+                        console.log('X is ', x);
+                    </script>
+                SRC,
+                ...$parmSet4
+            ]
+        ];
+    }
+}

_build/test/phpunit.xml

@@ -54,6 +54,9 @@
             <directory>Tests/Cases/Modx/</directory>
             <directory>Tests/Cases/Request/</directory>
         </testsuite>
+        <testsuite name="Utilities">
+            <directory>Tests/Utilities</directory>
+        </testsuite>
         <testsuite name="Teardown">
             <file>Tests/modXTeardownTest.php</file>
         </testsuite>

core/lexicon/en/default.inc.php

@@ -1,11 +1,13 @@
 <?php
+
 /**
  * Default English lexicon topic
  *
  * @language en
  * @package modx
  * @subpackage lexicon
  */
+
 $_lang['access'] = 'Access';
 $_lang['access_denied'] = 'Access denied.';
 $_lang['action'] = 'Action';
@@ -150,6 +152,7 @@
 $_lang['error_grid_get_content_tolog'] = 'A server error prevented this grid’s content from loading. Refer to your browser’s console, manager logs, and/or php server logs for more information.';
 $_lang['error_grid_get_content_toscreen'] = 'This grid’s content could not be loaded due to the following server error: [[+message]]';
 $_lang['error_loading_feed'] = 'An error occurred loading the feed.';
+$_lang['error_ui_message_error'] = 'A system error occurred while retrieving the message to display here. See browser console for more details.';
 $_lang['event_id'] = 'Event Id';
 $_lang['existing_category'] = 'Existing Category';
 $_lang['expand_all'] = 'Expand All';
@@ -527,6 +530,7 @@
 $_lang['updated'] = 'Updated';
 $_lang['upload'] = 'Upload';
 $_lang['username'] = 'Username';
+$_lang['validation_error'] = 'Validation Error';
 $_lang['value'] = 'Value';
 $_lang['version'] = 'Version';
 $_lang['view'] = 'View';

core/lexicon/en/workspace.inc.php

@@ -130,6 +130,7 @@
 $_lang['package_select_upload'] = 'Select a Package to Upload';
 $_lang['package_signature'] = 'Signature';
 $_lang['package_state'] = 'State';
+$_lang['package_status'] = 'Package Status';
 $_lang['package_uninstall'] = 'Uninstall Package';
 $_lang['package_uninstall_info_find'] = 'Finding package with signature: [[+signature]]';
 $_lang['package_uninstall_info_prep'] = 'Package found. Preparing to uninstall.';

core/src/Revolution/Error/modError.php

@@ -30,6 +30,17 @@ class modError
      * @var string The error message to output.
      */
     public  $message;
+
+    /**
+     * @var array An optional set of customization specifications to tailor the message output. Config options include:
+     * - messageType: (string) The type of message to output. Options include the status
+     * constants defined in the base processor (i.e., Processor::STATUS_TYPE_INFO, etc.).
+     * - messageWindowTitle: (string) Overrides the default window title
+     * - messageIsFormatted: (bool) Whether the message source is html-formatted
+     * (must be set to true to preserve formatting).
+     */
+    public $messageConfig = [];
+
     /**
      * @var modX A reference to the $modx object.
      */
@@ -160,13 +171,20 @@ public function process($message = '', $status = false, $object = null) {
             unset ($obj);
         }
         $objarray = $this->toArray($object);
-        return [
+
+        $data = [
             'success' => $status,
             'message' => $this->message,
-            'total' => isset ($this->total) && $this->total != 0 ? $this->total : count($this->errors),
+            'total' => isset($this->total) && $this->total != 0
+                ? $this->total
+                : count($this->errors),
             'errors' => $this->errors,
             'object' => $objarray,
+            ...$this->messageConfig
         ];
+        $this->messageConfig = [];
+
+        return $data;
     }
 
     /**
@@ -312,5 +330,6 @@ public function reset() {
         $this->message = '';
         $this->total = 0;
         $this->status = true;
+        $this->messageConfig = [];
     }
 }