feat(crons): Use appropriate email for notifying users of broken detections/auto-mutes

[davidenwang]

If the user set up alternative email routing for the given project of a monitor via settings like this:

Then respect it when sending the broken monitor emails.

[sentry-io]

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: src/sentry/monitors/tasks/detect_broken_monitor_envs.py

Function	Unhandled Issue
detect_broken_monitor_envs_for_org	SoftTimeLimitExceeded: SoftTimeLimitExceeded() se... Event Count: 1

---

_{Did you find this useful? React with a 👍 or 👎}

[mdtro]

Can we only send these emails to those that are verified email addresses? This has potential to be abused to spam accounts not under the user's control.

[davidenwang]

Can we only send these emails to those that are verified email addresses? This has potential to be abused to spam accounts not under the user's control.

Ahh yes we could check to make sure the email is verified, but could you elaborate more here? The user will be the one picking which email address they want communications to, so just want to understand how it might not be in their control.

[wedamija]

Can we only send these emails to those that are verified email addresses? This has potential to be abused to spam accounts not under the user's control.

Ahh yes we could check to make sure the email is verified, but could you elaborate more here? The user will be the one picking which email address they want communications to, so just want to understand how it might not be in their control.

An attacker could make a bunch of accounts for users using emails they don't own, then sentry would send them a bunch of spam. We've actually had this happen before from what I remember. I wonder if the function you're calling should just be getting validated emails, since get_email_addresses calls user_service.verify_user_emails, which sounds to me like it should return validated emails only. We could discuss that with hybrid cloud

[davidenwang]

Can we only send these emails to those that are verified email addresses? This has potential to be abused to spam accounts not under the user's control.

Ahh yes we could check to make sure the email is verified, but could you elaborate more here? The user will be the one picking which email address they want communications to, so just want to understand how it might not be in their control.

An attacker could make a bunch of accounts for users using emails they don't own, then sentry would send them a bunch of spam. We've actually had this happen before from what I remember. I wonder if the function you're calling should just be getting validated emails, since get_email_addresses calls user_service.verify_user_emails, which sounds to me like it should return validated emails only. We could discuss that with hybrid cloud

Ahh I see now ty @wedamija @mdtro for the headsup, yea we should definitely guard against this. Reached out to hybrid cloud but they don't seem to have opinions about this since they just ported the code over to make it silo-safe. Will probably add a parameter to get_email_addresses to only return emails if they are verified and run it by the alert folks.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.89%. Comparing base (5325518) to head (55f5eb5).
Report is 227 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #69595      +/-   ##
==========================================
+ Coverage   79.82%   79.89%   +0.06%     
==========================================
  Files        6495     6499       +4     
  Lines      288943   289669     +726     
  Branches    49759    49877     +118     
==========================================
+ Hits       230661   231436     +775     
+ Misses      57886    57837      -49     
  Partials      396      396              

Files	Coverage Δ
...entry/monitors/tasks/detect_broken_monitor_envs.py	94.28% <100.00%> (+0.05%)	⬆️

... and 191 files with indirect coverage changes