Live page render includes links to plugin javascript and css

stevedrobinson commented 3 years ago

When viewing the final page created by the builder (not logged into Mautic, as an outside visitor), the builder javascript and css are linked from the page html, resulting in errors in the console:

alexhammerschmied commented 3 years ago

Yes. The finished page holds all links to the builder css and js files. We need to purge this code on save.

adiux commented 3 years ago

I found the problem with this now. So the styles that get added to the head of the landing page actually come from rendeting the “theme”.

In the base.html.twig are all the styles loaded that got added with ->addStylesheet(). And all the scripts that got added by using ->addScript().

<!DOCTYPE html>
<html>
    <head>
        <meta name="viewport" content="width=device-width, initial-scale=1.0">
        {% if page is defined %}
        <title>{pagetitle}</title>
            <meta name="description" content="{pagemetadescription}">
            {% endif %}
        {% block stylesheets %}{% endblock %}
            {{ outputHeadDeclarations() }}
    </head>
    <body>
        {{ outputScripts('bodyOpen') }}
        {% block content %}{% endblock %}
        {{ outputScripts('bodyClose') }}
    </body>
</html>

So we are a bit in a tricky situation. If we load the assets like we do now we get them loaded in the live page as well. If we load them automatically the grapesjs builder is loaded all the time.

I think the best way forward is to move the grapesjs assets back to the Assets/js and Assets/css folder. And to find a way how the grapesjs javascript is not loaded if the plugin is not activated.

dennisameling commented 3 years ago

A bit more context here - so it turns out that when creating a page, when the Twig template is rendered, it actually stores all the CSS/JS tags hardcoded in the database:

I just checked and this is not the case in the legacy builder in Mautic 3.3.3, so I went a bit deeper. outputHeadDeclarations() renders all the stylesheets that were added using ->addStylesheet() as @adiux mentioned. Those stylesheets get a data-source="mautic" attribute. The legacy builder removes those prior to persisting the data to the database, using a function called Mautic.sanitizeHtmlBeforeSave() (look how it removes all items with the data-source="mautic" attributes):

https://github.com/mautic/mautic/blob/cb1ff40f4898845a2a0355f99ddc10d675cd423e/app/bundles/CoreBundle/Assets/js/4.builder.js#L612-L633

So, if we simply run the data through Mautic.sanitizeHtmlBeforeSave() prior to saving it to the database, it won't be included in the database and therefore should not pop up anymore on public pages.

I'll mark this issue as "Essential" for now, since we can't easily fix this as soon as folks have this buggy data in their database. Fixing this before the 4.0.0 release would be ideal and shouldn't be too difficult with the function above 👍🏼

RCheesley commented 3 years ago

Could we get a PR up to address this issue do you think @dennisameling @adiux ?

dennisameling commented 3 years ago

Sure, working on a PR for this now, will be done either today or tomorrow 👍🏼

dennisameling commented 3 years ago

So this is turning out to be more difficult than expected. Mautic.sanitizeHtmlBeforeSave() expects a jQuery object (which isn't a problem), but the getCanvasAsHtmlDocument() function of the GrapesJS preset uses JavaScript's built-in DOMParser. That basically strips out JS and CSS tags (security feature from what I read in several online posts). @adiux worked around that in https://github.com/mautic/grapesjs-preset-mautic/blob/1128408c2a6cf5aaa622ac41f3d0f24b577e4638/src/content.service.js#L80-L92, which returns the DOM as a string. But I need the full DOM (including the JS/CSS tags) when running Mautic.sanitizeHtmlBeforeSave(). I saw in Mautic's own builder code that it uses an iFrame for the content, I guess to work around that same issue.

Will try the following two paths later:

Create a temporary iFrame to create a DOM and sanitize from there
Look at the logic in Mautic.sanitizeHtmlBeforeSave() and see if we can sanitize the string instead (using some regex, but really won't prefer this method)

Have to go now, will check again tomorrow but can't promise a time for that yet

adiux commented 3 years ago

@dennisameling maybe this helps? https://github.com/mautic/grapesjs-preset-mautic/blob/main/src/content.service.js#L84

or cant you convert an HTMLDocument to a jquery object? There must be a way.

dennisameling commented 3 years ago

@adiux saw that, but the HTMLDocument misses the head JS + CSS script tags in the DOM. And contentDocument.head.outerHTML + contentDocument.body.outerHTML return strings instead of actual DOM elements. So I guess the iFrame way should be feasible. Having a look at that now, just finished some other stuff

dennisameling commented 3 years ago

PR ready with the fix for this issue: https://github.com/mautic/mautic/pull/10360

RCheesley commented 3 years ago

@stevedrobinson can you give #10360 a test?

github-actions[bot] commented 3 years ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If this issue is continuing with the lastest stable version of Mautic, please open a new issue that references this one.

mautic / mautic

Live page render includes links to plugin javascript and css #9998

⚠️COMMENT VISIBILITY WARNING⚠️