feat(alerts): email alerts to configured recipients - #403
Open
AryamanSharma14 wants to merge 4 commits into
Open
feat(alerts): email alerts to configured recipients#403AryamanSharma14 wants to merge 4 commits into
AryamanSharma14 wants to merge 4 commits into
Conversation
Alerts already fan out to webhook endpoints and Central. Add mail as a third sink in notify(), so a bench can reach an inbox directly. Connection details live in one URL: smtp:// upgrades with STARTTLS, smtps:// connects over SSL, and the login name doubles as the sender. The password stays in its own field and is rejected inside the URL.
Contributor
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (4): Last reviewed commit: "fix(alerts): stop the mail sink from los..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
29
to
+31
| ) | ||
| webhook_endpoints: dict[str, str] = field(default_factory=dict) | ||
| smtp_url: str = "" # smtp://user@host:587 (STARTTLS) or smtps://user@host:465 (SSL) |
Contributor
There was a problem hiding this comment.
The new operator-facing fields ship without a docs/*.md update describing supported schemes, URL syntax, secret handling, and recipient configuration, making the interface difficult to configure safely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pilot/config/alert_limit.py
Line: 29-31
Comment:
**SMTP configuration lacks documentation**
The new operator-facing fields ship without a `docs/*.md` update describing supported schemes, URL syntax, secret handling, and recipient configuration, making the interface difficult to configure safely.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| data["datum"] = {"endpoint": self.datum.endpoint, "token": self.datum.token} | ||
| if self.resource_limits != ResourceLimitConfig(): | ||
| data["resource_limits"] = self._resource_limits_section() | ||
| data["resource_limits"] = asdict(self.resource_limits) |
Contributor
There was a problem hiding this comment.
asdict(self.resource_limits) serializes smtp_password directly into the ordinary TOML configuration, violating the repository requirement that secrets use protected password storage and exposing the credential to anyone who can read or copy the configuration. How this was verified: The new dataclass field is included unchanged by asdict() and passed to the TOML writer.
Context Used: Guidelines for reviewing Frappe Framework applicat... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: pilot/config/common.py
Line: 95
Comment:
**SMTP password persists as plaintext**
`asdict(self.resource_limits)` serializes `smtp_password` directly into the ordinary TOML configuration, violating the repository requirement that secrets use protected password storage and exposing the credential to anyone who can read or copy the configuration. **How this was verified:** The new dataclass field is included unchanged by `asdict()` and passed to the TOML writer.
**Context Used:** Guidelines for reviewing Frappe Framework applicat... ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.# Conflicts: # admin/frontend/dashboard/src/components/settings/Notifications.vue
smtplib's default context accepts any certificate, so both transports were encrypting to whoever answered - enough to hand over the SMTP password on an intercepted connection. Pass ssl.create_default_context() to SMTP_SSL and starttls(), and document the [resource_limits] delivery settings.
Comment on lines
+241
to
+242
| - omit the login name for a relay that takes no credentials | ||
| - the login name is also the sender address |
Contributor
There was a problem hiding this comment.
If an operator follows this guidance and omits the login name, send_mail() also leaves the From header empty, causing relays that require a sender address to reject every alert.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/configuration.md
Line: 241-242
Comment:
**Anonymous relay leaves sender empty**
If an operator follows this guidance and omits the login name, `send_mail()` also leaves the `From` header empty, causing relays that require a sender address to reject every alert.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.A malformed smtp_url raised out of send_mail, past notify, killing the whole tick - including the Central delivery that is meant to always work. Only the Admin PATCH path validates, so a hand-edited common_config.toml reached the send with anything in it. get_mail_endpoint now rejects a bad URL itself and is_mail_configured reads one as "no mail sink". send_message reports refused recipients by returning them rather than raising, so a single bad address counted as delivered and retired the alert for the mailboxes that never got it. Refusals now raise. An anonymous relay sent From:<>, the null bounce sender most relays refuse. Recipients are checked against the same pattern the form uses, and the form now mirrors the server's URL rules instead of a looser regex. Clearing the server URL clears the stored password, so a rotated credential has a way out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alerts already fan out to webhook endpoints and Central. This adds mail as a third sink in
notify(), so a bench can reach an inbox directly.Connection details live in a single URL —
smtp://upgrades with STARTTLS,smtps://connects over SSL, and the login name doubles as the sender. The password stays in its own field and is rejected if placed inside the URL. Settings sit under the existing Notifications panel next to the webhook list, and reuse its save flow, including the blank-means-unchanged rule for secrets.Verified against a real SMTP server with a real certificate: STARTTLS and SSL both deliver, a relay without encryption is refused rather than leaking the password, and a bad password surfaces as
OSErrorso the monitor tick survives it. Also checked end to end — saved through the settings API, read back from disk, and delivered on a sustained outage without re-sending.