feat: email notification delivery via SMTP (#961)#971
Conversation
Add SMTP-based email delivery for notifications so self-hosters receive alerts when not actively using the app. email_delivery.py: - send_notification_email() — sends via SMTP with TLS support - format_notification_email() — converts notification to email with severity-colored HTML template - is_email_delivery_configured() — check if SMTP is available - Configured via env: SMTP_HOST, SMTP_PORT, SMTP_USER, SMTP_PASSWORD, SMTP_FROM_ADDRESS, SMTP_USE_TLS .env.example: - Added SMTP configuration section with commented defaults Follow-up needed: - Hook into jobs_notifications_service to send emails on notification creation (check user email preference + SMTP availability) - Add user email address storage to notification preferences - Web Push API support (separate effort) Closes #961 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new email notification delivery module using SMTP, along with the necessary configuration updates in the .env file. The review identified several critical issues: the synchronous implementation conflicts with the asynchronous usage example in the docstring, there is a potential XSS vulnerability due to unescaped HTML content, and the port configuration lacks robust error handling. Additionally, the code contains redundant logic in the SMTP connection flow and an unused parameter in the formatting function.
| return _get_smtp_config() is not None | ||
|
|
||
|
|
||
| def send_notification_email( |
There was a problem hiding this comment.
The function is defined as synchronous (def), but the usage example in the module docstring (line 12) shows it being awaited (await). If this function is intended to be used within an asynchronous context (e.g., a FastAPI endpoint), it will block the event loop during network I/O, which can severely impact performance. Consider defining it as async def and using asyncio.to_thread for the smtplib calls, or using an asynchronous library like aiosmtplib.
| body_html = f""" | ||
| <div style="font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', sans-serif; max-width: 600px; margin: 0 auto;"> | ||
| <div style="border-left: 4px solid {severity_color}; padding: 16px; margin: 16px 0; background: #f9fafb; border-radius: 0 8px 8px 0;"> | ||
| <h2 style="margin: 0 0 8px 0; font-size: 16px; color: #111827;">{title}</h2> | ||
| <p style="margin: 0; color: #6b7280; font-size: 14px;">{message}</p> | ||
| </div> | ||
| {"<p><a href='" + link_url + "' style='color: " + severity_color + "; text-decoration: none; font-weight: 500;'>View details →</a></p>" if link_url else ""} | ||
| <p style="color: #9ca3af; font-size: 12px; margin-top: 24px;">Sent by tldw server</p> | ||
| </div> | ||
| """ |
There was a problem hiding this comment.
The HTML body is constructed using f-strings without escaping the title, message, or link_url variables. This creates a potential XSS vulnerability if these values contain malicious HTML or scripts, especially since they may originate from external scraped content. Use html.escape to sanitize these inputs.
| return None | ||
| return { | ||
| "host": host, | ||
| "port": int(os.environ.get("SMTP_PORT", "587")), |
There was a problem hiding this comment.
Directly calling int() on an environment variable without validation will raise a ValueError if the value is non-numeric. It's safer to handle this gracefully with a default value.
| "port": int(os.environ.get("SMTP_PORT", "587")), | |
| "port": int(os.environ.get("SMTP_PORT", "587")) if os.environ.get("SMTP_PORT", "587").isdigit() else 587, |
| if config["use_tls"]: | ||
| context = ssl.create_default_context() | ||
| with smtplib.SMTP(str(config["host"]), int(config["port"])) as server: | ||
| server.starttls(context=context) | ||
| if config["user"] and config["password"]: | ||
| server.login(str(config["user"]), str(config["password"])) | ||
| server.sendmail(str(config["from_address"]), to, msg.as_string()) | ||
| else: | ||
| with smtplib.SMTP(str(config["host"]), int(config["port"])) as server: | ||
| if config["user"] and config["password"]: | ||
| server.login(str(config["user"]), str(config["password"])) | ||
| server.sendmail(str(config["from_address"]), to, msg.as_string()) |
There was a problem hiding this comment.
There is significant code duplication between the TLS and non-TLS branches. The logic can be simplified by moving the shared connection and login steps outside the conditional blocks.
with smtplib.SMTP(str(config["host"]), config["port"]) as server:
if config["use_tls"]:
server.starttls(context=ssl.create_default_context())
if config["user"] and config["password"]:
server.login(str(config["user"]), str(config["password"]))
server.sendmail(str(config["from_address"]), to, msg.as_string())| # --------------------------------------------------------------------------- | ||
|
|
||
| def format_notification_email( | ||
| kind: str, |
There was a problem hiding this comment.
The parameter kind is defined but never used within the function body.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tldw_Server_API/app/core/Notifications/email_delivery.py`:
- Around line 128-135: The HTML template injects raw title, message and link_url
into body_html; escape title and message with an HTML-escaping utility (e.g.,
html.escape) before interpolation and also sanitize severity_color if it's
derived from user input; validate and normalize link_url using a URL parser
(e.g., urllib.parse) and only allow safe schemes (http, https, mailto) and safe
hosts/ports, then use the escaped normalized URL for the anchor href and text;
update the body_html construction (where title, message, link_url are used) to
interpolate the escaped values and drop any raw user-supplied content without
validation.
- Around line 31-43: The SMTP port parsing in _get_smtp_config can raise
ValueError and should be treated as a handled "not configured" case instead of
letting exceptions propagate; wrap the int(os.environ.get("SMTP_PORT", "587"))
parse in a try/except (catch ValueError) and if parsing fails (or the parsed
port is out of 1–65535 range) return None (or otherwise surface a controlled
config-error object) so is_email_delivery_configured() and
send_notification_email() continue to see an unconfigured SMTP setup rather than
crash; update _get_smtp_config accordingly and keep callers' existing
None-handling behavior.
- Around line 89-92: The success and error logs in the email sending block (the
logger.info and logger.error calls in email_delivery.py inside the send/notify
function) are currently recording raw recipient addresses and subjects; change
them to avoid PII by either logging a generated/internal notification_id or a
masked recipient (e.g., hash or show only first char + domain) and remove the
subject from logs. Update the code paths that call logger.info(f"Notification
email sent to {to}: {subject}") and logger.error(f"Failed to send notification
email to {to}: {exc}") to instead log the safe identifier (notification_id or
masked_to) plus the exception text where appropriate, ensuring no raw email
addresses or subject content are written to logs. Ensure notification_id is
created/returned by the send function or passed in from the caller so it can be
used in both success and error logs.
In `@tldw_Server_API/Config_Files/.env.example`:
- Around line 201-208: The SMTP variables in .env.example are misnamed and don't
match the live mailer used by NotificationsService.deliver_email (which calls
_email_service.send_email) and the AuthNZ backend that expects SMTP_USERNAME and
EMAIL_FROM; either rename the example keys to SMTP_USERNAME and EMAIL_FROM (and
SMTP_PASSWORD/SMTP_HOST/SMTP_PORT/SMTP_USE_TLS as appropriate) to match the
backend, or update/centralize the mailer wiring so NotificationsService uses the
new keys; locate NotificationsService.deliver_email and the email backend in
app/core/AuthNZ/email_service.py (and the _email_service reference) and make the
env var names consistent across both places.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e9cfe47-feaa-4ec6-b38c-88ea2ba67f42
📒 Files selected for processing (2)
tldw_Server_API/Config_Files/.env.exampletldw_Server_API/app/core/Notifications/email_delivery.py
| def _get_smtp_config() -> dict[str, str | int | bool] | None: | ||
| """Read SMTP config from environment. Returns None if not configured.""" | ||
| host = os.environ.get("SMTP_HOST", "").strip() | ||
| if not host: | ||
| return None | ||
| return { | ||
| "host": host, | ||
| "port": int(os.environ.get("SMTP_PORT", "587")), | ||
| "user": os.environ.get("SMTP_USER", "").strip(), | ||
| "password": os.environ.get("SMTP_PASSWORD", "").strip(), | ||
| "from_address": os.environ.get("SMTP_FROM_ADDRESS", "noreply@tldw.local").strip(), | ||
| "use_tls": os.environ.get("SMTP_USE_TLS", "true").lower() in ("true", "1", "yes"), | ||
| } |
There was a problem hiding this comment.
Treat malformed SMTP config as a handled failure, not an exception.
int(os.environ.get("SMTP_PORT", "587")) can raise inside _get_smtp_config(), which means both is_email_delivery_configured() and send_notification_email() can blow up on a bad SMTP_PORT. That breaks the module’s advertised graceful-failure behavior. Validate the port here and return None or a handled config error when parsing fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tldw_Server_API/app/core/Notifications/email_delivery.py` around lines 31 -
43, The SMTP port parsing in _get_smtp_config can raise ValueError and should be
treated as a handled "not configured" case instead of letting exceptions
propagate; wrap the int(os.environ.get("SMTP_PORT", "587")) parse in a
try/except (catch ValueError) and if parsing fails (or the parsed port is out of
1–65535 range) return None (or otherwise surface a controlled config-error
object) so is_email_delivery_configured() and send_notification_email() continue
to see an unconfigured SMTP setup rather than crash; update _get_smtp_config
accordingly and keep callers' existing None-handling behavior.
| logger.info(f"Notification email sent to {to}: {subject}") | ||
| return True | ||
| except Exception as exc: | ||
| logger.error(f"Failed to send notification email to {to}: {exc}") |
There was a problem hiding this comment.
Stop logging recipient addresses and subjects.
Both paths persist raw email addresses, and the success log also records notification content via subject. That leaks PII and potentially sensitive alert data into application logs; log a masked recipient or an internal notification ID instead. As per coding guidelines, "Never log sensitive information (API keys, passwords)".
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 90-90: Consider moving this statement to an else block
(TRY300)
[warning] 91-91: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tldw_Server_API/app/core/Notifications/email_delivery.py` around lines 89 -
92, The success and error logs in the email sending block (the logger.info and
logger.error calls in email_delivery.py inside the send/notify function) are
currently recording raw recipient addresses and subjects; change them to avoid
PII by either logging a generated/internal notification_id or a masked recipient
(e.g., hash or show only first char + domain) and remove the subject from logs.
Update the code paths that call logger.info(f"Notification email sent to {to}:
{subject}") and logger.error(f"Failed to send notification email to {to}:
{exc}") to instead log the safe identifier (notification_id or masked_to) plus
the exception text where appropriate, ensuring no raw email addresses or subject
content are written to logs. Ensure notification_id is created/returned by the
send function or passed in from the caller so it can be used in both success and
error logs.
| body_html = f""" | ||
| <div style="font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', sans-serif; max-width: 600px; margin: 0 auto;"> | ||
| <div style="border-left: 4px solid {severity_color}; padding: 16px; margin: 16px 0; background: #f9fafb; border-radius: 0 8px 8px 0;"> | ||
| <h2 style="margin: 0 0 8px 0; font-size: 16px; color: #111827;">{title}</h2> | ||
| <p style="margin: 0; color: #6b7280; font-size: 14px;">{message}</p> | ||
| </div> | ||
| {"<p><a href='" + link_url + "' style='color: " + severity_color + "; text-decoration: none; font-weight: 500;'>View details →</a></p>" if link_url else ""} | ||
| <p style="color: #9ca3af; font-size: 12px; margin-top: 24px;">Sent by tldw server</p> |
There was a problem hiding this comment.
Escape and validate the HTML fields before interpolation.
title, message, and link_url are injected straight into the HTML template. Any notification text containing markup or quotes can rewrite the rendered email body or anchor target. HTML-escape the text fields and only render links after normalizing/restricting them to expected schemes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tldw_Server_API/app/core/Notifications/email_delivery.py` around lines 128 -
135, The HTML template injects raw title, message and link_url into body_html;
escape title and message with an HTML-escaping utility (e.g., html.escape)
before interpolation and also sanitize severity_color if it's derived from user
input; validate and normalize link_url using a URL parser (e.g., urllib.parse)
and only allow safe schemes (http, https, mailto) and safe hosts/ports, then use
the escaped normalized URL for the anchor href and text; update the body_html
construction (where title, message, link_url are used) to interpolate the
escaped values and drop any raw user-supplied content without validation.
| # Configure to receive notification emails (watchlist alerts, job failures). | ||
| # Without SMTP, notifications are only visible in-app. | ||
| #SMTP_HOST=smtp.gmail.com | ||
| #SMTP_PORT=587 | ||
| #SMTP_USER=your-email@gmail.com | ||
| #SMTP_PASSWORD=your-app-password | ||
| #SMTP_FROM_ADDRESS=noreply@tldw.local | ||
| #SMTP_USE_TLS=true |
There was a problem hiding this comment.
This SMTP block does not match the notification sender the app currently uses.
NotificationsService.deliver_email() in tldw_Server_API/app/core/Notifications/service.py, Lines 39-84 still routes mail through _email_service.send_email(...), and the existing backend in tldw_Server_API/app/core/AuthNZ/email_service.py, Lines 367-377 reads SMTP_USERNAME/EMAIL_FROM, not SMTP_USER/SMTP_FROM_ADDRESS. As written, operators can fill in this new section and still leave the live notification path unconfigured. Align these names with the existing backend or wire notifications to this new helper before documenting this as the SMTP setup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tldw_Server_API/Config_Files/.env.example` around lines 201 - 208, The SMTP
variables in .env.example are misnamed and don't match the live mailer used by
NotificationsService.deliver_email (which calls _email_service.send_email) and
the AuthNZ backend that expects SMTP_USERNAME and EMAIL_FROM; either rename the
example keys to SMTP_USERNAME and EMAIL_FROM (and
SMTP_PASSWORD/SMTP_HOST/SMTP_PORT/SMTP_USE_TLS as appropriate) to match the
backend, or update/centralize the mailer wiring so NotificationsService uses the
new keys; locate NotificationsService.deliver_email and the email backend in
app/core/AuthNZ/email_service.py (and the _email_service reference) and make the
env var names consistent across both places.
1 participant
Summary
Closes #961. Adds SMTP-based email delivery for notifications.
What's included
Configuration
Follow-up needed
jobs_notifications_serviceto send emails alongside in-app notificationsTest plan
is_email_delivery_configured()returns Truesend_notification_email()→ email received with HTML template🤖 Generated with Claude Code