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

fix: hide other recipients #911

Merged
merged 13 commits into from
Nov 19, 2024
Merged

fix: hide other recipients #911

merged 13 commits into from
Nov 19, 2024

Conversation

ColinBuyck
Copy link
Collaborator

@ColinBuyck ColinBuyck commented Oct 23, 2024

This PR addresses #873

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

This PR sends emails via 'bcc' rather than the 'to' field in order to hide the other recipients of the email. My research showed that the options are simple: use the bcc field to send in bulk or loop through each email and send individually. I've included some resources below mentioning the use of the bcc field. Both implementations are straightforward but I worried that sending emails individually could lead to serious performance issues. I also heard mentions of this approach requiring queuing systems which seemed out of scope. The risk of the bcc approach is that the empty "to" field is more likely to be flagged as suspicious so a dummy email like "[email protected]" could be used to help address that concern... though seemingly not entirely.

Nodemailer discussion: nodemailer/nodemailer#385

SES advocating for the altenative of sending individually: https://aws.amazon.com/blogs/messaging-and-targeting/how-to-send-messages-to-multiple-recipients-with-amazon-simple-email-service-ses/

Stackoverflow supporting bcc: https://stackoverflow.com/questions/66627529/send-email-to-undisclosed-recipients-nodemailer

Mentions of potential flagging with empty "to" field: https://security.stackexchange.com/questions/177713/how-did-i-get-this-email-without-a-to-field

https://stackoverflow.com/a/27855503

UPDATE: After discussing with product and Morgan, we're going to utilize bcc to send the bulk emails but use the jurisdiction's from address to avoid the spam flagging issue mentioned above.

UPDATE PART 2: The public side will have to be tested with totally separate emails not just modified with a plus symbol since two bcc emails to the same inbox will only show as one email.

How Can This Be Tested/Reviewed?

This can be tested most easily with the lottery flow. Create an admin and partner user connected to your email. Find a listing with lottery enabled and fill out two applications with two separate accounts on the public side. Then, go through the lottery flow with the two partners portal users and notice that emails sent to admins don't show [email protected] in the recipient list. Then, publish the results to the public and you're two applicant emails should send separately and not have visibility to one another in the email information.

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@ColinBuyck ColinBuyck marked this pull request as ready for review October 30, 2024 23:56
@ColinBuyck ColinBuyck added question Further information is requested 2 reviews needed labels Oct 30, 2024
@ColinBuyck
Copy link
Collaborator Author

Looking for thoughts on the decision between bcc and looping through emails described in the description above 🧼

@ludtkemorgan
Copy link
Collaborator

@ColinBuyck is this still on Hold?

@ColinBuyck ColinBuyck added 2 reviews needed and removed question Further information is requested on hold labels Nov 18, 2024
Copy link
Collaborator

@mcgarrye mcgarrye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧶

yarn.lock Outdated Show resolved Hide resolved
Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! super minor nit comment but giving approval!

api/src/services/listing.service.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@emilyjablonski emilyjablonski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hefty to test 😰 looks great!

@emilyjablonski emilyjablonski added ready to merge Should be applied when a PR has been reviewed and approved and removed 1 review needed labels Nov 18, 2024
@ColinBuyck ColinBuyck merged commit 7284e97 into main Nov 19, 2024
16 checks passed
@ColinBuyck ColinBuyck deleted the 873/email-bcc branch November 19, 2024 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Should be applied when a PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants