diff --git a/bot/slack.py b/bot/slack.py index 75deabd..ce104d9 100644 --- a/bot/slack.py +++ b/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) diff --git a/tests/test_slack.py b/tests/test_slack.py index cf84720..7c0e961 100644 --- a/tests/test_slack.py +++ b/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="Slack") + + 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()