From c5aeb6521f591ccf4e142e588bcef7f9e6a945d4 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 19 Aug 2026 16:57:32 -0400 Subject: [PATCH 1/2] Add Slack interactivity webhook support Add a dedicated Slack interactions endpoint with request signature verification. --- services/hackbot-api/app/auth.py | 45 ++++++ services/hackbot-api/app/config.py | 13 ++ services/hackbot-api/app/main.py | 8 +- services/hackbot-api/app/routers/__init__.py | 3 +- services/hackbot-api/app/routers/slack.py | 98 ++++++++++++ services/hackbot-api/app/slack_webhook.py | 150 +++++++++++++++++++ services/hackbot-api/pyproject.toml | 1 + services/hackbot-api/tests/conftest.py | 9 +- 8 files changed, 321 insertions(+), 6 deletions(-) create mode 100644 services/hackbot-api/app/routers/slack.py create mode 100644 services/hackbot-api/app/slack_webhook.py diff --git a/services/hackbot-api/app/auth.py b/services/hackbot-api/app/auth.py index 597acd3d7c..1f23ea9427 100644 --- a/services/hackbot-api/app/auth.py +++ b/services/hackbot-api/app/auth.py @@ -5,6 +5,7 @@ from fastapi import Header, HTTPException, Request, status from google.auth.transport import requests as google_requests from google.oauth2 import id_token +from slack_sdk.signature import SignatureVerifier from app.config import settings @@ -46,6 +47,50 @@ async def require_phabricator_signature( ) +def verify_slack_signature( + raw_body: bytes, timestamp: str | None, signature: str | None +) -> bool: + """Constant-time-check Slack's `X-Slack-Signature` over the raw request body. + + Slack signs `v0:{timestamp}:{body}` with the app's signing secret and sends the + digest as `v0=` in the header. `slack_sdk`'s verifier does that comparison + and additionally rejects a timestamp more than five minutes from now, which is + what stops a captured delivery from being replayed later. The Phabricator + signature has no such window, so this cannot simply reuse it. + + Returns False if the secret is unconfigured or either header is missing or + garbled, so a service without `SLACK_SIGNING_SECRET` rejects every delivery + instead of accepting them all. + """ + secret = settings.slack.signing_secret + if not secret or not timestamp or not signature: + return False + try: + return SignatureVerifier(secret).is_valid(raw_body, timestamp, signature) + except ValueError: + # A non-numeric timestamp header reaches an `int()` inside the verifier. + return False + + +async def require_slack_signature( + request: Request, + x_slack_request_timestamp: str | None = Header(default=None), + x_slack_signature: str | None = Header(default=None), +) -> None: + """Reject the request unless Slack's delivery signature is valid. + + Same shape as `require_phabricator_signature`: the raw body is read here (and + cached by Starlette, so the route can read it again) because the signature + covers the bytes as sent, which a parse-and-reserialise would not reproduce. + """ + raw = await request.body() + if not verify_slack_signature(raw, x_slack_request_timestamp, x_slack_signature): + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid or missing Slack signature", + ) + + async def require_api_key(x_api_key: str | None = Header(default=None)) -> None: if not settings.external_api_key: raise HTTPException( diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index e812f42ad1..95cf5df723 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -22,6 +22,17 @@ class WebhookSettings(BaseModel): dedupe_ttl_seconds: int = 6 * 60 * 60 +class SlackSettings(BaseModel): + """Inbound Slack interactivity config (clicks on the app's own messages). + + Populated from SLACK_* env vars as part of the single settings parse. + """ + + # Slack's app-level signing secret (Basic Information -> App Credentials), + # used to verify the HMAC on every interaction delivery. + signing_secret: str + + class Settings(BaseSettings): # GCP gcp_project: str = "" @@ -50,6 +61,8 @@ class Settings(BaseSettings): # Required via its `secret` field, so WEBHOOK_SECRET must be set at startup. webhook: WebhookSettings + slack: SlackSettings + # The webhook receiver triggers runs over the public API (rather than calling # the DB/jobs internals directly), so splitting it into its own service later # is just a matter of repointing this at the remote API. While co-located, diff --git a/services/hackbot-api/app/main.py b/services/hackbot-api/app/main.py index 080b7a076d..99164cc7a0 100644 --- a/services/hackbot-api/app/main.py +++ b/services/hackbot-api/app/main.py @@ -7,7 +7,12 @@ from app import __version__ from app.config import settings from app.database.connection import close_db, init_db -from app.routers import events_router, runs_router, webhooks_router +from app.routers import ( + events_router, + runs_router, + slack_router, + webhooks_router, +) if settings.sentry_dsn: sentry_sdk.init( @@ -42,6 +47,7 @@ async def lifespan(app: FastAPI): app.include_router(runs_router) app.include_router(events_router) app.include_router(webhooks_router) +app.include_router(slack_router) @app.get("/health") diff --git a/services/hackbot-api/app/routers/__init__.py b/services/hackbot-api/app/routers/__init__.py index f7655c217f..5cfd97bb6d 100644 --- a/services/hackbot-api/app/routers/__init__.py +++ b/services/hackbot-api/app/routers/__init__.py @@ -1,5 +1,6 @@ from app.routers.events import router as events_router from app.routers.runs import router as runs_router +from app.routers.slack import router as slack_router from app.routers.webhooks import router as webhooks_router -__all__ = ["events_router", "runs_router", "webhooks_router"] +__all__ = ["events_router", "runs_router", "slack_router", "webhooks_router"] diff --git a/services/hackbot-api/app/routers/slack.py b/services/hackbot-api/app/routers/slack.py new file mode 100644 index 0000000000..d05f05909b --- /dev/null +++ b/services/hackbot-api/app/routers/slack.py @@ -0,0 +1,98 @@ +"""Inbound Slack interactivity receiver: clicks on the messages hackbot posts. + +A message recorded with buttons (``hackbot_runtime.actions.slack.button``) is +posted as Block Kit by the apply step; when someone clicks one, Slack POSTs the +click here. Authenticated by Slack's HMAC signature rather than the ``X-API-Key`` +the other routes use, so this lives on its own router without ``require_api_key``, +the same way the Phabricator receiver does. + +Slack app setup (one URL for the whole app, no new OAuth scopes, no reinstall): + +- Interactivity & Shortcuts -> Interactivity: on +- Request URL: ``https:///slack/interactions`` +- ``SLACK_SIGNING_SECRET`` in this service's env, from Basic Information -> + App Credentials. Until it is set every delivery is rejected with a 401. + +Two constraints shape what may go in this route. Slack expects a response within +**3 seconds** and shows the clicker an error if it does not arrive, so real work +belongs off this request (publish an event, as ``run.completed`` does, and answer +the message afterwards through ``response_url`` or ``chat.update``). And Slack +retries a non-2xx delivery, so a payload this cannot act on is answered 200 and +logged, not 4xx/5xx: a retry of it would fail identically while the person who +clicked watches it fail. +""" + +import logging + +from fastapi import APIRouter, Depends, Request, Response, status +from hackbot_runtime.actions.slack import BUTTON_KINDS + +from app.auth import require_slack_signature +from app.slack_webhook import parse_interaction + +log = logging.getLogger(__name__) + +router = APIRouter(prefix="/slack") + + +@router.post( + "/interactions", + status_code=status.HTTP_200_OK, + dependencies=[Depends(require_slack_signature)], +) +async def slack_interactions(request: Request) -> Response: + # Already read (and cached) by the signature dependency: the signature covers + # the bytes as sent, and the form body is parsed from those same bytes. + click = parse_interaction(await request.body()) + if click is None: + # Not a click this can act on. Already logged with the reason. + return Response(status_code=status.HTTP_200_OK) + + if click.kind not in BUTTON_KINDS: + # A button whose kind no longer has a receiver: an older message still in + # a channel's history, or a kind retired without retiring its buttons. + log.warning( + "Slack: no receiver for button kind %r (from user %s in channel %s)", + click.kind, + click.user_id, + click.channel_id, + ) + return Response(status_code=status.HTTP_200_OK) + + log.info( + "Slack: %s clicked by %s (%s) in channel %s on message %s, args=%s", + click.kind, + click.user_name or "unknown", + click.user_id, + click.channel_id, + click.message_ts, + click.args, + ) + + # ACTION HANDLING GOES HERE + # + # The click is authenticated and parsed at this point; nothing acts on it yet. + # What belongs here, and what it needs from `click`: + # + # 1. Authorize the clicker. `click.user_id` is a Slack id, not an identity this + # service trusts: resolve it to an email with `users.info` (needs the + # `users:read` / `users:read.email` scopes, so a reinstall) and require the + # same @mozilla.com bar the UI applies. Check `click.team_id` is the expected + # workspace too. Fail closed. + # 2. Make it at-most-once. Slack retries deliveries and people double-click, so + # the effect has to be keyed on something stable, e.g. (message_ts, kind), + # in a row that only one caller can transition out of pending. + # 3. Hand the work off rather than doing it here, to stay inside the 3-second + # budget: publish the click and let a push subscription act on it (see + # `app/pubsub.py`), which is how run completions already reach their handler. + # 4. Answer the person who clicked, twice: strip the buttons immediately via + # `click.response_url` so a second click has nothing to hit, then report the + # outcome from the worker with `chat.update` on the message the posted action + # recorded (`{"channel", "ts"}` in its result). + # + # For `trigger_bug_fix` specifically, `click.args` carries `bug_id` and the + # `run_id` of the triage run that proposed the fix, which is everything a + # `POST /agents/bug-fix/runs` needs (attributed to the clicker through + # `X-On-Behalf-Of`). + + return Response(status_code=status.HTTP_200_OK) diff --git a/services/hackbot-api/app/slack_webhook.py b/services/hackbot-api/app/slack_webhook.py new file mode 100644 index 0000000000..f633bf9bef --- /dev/null +++ b/services/hackbot-api/app/slack_webhook.py @@ -0,0 +1,150 @@ +"""Slack interaction payload handling: parsing a verified delivery into a click. + +Slack posts every interaction with the app's messages to one URL, so this turns a +delivery into either a :class:`ButtonClick` or None, and the route in +``app/routers/slack.py`` decides what to do with it. Kept separate from the route +for the same reason as ``app/phabricator_webhook.py``: payload shapes are worth +testing without a client. + +Two things about the delivery are easy to get wrong. It is not JSON: the body is +``application/x-www-form-urlencoded`` with the JSON in a single ``payload`` field, +which is why this takes raw bytes (already needed for signature verification) and +parses them itself rather than reading a model off the request. And a body that +cannot be understood returns None rather than raising: it will not parse on a +retry either, and a non-2xx makes Slack both retry it and show the person who +clicked an error for something they cannot fix. +""" + +from __future__ import annotations + +import json +import logging +from dataclasses import dataclass +from typing import Any +from urllib.parse import parse_qs + +log = logging.getLogger(__name__) + +# Only clicks on message elements are handled here. Slack sends other interaction +# types to the same URL (`view_submission` when a modal is submitted, +# `block_suggestion` for a select's options), which are ignored until something +# records a button that needs them. +BLOCK_ACTIONS = "block_actions" + +# The `v` an encoded button `value` must carry, matching +# `hackbot_runtime.actions.slack.VALUE_VERSION` at the time this was written. A +# click on a button posted before a shape change reports a version this does not +# know, and is dropped rather than read with the wrong meaning. +SUPPORTED_VALUE_VERSION = 1 + + +@dataclass(frozen=True) +class ButtonClick: + """A click on one button of a message this app posted. + + ``kind`` is the button's Slack ``action_id``, which is the kind the recording + side gave it (see ``hackbot_runtime.actions.slack.BUTTON_KINDS``), and ``args`` + is what that side put on the button. Everything else identifies the click: + who, where, on which message, and the two single-use handles Slack provides + for replying (``response_url``, valid ~30 minutes) and for opening a modal + (``trigger_id``, valid ~3 seconds). + """ + + kind: str + args: dict[str, Any] + user_id: str + user_name: str | None + team_id: str | None + channel_id: str | None + message_ts: str | None + response_url: str | None + trigger_id: str | None + + +def _decode_value(raw: str | None) -> dict[str, Any] | None: + """The args off a button's ``value``, or None if it is not one of ours.""" + if not raw: + return None + try: + decoded = json.loads(raw) + except ValueError: + log.warning("Slack interaction: button value is not JSON") + return None + if not isinstance(decoded, dict) or decoded.get("v") != SUPPORTED_VALUE_VERSION: + log.warning( + "Slack interaction: unsupported button value version %r", + (decoded or {}).get("v") if isinstance(decoded, dict) else None, + ) + return None + args = decoded.get("args") + return args if isinstance(args, dict) else {} + + +def parse_payload(payload: dict[str, Any]) -> ButtonClick | None: + """Turn an interaction payload into a :class:`ButtonClick`, or None. + + None covers every payload this cannot act on: another interaction type, a + click carrying no action, or a button whose value did not come from a version + of the recording side this understands. Each is logged, since a button that + silently does nothing is indistinguishable from a broken receiver. + """ + kind_of_payload = payload.get("type") + if kind_of_payload != BLOCK_ACTIONS: + log.info("Ignoring Slack interaction of type %r", kind_of_payload) + return None + + actions = payload.get("actions") or [] + # A click reports exactly one action even in a block of several buttons, so + # anything past the first would be a payload shape this does not know. + action = actions[0] if actions else None + if not isinstance(action, dict) or not action.get("action_id"): + log.warning("Ignoring Slack %s delivery with no action", BLOCK_ACTIONS) + return None + + args = _decode_value(action.get("value")) + if args is None: + return None + + user = payload.get("user") or {} + if not user.get("id"): + # Every real click names its user; without one there is nobody to + # authorize, so this is a payload to drop rather than guess at. + log.warning("Ignoring Slack %s delivery with no user", BLOCK_ACTIONS) + return None + + return ButtonClick( + kind=action["action_id"], + args=args, + user_id=user["id"], + user_name=user.get("username") or user.get("name"), + team_id=(payload.get("team") or {}).get("id"), + channel_id=(payload.get("channel") or {}).get("id"), + message_ts=(payload.get("message") or {}).get("ts"), + response_url=payload.get("response_url"), + trigger_id=payload.get("trigger_id"), + ) + + +def parse_interaction(raw_body: bytes) -> ButtonClick | None: + """Parse a raw interaction delivery: form body, then ``payload`` JSON.""" + try: + form = parse_qs(raw_body.decode("utf-8")) + except UnicodeDecodeError: + log.warning("Slack interaction: body is not UTF-8") + return None + + encoded = form.get("payload") + if not encoded: + log.warning("Slack interaction: body has no payload field") + return None + + try: + payload = json.loads(encoded[0]) + except ValueError: + log.warning("Slack interaction: payload is not JSON") + return None + if not isinstance(payload, dict): + log.warning("Slack interaction: payload is not an object") + return None + + return parse_payload(payload) diff --git a/services/hackbot-api/pyproject.toml b/services/hackbot-api/pyproject.toml index fe79eeee31..bac13acb58 100644 --- a/services/hackbot-api/pyproject.toml +++ b/services/hackbot-api/pyproject.toml @@ -18,6 +18,7 @@ dependencies = [ "google-auth>=2.29.0", "sentry-sdk>=2.51.0", "cachetools>=5.3.0", + "slack-sdk>=3.27.0", "httpx>=0.26.0", "hackbot-runtime", "phabricator-client", diff --git a/services/hackbot-api/tests/conftest.py b/services/hackbot-api/tests/conftest.py index 69ee05da91..e38d0b077c 100644 --- a/services/hackbot-api/tests/conftest.py +++ b/services/hackbot-api/tests/conftest.py @@ -1,9 +1,10 @@ import os # The global Settings embeds required nested models, validated when Settings() is -# built at import: PhabricatorSettings needs a 32-char api_key, and -# WebhookSettings needs a secret. Provide dummies here (before app.config is -# imported) so the suite imports even in tests that don't exercise these. -# `setdefault` leaves any real env value intact. +# built at import: PhabricatorSettings needs a 32-char api_key, WebhookSettings +# needs a secret, and SlackSettings needs a signing secret. Provide dummies here +# (before app.config is imported) so the suite imports even in tests that don't +# exercise these. `setdefault` leaves any real env value intact. os.environ.setdefault("PHABRICATOR_API_KEY", "api-" + "a" * 28) os.environ.setdefault("WEBHOOK_SECRET", "test-webhook-secret") +os.environ.setdefault("SLACK_SIGNING_SECRET", "test-signing-secret") From 8a90b01141501e73580424a4b8fef846fa06ba6f Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 19 Aug 2026 21:57:04 -0400 Subject: [PATCH 2/2] Add slack-sdk to locked dependencies --- uv.lock | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/uv.lock b/uv.lock index 152dbb598c..a7dddcf0b8 100644 --- a/uv.lock +++ b/uv.lock @@ -2719,6 +2719,7 @@ dependencies = [ { name = "pydantic" }, { name = "pydantic-settings" }, { name = "sentry-sdk" }, + { name = "slack-sdk" }, { name = "sqlalchemy", extra = ["asyncio"] }, { name = "uvicorn", extra = ["standard"] }, ] @@ -2750,6 +2751,7 @@ requires-dist = [ { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.0.0" }, { name = "pytest-asyncio", marker = "extra == 'dev'", specifier = ">=0.23.0" }, { name = "sentry-sdk", specifier = ">=2.51.0" }, + { name = "slack-sdk", specifier = ">=3.27.0" }, { name = "sqlalchemy", extras = ["asyncio"], specifier = ">=2.0.25" }, { name = "uvicorn", extras = ["standard"], specifier = ">=0.27.0" }, ] @@ -4656,9 +4658,9 @@ resolution-markers = [ "python_full_version < '3.13' and platform_machine == 'x86_64' and sys_platform == 'darwin'", ] dependencies = [ - { name = "llvmlite", version = "0.36.0", source = { registry = "https://pypi.org/simple" } }, - { name = "numpy" }, - { name = "setuptools" }, + { name = "llvmlite", version = "0.36.0", source = { registry = "https://pypi.org/simple" }, marker = "platform_machine == 'x86_64' and sys_platform == 'darwin'" }, + { name = "numpy", marker = "platform_machine == 'x86_64' and sys_platform == 'darwin'" }, + { name = "setuptools", marker = "platform_machine == 'x86_64' and sys_platform == 'darwin'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/e3/7d/3d61160836e49f40913741c464f119551c15ed371c1d91ea50308495b93b/numba-0.53.1.tar.gz", hash = "sha256:9cd4e5216acdc66c4e9dab2dfd22ddb5bef151185c070d4a3cd8e78638aff5b0", size = 2213956, upload-time = "2021-03-26T09:15:50.402Z" } @@ -4678,8 +4680,8 @@ resolution-markers = [ "(python_full_version < '3.13' and platform_machine != 'x86_64' and sys_platform == 'darwin') or (python_full_version < '3.13' and sys_platform != 'darwin' and sys_platform != 'emscripten' and sys_platform != 'win32')", ] dependencies = [ - { name = "llvmlite", version = "0.48.0", source = { registry = "https://pypi.org/simple" } }, - { name = "numpy" }, + { name = "llvmlite", version = "0.48.0", source = { registry = "https://pypi.org/simple" }, marker = "platform_machine != 'x86_64' or sys_platform != 'darwin'" }, + { name = "numpy", marker = "platform_machine != 'x86_64' or sys_platform != 'darwin'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/ae/a0/570e3dc53e5602b49108f62a13e529f1eec8bfc7ef37d49c825924dcf546/numba-0.66.0.tar.gz", hash = "sha256:b900e63a0e26c05ea9a6d5a3a5a0a177cb64c5011887bf43edb8c3ed2c38d363", size = 2806181, upload-time = "2026-07-01T23:12:46.36Z" } wheels = [ @@ -5109,7 +5111,7 @@ name = "pexpect" version = "4.9.0" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "ptyprocess" }, + { name = "ptyprocess", marker = "sys_platform != 'emscripten' and sys_platform != 'win32'" }, ] sdist = { url = "https://files.pythonhosted.org/packages/42/92/cc564bf6381ff43ce1f4d06852fc19a2f11d180f23dc32d9588bee2f149d/pexpect-4.9.0.tar.gz", hash = "sha256:ee7d41123f3c9911050ea2c2dac107568dc43b2d3b0c7557a33212c398ead30f", size = 166450, upload-time = "2023-11-25T09:07:26.339Z" } wheels = [ @@ -6697,8 +6699,8 @@ name = "secretstorage" version = "3.5.0" source = { registry = "https://pypi.org/simple" } dependencies = [ - { name = "cryptography" }, - { name = "jeepney" }, + { name = "cryptography", marker = "(platform_machine != 'x86_64' and sys_platform == 'darwin') or (sys_platform != 'darwin' and sys_platform != 'emscripten' and sys_platform != 'win32')" }, + { name = "jeepney", marker = "(platform_machine != 'x86_64' and sys_platform == 'darwin') or (sys_platform != 'darwin' and sys_platform != 'emscripten' and sys_platform != 'win32')" }, ] sdist = { url = "https://files.pythonhosted.org/packages/1c/03/e834bcd866f2f8a49a85eaff47340affa3bfa391ee9912a952a1faa68c7b/secretstorage-3.5.0.tar.gz", hash = "sha256:f04b8e4689cbce351744d5537bf6b1329c6fc68f91fa666f60a380edddcd11be", size = 19884, upload-time = "2025-11-23T19:02:53.191Z" } wheels = [