Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple recipients with a separate email is really hard, causing data leaks #913

Closed
lode opened this issue Nov 29, 2019 · 3 comments
Closed
Labels
difficulty: unknown or n/a fix is unknown in difficulty status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap

Comments

@lode
Copy link

lode commented Nov 29, 2019

We're using the library to send a single template, with substitutions, to many recipients. They should not get each other's information thus we don't want a single email with everyone in the To/Cc field. We've used v5 of the library with success, however, since v7 of the library there's a bug in the usage of the Personalization object. We had to change our implementation due to this and made a mistake in this change which has caused a data leak of our customers.

We now found a safe way to do this in v7, but this is not a very common way and most other ways to do this leak data or don't send a correct email.

There's a few different methods we know of:

  1. Using Mail->addPersonalization() (see https://gist.github.com/lode/1daed164c0d5897be4ff8a1feadb0ce0#file-1-md)

    We were using this and it seems the most nice way. However since v7 this causes exceptions (maybe also v6, didn't test that), see Issue with personalization #882.

  2. Using Mail->addTo() with To->setSubstitutions() (see https://gist.github.com/lode/1daed164c0d5897be4ff8a1feadb0ce0#file-2-md)

    We used this as a replacement in v7. We thought using the substitutions would cause the library or the API to split it into multiple recipients. However this caused a single email to multiple recipients in the To-header and only the last recipient's substitution added to be used.

  3. Using Mail->addBcc() with Bcc->setSubstitutions() (see https://gist.github.com/lode/1daed164c0d5897be4ff8a1feadb0ce0#file-3-md)

    We tested this as well as a replacement for 2. This resulted in a single email with everyone in the bcc (that's good) but the substitutions were completely ignored (that's bad).

  4. Using Mail->addPersonalization() but using Mail->addTo() with To->setSubstitutions() for the first recipient (see https://gist.github.com/lode/1daed164c0d5897be4ff8a1feadb0ce0#file-4-md)

    This is the variant we're using right now and it seems to fix the bug of 1. However, it feels really weird to use it like this and makes for hard to read code.

  5. Using a custom generated json blob to talk to the API (see https://github.com/sendgrid/sendgrid-php/blob/master/examples/mail/mail.php#L39)

    We never tried this, but it is adviced by @thinkingserious in Global substitutions vs personalization substitutions #849 (comment) as a work around for the bug of 1. It is very error prone though. And as it completely ignores the library, we could just as well do an own Guzzle call.

  6. Sending separate emails.

    While obvisouly theoretically a working solution, it costs both us and SendGrid performance and bandwidth.

A fix for the bug in 1 was made in #705 half a year ago. However, no release has been issued since.

It would be great if we can get a release ASAP for this fix.

Next to that the usage patterns of 2 and 3 should be improved. Either the library should actually use different personalizations including correct substitutions. Or it should thrown an exception when trying to use substitutions for the addTo()/addBcc() methods.

I think on top the library could be improved in its architecture and coding standards. I've been diving into the code a little and find it very hard to find out what actually happens when using these different methods. Like how and when the Personalization objects are created. Right now the only way to discover the outcome is to adjust the API host and test the output of the API call. Cumbersome at least. Also it seems nowhere are there any checks on data integrety, causing the issues of 2 and 3. If this is all improved, I'd love to be able to call a method like Mail->willUseSeparateEmailsForAllRecipients() to verify that our usage is correct in automated processes and unit tests.

The data leak we had is our own fault. But it would help a lot if the library is less buggy and has more safe guards to do things right. This unclarity of how to do it properly is affecting multiple customers, see #568, #629, #686, #849, #856 (comment), #882.

I love to hear and help out where possible!

@thinkingserious
Copy link
Contributor

Wow @lode, thanks for the detailed feedback, I greatly appreciate it!

We are currently on track to implement a 2 week release cycle by next year on this library.

We always welcome PRs and if you would like to contribute I would greatly appreciate it, but right now, because of a very large backlog it will likely take us some time to review.

For now, I'm going to leave this one open until we make that next release and create follow up tickets to address the deficiencies you mentioned.

With best regards,

Elmer

@thinkingserious thinkingserious added difficulty: unknown or n/a fix is unknown in difficulty status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap labels Dec 6, 2019
@bog-h
Copy link

bog-h commented Dec 26, 2019

@thinkingserious Hi pal! Could you help me please, I'm a bit confused. I'm trying to find a way how to send multiple emails with single To for the security reason. I found that nodejs library has this option with sendMultiple() method, but I cannot find the similar for the php library!

From the @lode message I can understand that the real way to do it is number 4, "Using Mail->addPersonalization() but using Mail->addTo() with To->setSubstitutions()" is this right?

Thanks, Bogdan.

@thinkingserious
Copy link
Contributor

Since there has been no activity on this issue since March 1, 2020, we are closing this issue. Please feel free to reopen or create a new issue if you still require assistance. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: unknown or n/a fix is unknown in difficulty status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

No branches or pull requests

3 participants