`Mailer` does not fully clear message state after sending message, for example Reply-To header, leading to unexpected behavior

[alexweissman]

When Mailer::send is called, it is supposed to fully clear the state of the underlying PHPMailer object as it pertains to message headers and content. That way, the Mailer instance can be used as a service to send additional emails during the lifetime of a request. The way this is currently accomplished is by calling clearAllRecipients.

The problem is that at least in the current version of PHPMailer, this does not clear Reply-To, Attachment, or custom headers. Those would appear to require separate calls to PHPMailer:: clearReplyTos, PHPMailer::clearAttachments, and PHPMailer::clearCustomHeaders, respectively. So if I create an email message, send it, then create another message with a different reply-to value, the new reply address will get appended instead of replacing the old reply address. This leads to unexpected behavior and a potential privacy concern, with the Reply-To header containing both addresses.

Honestly this looks like it needs a significant refactor to make our Mailer class truly stateless. One possibility is to simply create a new PHPMailer instance on demand when send is called. I'm not sure if that's something we considered and rejected for whatever reason in the past.

In the meantime, a quick fix might just be to call those extra clear methods wherever clearAllRecipients is called.

[lcharette]

Could be worth cloning the PHPMailer object at the beginning of both send method ?

https://github.com/userfrosting/sprinkle-core/blob/50eb7feb6973ce281ed0dab7b4f1649957b9bb72/app/src/Mail/Mailer.php#L94

[lcharette]

On a side note, this line sound useless, since the message it not returned ?

https://github.com/userfrosting/sprinkle-core/blob/50eb7feb6973ce281ed0dab7b4f1649957b9bb72/app/src/Mail/Mailer.php#L123

[alexweissman]

Could be worth cloning the PHPMailer object at the beginning of both send method ?

Yeah that might be a good way to do it. Have a "template" PHPMailer object that preserves configuration state, and then clone it before setting any message-specific properties and sending the message?

[lcharette]

On a side note, this line sound useless, since the message it not returned ?

https://github.com/userfrosting/sprinkle-core/blob/50eb7feb6973ce281ed0dab7b4f1649957b9bb72/app/src/Mail/Mailer.php#L123

Hum, this actually work because objects are passed as reference. Still, not sure Mailer should actually modify the message, to avoid unwanted result ?

[lcharette]

The clone approach does work. I tested it here : https://github.com/userfrosting/sprinkle-core/commit/cd31acb270851dd8d16b4ff8a1ab1ef8924cb1de#diff-5df6992aec5c88a4fa0356edd529a496241b08eec82b924924c8bf5f0d1d5d3eR107

Just note this commit is targeted toward 5.0 only ;). If it's an immediate issue on your side, it could be ported to 4.x...

[alexweissman]

It's a bit of an issue, though I was able to work around it by accessing the underlying PHPMailer object in my own wrapper and calling the clear methods there. I would say it's worth fixing in a 4.x release if there are other minor issues that need to be resolved as well. #917 is one thing that comes to mind.