amariussi commented 7 years ago

Issue

When saving an email template created with the HTML editor (textarea) option, the code is filtered and many tags are stripped and others are modified.

Since the option of an HTML editor using a textarea has been provided, the html should not be filtered if this editor is chosen. Otherwise the option is useless!

Expected Behavior

The textarea should save the email template exactly as it has been created. Without strpping or "purifying". It would be acceptable that an option could be given within the edit mode of the template with checkmarks to allow tags stripping as well as purifying.

Actual Behavior

The HTML gest stripped (tags such as body, style, head, etc..., disappear) and purified (eg a pure line of text is transformed into a

tag and many more things happen to the html code. At this point the editor is useless!

Possible Fix

Save the html code "as is". Nothing else!

Steps to Reproduce

Edit or create an email template using the HTML Editor
Save the html
Reopen it
Most of the code has either disappeared or has been modified
See SuiteCRM Forum example: https://suitecrm.com/forum/feedback/14487-bug-html-template-editor-removes-html-suitecrm-7-9-0

Context

Medium/High Plain HTML Editor is useless if it strips html tags and also changes the html to some uncontrolled new html.

Your Environment

SuiteCRM Version used: SuiteCRM 7.9.0
Browser name and version (e.g. Chrome Version 51.0.2704.63 (64-bit)): Latest 64 bit Chrome
Environment name and version (e.g. MySQL, PHP 7): Apache-PHP-MySQL
Operating System and version (e.g Ubuntu 16.04): Tested both on Linux and Windows

Dillon-Brown commented 7 years ago

@amariussi The HTML is run through a purifier which strips out data. This is done as certain elements of HTML can pose a security risk.

amariussi commented 7 years ago

@Dillon-Brown I know. But if the code is filtered there is no point in giving this option.

I would let the user decide and not the software.

With three editors we have: . mozaik - less flexible but very easy to use and effective . TinyMCE - medium flexibility, slightly less easy than mozaik . DirectHTML - full control, but only for experts

If we agree on this concept and make SuiteCRM really great, any kind of filtering should be removed from DirectHTML.

By the way an expert can save HTML directly in the DB so the security can ne bypassed anyway so I don't understand why not follow the above concept when it's nearly great. Please note that most e-mail marketing products allow editing HTML without any filtering taking place whatsoever.

samus-aran commented 7 years ago

Hi @amariussi - get what you are saying. We'll investigate this and over a suitable solution that is safe and provides users with the best means.

squestel commented 7 years ago

I have the same issue and we need to use our html template for ou campaigns but you don't allow us to do so.. !

stfnet commented 7 years ago

I have the same issue and I use to bypass it writing HTML directly to the table. I really hope to get an update like @amariussi said.

njovanovich commented 7 years ago

Please action this as we need style tags in our email templates!

apoonawa commented 7 years ago

html purifier strips out a lot of useful properties (for example border radius on buttons). Basically it just creates a lot of work for no reason. Html entry box is typically only used by developers and there should be a configuration option to turn off all html filtering.

apoonawa commented 7 years ago

Work around is as follows:

in file /data/SugarBean.php comment out line 1938 and 1944

//$this->$key = SugarCleaner::cleanHtml($this->$key, true); //$this->$key = SugarCleaner::cleanHtml($this->$key);

Please note : This disables cleaning Html all together. So be very careful about any html you enter anywhere. Use at your own risk. This change is not upgrade safe.

squestel commented 7 years ago

Hi @apoonawa , thanks for the tips. But unfortunately, that's not enough to bypass the "mashup" of my template... I've seeked the SugarCleaner method, it's places in clean.php which is in include folder, so I also tried commenting this method, or renaming the file so it's ignored in the application but I get blank page if I do so...

apoonawa commented 7 years ago

What version are you using? Have you set in your options to use direct html and disable mozaik?

On Aug 18, 2017 1:08 PM, "squestel" notifications@github.com wrote:

Hi @apoonawa https://github.com/apoonawa , thanks for the tips. But unfortunately, that's not enough to bypass the "mashup" of my template... I've seeked the SugarCleaner method, it's places in clean.php which is in include folder, so I also tried commenting this method, or renaming the file so it's ignored in the application but I get blank page if I do so...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/salesagility/SuiteCRM/issues/3672#issuecomment-323423051, or mute the thread https://github.com/notifications/unsubscribe-auth/AXdGrvWJ4IfamM50Yd9gdNnc_hDJDvkUks5sZdMrgaJpZM4Nyhli .

squestel commented 7 years ago

We're using the 7.8.5 version, and don't have the mozaik option on this version

bennnjamin commented 7 years ago

What if there is an "Insecure Mode" checkbox in your profile that disables sanitization of input (for direct HTML editor only, could even be an Admin level setting)? This way SuiteCRM can still provide security in their product by default, but advanced users can opt out with full knowledge of the risks. And lets face it, advanced users are putting HTML in the DB directly already. @samus-aran

spacexplorer commented 6 years ago

I have even tried to copy HTML code directly into the database field:

email_templates > bodyhtml

without ever opening it up in the bac-kend - just reference in choose template my email might look like this: (a snippet)

[code]

<td style="font-family:Segoe UI, News Gothic MT,Helvetica,Arial,sans-serif; font-size: 15px; line-height: 24px; margin: 0; background-color: #fff; color: #2d4050; ">
<p>
Hi $contact_first_name,
</p>                
<p>
Join us for another networking session on the terrace.
<br />
Come along and catch up with what&rsquo;s happening in our industry
<br />
Drinks and nibblies with a great mix of industry people.
</p>
</td>
<td style="font-family:Segoe UI, News Gothic MT,Helvetica,Arial,sans-serif; font-size: 15px; line-height: 24px; margin: 0; background-color: #fff; color: #2d4050; ">
<p>
Hi $contact_first_name,
</p>                
<p>
Join us for another networking session on the terrace.
<br />
Come along and catch up with what&rsquo;s happening in our industry
<br />
Drinks and nibblies with a great mix of industry people.
</p>
</td>

[/code]

then I send the email and it is not as formatted

I then copy the code again from the field in database and it has changed (at what stage??? During send???)

to this:

[code]

<td style="font-family:'Segoe UI', 'News Gothic MT', Helvetica, Arial, sans-serif;font-size:15px;line-height:24px;margin:0px;background-color:#ffffff;color:#2d4050;padding:3px 3px 3px 0px;">
<p style="font-family:Arial, Helvetica, sans-serif;font-size:14px;line-height:22.4px;color:#444444;padding:0px;margin:0px;">
Hi $contact_first_name,
</p>
<p style="font-family:Arial, Helvetica, sans-serif;font-size:14px;line-height:22.4px;color:#444444;padding:0px;margin:0px;">
Join us for another networking session on the terrace. 
<br style="font-family:Arial, Helvetica, sans-serif;font-size:14px;line-height:22.4px;color:#444444;padding:0px;margin:0px;" />
Come along and catch up with what&rsquo;s happening in our industry 
<br style="font-family:Arial, Helvetica, sans-serif;font-size:14px;line-height:22.4px;color:#444444;padding:0px;margin:0px;" />
Drinks and nibblies with a great mix of industry people.
</p>
</td>

[/code]

it adds
tags in some places with overriding style

it changes all my styles etc...

How can I disable this

belchfireid commented 6 years ago

I wonder if there are two separate problems here. We find that randomly, after editing, Mozaik applies a default style that changes the background, width, and other formatting, overriding what the user put in. The Mozaik style is inconsistent with what the users want, and it takes a technical person to reliably edit the html. Most of the time it works, but then the style will occasionally pop up again. This reformatting is different than purifying. Purifying is ok with us, just don't apply different document styles.

Either that, or document where the Mozaik style sheet is (editor options seem to be scattered all over the place).

raicomm commented 6 years ago

I am really stuck. When I change the editor, nothing happens.

If i put in basic code.

test

Everything appears fine,

example 1 example 2

I have tried the comment out of sugarclean, restarted the server, logged out/in. Changed to each of the 3 editors and have no luck.

I am on Version 7.10.7 Sugar Version 6.5.25 (Build 344)

Thanks

bennnjamin commented 6 years ago

Hello, I am not longer using SuiteCRM, but the workaround I used that always worked was to edit the HTML template in the database before using. This obviously not ideal but it DOES WORK. However, if you at any put use the WYSIWYG editor, it will overwrite your changes in the database and you'll have to do it again.

mennoachterberg commented 5 years ago

Work around is as follows:

in file /data/SugarBean.php comment out line 1938 and 1944

//$this->$key = SugarCleaner::cleanHtml($this->$key, true); //$this->$key = SugarCleaner::cleanHtml($this->$key);

Please note : This disables cleaning Html all together. So be very careful about any html you enter anywhere. Use at your own risk. This change is not upgrade safe.

In Suitecrm 7.10.10 & 7.10.11: You have to comment out lines: 2451 & 2456 (and NOT1938 and 1944) in: in file /data/SugarBean.php THEN IT WORKS FINE, ONLY FOR DIRECT HTML EMAIL EDITOR, EVEN WITH COMPLEX HTML SOURCE !

amariussi commented 5 years ago

It's nearly two years since we are discussing about this. It would be extremely simple to use a config boolean variable (to be entered manually in config_override.php) called DirectHTMLExpertMode: TRUE/FALSE Then in the part of code where purifying takes place just add an if statement that checks that you are editing a template, that you are using DirectHTML editor and that you have DirectHTMLExpertMode set to to TRUE. By doing so we resolve the issue in 5 minutes and we end endless and useless discussions making SuiteCRM greater. I just don't understand why this does not happen!

markbond1007 commented 5 years ago

There is bits missing to all of this. I completely agree this is higher priority than it has been assigned though.

Even with all the hacks/mashups etc, this still doesnt work properly. I can build a lovely mobile friendly email with this https://mosaico.io/ and even copying the code direct to the DB and disabling cleanHTML it still doesnt work (havent worked out completly why yet). I have worked out what needs to happen though:

Easiest Developed solution needs to have access to the tag of the Email sender. Currently this is hardcoded in SugarPHPMailer without access to this its never going to work properly (you need a meta viewport). So easiest would be to have a "secured" section of the template editor that had a box we can enter CSS and meta tags (possibly checked via CSS Tidy) that only admins/defined users get access to and this gets added into the head of the email by the system. We can then use standard classes etc to build/edit the emails (or possibly directHTML).

Regards

mark

pgorod commented 5 years ago

I would support an option for direct HTML entry, allowed only to Admin users. If an Admin wants to do an injection attack on his own system, I think we should let him :-)

But there really should be a way to just tell SuiteCRM "send this, exactly this, and stop fussing"... imho

shokupanjamin commented 4 years ago

I found commenting out the calls to SugarCleaner::cleanHtml() in /data/SugarBean.php didn't work in SuiteCRM 7.11.11.

The SugarCleaner class is defined in include/clean.php at line 56.

class SugarCleaner extends \SuiteCRM\HtmlSanitizer
{
}

To prevent the cleanHtml() function from running you can override the cleanHtml() function in the SugarCleaner class as follows:

class SugarCleaner extends \SuiteCRM\HtmlSanitizer
{
    public static function cleanHtml($dirtyHtml, $removeHtml = false) {
        return $dirtyHtml;
    }
}

This sends the $dirtyHtml parameter straight back without modification. I've still found that something is stripping out <head> tags from the HTML, but it does not seem to be mangling the html in the <body> tag.

(Usual flag that this modification would potentially allow malicious HTML to be inserted)

604media commented 4 years ago

Hello guys,

Has there been any progress on this - I would like to put my guys on this to try to help solve this issue - I feel it is a pretty high priority item. Please advise

markbond1007 commented 4 years ago

Hi, Well I found a few gotchas at my end at least. The Sugar/Suite process cleans ALL text/html fields as it saves and ideally you would want to keep most of that in place.

I did manage to create a small piece of code that works around that just for email templates though but it wasn’t something that could be made core ( it was a small hook/event based plugin )

Regards

Mark

604media commented 4 years ago

Hello Mark,

I would be interested in the details of your workaround while we look closer at the core updates.

I believe the team is dealing with this item as we speak - but I will be following up on this point.

markbond1007 commented 4 years ago

@604media

Okay well it’s basically working on the before_save hook. The clean process happens just before the before save hook. So when the hook is triggered in Edit mode for Email Templates I just re-copy the form posted data for the template back into the Sugar Object field. Because the clean has already happened recopying the posted data means it doesn’t get recleaned and saves as you posted it.

You do need to have HTML mode in as the JavaScript editors do have their own cleaners as well.

Regards

Mark

604media commented 4 years ago

@markbond1007 thank you for this info - nice little workaround. Did you post the code? I don't see anything here. Would you care to share that?

shokupanjamin commented 4 years ago

I'm still working on a solution for this. I've been a bit held up due to the global craziness right now.

I have managed to modify the code to allow the TinyMCE editor to be more configurable by referencing a custom JSON configuration file on the server.

However, HTMLPurifier is still removing any <style> tags (even though with the TinyMCE tweak I can configure TinyMCE to allow <style> tags).

The solution I'm working on for the CSS issue is to modify the EmailMan module code to read the stylesheet from a CSS file and inject it into a <style> tag just before the mailer processes send on the email. This bypasses HTMLPurifier entirely.

So far that seems to work ok, but it doesn't allow front-end user configuration. (Some might argue that's a good thing). A CSS file has to be on the server to be referenced.

Pasted my code below 👇🏼

shokupanjamin commented 4 years ago

Pasting my code below in case anyone else can tweak or improve it.

Tweak for TinyMCE JSON Configuration

File: include\SuiteEditor\SuiteEditorSettingsforTinyMCE.php

class SuiteEditorSettingsForTinyMCE extends SuiteEditorSettingsForDirectHTML
{
    /**
     * JSON setting for TinyMCE initializer script
     *
     * @var string
     */
    public $tinyMCESetup = '{}';

    /**
     * SuiteEditorSettingsForTinyMCE constructor.
     *
     * @param null $settings (optional)
     */
    public function __construct($settings = null)
    {
        parent::__construct($settings);

        $this->loadTinyMCEConfig(); //Calls private function to load TinyMCE configuration from JSON file.
    }

    /**
     *  Load TinyMCE Configuration from external JSON file.
     * 
     *  @see https://www.tiny.cloud/docs/ TinyMCE Documentation
     */
    private function loadTinyMCEConfig() {
        global $sugar_config;
        if(isset($sugar_config['tinyMCE_config_file'])) {
            //Check if file exists
            if(!file_exists($sugar_config['tinyMCE_config_file'])) {
                LoggerManager::getLogger()->error('TinyMCE Config File not found at: '.$sugar_config['tinyMCE_config_file']);
                return;
            }

            //Read the file
            $tinyMCEJson = file_get_contents($sugar_config['tinyMCE_config_file']);

            //Check if valid JSON
            if(is_null(json_decode($tinyMCEJson))) {
                LoggerManager::getLogger()->error("TinyMCE Configuration is not valid JSON. ".json_last_error_msg());
                return;
            }

            $this->tinyMCESetup = $tinyMCEJson;
        }
    }
}

This requires the following to be set in config_override.php:

$sugar_config['tinyMCE_config_file'] set to the filepath of a valid JSON file.

Other Notes:

JSON validity is checked by the code. If the JSON is not valid, then it will default back to 'no' configuration.
The JSON must also conform to the TinyMCE Configuration specification.
Some configuration allowed under the TinyMCE spec is still not supported and/or thrown away by HTMLPurifier (eg if you allow <head> or <style> tags in TinyMCE, this will still be ignored by HTMLPurifier.)

Tweak to Inject Stylesheet directly into Campaign Email Template before sending

File: modules\EmailMan\EmailMan.php @ line 1094:

            //if this template is textonly, then just send text body.  Do not add tracker, opt out,
            //or perform other processing as it will not show up in text only email
            if ($text_only) {
                $this->description_html = '';
                $mail->IsHTML(false);
                $mail->Body = $template_data['body'];
            } else {
                //=====BEGIN INSERTION OF CSS STYLESHEET========
                if(isset($sugar_config['campaign_email_insert_styles']) && isset($sugar_config['campaign_email_css_file'])) {

                    //Check if CSS is enabled in Sugar Config
                    if($sugar_config['campaign_email_insert_styles']) {
                        LoggerManager::getLogger()->debug('Campaign Email Insert Styles is ENABLED.');

                        //Check if CSS file exists
                        if(!file_exists($sugar_config['campaign_email_css_file'])) {
                            LoggerManager::getLogger()->error('Campaign Email CSS File not found at: '.$sugar_config['campaign_email_css_file']);
                        } else {

                            //Read CSS file contents and ensure it's not empty
                            $styles = file_get_contents($sugar_config['campaign_email_css_file']);
                            if(empty($styles)) {
                                LoggerManager::getLogger()->error('Campaign Email CSS File was empty or could not be read at: '.$sugar_config['campaign_email_css_file']);
                            } else {

                                //Wrap the CSS file contents in a <style> tag.
                                $styles = '<style><!-- '.$styles.' --></style>';
                                LoggerManager::getLogger()->debug('Styles Inserted into Campaign Email: '.PHP_EOL.$styles);

                                //Prepend Style at the start of the email body.
                                $template_data['body_html'] = $styles.$template_data['body_html'];
                            }
                        }
                    }
                }

                //Prepend a Meta tag for viewport control.
                $template_data['body_html'] = '<meta name="viewport" content= "width=device-width, initial-scale=1.0">'
                    .$template_data['body_html'];
                //=====END INSERTION OF CSS STYLESHEET========

                $mail->Body = wordwrap($template_data['body_html'], 900);

This requires the following variables to be set in config_override.php:

$sugar_config['campaign_email_insert_styles'] set to true
$sugar_config['campaign_email_css_file'] set to the filepath of a valid CSS file.

Other Notes:

Minified CSS may be used.
The code does not check for CSS validity, so recommend that admins test their CSS before deploying in production.
This only inserts CSS when sending an email campaign template.

pgorod commented 4 years ago

Hi everyone. I've been coding some advanced improvements to Email templates and campaigns for my PowerReplacer add-on. It's SponsorWare, which means that if everybody pulls together and sponsors a bit of it, all the code will eventually end up here in the core product. And sponsors get early access.

About the problem in this topic: in my code I use Twig templates, which in turn use a package called CssToInlineStyles. This actually moves all the prefixed CSS (<style> tags) into style attributes inlined in each referenced element (<p style = "...", for example). This greatly improves the results for restrictive email clients. You can write clean HTML + CSS and it will produce the ugly bloated crap that email clients prefer.

A further advantage is that you can have a big CSS file, with tons of styles, but the inlining only includes the ones actually used. I once received a commercial email that Gmail was showing like this:

Looking into it, I found this is caused by the email exceeding around 100 KB due to a huge included CSS file. The email content was only using a few styles, of course. Inlining would have made all this a lot more lightweight.

I think this is the correct way to go - CSS in emails is a tough problem to crack, far from trivial, and there are packages doing this in an advanced manner, we should consider them.

pgorod commented 4 years ago

A much more focused work-around, which affects only this Email Templates screen, is changing this function

https://github.com/salesagility/SuiteCRM/blob/v7.11.15/modules/EmailTemplates/EmailTemplate.php#L855

to become just this:

    public function cleanBean() {
        parent::cleanBean();
        $this->body_html = $GLOBALS['RAW_REQUEST']['body_html'];
    }

Having debugged this, I must say the amount of back and forth cleaning and uncleaning the body_html parameter is just insane. It should be cleansed just once, right before outputting. Instead, it's "cleaned" a ton of times, throwing away user's data for no good reason, several times... sigh...

beatific-angel commented 4 years ago

i upgrade tinymce editor (lastest version) but it doesnt work in pdf template it works in email template why this issues appeared? Could you help me? I need in today....

dtosun61 commented 4 years ago

How did you update it🤔

pgorod commented 4 years ago

@Beatific-Angel Cam you please ask your question in the Forums?

https://community.suitecrm.com/

It's appropriate there, but not here, Github is for something else. Thanks

mabullo commented 3 years ago

A much more focused work-around, which affects only this Email Templates screen, is changing this function

https://github.com/salesagility/SuiteCRM/blob/v7.11.15/modules/EmailTemplates/EmailTemplate.php#L855

to become just this:
    public function cleanBean() {
        parent::cleanBean();
        $this->body_html = $GLOBALS['RAW_REQUEST']['body_html'];
    }
Having debugged this, I must say the amount of back and forth cleaning and uncleaning the body_html parameter is just insane. It should be cleansed just once, right before outputting. Instead, it's "cleaned" a ton of times, throwing away user's data for no good reason, several times... sigh...

Thank you, I had the same problem with the email template editor, the html content was being 'purified' losing all the styles. Putting the suggested patch now works. It would be useful to be able to have a switch on the purification functionality in the admin.

pgorod commented 3 years ago

Fixing the over-zealous string clean-up in SuiteCRM is a major issue that should be addressed ASAP. There is no need for an Admin setting - there is just a need for "doing things right", following best-practices. I've posted about this elsewhere.

jalbaiges commented 3 years ago

Tons of comments have been added to this thread (and to others) and I think that at this time everybody is fully convinced that this is a big question that should be addressed ASAP. Just to bring another point of view, our customers use campaigns to send emails to their own customers and leads because their data is in the CRM, of course, but many of them use Mailchimp or other apps to create email templates (as they feel more comfortable and powerful using them). Once these templates come to the SuiteCRM editor they become mainly distorted and big efforts must be invested in order to put things well again (and usually without full success). More being said, most of these users are not techies so they just need a WYSWYG editor that simply respects their job. Being campaigning a core key functionality in SuiteCRM, I hardly understand why not simply take this question with the due priority and go ahead. Many of us would enjoy giving ideas and code but things should move in the other side. Thank you for reading, moderators. I'm sure most of us will be really graceful if things properly improve in the very coming times.

mabullo commented 2 years ago

It seems to me that the problem persists because I had to use the workaround proposed by @markbond1007 to be able to correctly save the content of a WYSIWYG type field...

SuiteBot commented 2 years ago

This issue has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/wysiwyg-cant-save-images/84070/8

pgorod commented 2 years ago

Different places where content is being messed with

Here are some stack traces to understand where it is and how it gets there...

Loading the Template editor Editview

If the text-only "body" is empty, it gets generated from the "body_html":

The retrieve function has an encode=true parameter default that causes this clean-up to happen:

Then there is this very ad-hoc clean-up which is so specific that it shouldn't be a problem...

Saving the Template

All of the above seem to happen both on opening the Edit view and when saving it. But when saving there are additional ones...

Here is the EmailTemplate override of the cleanBean with some attempt at leaving things more or less the same:

(never mind those "Custom" in the stack trace, just pretend they aren't there)

And this is what that call to parent::cleanBean does:

What should be there instead of all those clean-ups...

SQL cleanups in the DBManager right before sending data to the database. This is already there.
HTML clean-ups in the Smarty templates right before producing content to be sent to the browser, something as simple as this change in include/SuiteEditor/tpls/SuiteEditorDirectHTML.tpl and the other relevant tpls:

<textarea id="{$elementId}" name="{$elementId}" title="">{$contents}</textarea>

change to

<textarea id="{$elementId}" name="{$elementId}" title="">{$contents|escape:'htmlall'}</textarea>

SuiteBot commented 2 years ago

This issue has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/schedule-e-mails-for-email-marketing-campaigns-for-highest-deliverability/84731/6

SuiteBot commented 2 years ago

This issue has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/schedule-e-mails-for-email-marketing-campaigns-for-highest-deliverability/84731/8

salesagility / SuiteCRM

HTML email template editor output is filtered with purifier upon saving #3672

Issue

Expected Behavior

Actual Behavior

Possible Fix

Steps to Reproduce

Context

Your Environment

test

test

test

test

test

Tweak for TinyMCE JSON Configuration

Tweak to Inject Stylesheet directly into Campaign Email Template before sending

Different places where content is being messed with

Loading the Template editor Editview

Saving the Template

What should be there instead of all those clean-ups...