Magento2: Error handling causes customer data leak

[ryanpalmerweb]

Preconditions (*)

1. Magento 2.3.1

Steps to reproduce (*)

1. Asynchronous email sending must be enabled in global settings
2. Backlog of unsent emails (e.g in Magento\Sales\Model\Order\Email\Sender\CreditmemoSender)
3. An error must be encountered during the getTransport method (e.g a plugin that references this tries to call an undefined function)
4. The application must run via cron so that any \Errors are automatically converted to \Exceptions by Magento\Framework\App\ErrorHandler.php

Expected result (*)

The class Magento\Framework\Mail\Template\TransportBuilder is implemented using the Singleton pattern, so should reset its internal variables after an error to prevent customers from seeing other customers order details.

Actual result (*)

The class Magento\Framework\Mail\Template\TransportBuilder is not reset, causing the customer who experienced the original issue to receive a copy of the next customers order/invoice/credit memo email.

Background information

One of our clients was receiving complaints from customers that they were being emailed the wrong customers orders, and were therefore concerned about their own privacy.

We isolated the error and found that the Fooman EmailAttachments module was referencing the createAttachment function which existed in Magento 2.2 but was removed in Magento 2.3

In Magento 2.2 :
class Message extends \Zend_mail implements MessageInterface

In Magento 2.3 :
class Message implements MailMessageInterface

Searching the web for "Call to undefined method Magento\Framework\Mail\Message\Interceptor::createAttachment()" showed 68 results, meaning that this is not an isolated fault.

The probability of this happening to other installations as a result of breaking changes to the core is very high

Suggested fix

The potential for a third party module to cause Magento to email the wrong customer is caused by the error handling in the SenderBuilder not resetting the TransportBuilder.

The send() function of the SenderBuilder should catch any errors, and reset the TransportBuilder to prevent this from happening.

Alternatively the TransportBuilder could be converted from a Singleton into a Factory.

[m2-assistant[bot]]

Hi @ryanpalmerweb. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

• [x] Summary of the issue
• [x] Information on your environment
• [x] Steps to reproduce
• [x] Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.3-develop instance - upcoming 2.3.x release

For more details, please, review the Magento Contributor Assistant documentation.

@ryanpalmerweb do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

• [ ] yes
• [ ] no

---

• Join Magento Community Engineering Slack and ask your questions in #github channel.

[ryanpalmerweb]

@okorshenko I can see this change relates to MAGETWO-67680

[orlangur]

Suggested fix
The potential for a third party module to cause Magento to email the wrong customer is caused by the error handling in the SenderBuilder not resetting the TransportBuilder.

The send() function of the SenderBuilder should catch any errors, and reset the TransportBuilder to prevent this from happening.

Alternatively the TransportBuilder could be converted from a Singleton into a Factory.

Core code must not take into consideration buggy plugins/interceptors. Sounds like the bug is in Fooman module.

[ryanpalmerweb]

It's surely dangerous to assert that there could never be an exception in this area of code, given the consequences.

In this case, a backwards incompatible change to the Magento core (a function being removed) led to the issue being triggered by a 3rd party plugin, however any exception in this area of code could lead to customers being inadvertently emailed and it's quite a serious breach of customer privacy if this happens.

"A personal data breach may, if not addressed in an appropriate and timely manner, result in physical, material or non-material damage to natural persons such as loss of control over their personal data or limitation of their rights, discrimination, identity theft or fraud, financial loss, unauthorised reversal of pseudonymisation, damage to reputation, loss of confidentiality of personal data protected by professional secrecy or any other significant economic or social disadvantage to the natural person concerned."

Generally, pushing customer data into a singleton object and relying on a specific execution path to reset this for the next use (which is bypassed by any exception handling) isn't safe behaviour

It would be relatively easy to safeguard this area of code, by initially resetting the transportBuilders internals or retrieving it via a factory pattern - is there any reason that this can't / shouldn't be done?

[orlangur]

@ryanpalmerweb no objections to re-create TransportBuilder each time, just that there is no defect in core currently which needs to be fixed. Third-party extensions need to be tested properly when upgrading.

[fooman]

As I was mentioned in here I wanted to note for future people coming across this that the issue would not exhibit when running the latest released versions of our extensions (and Composer should have prevented the 2.3.1 upgrade without also upgrading our extension) but I fully support making this code more defensive.

[ihor-sviziev]

@engcom-Golf @engcom-Alfa @engcom-Charlie could you check if this issue still valid on 2.4-develop?