[3.x] Check if Html::decodeEntities is required and secure

Thanks for the report @LRwebks.

The change has been introduced here: #3246512: Strip HTML in error messages received via email → by @cameron prince.

This indeed looks risky to me, could you please explain the changes made @cameron prince?

I think this is a security risk, see
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

Be careful when using this function, as it will revert previous sanitization efforts (<script> will become

).

I think the issue description is pretty clear. The emails are sent as plain text, not HTML. This was added so that contents of the emails are more legible and don't include HTML code or encoded entities.

As the content isn't being rendered as HTML, I don't really get the security concern. If there is a better or more appropriate way, I'm all ears.

@cameron prince thanks for your super fast feedback!

The question is, if it might happen, that the mail is sent as HTML using contrib mail systems like Symfony mailer or others?

The point you made definitely mitigates the risk a lot. Now I'm unsure if there's risk left and on the other hand if there isn't a more safe way to fix this then, for example by using

1. https://www.php.net/manual/en/function.strip-tags.php
2. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Mail%21Ma...

instead?

https://api.drupal.org/api/drupal/core%21core.api.php/function/hook_mail already uses MailFormatHelper::htmlToText at the end for example.

So removing these characters is fine, but I think the decoding (which is the risky part) might not be needed?

Maybe the functions need to be reversed to alleviate your concerns, i.e. decode entities before stripping tags so that any decoded items that become tags are then stripped, too.

Thanks, yes I'll have a deeper look and provide a MR when I have the time!

It already uses strip_tags. This is what was added:

$body = Html::decodeEntities(strip_tags($body->__toString()));

Let's set this to minor, I think the risk is not existing or low. I'll test some cases asap.