From c5aeb6521f591ccf4e142e588bcef7f9e6a945d4 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 19 Aug 2026 16:57:32 -0400 Subject: [PATCH 1/4] 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/4] 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 = [ From f8a8ac9e23ba15f3190b892b498bb5f5f40cbeed Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Mon, 24 Aug 2026 20:58:17 -0400 Subject: [PATCH 3/4] Align Slack interaction webhook routing/docs --- docs/hackbot/api.md | 44 ++++++++++++-- docs/hackbot/deployment.md | 9 ++- docs/hackbot/security.md | 24 +++++++- services/hackbot-api/app/auth.py | 13 ++-- services/hackbot-api/app/config.py | 4 +- services/hackbot-api/app/routers/slack.py | 73 ++++++----------------- services/hackbot-api/app/slack_webhook.py | 41 ++++++------- 7 files changed, 111 insertions(+), 97 deletions(-) diff --git a/docs/hackbot/api.md b/docs/hackbot/api.md index ee10a49698..8dd17c209a 100644 --- a/docs/hackbot/api.md +++ b/docs/hackbot/api.md @@ -27,10 +27,40 @@ the download to that run's prefix and prevents probing unrelated objects. ### Inbound webhooks — HMAC signature -| POST | `/webhooks/phabricator` | `@hackbot` mention on a revision triggers a bug-fix run | - -Authenticated by Phabricator's own HMAC signature over the raw body, so it sits on its own -router without the API-key dependency. See [triggers.md](triggers.md). +| Method | Path | Does | +| ------ | ------------------------------ | ------------------------------------------------------- | +| POST | `/webhooks/phabricator` | `@hackbot` mention on a revision triggers a bug-fix run | +| POST | `/webhooks/slack/interactions` | A click on an interactive element of a hackbot message | + +Each is authenticated by the sender's own HMAC signature over the raw body, so each sits on +its own router without the API-key dependency: neither sender can send an `X-API-Key`. The +Phabricator receiver is covered in [triggers.md](triggers.md). + +**Slack interactions** all arrive on this one route: Slack posts every click on every +interactive element to the single Request URL configured for Interactivity, so the receiver +demultiplexes on the element's `action_id`. The path names the feature because Slack +configures one Request URL **per feature** — Event Subscriptions and Slash Commands are +separate URLs with their own payload shapes, and they would get their own routes beside this +one rather than sharing it. Three things about the delivery shape the route: + +- The body is **not JSON**. It is `application/x-www-form-urlencoded` with the JSON in a + single `payload` field, parsed from the same raw bytes the signature covers. +- **Slack expects a response within 3 seconds** and shows the person who clicked an error if + it does not arrive, so real work belongs off the request: publish an event and answer the + message afterwards through the delivery's `response_url` or `chat.update`. +- **A delivery this cannot act on is answered `200` and logged**, not `4xx`/`5xx`. Slack + retries a non-2xx, and a payload that cannot be parsed will not parse on retry either, + so refusing it only shows a failure nobody can fix. + +Nothing posts an interactive element yet, so nothing reaches this route in practice: it +authenticates a delivery, parses it, and records that it happened. Acting on a click, and +the authorization that has to come first, lands with the first button. A click that starts +a run will then appear in [triggers.md](triggers.md). + +Turning it on is Slack-app config, not a deploy: **Interactivity & Shortcuts → Request URL** += `https:///webhooks/slack/interactions`, and `SLACK_SIGNING_SECRET` from **Basic +Information → App Credentials**. Interactivity needs no new OAuth scopes, so no workspace +reinstall. Until the secret is set the endpoint rejects every delivery with a `401`. ## Creating a run @@ -122,8 +152,10 @@ idempotent — see [actions.md](actions.md). Commands and the full config reference are in [deployment.md](deployment.md). Two things specific to this service: -- **`WEBHOOK_SECRET` has no default**, so a missing one fails at startup rather than - silently accepting or rejecting deliveries. +- **`WEBHOOK_SECRET` and `SLACK_SIGNING_SECRET` have no defaults**, so a missing one fails + at startup rather than silently accepting or rejecting deliveries. Both are HMAC keys for + an inbound receiver: an empty one would mean either accepting every delivery + unauthenticated or rejecting every real one, and neither is a state worth booting into. - **Signing GCS URLs needs an impersonating credential** — `gcloud auth application-default login --impersonate-service-account=`. See [security.md](security.md) for why, and what the deployed service needs instead. diff --git a/docs/hackbot/deployment.md b/docs/hackbot/deployment.md index 7fc2f5d02c..7a6094f995 100644 --- a/docs/hackbot/deployment.md +++ b/docs/hackbot/deployment.md @@ -64,7 +64,14 @@ fastest: Two things those files do not tell you: - **Nested models bind from prefixed vars**, splitting on the first underscore only: - `PHABRICATOR_API_KEY` → `phabricator.api_key`, `WEBHOOK_SECRET` → `webhook.secret`. + `PHABRICATOR_API_KEY` → `phabricator.api_key`, `WEBHOOK_SECRET` → `webhook.secret`, + `SLACK_SIGNING_SECRET` → `slack.signing_secret`. +- **A prefix does not make a var a setting.** `SLACK_BOT_TOKEN` looks like it belongs to the + same nested model and does not: it is read straight from the environment by the apply-side + Slack handler in `hackbot-runtime`, which has no access to these settings. The two live + side by side on hackbot-api and serve opposite directions — the token posts messages, the + signing secret verifies clicks coming back — so a deployment that posts fine can still + reject every interaction, and vice versa. - **An agent container's env arrives from three places.** Per-execution overrides from the API (`RUN_ID`, the results bucket/prefix/policy, one var per input-schema field); static Job env fixed at deploy time (`BROKER_URL`, `SOURCE_REPO`, the Anthropic federation ids, diff --git a/docs/hackbot/security.md b/docs/hackbot/security.md index 5d72028d27..38eb6f4f63 100644 --- a/docs/hackbot/security.md +++ b/docs/hackbot/security.md @@ -70,14 +70,23 @@ delegating `sign_bytes` to the IAM `signBlob` API. This is why its service accou ## Authenticating callers of hackbot-api -Three distinct schemes, one per class of caller: +Four distinct schemes, one per class of caller: | Caller | Scheme | | --------------------------- | -------------------------------------------------------------------------------------- | | UI, pulse listener, scripts | `X-API-Key`, compared in constant time | | Phabricator | HMAC-SHA256 over the raw body, constant-time compared | +| Slack | HMAC-SHA256 over `v0:{timestamp}:{raw body}`, plus a 5-minute timestamp window | | Eventarc / Pub/Sub push | Google-signed OIDC bearer token, verified for audience **and** issuing service account | +The two HMAC schemes are not interchangeable. Slack signs a base string that **includes the +delivery's timestamp**, and that timestamp is checked against the clock, so a captured +delivery cannot be replayed later: refreshing the timestamp invalidates the signature, and +keeping it puts the delivery outside the window. The Phabricator signature covers the body +alone and has no such window, which is why the Slack receiver has its own verifier rather +than reusing the existing one. Both fail closed on an unconfigured secret, so a deployment +missing the key rejects every delivery instead of accepting them all. + The push-token check is not redundant with platform IAM. The service allows unauthenticated invocations — that is how API-key callers reach it at all — so IAM on the subscription does not protect these routes on its own. The token is verified in the route. @@ -95,6 +104,19 @@ A webhook signature proves the delivery came from Phabricator; it says nothing a commented. So the comment author must additionally be a member of the `bmo-editbugs-team` project. [triggers.md](triggers.md) covers that check and the other guards on the path. +## Authorizing Slack clicks + +The same split applies, and the second half is not built yet. A valid signature proves the +delivery came from the Slack app; it says nothing about _who_ clicked, and a Slack user id +is not an identity this platform trusts. Resolving one to a `@mozilla.com` address (through +`users.info`, which needs the `users:read` and `users:read.email` scopes) and checking the +click's workspace is what a click would need before it could cause anything. + +Until then the receiver is deliberately inert: it verifies, parses and logs, and no click +path reaches an effect. That ordering is the point — the authorization lands in the same +change as the first button, so no interactive element ever exists ahead of the check that +guards it. See [api.md](api.md). + ## Recorded actions as a review gate The record-then-apply split ([actions.md](actions.md)) is a security property as much as a diff --git a/services/hackbot-api/app/auth.py b/services/hackbot-api/app/auth.py index 1f23ea9427..07eb7c510c 100644 --- a/services/hackbot-api/app/auth.py +++ b/services/hackbot-api/app/auth.py @@ -52,15 +52,10 @@ def verify_slack_signature( ) -> 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. + Delegates to `slack_sdk`'s verifier, which compares the HMAC and also rejects a + timestamp more than five minutes off. Returns False on an unconfigured secret or + a missing or garbled header, so it fails closed. `docs/hackbot/security.md` has + the scheme and why it is not the Phabricator one. """ secret = settings.slack.signing_secret if not secret or not timestamp or not signature: diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index 95cf5df723..4feec89b56 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -28,8 +28,8 @@ class SlackSettings(BaseModel): 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. + # Slack's app-level signing secret, verifying the HMAC on every interaction + # delivery. Required (no default), for the reason WEBHOOK_SECRET is. signing_secret: str diff --git a/services/hackbot-api/app/routers/slack.py b/services/hackbot-api/app/routers/slack.py index d05f05909b..5e9773e8b9 100644 --- a/services/hackbot-api/app/routers/slack.py +++ b/services/hackbot-api/app/routers/slack.py @@ -1,38 +1,24 @@ """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. +``docs/hackbot/api.md`` covers the endpoint, the delivery shape and the Slack-app +config; ``docs/hackbot/security.md`` covers the signature and what it does not +prove. """ 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") +# Under `/webhooks` with the Phabricator receiver, since both are signature-verified +# inbound deliveries, and one level deeper because Slack configures a Request URL per +# feature: Event Subscriptions and Slash Commands are separate URLs with their own +# payload shapes, and they belong beside this one rather than sharing it. +router = APIRouter(prefix="/webhooks/slack") @router.post( @@ -48,17 +34,6 @@ async def slack_interactions(request: Request) -> Response: # 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, @@ -71,28 +46,16 @@ async def slack_interactions(request: Request) -> Response: # 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). + # The click is authenticated and parsed; nothing acts on it. What is still + # missing, in the order it has to happen (the reasoning is in + # `docs/hackbot/api.md` and `docs/hackbot/security.md`): # - # 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`). + # 1. Dispatch on `click.kind` against the kinds that have a receiver, and + # answer 200 for one that does not. + # 2. Authorize the clicker from `click.user_id` and `click.team_id`. + # 3. Make the effect at-most-once, keyed on something stable such as + # (`click.message_ts`, `click.kind`). + # 4. Publish the click and act on it off this request (see `app/pubsub.py`). + # 5. Answer through `click.response_url`, then `chat.update` the message. 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 index f633bf9bef..9faba17d36 100644 --- a/services/hackbot-api/app/slack_webhook.py +++ b/services/hackbot-api/app/slack_webhook.py @@ -1,18 +1,12 @@ """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. +Turns a delivery into either a :class:`ButtonClick` or None, leaving the route in +``app/routers/slack.py`` to decide what to do with it. + +Takes raw bytes, because the body is form-encoded rather than JSON and those bytes +are already needed for signature verification. Returns None rather than raising on +anything it cannot understand, because the route answers such a delivery 200. Both +are explained in ``docs/hackbot/api.md``. """ from __future__ import annotations @@ -31,10 +25,12 @@ # 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. +# The `v` a button's `value` must carry: an envelope around the button's args, +# `{"v": 1, "args": {...}}`, so 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. Buttons outlive deploys, since a message stays clickable for as +# long as it is in the channel's history. Whatever draws the first button writes +# this envelope. SUPPORTED_VALUE_VERSION = 1 @@ -42,12 +38,11 @@ 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`` is the button's Slack ``action_id``, the name the side that drew the + button gave it, 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 From 25f4347d6c18c96e2b1931704aa6bd12d7868be7 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Mon, 24 Aug 2026 21:58:00 -0400 Subject: [PATCH 4/4] Enforce non-blank Slack signing secret at startup --- docs/hackbot/api.md | 23 +++++++++++++++--- docs/hackbot/security.md | 5 ++-- services/hackbot-api/app/auth.py | 38 +++++++----------------------- services/hackbot-api/app/config.py | 13 +++++++--- 4 files changed, 42 insertions(+), 37 deletions(-) diff --git a/docs/hackbot/api.md b/docs/hackbot/api.md index 8dd17c209a..a957e7b52d 100644 --- a/docs/hackbot/api.md +++ b/docs/hackbot/api.md @@ -57,10 +57,24 @@ authenticates a delivery, parses it, and records that it happened. Acting on a c the authorization that has to come first, lands with the first button. A click that starts a run will then appear in [triggers.md](triggers.md). +What the endpoint answers: + +| Delivery | Status | +| -------------------------------------------------- | ------ | +| Signature verifies | `200` | +| A signature header is absent (a malformed request) | `422` | +| Both headers present, signature or freshness fails | `401` | +| Signed, but the payload cannot be understood | `200` | + +The two signature headers are declared required, so an absent one is a validation failure +rather than an authentication one. Slack always sends both, so a delivery missing them is +not a Slack delivery. + Turning it on is Slack-app config, not a deploy: **Interactivity & Shortcuts → Request URL** -= `https:///webhooks/slack/interactions`, and `SLACK_SIGNING_SECRET` from **Basic -Information → App Credentials**. Interactivity needs no new OAuth scopes, so no workspace -reinstall. Until the secret is set the endpoint rejects every delivery with a `401`. += `https:///webhooks/slack/interactions`, and `SLACK_SIGNING_SECRET` from +**Basic Information → App Credentials**. Interactivity needs no new OAuth scopes, so no +workspace reinstall. The secret is not optional: without a usable one the service does not +start at all (see below). ## Creating a run @@ -156,6 +170,9 @@ specific to this service: at startup rather than silently accepting or rejecting deliveries. Both are HMAC keys for an inbound receiver: an empty one would mean either accepting every delivery unauthenticated or rejecting every real one, and neither is a state worth booting into. + `SLACK_SIGNING_SECRET` is validated **non-blank** rather than merely present, so `=""` + fails at startup too, and every consumer downstream can take a usable key for granted + instead of carrying an unconfigured case. - **Signing GCS URLs needs an impersonating credential** — `gcloud auth application-default login --impersonate-service-account=`. See [security.md](security.md) for why, and what the deployed service needs instead. diff --git a/docs/hackbot/security.md b/docs/hackbot/security.md index 38eb6f4f63..b6552a5084 100644 --- a/docs/hackbot/security.md +++ b/docs/hackbot/security.md @@ -84,8 +84,9 @@ delivery's timestamp**, and that timestamp is checked against the clock, so a ca delivery cannot be replayed later: refreshing the timestamp invalidates the signature, and keeping it puts the delivery outside the window. The Phabricator signature covers the body alone and has no such window, which is why the Slack receiver has its own verifier rather -than reusing the existing one. Both fail closed on an unconfigured secret, so a deployment -missing the key rejects every delivery instead of accepting them all. +than reusing the existing one. Neither can run without its key: the Phabricator secret is +required, and the Slack one is required **and** validated non-blank, so a deployment without +a usable key fails to start rather than quietly rejecting every delivery it receives. The push-token check is not redundant with platform IAM. The service allows unauthenticated invocations — that is how API-key callers reach it at all — so IAM on the subscription does diff --git a/services/hackbot-api/app/auth.py b/services/hackbot-api/app/auth.py index 07eb7c510c..d85bc4d371 100644 --- a/services/hackbot-api/app/auth.py +++ b/services/hackbot-api/app/auth.py @@ -47,39 +47,19 @@ 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. - - Delegates to `slack_sdk`'s verifier, which compares the HMAC and also rejects a - timestamp more than five minutes off. Returns False on an unconfigured secret or - a missing or garbled header, so it fails closed. `docs/hackbot/security.md` has - the scheme and why it is not the Phabricator one. - """ - 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), + x_slack_request_timestamp: str = Header(), + x_slack_signature: str = Header(), ) -> 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. - """ + verifier = SignatureVerifier(settings.slack.signing_secret) raw = await request.body() - if not verify_slack_signature(raw, x_slack_request_timestamp, x_slack_signature): + try: + valid = verifier.is_valid(raw, x_slack_request_timestamp, x_slack_signature) + except ValueError: + valid = False + + if not valid: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid or missing Slack signature", diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index 4feec89b56..3930a9bd8d 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -1,7 +1,14 @@ +from typing import Annotated + from phabricator_client import PhabricatorSettings -from pydantic import BaseModel +from pydantic import BaseModel, StringConstraints from pydantic_settings import BaseSettings +# An HMAC key that must actually be a key. Required alone only rejects a *missing* +# value, and an empty or whitespace one would start a service that rejects every +# delivery it receives -- worse than not starting, because nothing says so. +HmacSecret = Annotated[str, StringConstraints(strip_whitespace=True, min_length=1)] + class WebhookSettings(BaseModel): """Inbound webhook receiver config (Phabricator "@hackbot" mentions). @@ -29,8 +36,8 @@ class SlackSettings(BaseModel): """ # Slack's app-level signing secret, verifying the HMAC on every interaction - # delivery. Required (no default), for the reason WEBHOOK_SECRET is. - signing_secret: str + # delivery. + signing_secret: HmacSecret class Settings(BaseSettings):