joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.74k stars 3.65k forks source link

PunycodeHelper::emailToPunycode does not support multiple email addresses #39247

Open alikon opened 1 year ago

alikon commented 1 year ago

Discussed in https://github.com/joomla/joomla-cms/discussions/39246

Originally posted by **ryandemmer** January 9, 2018 ### Steps to reproduce the issue In the Global Configuration, set to the Default Editor to "Editor - None" Go to Content -> Articles -> Create New Article. Enter in an article title. In the Content textarea, paste in: `Email List` Click Save. ### Expected result The content is saved as `Email List` ### Actual result The content is saved as `Email List` The list is clearly truncated after the first @ symbol. This happens regardless of whether a comma or semi-colon separator is used for the addresses. ### System information (as much as possible) Joomla 3.8.3 ### Additional comments This can be traced to [Joomla\CMS\String\PunycodeHelper::emailToPunycode](https://github.com/joomla/joomla-cms/blob/staging/libraries/src/String/PunycodeHelper.php#L207) via [Joomla\CMS\Component\ComponentHelper::filterText ](https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Component/ComponentHelper.php#L115) and [Joomla\CMS\Filter\InputFilter::emailToPunycode](https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Filter/InputFilter.php#L464)
richard67 commented 1 year ago

Hmm, the same applies to the method emailToUTF8: Both methods emailToPunycode and emailToUTF8 start with $explodedAddress = explode('@', $email); and so are not made for handling a comma-separated list of email addresses:

https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/String/PunycodeHelper.php#L198

https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/String/PunycodeHelper.php#L231

It could be fixed by surrounding that code with an explode(',', $email) and a forEach loop over the result.

brianteeman commented 1 year ago

And that's why we don't sweep issues under the carpet

richard67 commented 1 year ago

I could try to make a PR later today or tomorrow. If someone is faster just let us know here.

alikon commented 1 year ago

if you try to do it then probably you got a reply

if you don't ...

no response at all

what should be the better pattern ?

richard67 commented 1 year ago

Am working on it.

richard67 commented 1 year ago

Hmm, the same applies to the method emailToUTF8: Both methods emailToPunycode and emailToUTF8 start with $explodedAddress = explode('@', $email); and so are not made for handling a comma-separated list of email addresses:

https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/String/PunycodeHelper.php#L198

https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/String/PunycodeHelper.php#L231

It could be fixed by surrounding that code with an explode(',', $email) and a forEach loop over the result.

Not sure now after having read the previous discussion in #39246 if that would be the right fix.

The PunicodeHelper's emailToPunycode is used at several places:

./administrator/components/com_contact/src/Table/ContactTable.php:110: $this->email_to = PunycodeHelper::emailToPunycode($this->email_to);
./api/components/com_contact/src/Controller/ContactController.php:202: 'email'    => PunycodeHelper::emailToPunycode($data['contact_email']),
./components/com_contact/src/Controller/ContactController.php:234:     'email'    => PunycodeHelper::emailToPunycode($data['contact_email']),
./components/com_privacy/src/Model/RemindModel.php:47:                 $data['email'] = PunycodeHelper::emailToPunycode($data['email']);
./components/com_users/src/Model/ProfileModel.php:248:                 $data['email']    = PunycodeHelper::emailToPunycode($data['email1']);
./components/com_users/src/Model/RegistrationModel.php:432:            $data['email'] = PunycodeHelper::emailToPunycode($data['email1']);
./components/com_users/src/Model/RemindModel.php:108:                  $data['email'] = PunycodeHelper::emailToPunycode($data['email']);
./components/com_users/src/Model/ResetModel.php:350:                   $data['email'] = PunycodeHelper::emailToPunycode($data['email']);
./libraries/src/Filter/InputFilter.php:155:                            $text   = (string) str_replace($match, PunycodeHelper::emailToPunycode($match), $text);
./libraries/src/Form/Rule/EmailRule.php:83:                            $value = PunycodeHelper::emailToPunycode($value);
./libraries/src/Form/Rule/EmailRule.php:94:                            $value = PunycodeHelper::emailToPunycode($value);
./libraries/src/Mail/MailHelper.php:41:                                $value = PunycodeHelper::emailToPunycode($value);
./libraries/src/Table/User.php:234:                                    $this->email = PunycodeHelper::emailToPunycode($this->email);

It would need to check each of them if splitting the string by commas could break anything. But as far as I know, commas are not allowed in user names in email addresses (which are not punicoded) and also not in the domain part even if a unicode domain (which will be punicoded), so maybe we are safe with that.

richard67 commented 1 year ago

Maybe we should apply a fix only in the Joomla\CMS\Component\ComponentHelper::filterText method so it handles comma-separated email address lists only when embedded into content but not when used e.g. for a field which should still allow only 1 receiver.

richard67 commented 1 year ago

So or so, allowing comma or semicolon separated receiver lists and maybe also cc and bcc would also require changes on the email cloaking plugin, as far as I can see. There is not really a standard for that, so it depends on the email client. Some use commas and some semicolon, and some accept both. So it could be a valid use case for a corporate intranet site where you can rely on people using all the same email client, but not for a website where you can't rely on that.

A quick fix could be to fix the "emailToPunycode" method in the InputFilter so it correctly uses the first email address of the receiver list and not the wrong combination of the first email address plus the separator (comma or semicolon) plus the name part of the 2nd email address.

richard67 commented 1 year ago

I have a solution ready for for punicode encoding which works with email lists and also cc and bcc. But that does not help much because when you show an article with such mailto links in frontend you will see that the links are still broken because the email cloaking plugin needs to be fixed (or extended), too, and that's the bigger task. Not sure yet if I can do that, but will try.