Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion backend/app/services/sms_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
def send_sms(to_number: str, message: str) -> None:
"""Send an SMS to to_number. Silently skips if Brevo is not configured."""
if settings.brevo_api_key in _UNSET:
_log.warning("Brevo API key not configured - SMS not sent to %s", to_number)
# I omit the recipient from this log because phone numbers are PII.
_log.warning("Brevo API key not configured - SMS not sent")
return
if not to_number:
return
Expand Down
21 changes: 19 additions & 2 deletions backend/app/services/webhook_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"""

import logging
from urllib.parse import urlparse

import httpx
from sqlalchemy.orm import Session
Expand All @@ -32,12 +33,28 @@ def _build_text(webhook: Webhook, context: dict) -> str:
return tpl.format_map(context)


def _host(url: str) -> str:
# I extract only the netloc so downstream checks cannot be fooled by the
# target domain appearing in the path or query string of a malicious URL.
try:
return urlparse(url).netloc.lower()
except Exception:
return ""


def _is_discord(url: str) -> bool:
return "discord.com/api/webhooks" in url
host = _host(url)
return host == "discord.com" or host.endswith(".discord.com")


def _is_teams(url: str) -> bool:
return "webhook.office.com" in url or "teams.microsoft.com" in url
host = _host(url)
return (
host == "webhook.office.com"
or host.endswith(".webhook.office.com")
or host == "teams.microsoft.com"
or host.endswith(".teams.microsoft.com")
)


def _build_payload(webhook: Webhook, text: str, severity: str) -> dict:
Expand Down
18 changes: 18 additions & 0 deletions logs/2026-06-15.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,21 @@
## Why

GitHub Sponsors is now active. GitHub displays the Sponsor button entries in the order they appear in FUNDING.yml, so GitHub Sponsors needs to be listed first to show prominently.

---

## [DONE] fix/codeql-security branch - backend security fixes

### backend/app/services/webhook_service.py

**What changed:** Added `from urllib.parse import urlparse` import. Replaced `_is_discord` and `_is_teams` with a `_host()` helper that extracts `urlparse(url).netloc.lower()` first, then checks the exact hostname.

**Why:** CodeQL flagged both functions (findings #2 and #3, both at line 40) for "Incomplete URL substring sanitization." The old code used `"discord.com/api/webhooks" in url` and `"webhook.office.com" in url` - a substring check against the full URL string. An attacker could register `evil.com/discord.com/api/webhooks` or embed the target domain in the path/query of a redirect, bypass the check, and get the service to post the alert payload to an arbitrary URL. Parsing via `urlparse` isolates the netloc (hostname + port) so only genuine Discord or Teams endpoints match.

**Design tradeoff:** I used `endswith(".discord.com")` in addition to an exact match to allow subdomains (Discord uses `discord.com` directly but future subdomain routing should not break). Same for Teams - `webhook.office.com` and `teams.microsoft.com` with subdomain variants. If `urlparse` raises (malformed URL), `_host` returns `""` which matches nothing - safe default.

### backend/app/services/sms_service.py

**What changed:** Removed `to_number` from the warning log at line 19. Old: `"SMS not sent to %s", to_number`. New: `"SMS not sent"` with no interpolated value. Added inline comment explaining why.

**Why:** CodeQL finding #1 flagged "Clear-text logging of sensitive information" at sms_service.py:19. Phone numbers are PII. Logging them in warning output risks them appearing in aggregated log streams, monitoring dashboards, or crash reports where they do not belong. The warning message still conveys all actionable information - the operator knows Brevo is not configured - without leaking recipient data.
Loading