[#32740] - [4.0] Introduce an HTML sanitiser
- Closed
- 19 Apr 2021
- Medium
- Build: 4.0-dev
- # 32740
- Diff
- dgrammatiko:—sanitizer—4.0-dev
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
This [when applied to all the scripts using innerHTML] fixes XSS vulnerabilities, so although it could be categorised as a new feature actually it's a feature to strengthen the security, so I guess an exception should NOT be debatable here. (then again I might be wrong, won't be the first time)
Summary of Changes
- Since the build tools allow us now to utilise properly the
importof the modern JS we will do exactly that!!! - Use the Bootstrap sanitiser
- Expose it as
Joomla.sanitizeHtml - Patch the
Joomla.renderMessagesas an example case
There is a slight penalty since we're adding code here, but I think it's acceptable, benefits outweigh the performance here:
Without the sanitiser:
With:
Testing Instructions
- Either download the installable zip from the PR's github page or apply the PR using git
- Try to login using wrong username or password
- There should be an alert warning you that username and password were incorrect
- Disable the Smart Search and all the plugins
- Check the components->Smart Search -> Index page (the page should have an alert with a link)
- That's it
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required
The bootstrap documentation: https://getbootstrap.com/docs/5.0/getting-started/javascript/#sanitizer
Documenting ALL the core.js functions should be done at some point
BUT more importantly, if this PR is approved and merged, there are countless instances of innerHTML that will have to be patched.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | JavaScript Repository NPM Change |
I’m a bit scared of the countless instances to be patched later.
BTW someone needs to patch the Joomla-browser thing used in codeception as we had to inject jQuery just to have the test pass...
BTW someone needs to patch the Joomla-browser thing used in codeception as we had to inject jQuery just to have the test pass...
Is that the reason why system tests are failing for this PR?
Is that the reason why system tests are failing for this PR?
Nope, this is probably a correct failure, checking rn
Is that the reason why system tests are failing for this PR?
Nope, this is probably a correct failure, checking rn
Here the last action in the system test log:
I upload file "com_media/test-image-1.png"
I see element in dom "input[name='file']"
I attach file "input[name='file']","com_media/test-im..."
I see and close system message "Item uploaded."
I get config "timeout"
I wait for text "Item uploaded.",90,{"id":"system-mes...}
I.e. it uploads an image in media manager and then waits for the message "Item uploaded."
That could be the Joomla.renderMessages failing.
Maybe your sanitizer found an insane html and sanitized it a bit too much?
| Labels |
Added:
NPM Resource Changed
?
|
||
Maybe your sanitizer found an insane html and sanitized it a bit too much?
No, it was far simpler: there wasn't anything in the alerts
Should be ok now
@brianteeman @infograf768 can you help here defining the allowed list of elements/attributes? Thanks
Happy to help but not sure I understand the question. Are the text-filters from global config useful?
OR perhaps this from tinymce https://www.tiny.cloud/docs/configure/content-filtering/#extended_valid_elements
Happy to help but not sure I understand the question
So the renderMessage is a JS function in the core.js script and it accepts an object like {error: ['some HTML text']
We need to define what we accept for the some HTML text, rn I just added div and span but probably we need p, a
Basically, the list would be derived from the ini files
@brianteeman so, I opted to use Bootsrap's default rules, if we need another set of rules we can simply compose them per case. Allowed list available here: https://github.com/twbs/bootstrap/blob/df2d5b6a37b54ff967edfa21a630cd1ecd223f7c/js/src/util/sanitizer.js#L58-L90
I think this should go into 4.0.0, hopefully it is RTC soon :-)
so, I opted to use Bootsrap's default rules
I'm wondering if the default set misses
<a href="javascript:[...]"
as an attack vector?
I'm wondering if the default set misses
It's covered due to https://github.com/twbs/bootstrap/blob/df2d5b6a37b54ff967edfa21a630cd1ecd223f7c/js/src/util/sanitizer.js#L26 and the rule a: ['target', 'href', 'title', 'rel'],
Edit: it goes without saying that some of our current code will fail to pass sanitization, but that's something that we have to deal on a per case approach
So if documentation is required where would we put it? We could link something from the Article Edit Help screen. Or do we need something aimed at coders? Tooltips and Popovers use the built-in sanitizer but right now ordinary users cannot use them (can they?). Something I have been working on today. Just noting interest in documentation!
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32740.
Or do we need something aimed at coders
This affects largely the coders, we need a section for Javascript and then some subsections for the API, Bootstrap, etc...
| Category | JavaScript Repository NPM Change | ⇒ | Repository Administration com_admin com_associations com_categories com_contact com_content com_fields com_finder com_installer com_joomlaupdate com_languages com_login JavaScript com_media NPM Change |
| Category | JavaScript Repository NPM Change Administration com_admin com_associations com_categories com_contact com_content com_fields com_finder com_installer com_joomlaupdate com_languages com_login com_media | ⇒ | JavaScript Repository NPM Change |
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-04-19 11:07:00 |
| Closed_By | ⇒ | dgrammatiko |
This sounds to me like a very good step into the right direction.?