fix(slack): reject URLs that are not Incoming Webhooks

bot/slack.py

@@ -10,6 +10,11 @@
 from bot.config import logger, settings
 
 _BACKOFF_SECONDS = (0.5, 1.0, 2.0)
+
+# Only this prefix is an Incoming Webhook endpoint. A Slack client URL
+# (https://app.slack.com/client/...) answers a POST with the web app and HTTP
+# 200, which would otherwise read as a delivered message.
+_WEBHOOK_PREFIX = "https://hooks.slack.com/services/"
 _MAX_TITLE_LEN = 200
 _MAX_PREVIEW_LEN = 600
 
@@ -93,10 +98,24 @@ def __init__(self, webhook_url: str | None = None, timeout: float | None = None)
         self._webhook_url = (webhook_url if webhook_url is not None else settings.slack_webhook_url).strip()
         self._timeout = timeout if timeout is not None else float(settings.slack_http_timeout)
         self._client: httpx.AsyncClient | None = None
+        # Validated once, at startup, so a misconfigured URL is one loud error
+        # in the logs instead of one per notification.
+        self._enabled = self._validate_url()
+
+    def _validate_url(self) -> bool:
+        if not self._webhook_url:
+            return False
+        if not self._webhook_url.startswith(_WEBHOOK_PREFIX):
+            logger.error(
+                "SLACK_WEBHOOK_URL is not an Incoming Webhook (must start with %s); notifications are disabled",
+                _WEBHOOK_PREFIX,
+            )
+            return False
+        return True
 
     @property
     def enabled(self) -> bool:
-        return bool(self._webhook_url)
+        return self._enabled
 
     def _get_client(self) -> httpx.AsyncClient:
         if self._client is None or self._client.is_closed:
@@ -124,7 +143,20 @@ async def post(self, payload: dict) -> bool:
                 last_error = type(e).__name__
             else:
                 if resp.status_code < 400:
-                    return True
+                    # An Incoming Webhook answers "ok". Anything else behind a
+                    # 2xx means the URL is not a webhook, so the message was
+                    # never delivered however healthy the status looks.
+                    body = resp.text.strip()
+                    if body.lower() == "ok":
+                        return True
+                    logger.error(
+                        "Slack returned HTTP %d but not an Incoming Webhook response (body starts with %r); "
+                        "check that SLACK_WEBHOOK_URL is a %s… URL",
+                        resp.status_code,
+                        body[:40],
+                        _WEBHOOK_PREFIX,
+                    )
+                    return False
                 # 4xx means a bad payload or a revoked webhook: retrying cannot help.
                 if resp.status_code < 500 and resp.status_code != 429:
                     logger.error("Slack webhook rejected the message (HTTP %d)", resp.status_code)

tests/test_slack.py

@@ -103,7 +103,7 @@ async def handler(request: httpx.Request) -> httpx.Response:
         sent.append(__import__("json").loads(request.content))
         return httpx.Response(200, text="ok")
 
-    notifier = SlackNotifier(webhook_url="https://hooks.slack.test/services/T/B/X")
+    notifier = SlackNotifier(webhook_url="https://hooks.slack.com/services/T/B/X")
     notifier._client = httpx.AsyncClient(transport=httpx.MockTransport(handler))
 
     assert await notifier.notify_support_thread(EVENT) is True
@@ -120,7 +120,7 @@ async def handler(request: httpx.Request) -> httpx.Response:
         calls += 1
         return httpx.Response(404, text="no_service")
 
-    notifier = SlackNotifier(webhook_url="https://hooks.slack.test/services/T/B/X")
+    notifier = SlackNotifier(webhook_url="https://hooks.slack.com/services/T/B/X")
     notifier._client = httpx.AsyncClient(transport=httpx.MockTransport(handler))
 
     assert await notifier.notify_support_thread(EVENT) is False
@@ -139,7 +139,7 @@ async def handler(request: httpx.Request) -> httpx.Response:
             return httpx.Response(503)
         return httpx.Response(200, text="ok")
 
-    notifier = SlackNotifier(webhook_url="https://hooks.slack.test/services/T/B/X")
+    notifier = SlackNotifier(webhook_url="https://hooks.slack.com/services/T/B/X")
     notifier._client = httpx.AsyncClient(transport=httpx.MockTransport(handler))
 
     assert await notifier.notify_support_thread(EVENT) is True
@@ -156,7 +156,7 @@ async def handler(request: httpx.Request) -> httpx.Response:
         calls += 1
         raise httpx.ConnectError("boom", request=request)
 
-    notifier = SlackNotifier(webhook_url="https://hooks.slack.test/services/T/B/X")
+    notifier = SlackNotifier(webhook_url="https://hooks.slack.com/services/T/B/X")
     notifier._client = httpx.AsyncClient(transport=httpx.MockTransport(handler))
 
     assert await notifier.notify_support_thread(EVENT) is False
@@ -186,3 +186,45 @@ def test_httpx_request_logging_cannot_leak_the_webhook_secret(caplog):
     import bot.config  # noqa: F401  (importing configures logging)
 
     assert logging.getLogger("httpx").level >= logging.WARNING
+
+
+# --- the URL must actually be an Incoming Webhook -------------------------
+
+
+@pytest.mark.parametrize(
+    "url",
+    [
+        "https://app.slack.com/client/T047B34QHKN/D09KD6JK0JE",  # a DM link, not a webhook
+        "https://slack.com/services/T/B/X",
+        "http://hooks.slack.com/services/T/B/X",  # no TLS
+        "https://hooks.slack.example/services/T/B/X",
+    ],
+)
+async def test_notifier_refuses_urls_that_are_not_incoming_webhooks(url):
+    notifier = SlackNotifier(webhook_url=url)
+    assert notifier.enabled is False
+    assert await notifier.notify_support_thread(EVENT) is False
+
+
+async def test_a_slack_client_url_answering_200_with_html_is_not_a_delivery():
+    """The Slack web app answers any POST with HTTP 200 and a page of HTML."""
+
+    async def handler(request: httpx.Request) -> httpx.Response:
+        return httpx.Response(200, text="<!DOCTYPE html><html><head><title>Slack</title></head></html>")
+
+    notifier = SlackNotifier(webhook_url="https://hooks.slack.com/services/T/B/X")
+    notifier._client = httpx.AsyncClient(transport=httpx.MockTransport(handler))
+
+    assert await notifier.notify_support_thread(EVENT) is False
+    await notifier.close()
+
+
+async def test_only_an_ok_body_counts_as_delivered():
+    async def handler(request: httpx.Request) -> httpx.Response:
+        return httpx.Response(200, text="OK\n")
+
+    notifier = SlackNotifier(webhook_url="https://hooks.slack.com/services/T/B/X")
+    notifier._client = httpx.AsyncClient(transport=httpx.MockTransport(handler))
+
+    assert await notifier.notify_support_thread(EVENT) is True
+    await notifier.close()