enonic / lib-admin-ui

Enonic XP Administration UI.
GNU General Public License v3.0
5 stars 5 forks source link

CKE strips script tags from HTML Area contents #677

Closed Bellfalasch closed 6 years ago

Bellfalasch commented 6 years ago

The cleanup CKEditor (at least in Text Component) does is too strong. On Enonic.com we rely heavily on Hubspot and using it to generate forms. To embed these we add we paste some HTML into CKEditor source code. These worked in TinyMCE but after switch to CKEditor, editing any of these blocks of text will instantly remove that script snippet.

Original content JSON looks like so. See the script in one of the Text Component blocks on this page (the last component on the page).

{
  "_id": "5ff8f67e-24e0-4604-9bf8-04cfdd6479e7",
  "_name": "progressive-web-apps",
  "_path": "/enonic-homepage/progressive-web-apps",
  "creator": "user:system:tsi",
  "modifier": "user:system:morten-ien-eriksen",
  "createdTime": "2017-08-03T09:08:05.724Z",
  "modifiedTime": "2018-05-15T11:55:49.799Z",
  "owner": "user:system:tsi",
  "type": "com.enonic.web.enonic.com:landing-page",
  "displayName": "Progressive Web Apps",
  "hasChildren": true,
  "language": "en",
  "valid": true,
  "data": {},
  "x": {
    "com-enonic-app-metafields": {
      "meta-data": {
        "seoDescription": "Gartner predicts that by 2020 Progressive Web Apps (PWA) will replace 50% of consumer-facing apps. Here's our report on Progressive Web Apps.",
        "blockRobots": false
      }
    },
    "com-enonic-web-enonic-com": {
      "menu-item": {
        "menuItem": false
      },
      "html-meta": {}
    }
  },
  "page": {
    "controller": "com.enonic.web.enonic.com:default",
    "config": {
      "offsetHeadingRank": false,
      "sidebarLink": "tryNow"
    },
    "regions": {
      "main": {
        "components": [
          {
            "name": "1 column layout",
            "path": "main/0",
            "type": "layout",
            "descriptor": "com.enonic.web.enonic.com:layout-1-col",
            "config": {
              "paddingTop": false
            },
            "regions": {
              "main": {
                "components": [
                  {
                    "name": "Text",
                    "path": "main/0/main/0",
                    "type": "text",
                    "text": "<h1 style=\"text-align: center;\">\"50% of consumer-facing apps will be<br /> Progressive Web Apps by 2020\"</h1>\n<figure class=\"center\" style=\"float: none; margin: auto;width: 60%;\"><img src=\"image://ce090bca-c09d-414c-ab57-0721b771e548\" alt=\"pwa\" style=\"text-align: center; width: 100%;\" />\n<figcaption style=\"text-align: left;\"></figcaption>\n</figure>"
                  }
                ],
                "name": "main"
              }
            }
          },
          {
            "name": "1 column - for narrow and big text",
            "path": "main/1",
            "type": "layout",
            "descriptor": "com.enonic.web.enonic.com:1-col-text",
            "config": {
              "paddingTop": false
            },
            "regions": {
              "main": {
                "components": [
                  {
                    "name": "Text",
                    "path": "main/1/main/0",
                    "type": "text",
                    "text": "<p><strong>Gartner predicts that<span>&nbsp;</span><a href=\"https://www.oreilly.com/ideas/5-ways-the-web-will-evolve-in-2018\" target=\"_blank\" rel=\"noopener noreferrer\">by 2020 Progressive Web Apps (PWA) will replace 50% of consumer-facing apps</a>. &nbsp;This initiative has been led by Google through enhancements to the Chrome browser and Android operating system, allowing developers to deliver web-based applications that rival, or even out-perform the native apps user experience.</strong></p>\n<p>Developments in web standards and browser functionality now enable you to build web applications that work offline and utilize functionality that was previously exclusive to native apps. All the major players in the browser and OS arena are now racing to deliver the best user experience for Progressive Web Apps.</p>\n<p>It stands to reason that a better user experience results in higher conversion rates. If you remove friction and frustrations from your application, you make it easier for customers to achieve their goals - and therefore yours. PWA presents huge opportunities to deliver better experiences, so whether its ecommerce, subscriptions, bookings, engagement, dwell time, loyalty, retention, cross selling, upselling or clicks, PWA can help boost these metrics.</p>"
                  },
                  {
                    "name": "Text",
                    "path": "main/1/main/1",
                    "type": "text",
                    "text": "<h2 style=\"text-align: center;\">REGISTER TO RECEIVE OUR PWA GUIDE</h2>\n<p style=\"text-align: left;\">Submit your e-mail below and we will send you our guide:&nbsp;<strong>10 reasons why you should consider Progressive Web Apps</strong></p>\n<p style=\"text-align: center;\"><strong></strong></p>\n<!-- [if lte IE 8]>\n<script charset=\"utf-8\" type=\"text/javascript\" src=\"//js.hsforms.net/forms/v2-legacy.js\"></script>\n<![endif]-->\n<script charset=\"utf-8\" type=\"text/javascript\" src=\"//js.hsforms.net/forms/v2.js\"></script>\n<script>// <![CDATA[\nhbspt.forms.create({\n\tportalId: \"3462655\",\n\tformId: \"d36bbcf5-1cea-4a99-9499-edbcfcabc97c\"\n});\n// ]]></script>"
                  }
                ],
                "name": "main"
              }
            }
          },
          {
            "name": "Call to action bar",
            "path": "main/2",
            "type": "part",
            "descriptor": "com.enonic.web.enonic.com:call-to-action-bar",
            "config": {
              "link": [
                {
                  "linkText": "Contact us",
                  "linkPage": "b37fee93-b4d0-4759-8dac-d27b102615d5"
                },
                {
                  "linkText": "Downloads",
                  "linkPage": "2d828f11-c6b3-49c9-9c16-aa7eb4c7e345"
                },
                {
                  "linkText": "PWA case study",
                  "linkPage": "6e1d03af-070c-452f-abc7-f7c2a666527b"
                }
              ]
            }
          }
        ],
        "name": "main"
      }
    }
  },
  "attachments": {},
  "publish": {
    "from": "2017-08-03T12:24:30.883Z",
    "first": "2017-08-03T12:24:30.883Z"
  }
}

After opening the text component for editing, the source will turn into this:

<h2 style="text-align:center">REGISTER TO RECEIVE OUR PWA GUIDE</h2>

<p style="text-align:left">Submit your e-mail below and we will send you our guide:&nbsp;<strong>10 reasons why you should consider Progressive Web Apps</strong></p>

<p style="text-align:center">&nbsp;</p>
<!-- [if lte IE 8]>
<script charset="utf-8" type="text/javascript" src="//js.hsforms.net/forms/v2-legacy.js"></script>
<![endif]-->

This removes the form we embedded.

sigdestad commented 6 years ago

Have you been allowed to paste scripts into HTML area???

Ai ai ai.... This is a massive security hole in XP! This must definetly NOT be allowed with CK.

My biggest question here is how this can be possible regardless of editor as we are using a back-end function to cleanup html to avoid this. Any comments @aro @GlennRicaud

You must find a different way to add scripts to pages

Bellfalasch commented 6 years ago

This has been allowed and working since 5.0 and everyone is using it. We can't stop allowing it without some kind of upgrade step / migration, at minimum wrap it in a script-macro or similar.

CKE keeps the script inclusion, but removes some important config that Hubspot needs from within the script.

sigdestad commented 6 years ago

At least we have identified a breaking change we need to do for 7.0 then

alansemenov commented 6 years ago

So what do we do here? Allow scripts for 6.15 and then forbid it in 7.0 and use migration script to extract scripts (where?) ?

Bellfalasch commented 6 years ago

Found that script on enonic.com, it fully looks like this: But yes, in the perfect world, this should have used a macro to wrap/protect the script.

<p style="text-align: center;"><strong></strong></p>
<!-- [if lte IE 8]>
<script charset="utf-8" type="text/javascript" src="//js.hsforms.net/forms/v2-legacy.js"></script>
<![endif]-->
<p>
<script charset="utf-8" type="text/javascript" src="//js.hsforms.net/forms/v2.js"></script>
<script>// <![CDATA[
hbspt.forms.create({
    portalId: "3462655",
    formId: "20ff7fa4-4940-4bbb-9f7a-06cc26ba9c33"
});
// ]]></script>
</p>
Bellfalasch commented 6 years ago

So, might be that it is not the <script> itself but perhaps the emptiness of the <p> that causes this? Not sure, but worth checking.

sigdestad commented 6 years ago

I agree with alan, allow for 6.15 and forbid in 7.0 - combined with upgrade script that will remove script tags from all htmlArea fields... Ok?

sigdestad commented 6 years ago

We need backlog tasks to clean this up in 7.0 so we don't forget!

Bellfalasch commented 6 years ago

I'm not ok with deleting any user input, actually. =P That would break many forms across our entire website, and that is just us, I can imagine tens of websites out there possibly also using the HtmlArea to add stuff they need, like scripts and whatnot. We need to properly take care of that in an upgrade, not deleting it. Could we break it out into TextArea blocks next to the HtmlArea and use a macro to fetch contents of these blocks?

To be honest I'm not sure why scripts are so dangerous. Only admins have access to these fields, and only high site privileges have access to HTML source. The few people that work in there do know what they're doing. I would argue the risks here are minimal. Basically any other system out there allows this type of input.

sigdestad commented 6 years ago

This will be done, and it will be documented thoroughly in the upgrade script.

Scripts are extremely dangerous as any editor on gjensidige for instance could add malware or cracking scripts on gjensidiges production site, potentially triggering transactions or other events in the name of other users. This is very very bad.

alansemenov commented 6 years ago

Enabled scripts in 6.15 and created a task for 7.0: https://github.com/enonic/lib-admin-ui/issues/683