From 910786104e206d8bf732e1c4f1fd29f9e9834e7b Mon Sep 17 00:00:00 2001
From: Isaac Adjei <offices.isaac@gmail.com>
Date: Mon, 15 Jun 2026 13:33:35 +0100
Subject: [PATCH] fix: resolve CodeQL high-severity security findings in
 backend services

- webhook_service: replace substring URL checks with urlparse netloc
  comparison so platform detection cannot be bypassed by embedding
  target domains in the path or query string of a malicious URL
- sms_service: remove phone number from warning log to prevent
  clear-text PII exposure in log streams
---
 backend/app/services/sms_service.py     |  3 ++-
 backend/app/services/webhook_service.py | 21 +++++++++++++++++++--
 logs/2026-06-15.md                      | 18 ++++++++++++++++++
 3 files changed, 39 insertions(+), 3 deletions(-)

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.