Enabled script/style tags support in TinyMCE 6 #3653

empiricompany · 2023-11-13T17:03:05Z

Description (*)

Tinymce 6 editor removes the <script> and <style> tags.

Related Pull Requests

Fixed Issues (if relevant)

Fixes New TinyMCE 6 removes script and style tags #3651

Manual testing scenarios (*)

open some content that contains <script> or <style> tag with wysywyg editor (example content below)
hide editor and return to html

Expected result (*)

<script> / <style> tag not removed

Actual result (*)

<script> / <style> tags removed from html

Example Content to Test out https://regex101.com/r/Ub4FeY/2

<script>
console.log("This message should not appear when tinyMCE is active, as it means that it is executing JavaScript, exposing security issues");
</script> 
<script type="text/javascript">
console.log("This message should not appear when tinyMCE is active, as it means that it is executing JavaScript, exposing security issues");
</script> 
<script>
console.log("This message should not appear when tinyMCE is active, as it means that it is executing JavaScript, exposing security issues");
</script no-close >
<script type="text/javascript">// <![CDATA[ 
console.log("This message should not appear when tinyMCE is active, as it means that it is executing JavaScript, exposing security issues");
// ]]></script> 
<script type="text/javascript">// <![CDATA[ 
console.log("This message should not appear when tinyMCE is active, as it means that it is executing JavaScript, exposing security issues");
    jQuery(function($){ 
        $("#carousel").owlCarousel({autoPlay:3500,lazyLoad:true,stopOnHover: true,pagination: false, autoPlay: true,navigation: true,navigationText:["<i class='icon-left-open-big'></i>","<i class='icon-right-open-big'></i>"],slideSpeed : 1900,paginationSpeed : 900,singleItem:true,transitionStyle:"fade"}); 
  }); 
// ]]></script> 
<style><!-- 
.owl-bottom-narrow .owl-controls{width:95%;} 
  .owl-bottom-narrow .owl-controls .owl-buttons .owl-prev{color:#fff; float:left;} 
  .owl-bottom-narrow .owl-controls .owl-buttons .owl-next{color:#fff; float:right;} 
    .hidden-xs{display:block;} 
    .visible-xs{display:none;} 
    @media(max-width:540px){ 
        .hidden-xs{display:none;} 
      .visible-xs{display:block;} 
      .owl-carousel.owl-theme .owl-controls.clickable .owl-buttons div i{visibility:visible; vertical-align:middle;} 
      .owl-banner-carousel.owl-middle-narrow .owl-controls .owl-buttons div.owl-prev, .owl-banner-carousel.owl-middle-narrow .owl-controls .owl-buttons div.owl-next{visibility:visible;opacity:1; width: 50px; 
        height: 50px; padding:10px 10px; border-radius:50px; line-height:20px; background: rgba(0,0,0,0.3); color:#fff;} 
      .owl-middle-narrow .owl-controls{display:block!important;} 
  } 
  --></style> 

<div class="hide-mob hidden-xs">
	<div class="owl-carousel owl-theme owl-bottom-narrow owl-banner-carousel" id="carousel" style="border-bottom: solid 5px #0c06f7;">

		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		<div class="item">
			<a href="{{store direct_url='link'}}"><img alt="altImg" src="{{media url="wysiwyg/banner/test.jpg"}}" style="width: 100%;" /></a>
		</div>
		
	</div>
</div>

Themes like Porto include <script> and <style> tags in CMS content. However, when you modify this content using TinyMCE, they lose all the included scripts and styles.

It seems quite urgent because I had to restore the content from a backup.

Same resource to deal with:
TinyMCE 6 Docs - Content Filtering
Dealing with protect
stackoverflow

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All automated tests passed successfully (all builds are green)
Add yourself to contributors list

js/mage/adminhtml/wysiwyg/tinymce/setup.js

fballiano · 2023-11-13T17:07:03Z

I think they decided to filter scripts because of the vulnerability that we were asked to address with the update :-\

empiricompany · 2023-11-13T17:16:31Z

I think they decided to filter scripts because of the vulnerability that we were asked to address with the update :-\

From what I have read, this is because following the guidelines of HTML5 schema, the <style> tags should be placed within the and not in the body, whereas the <script> tag should not have type="text/javascript".
The <script> tags are converted to type="mce-text/javascript" (if they are allowed in valid_elements).
Anyway... with 'protect', they are converted like this.

However, for each line break it inserts that <p> tag, which, when switch to textarea mode, becomes <p> </p>.

js/mage/adminhtml/wysiwyg/tinymce/setup.js

empiricompany · 2023-11-13T18:02:42Z

Now it seems okay to me. I have tested it with complex client HTML that includes <script> / <style> tags and it doesn't break.
The empty

tag is no longer inserted.

fballiano · 2023-11-13T18:03:24Z

I'll try to test this ASAP

empiricompany · 2023-11-16T07:24:21Z

Has anyone had the opportunity to test it with various types of content? So far, I haven't encountered any issues.

fballiano · 2023-11-16T10:49:18Z

@empiricompany how exactly do you test it?

first example, I write the style in the textarea:

then I enable the editor and I don't see anything:

then I go back and the style is not there anymore:

empiricompany · 2023-11-16T13:10:07Z

Exactly how you did it:

Patch editor.js and do a hard refresh without cache to ensure you have the updated version.
Set the editor to be initially invisible.
Paste/write in the textarea, including the relevant tags.
Activate the editor.
Deactivate the editor.

I tried your text, and it seems to have an issue if the style is in the first line. If you put it within the text, it works but moves it to first line (due to the HTML5 directives that TinyMCE follow).

So it's still not perfect (definitely better than before), but does it work with my sample text?

empiricompany · 2023-11-16T15:26:43Z

@fballiano please retry

fballiano · 2023-11-16T22:23:43Z

it seems to work fine now, although sometimes it introduces some empty lines for some reasons (who cares) (in the screenshot I didn't type the empty lines between script and style, they were introduced switching the editor on and off):

it also supports <script type="text/javascript"> which the regex shouldn't support... I don't understand...

but it also supports <script src="xxxxxxx"> which... I don't know if that's a good thing...

empiricompany · 2023-11-17T09:20:04Z

@fballiano From my tests, the empty line is inserted if the <style> tag is on the first line, but it doesn't seem to be a problem since it's not a <p> tag.

The regex supports all attributes in the opening tag, in this part: <script([\s\S]*?)>

so it also supports the src attribute, but it was already like that before.

For this minor security issue, I would open another PR. For now, it works and doesn't cause any problems with the themes I'm using (which I hate 😅).

I have conducted numerous tests with different options, and this seems to be the best solution.

fballiano · 2023-11-17T09:21:43Z

what the heck, \s alwasy meant spaces in regex 😂

js/mage/adminhtml/wysiwyg/tinymce/setup.js

empiricompany · 2023-11-17T09:28:05Z

what the heck, \s alwasy meant spaces in regex 😂

yes 😅 it's a bit tricky

I have removed the style from protect due to conflicts with other options that were causing the some issues.

empiricompany · 2023-11-21T23:06:12Z

I think this PR is a priority because otherwise the new editor can cause data loss for many people. can we have more reviews?

empiricompany · 2023-11-21T23:10:05Z

maybe it's a good idea to release a patch in this critical cases before the next release?

js/mage/adminhtml/wysiwyg/tinymce/setup.js

addison74 · 2023-11-23T08:12:12Z

I use the <script> tag in pages and blocks. For example, there is a slider that needs to be initialized and customized. Since I'm a "good" content manager, I don't see any reason for concern. The fact that TinyMCE cleans the code is a preventive measure, but it should not limit the use.

If you want related advice I would ask for help in the Git page of TinyMCE project. I would do it myself, but I am not fully familiar with this issue, I have not tested anything so far.

empiricompany · 2023-12-02T09:33:35Z

anyone can give the last review so we can release a patch asap?

empiricompany · 2023-12-05T08:41:09Z

@addison74 can you review this please?

fballiano · 2023-12-09T16:41:54Z

I can't make it to work, if I type something like this:

then enable tinymce and hit save, the script gets removed

empiricompany · 2023-12-10T10:26:47Z

I hope that this new version works in all cases
https://regex101.com/r/Ub4FeY/2

fballiano

done multiple tests and it couldn't find any problem

fballiano · 2023-12-13T09:04:58Z

@addison74 @elidrissidev @kiatng @Flyingmana can we check this cause we probably should merge it asap, together with #3658, thank you

fballiano · 2023-12-20T10:20:13Z

@addison74 @elidrissidev @kiatng @Flyingmana @colinmollenhour can we check this cause we probably should merge it and release it asap

colinmollenhour

Looks good to me, but I have not done any testing. From a security standpoint I see no problems, it is up to TinyMCE to disable script execution in the editor while still protecting the script tags and it seems the tests you have performed already cover this well.

fballiano · 2023-12-20T18:26:01Z

thanks @colinmollenhour !

addison74 · 2023-12-20T19:40:51Z

Unfortunately the end of the year found me extremely busy and I didn't have time to test simple PRs like this one.

We will have to evaluate other tags so that they are not deleted. You mentioned for evaluation.

Related to the editor, in the old Tiny when I switched between the editor and the form, the paragraph tag <p> was added to the text. I did not check if this behavior was preserved.

empiricompany · 2023-12-21T08:42:58Z

@fballiano how can we create and manage a composer patch before the next release?
I would like to avoid manually modifying the file on multiple installations. Maybe a discussion is needed on how to handle these situations, much like how older Varien dealt with it. I understand it would be an additional effort and I'm not sure if we can handle it with our current workforce.

fballiano · 2023-12-21T08:44:28Z

@empiricompany you can directly link a PR as a composer patch

@addison74 we need to get this out asap so all of the rest should be addressed (in case it's needed) later

empiricompany · 2023-12-21T09:13:48Z

"extra": {
    "enable-patching": true,
    "magento-core-package-type": "magento-source",
    "magento-root-dir": ".",
    "patches": {
        "openmage/magento-lts": {
            "OM-3653": "https://patch-diff.githubusercontent.com/raw/OpenMage/magento-lts/pull/3653.diff"
        }
    }
}

fballiano · 2023-12-21T09:17:46Z

I just want to update to 6.8.2 and then do a release

WIP still creates empty <p>

5217b05

github-actions bot added the JavaScript Relates to js/* label Nov 13, 2023

github-advanced-security bot found potential problems Nov 13, 2023

View reviewed changes

js/mage/adminhtml/wysiwyg/tinymce/setup.js Fixed Show fixed Hide fixed

better HTML filtering regexp

e71fb1c

github-advanced-security bot found potential problems Nov 13, 2023

View reviewed changes

js/mage/adminhtml/wysiwyg/tinymce/setup.js Fixed Show fixed Hide fixed

case insensitive regexp

51515d3

github-advanced-security bot found potential problems Nov 13, 2023

View reviewed changes

js/mage/adminhtml/wysiwyg/tinymce/setup.js Fixed Show fixed Hide fixed

empiricompany added 2 commits November 13, 2023 18:56

catch script tag with space in the end

f80025a

WIP

553e589

empiricompany marked this pull request as ready for review November 13, 2023 18:00

cleanup

5dbf2e2

fix <style> tag as first line

9a4aaaa

empiricompany added 2 commits November 16, 2023 16:26

Merge branch 'main' into tinymce6-fix-tags

47b6e98

no wrap <style> as first line in <p>

4ed5f2e

improvements

5d77428

fballiano reviewed Nov 17, 2023

View reviewed changes

js/mage/adminhtml/wysiwyg/tinymce/setup.js Outdated Show resolved Hide resolved

empiricompany added 2 commits November 22, 2023 13:19

Merge branch 'main' into tinymce6-fix-tags

8616a3a

catch </script > with space

95f3cc7

github-advanced-security bot found potential problems Nov 22, 2023

View reviewed changes

js/mage/adminhtml/wysiwyg/tinymce/setup.js Fixed Show resolved Hide resolved

Merge branch 'main' into tinymce6-fix-tags

89ceeea

Merge branch 'main' into tinymce6-fix-tags

7153a2a

empiricompany and others added 2 commits December 10, 2023 10:57

Merge branch 'main' into tinymce6-fix-tags

43de212

update regexp

69321ee

empiricompany requested review from fballiano and sreichel December 10, 2023 10:31

empiricompany changed the title ~~[WIP] [HELP WANTED] TinyMCE 6 removes script and style tags~~ [HELP WANTED] TinyMCE 6 removes script and style tags Dec 10, 2023

fballiano approved these changes Dec 10, 2023

View reviewed changes

fballiano changed the title ~~[HELP WANTED] TinyMCE 6 removes script and style tags~~ Enabled script/style tags support in TinyMCE 6 Dec 10, 2023

fballiano mentioned this pull request Dec 13, 2023

TinyMCE (both new and old) strips svg #3701

Open

Merge branch 'main' into tinymce6-fix-tags

3f6434f

Merge branch 'main' into tinymce6-fix-tags

d4404cc

colinmollenhour approved these changes Dec 20, 2023

View reviewed changes

fballiano merged commit a42a85b into OpenMage:main Dec 20, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled script/style tags support in TinyMCE 6 #3653

Enabled script/style tags support in TinyMCE 6 #3653

empiricompany commented Nov 13, 2023 •

edited

Loading

fballiano commented Nov 13, 2023

empiricompany commented Nov 13, 2023

empiricompany commented Nov 13, 2023

fballiano commented Nov 13, 2023

empiricompany commented Nov 16, 2023

fballiano commented Nov 16, 2023

empiricompany commented Nov 16, 2023

empiricompany commented Nov 16, 2023

fballiano commented Nov 16, 2023

empiricompany commented Nov 17, 2023

fballiano commented Nov 17, 2023

empiricompany commented Nov 17, 2023

empiricompany commented Nov 21, 2023

empiricompany commented Nov 21, 2023

addison74 commented Nov 23, 2023

empiricompany commented Dec 2, 2023

empiricompany commented Dec 5, 2023

fballiano commented Dec 9, 2023

empiricompany commented Dec 10, 2023

fballiano left a comment

fballiano commented Dec 13, 2023

fballiano commented Dec 20, 2023

colinmollenhour left a comment

fballiano commented Dec 20, 2023

addison74 commented Dec 20, 2023 •

edited

Loading

empiricompany commented Dec 21, 2023

fballiano commented Dec 21, 2023

empiricompany commented Dec 21, 2023 •

edited

Loading

fballiano commented Dec 21, 2023

Enabled script/style tags support in TinyMCE 6 #3653

Enabled script/style tags support in TinyMCE 6 #3653

Conversation

empiricompany commented Nov 13, 2023 • edited Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Expected result (*)

Actual result (*)

Questions or comments

Contribution checklist (*)

fballiano commented Nov 13, 2023

empiricompany commented Nov 13, 2023

empiricompany commented Nov 13, 2023

fballiano commented Nov 13, 2023

empiricompany commented Nov 16, 2023

fballiano commented Nov 16, 2023

empiricompany commented Nov 16, 2023

empiricompany commented Nov 16, 2023

fballiano commented Nov 16, 2023

empiricompany commented Nov 17, 2023

fballiano commented Nov 17, 2023

empiricompany commented Nov 17, 2023

empiricompany commented Nov 21, 2023

empiricompany commented Nov 21, 2023

addison74 commented Nov 23, 2023

empiricompany commented Dec 2, 2023

empiricompany commented Dec 5, 2023

fballiano commented Dec 9, 2023

empiricompany commented Dec 10, 2023

fballiano left a comment

Choose a reason for hiding this comment

fballiano commented Dec 13, 2023

fballiano commented Dec 20, 2023

colinmollenhour left a comment

Choose a reason for hiding this comment

fballiano commented Dec 20, 2023

addison74 commented Dec 20, 2023 • edited Loading

empiricompany commented Dec 21, 2023

fballiano commented Dec 21, 2023

empiricompany commented Dec 21, 2023 • edited Loading

fballiano commented Dec 21, 2023

empiricompany commented Nov 13, 2023 •

edited

Loading

addison74 commented Dec 20, 2023 •

edited

Loading

empiricompany commented Dec 21, 2023 •

edited

Loading