From 86025fd47dd325ae9809f66394f1d914e4489fcb Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 19 Aug 2026 16:57:32 -0400 Subject: [PATCH 1/7] 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 960ce7e1f0320ccbe6a5208dff6ab4fa24485897 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 19 Aug 2026 21:57:04 -0400 Subject: [PATCH 2/7] Add slack-sdk to locked dependencies --- uv.lock | 2 ++ 1 file changed, 2 insertions(+) diff --git a/uv.lock b/uv.lock index a4e0436590..f4e15b3b52 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" }, ] From 7046424fba91ce961e6becb94ae6a445b1afee1c Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Mon, 24 Aug 2026 20:58:17 -0400 Subject: [PATCH 3/7] 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 81e78c3b8a56937f5ed66ec63653e0421dcd5dcd Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Mon, 24 Aug 2026 21:58:00 -0400 Subject: [PATCH 4/7] 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): From 0181ef1d17a309cff95c1a0847d0c4f50b0f9226 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 26 Aug 2026 15:11:20 -0400 Subject: [PATCH 5/7] Validate Slack clicks with Pydantic models --- docs/hackbot/api.md | 19 +- services/hackbot-api/app/routers/slack.py | 25 ++- services/hackbot-api/app/slack_webhook.py | 201 +++++++++------------- 3 files changed, 105 insertions(+), 140 deletions(-) diff --git a/docs/hackbot/api.md b/docs/hackbot/api.md index a957e7b52d..1e846a2d89 100644 --- a/docs/hackbot/api.md +++ b/docs/hackbot/api.md @@ -48,9 +48,12 @@ one rather than sharing it. Three things about the delivery shape the route: - **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. +- **A delivery this cannot read fails with a `5xx`**, deliberately, and that includes an + interaction type the app does not handle. Someone pressed something and it did nothing, + so Slack shows them an error, which beats a silent `200` that lets them believe it + worked. It also puts the reason in Sentry, where a Slack payload change is a page rather + than a log line nobody greps. Slack does not retry an interaction (retries are an Events + API feature), so the failure costs no retry storm. 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 @@ -61,10 +64,16 @@ What the endpoint answers: | Delivery | Status | | -------------------------------------------------- | ------ | -| Signature verifies | `200` | +| A click this app can read | `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` | +| Signed, but not a click this app can read | `500` | + +The payload is validated by a pydantic model over +[Slack's `block_actions` payload](https://docs.slack.dev/reference/interaction-payloads/block_actions-payload), +declaring only the fields the receiver reads and ignoring the rest of the delivery. That is +what produces the last row: a missing or misshapen field, or an interaction type other than +`block_actions`, is a `ValidationError` naming the field. 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 diff --git a/services/hackbot-api/app/routers/slack.py b/services/hackbot-api/app/routers/slack.py index 5e9773e8b9..dedb1bc0ff 100644 --- a/services/hackbot-api/app/routers/slack.py +++ b/services/hackbot-api/app/routers/slack.py @@ -30,18 +30,16 @@ 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) + action = click.actions[0] 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, + "Slack: %s clicked by %s (%s) in channel %s on message %s, value=%s", + action.action_id, + click.user.username or "unknown", + click.user.id, + click.channel.id, + click.message.ts, + action.value, ) # ACTION HANDLING GOES HERE @@ -50,11 +48,12 @@ async def slack_interactions(request: Request) -> Response: # missing, in the order it has to happen (the reasoning is in # `docs/hackbot/api.md` and `docs/hackbot/security.md`): # - # 1. Dispatch on `click.kind` against the kinds that have a receiver, and + # 1. Dispatch on `action.action_id` against the ids that have a receiver, and # answer 200 for one that does not. - # 2. Authorize the clicker from `click.user_id` and `click.team_id`. + # 2. Authorize the clicker from `click.user.id`, and check the workspace, which + # means adding the payload's `team` to the model when this lands. # 3. Make the effect at-most-once, keyed on something stable such as - # (`click.message_ts`, `click.kind`). + # (`click.message.ts`, `action.action_id`). # 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. diff --git a/services/hackbot-api/app/slack_webhook.py b/services/hackbot-api/app/slack_webhook.py index 9faba17d36..88dc5b5424 100644 --- a/services/hackbot-api/app/slack_webhook.py +++ b/services/hackbot-api/app/slack_webhook.py @@ -1,145 +1,102 @@ """Slack interaction payload handling: parsing a verified delivery into a click. -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``. +are already needed for signature verification. Everything after that is pydantic's: +it parses the JSON, checks the interaction is one this app handles, and validates the +fields the receiver reads. + +The models mirror Slack's payload rather than flattening it, so each one can be read +against the reference side by side, and only the fields the receiver actually uses +are declared. Everything else a delivery carries is ignored. + +**Anything it cannot read raises**, and the 500 is deliberate. A signature-verified +delivery this cannot parse means Slack changed its payload shape or the signing +secret leaked, and someone did press a button that then did nothing. Slack shows +that person an error, which is the honest outcome: the alternative is a silent 200 +that lets them believe the click worked. The `ValidationError` names the field that +was wrong, so Sentry gets the reason rather than a stack trace to decipher. + +Slack does not retry an interaction (retries are an Events API feature), so the 500 +costs no retry storm either. + +https://docs.slack.dev/reference/interaction-payloads/block_actions-payload """ from __future__ import annotations -import json import logging -from dataclasses import dataclass -from typing import Any +from typing import Any, Literal from urllib.parse import parse_qs +from pydantic import BaseModel, Field, Json + 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` 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 +class User(BaseModel): + """Who clicked.""" + + id: str + # Only ever logged, so a delivery without it is still a click worth acting on. + username: str | None = None + + +class Channel(BaseModel): + """Where the message they clicked is.""" + id: str -@dataclass(frozen=True) -class ButtonClick: + +class Message(BaseModel): + """The message they clicked, identified for a later ``chat.update``.""" + + ts: str + + +class Action(BaseModel): + """The element that was clicked.""" + + action_id: str + # `Json` decodes the value the drawing side put on the button and checks it is + # an object, so a button carrying something else fails here rather than at the + # first use of it. + value: Json[dict[str, Any]] + + +class ButtonClick(BaseModel): """A click on one button of a message this app posted. - ``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). + Clicks on message elements are the only interaction this app handles. Slack sends + other types to the same URL (``view_submission`` when a modal is submitted, + ``block_suggestion`` for a select's options); this app posts nothing that produces + one, so a delivery carrying one is as unreadable as any other and fails the same + way. """ - 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. + type: Literal["block_actions"] + user: User + channel: Channel + message: Message + # A click reports exactly one action even in a block of several buttons, so an + # empty list is a payload shape this does not know. + actions: list[Action] = Field(min_length=1) + # Slack's two single-use handles: for replying (valid ~30 minutes) and for + # opening a modal (~3 seconds). + response_url: str + trigger_id: str + + +def parse_interaction(raw_body: bytes) -> ButtonClick: + """The click a raw interaction delivery carries. + + ``.decode`` raises on bytes that are not UTF-8, and ``model_validate_json`` + raises on a payload that is not JSON, is not an object, or is not a readable + click. None of them can be answered, so none of them return. """ - 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 + form = parse_qs(raw_body.decode("utf-8")) 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) + raise ValueError("Slack interaction: body has no payload field") + + return ButtonClick.model_validate_json(encoded[0]) From 34804d971a26c03d9cc4641a8df77631a49145dc Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 26 Aug 2026 16:11:43 -0400 Subject: [PATCH 6/7] Fix form parsing in Slack interactions endpoint --- services/hackbot-api/app/routers/slack.py | 7 ++--- services/hackbot-api/app/slack_webhook.py | 38 ++--------------------- services/hackbot-api/pyproject.toml | 1 + uv.lock | 2 ++ 4 files changed, 9 insertions(+), 39 deletions(-) diff --git a/services/hackbot-api/app/routers/slack.py b/services/hackbot-api/app/routers/slack.py index dedb1bc0ff..223265ef8a 100644 --- a/services/hackbot-api/app/routers/slack.py +++ b/services/hackbot-api/app/routers/slack.py @@ -10,7 +10,7 @@ from fastapi import APIRouter, Depends, Request, Response, status from app.auth import require_slack_signature -from app.slack_webhook import parse_interaction +from app.slack_webhook import ButtonClick log = logging.getLogger(__name__) @@ -27,9 +27,8 @@ 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()) + form = await request.form() + click = ButtonClick.model_validate_json(form["payload"]) action = click.actions[0] log.info( diff --git a/services/hackbot-api/app/slack_webhook.py b/services/hackbot-api/app/slack_webhook.py index 88dc5b5424..0d1a334ec6 100644 --- a/services/hackbot-api/app/slack_webhook.py +++ b/services/hackbot-api/app/slack_webhook.py @@ -1,24 +1,9 @@ -"""Slack interaction payload handling: parsing a verified delivery into a click. +"""Slack interaction payload handling. -Takes raw bytes, because the body is form-encoded rather than JSON and those bytes -are already needed for signature verification. Everything after that is pydantic's: -it parses the JSON, checks the interaction is one this app handles, and validates the -fields the receiver reads. - -The models mirror Slack's payload rather than flattening it, so each one can be read -against the reference side by side, and only the fields the receiver actually uses +The models mirror Slack's payload, so each one can be read against the reference +side by side, and only the fields the receiver actually uses are declared. Everything else a delivery carries is ignored. -**Anything it cannot read raises**, and the 500 is deliberate. A signature-verified -delivery this cannot parse means Slack changed its payload shape or the signing -secret leaked, and someone did press a button that then did nothing. Slack shows -that person an error, which is the honest outcome: the alternative is a silent 200 -that lets them believe the click worked. The `ValidationError` names the field that -was wrong, so Sentry gets the reason rather than a stack trace to decipher. - -Slack does not retry an interaction (retries are an Events API feature), so the 500 -costs no retry storm either. - https://docs.slack.dev/reference/interaction-payloads/block_actions-payload """ @@ -26,7 +11,6 @@ import logging from typing import Any, Literal -from urllib.parse import parse_qs from pydantic import BaseModel, Field, Json @@ -84,19 +68,3 @@ class ButtonClick(BaseModel): # opening a modal (~3 seconds). response_url: str trigger_id: str - - -def parse_interaction(raw_body: bytes) -> ButtonClick: - """The click a raw interaction delivery carries. - - ``.decode`` raises on bytes that are not UTF-8, and ``model_validate_json`` - raises on a payload that is not JSON, is not an object, or is not a readable - click. None of them can be answered, so none of them return. - """ - form = parse_qs(raw_body.decode("utf-8")) - - encoded = form.get("payload") - if not encoded: - raise ValueError("Slack interaction: body has no payload field") - - return ButtonClick.model_validate_json(encoded[0]) diff --git a/services/hackbot-api/pyproject.toml b/services/hackbot-api/pyproject.toml index bac13acb58..d42db8124e 100644 --- a/services/hackbot-api/pyproject.toml +++ b/services/hackbot-api/pyproject.toml @@ -19,6 +19,7 @@ dependencies = [ "sentry-sdk>=2.51.0", "cachetools>=5.3.0", "slack-sdk>=3.27.0", + "python-multipart>=0.0.9", "httpx>=0.26.0", "hackbot-runtime", "phabricator-client", diff --git a/uv.lock b/uv.lock index f4e15b3b52..02f899bf0f 100644 --- a/uv.lock +++ b/uv.lock @@ -2718,6 +2718,7 @@ dependencies = [ { name = "phabricator-client" }, { name = "pydantic" }, { name = "pydantic-settings" }, + { name = "python-multipart" }, { name = "sentry-sdk" }, { name = "slack-sdk" }, { name = "sqlalchemy", extra = ["asyncio"] }, @@ -2750,6 +2751,7 @@ requires-dist = [ { name = "pydantic-settings", specifier = ">=2.1.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.0.0" }, { name = "pytest-asyncio", marker = "extra == 'dev'", specifier = ">=0.23.0" }, + { name = "python-multipart", specifier = ">=0.0.9" }, { name = "sentry-sdk", specifier = ">=2.51.0" }, { name = "slack-sdk", specifier = ">=3.27.0" }, { name = "sqlalchemy", extras = ["asyncio"], specifier = ">=2.0.25" }, From 445f2784e9bf181ae00ed2345987d9ea374c9526 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Wed, 26 Aug 2026 18:22:00 -0400 Subject: [PATCH 7/7] Handle Slack block actions for agent runs --- services/hackbot-api/app/routers/slack.py | 45 ++++++++++------------- services/hackbot-api/app/slack_webhook.py | 22 ++++++----- 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/services/hackbot-api/app/routers/slack.py b/services/hackbot-api/app/routers/slack.py index 223265ef8a..454ed27404 100644 --- a/services/hackbot-api/app/routers/slack.py +++ b/services/hackbot-api/app/routers/slack.py @@ -10,7 +10,8 @@ from fastapi import APIRouter, Depends, Request, Response, status from app.auth import require_slack_signature -from app.slack_webhook import ButtonClick +from app.routers.webhooks import get_hackbot_client +from app.slack_webhook import BlockActionsEvent log = logging.getLogger(__name__) @@ -28,32 +29,26 @@ ) async def slack_interactions(request: Request) -> Response: form = await request.form() - click = ButtonClick.model_validate_json(form["payload"]) - - action = click.actions[0] + event = BlockActionsEvent.model_validate_json(form["payload"]) log.info( - "Slack: %s clicked by %s (%s) in channel %s on message %s, value=%s", - action.action_id, - click.user.username or "unknown", - click.user.id, - click.channel.id, - click.message.ts, - action.value, + "Slack action: %s by %s (%s)", + event.trigger_id, + event.user.username, + event.user.id, ) - # ACTION HANDLING GOES HERE - # - # 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`): - # - # 1. Dispatch on `action.action_id` against the ids that have a receiver, and - # answer 200 for one that does not. - # 2. Authorize the clicker from `click.user.id`, and check the workspace, which - # means adding the payload's `team` to the model when this lands. - # 3. Make the effect at-most-once, keyed on something stable such as - # (`click.message.ts`, `action.action_id`). - # 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. + if len(event.actions) != 1: + raise ValueError( + "Expected exactly one action in a click delivery, got %d" + % len(event.actions) + ) + + action = event.actions[0] + match action.value.type: + case "start_agent_run": + client = get_hackbot_client() + client.trigger_run(action.value.agent_name, action.value.inputs) + case _: + raise ValueError("Unsupported action type: %s" % action.value.type) 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 0d1a334ec6..b2d7b8a468 100644 --- a/services/hackbot-api/app/slack_webhook.py +++ b/services/hackbot-api/app/slack_webhook.py @@ -37,6 +37,12 @@ class Message(BaseModel): ts: str +class ActionValue(BaseModel): + type: Literal["start_agent_run"] + agent_name: str + params: dict[str, Any] = Field(default_factory=dict) + + class Action(BaseModel): """The element that was clicked.""" @@ -44,10 +50,10 @@ class Action(BaseModel): # `Json` decodes the value the drawing side put on the button and checks it is # an object, so a button carrying something else fails here rather than at the # first use of it. - value: Json[dict[str, Any]] + value: Json[ActionValue] -class ButtonClick(BaseModel): +class BlockActionsEvent(BaseModel): """A click on one button of a message this app posted. Clicks on message elements are the only interaction this app handles. Slack sends @@ -59,12 +65,8 @@ class ButtonClick(BaseModel): type: Literal["block_actions"] user: User - channel: Channel - message: Message - # A click reports exactly one action even in a block of several buttons, so an - # empty list is a payload shape this does not know. - actions: list[Action] = Field(min_length=1) - # Slack's two single-use handles: for replying (valid ~30 minutes) and for - # opening a modal (~3 seconds). - response_url: str + channel: Channel | None = None + message: Message | None = None + actions: list[Action] = Field(default_factory=list) + response_url: str | None = None trigger_id: str