[3.x] Restrict tag usage and ensure proper escaping of element name, caption, and description fields

[smg6511]

What does it do?

Strips tags and html encodes the name, caption/label, and description fields for elements—both in the back end (processors) and front end (js forms, grids, and windows). Made separate commits for processors and js in case that makes testing easier.

Why is it needed?

There should be more appropriate limitations on HTML tag usage within certain fields. The initial submission of this PR is highly restrictive, which I realize may need to be loosened depending on reviewer feedback.

How to test

1. Note, There are three areas covered that should be tested:
   • The main element create/edit form pages
   • The quick create/edit windows
   • The form customization grid (you'll need to create a new profile and set to test)
2. From your terminal app, run grunt build from within the _build/templates/default folder and clear your browser cache.
3. Cherry pick just the processors commit to verify tags can not be saved to the database for the name/templatename, and only select tags and attributes can be saved for caption (TVs only) and description fields for all elements (set with the 4 new system settings elements_caption_allowedtags, elements_caption_allowedattr, elements_description_allowedtags, and elements_description_allowedattr).
4. Add in the javascript commit and verify that tags are prevented in these form fields and special characters are html encoded as well.

Related issue(s)/PR(s)

Resolves #15925

[smg6511]

That was for rendering in the resource TV panel, as in-context help/guidance. As an example, consider a "hero type" TV with a narrow height image in the description to show what the 3 options in a select/radio tv would look like.

For the caption (again, rendering in the resource tv panel) I can also imagine adding just a help link to the caption.

I don't think I have an immediate example at hand as the few sites I'm actively managing these days are not set up for non-technical clients, but I just want my objection that this should not be classified as a XSS vulnerability but a feature noted, before #15936 is considered. #15936 does go beyond just what is reported here and from a cursory review some of that would certainly be good to resolve, but I don't want to be the one to tell people the help text they set up for their clients is no longer possible in 3.

(Just to make this reference to this PR easier to get to, the above is from @Mark-H re this PR.)

One quick question on the matter of XSS, and this is just to get a better understanding: Is it because of the context of these input forms being only accessible via a backend interface that XSS would not be applicable?

Regardless, it seems I should consider opening it up a bit in the following ways:

1. Allow the TV captions to have links (perhaps without scripting access?)
2. Allow the TV descriptions to use either a limited set of specified tags, or all tags other than <script>

[Jako]

Disallowing script tags is not enough. Each element can have several on attributes that can execute javascript.

[smg6511]

@Jako - Yes, it goes without saying that virtually all attributes should be discarded, especially event-based ones. ;-)

[Jako]

MODx.util.safeHtml does this and should be quite safe.

[smg6511]

OK all, I've made several updates in the latest commit to address the concerns raised above, as well as to make the processing more complete (on the php side) and flexible by making the tag and attribute restrictions customizable via new system settings.

[codecov-commenter]

Codecov Report

Attention: 95 lines in your changes are missing coverage. Please review.

Comparison is base (32aba73) 17.95% compared to head (c6812e4) 18.03%.
Report is 59 commits behind head on 3.x.

Files	Patch %	Lines
core/src/Revolution/modTemplateVar.php	5.55%	34 Missing ⚠️
core/src/Revolution/Processors/Element/Update.php	38.46%	24 Missing ⚠️
core/src/Revolution/modX.php	53.48%	20 Missing ⚠️
...evolution/Processors/Security/Forms/Set/Update.php	0.00%	11 Missing ⚠️
core/src/Revolution/Processors/Element/Create.php	86.48%	5 Missing ⚠️
core/src/Revolution/modParser.php	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                3.x   #15936      +/-   ##
============================================
+ Coverage     17.95%   18.03%   +0.08%     
- Complexity    10442    10468      +26     
============================================
  Files           561      561              
  Lines         39051    39161     +110     
============================================
+ Hits           7010     7062      +52     
- Misses        32041    32099      +58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JoshuaLuckers]

Would it be possible to add a Unit Test for this with various test cases?

[smg6511]

Yes, I will likely need a little guidance creating the tests (at least initially) as it'll be my first time doing so. I've been wanting to get some experience with UTs so I definitely don't want to pass it off ;-) I'll throw some questions to you on Slack via DM...

[Mark-H]

https://docs.modx.com/3.x/en/contribute/code/running-tests

[Mark-H]

I'm kinda worried this will impact existing sites and we'll get a bunch of issues reported that users suddenly can't use whatever fancy captions and descriptions they've used in the past, but as long as the safeHtml gets covered by sufficient tests and these new restrictions get added to the documentation I don't see a technical reason to reject it.