diff --git a/backend/app/services/sms_service.py b/backend/app/services/sms_service.py index cf46a44..fd2983e 100644 --- a/backend/app/services/sms_service.py +++ b/backend/app/services/sms_service.py @@ -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 diff --git a/backend/app/services/webhook_service.py b/backend/app/services/webhook_service.py index d23c42e..2a0b254 100644 --- a/backend/app/services/webhook_service.py +++ b/backend/app/services/webhook_service.py @@ -10,6 +10,7 @@ """ import logging +from urllib.parse import urlparse import httpx from sqlalchemy.orm import Session @@ -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: diff --git a/logs/2026-06-15.md b/logs/2026-06-15.md index 5a9f787..3a399c8 100644 --- a/logs/2026-06-15.md +++ b/logs/2026-06-15.md @@ -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.