chore(webhooks): withdrawn

src/agentex/lib/sdk/utils/webhook_signatures.py

@@ -0,0 +1,157 @@
+"""Prove an inbound webhook really came from GitHub or Slack, agent-side.
+
+The Agentex control plane verifies a webhook's HMAC at its
+``/agents/forward/name/{agent}/{path}`` ingress and then proxies the request to the
+agent's own HTTP route. That verification does NOT travel with the request: the
+agent's route is an ordinary FastAPI route on an app bound to ``0.0.0.0:8000``, so
+any peer that can reach the pod port can POST the same shape of body directly, and
+nothing about the request distinguishes it from a proxied one (VULN-3137). FastACP
+adds no authentication to agent-defined routes either -- its ``x-agent-api-key``
+check lives inside the JSON-RPC handler for ``POST /api`` only.
+
+The control plane forwards the source's own signature headers through to the agent
+(it strips only hop-by-hop headers), so the agent CAN re-verify them. That is what
+this module is for: :class:`WebhookVerification` carries the raw body, the request
+headers and the shared secret, and :meth:`WebhookVerification.check` fails closed
+unless the signature matches.
+
+Two properties matter and both need the RAW body:
+
+* the HMAC is computed over the exact bytes the source signed -- re-serialising a
+  parsed ``dict`` produces different bytes and will never verify;
+* Slack additionally binds a timestamp, so a captured request cannot be replayed
+  indefinitely.
+
+Stdlib only.
+"""
+
+from __future__ import annotations
+
+import hmac
+import time
+import hashlib
+from typing import Literal
+from dataclasses import dataclass
+from collections.abc import Mapping
+
+__all__ = [
+    "GITHUB_SIGNATURE_HEADER",
+    "SLACK_MAX_AGE_SECONDS",
+    "SLACK_SIGNATURE_HEADER",
+    "SLACK_TIMESTAMP_HEADER",
+    "WebhookSignatureError",
+    "WebhookSource",
+    "WebhookVerification",
+    "verify_github_signature",
+    "verify_slack_signature",
+]
+
+WebhookSource = Literal["github", "slack"]
+
+GITHUB_SIGNATURE_HEADER = "x-hub-signature-256"
+SLACK_SIGNATURE_HEADER = "x-slack-signature"
+SLACK_TIMESTAMP_HEADER = "x-slack-request-timestamp"
+
+# Slack's own guidance: refuse a request whose timestamp is more than five minutes
+# old, so a captured body cannot be replayed forever.
+SLACK_MAX_AGE_SECONDS = 300
+
+
+class WebhookSignatureError(RuntimeError):
+    """The payload could not be proved to come from the webhook source.
+
+    Deliberately distinct from ``WebhookError``: a handler should answer this with
+    401/403, not 500, and must not fall through to driving an agent turn.
+    """
+
+
+def _header(headers: Mapping[str, str], name: str) -> str:
+    """Case-insensitive header lookup over a plain mapping.
+
+    Starlette's ``request.headers`` is already case-insensitive, but a handler may
+    hand over a plain ``dict``, and HTTP header names are not case-sensitive.
+    """
+    value = headers.get(name)
+    if value is not None:
+        return value
+    lowered = name.lower()
+    for key, candidate in headers.items():
+        if isinstance(key, str) and key.lower() == lowered:
+            return candidate
+    return ""
+
+
+def verify_github_signature(secret: str, body: bytes, headers: Mapping[str, str]) -> None:
+    """Check GitHub's ``X-Hub-Signature-256``; raise :class:`WebhookSignatureError` if bad.
+
+    The signature is ``sha256=<hex>`` of HMAC-SHA256(secret, raw body). Gitea sends
+    the same header for the same reason, so this covers both.
+    """
+    if not secret:
+        raise WebhookSignatureError("No webhook secret configured; cannot verify the signature")
+    presented = _header(headers, GITHUB_SIGNATURE_HEADER).strip()
+    if not presented:
+        raise WebhookSignatureError(f"Missing {GITHUB_SIGNATURE_HEADER} header")
+    expected = "sha256=" + hmac.new(secret.encode("utf-8"), body, hashlib.sha256).hexdigest()
+    if not hmac.compare_digest(presented, expected):
+        raise WebhookSignatureError(f"Invalid {GITHUB_SIGNATURE_HEADER} signature")
+
+
+def verify_slack_signature(
+    secret: str,
+    body: bytes,
+    headers: Mapping[str, str],
+    *,
+    now: float | None = None,
+    max_age_seconds: int = SLACK_MAX_AGE_SECONDS,
+) -> None:
+    """Check Slack's ``X-Slack-Signature``; raise :class:`WebhookSignatureError` if bad.
+
+    The signature is ``v0=<hex>`` of HMAC-SHA256(secret, ``v0:<timestamp>:<raw body>``).
+    A timestamp further than ``max_age_seconds`` from now (either direction, so a
+    future-dated one is refused too) is rejected before the HMAC is compared.
+    """
+    if not secret:
+        raise WebhookSignatureError("No webhook secret configured; cannot verify the signature")
+    presented = _header(headers, SLACK_SIGNATURE_HEADER).strip()
+    if not presented:
+        raise WebhookSignatureError(f"Missing {SLACK_SIGNATURE_HEADER} header")
+
+    raw_timestamp = _header(headers, SLACK_TIMESTAMP_HEADER).strip()
+    try:
+        timestamp = int(raw_timestamp)
+    except (TypeError, ValueError):
+        raise WebhookSignatureError(f"Missing or unparseable {SLACK_TIMESTAMP_HEADER} header") from None
+    current = time.time() if now is None else now
+    if abs(current - timestamp) > max_age_seconds:
+        raise WebhookSignatureError(f"Slack timestamp {timestamp} is more than {max_age_seconds}s from now")
+
+    basis = b"v0:" + raw_timestamp.encode("utf-8") + b":" + body
+    expected = "v0=" + hmac.new(secret.encode("utf-8"), basis, hashlib.sha256).hexdigest()
+    if not hmac.compare_digest(presented, expected):
+        raise WebhookSignatureError(f"Invalid {SLACK_SIGNATURE_HEADER} signature")
+
+
+@dataclass(frozen=True)
+class WebhookVerification:
+    """Everything needed to prove one inbound webhook's provenance.
+
+    ``body`` must be the RAW request bytes (``await request.body()``), not a parsed
+    and re-serialised dict: the HMAC covers the bytes the source signed.
+    """
+
+    source: WebhookSource
+    secret: str
+    body: bytes
+    headers: Mapping[str, str]
+    #: Override for tests; ``None`` means ``time.time()`` (Slack freshness only).
+    now: float | None = None
+
+    def check(self) -> None:
+        """Raise :class:`WebhookSignatureError` unless the signature verifies."""
+        if self.source == "github":
+            verify_github_signature(self.secret, self.body, self.headers)
+        elif self.source == "slack":
+            verify_slack_signature(self.secret, self.body, self.headers, now=self.now)
+        else:  # pragma: no cover - Literal keeps this unreachable for typed callers
+            raise WebhookSignatureError(f"Unknown webhook source {self.source!r}")

src/agentex/lib/sdk/utils/webhooks.py

@@ -1,39 +1,71 @@
 """Drive an agent turn from an inbound webhook, inside a forward-route handler.
 
-The Agentex server already exposes a webhook ingress: a request to
+The Agentex server exposes a webhook ingress: a request to
 ``/agents/forward/name/{agent}/{path}`` is signature-verified (GitHub ``sha256=`` /
 Slack ``v0:`` HMAC via the agent's registered keys) and proxied to the agent's own
 HTTP route. This helper is what that route handler calls to turn the inbound payload
 into an agent turn — without each agent re-implementing payload shaping, config
 resolution, session continuity, and reply handling.
 
+**That ingress verification does not travel with the request.** The route below is an
+ordinary FastAPI route on an app bound to ``0.0.0.0:8000``, so any peer that can reach
+the pod port can POST the same body straight to it, and FastACP adds no authentication
+to agent-defined routes. Reaching this route is therefore NOT evidence that the
+signature was ever checked. The control plane does forward the source's own signature
+headers, so the route re-verifies them here — ``handle_webhook`` refuses to drive a
+turn without that proof (VULN-3137).
+
 Typical use inside an agent::
 
-    from fastapi import Request
-    from agentex.lib.sdk.utils.webhooks import handle_webhook
+    import json
+    import os
+
+    from fastapi import HTTPException, Request
+    from agentex.lib.sdk.utils.webhooks import (
+        WebhookVerification,
+        WebhookSignatureError,
+        handle_webhook,
+    )
 
 
     @acp.post("/github-pr")
     async def github_pr(request: Request):
-        body = await request.json()
-        result = await handle_webhook(
-            agent_name="my-agent",
-            payload=body,
-            acp_type="sync",
-            shaper="github_pr",
-            params_source="https://<host>/public/v5/agent_configs/<id>/resolve",
-            params_source_headers={"x-api-key": ..., "x-selected-account-id": ...},
-            wait=True,
-        )
+        # RAW bytes: the HMAC covers what GitHub signed, and a re-serialised dict
+        # produces different bytes that can never verify.
+        raw = await request.body()
+        try:
+            result = await handle_webhook(
+                agent_name="my-agent",
+                payload=json.loads(raw),
+                verify=WebhookVerification(
+                    source="github",
+                    secret=os.environ["GITHUB_WEBHOOK_SECRET"],
+                    body=raw,
+                    headers=request.headers,
+                ),
+                acp_type="sync",
+                shaper="github_pr",
+                params_source="https://<host>/public/v5/agent_configs/<id>/resolve",
+                params_source_headers={"x-api-key": ..., "x-selected-account-id": ...},
+                wait=True,
+            )
+        except WebhookSignatureError:
+            raise HTTPException(status_code=403, detail="bad webhook signature") from None
         return {"task_id": result.task_id, "reply": result.reply}
 
+``GITHUB_WEBHOOK_SECRET`` is the signing secret shown once when the trigger was
+registered (``POST /agents/{name}/webhook-triggers``); give it to the agent the same
+way as any other secret. Verifying also makes session continuity safe: the task name
+is keyed with that secret, so only a holder can name an existing conversation.
+
 Config-by-id: pass ``params_source`` pointing at the platform's config-resolve
 endpoint; the resolved params (e.g. system_prompt / harness / model / tools) are
 forwarded opaquely to ``task/create``. Or pass inline ``params`` for a one-off.
 """
 
 from __future__ import annotations
 
+import hmac
 import json
 import hashlib
 from typing import Any, Literal
@@ -43,6 +75,11 @@ async def github_pr(request: Request):
 from agentex.lib import adk
 from agentex.lib.utils.logging import make_logger
 from agentex.types.task_message_content import TextContent
+from agentex.lib.sdk.utils.webhook_signatures import (
+    WebhookSource as WebhookSource,
+    WebhookVerification as WebhookVerification,
+    WebhookSignatureError as WebhookSignatureError,
+)
 
 logger = make_logger(__name__)
 
@@ -69,11 +106,26 @@ class WebhookResult:
 # --------------------------------------------------------------------------- shaping
 
 
-def session_key(agent_name: str, channel: str, peer_id: str) -> str:
+def session_key(agent_name: str, channel: str, peer_id: str, *, secret: str | None = None) -> str:
     """Stable per-conversation task name → reused for get-or-create on task/create, so
-    repeat events from the same source fold into one task instead of spawning new ones."""
+    repeat events from the same source fold into one task instead of spawning new ones.
+
+    ``task/create`` is get-or-create BY NAME, so this name decides which existing
+    conversation a turn joins. Unkeyed it is computable from public data (a repo name
+    and a PR number), which let an unverified caller name — and read the reply from —
+    somebody else's conversation (VULN-3137). ``secret`` keys the digest with the
+    webhook's shared secret, so the name can only be derived by someone who already
+    holds it, and two installations watching the same repository stay separate.
+
+    Passing no ``secret`` keeps an unkeyed (but no longer SHA-1) name, for callers that
+    verified provenance themselves and have no secret to hand.
+    """
     basis = peer_id or "main"
-    digest = hashlib.sha1(f"{agent_name}:{channel}:{basis}".encode()).hexdigest()[:16]
+    material = f"{agent_name}:{channel}:{basis}".encode()
+    if secret:
+        digest = hmac.new(secret.encode("utf-8"), material, hashlib.sha256).hexdigest()[:16]
+    else:
+        digest = hashlib.sha256(material).hexdigest()[:16]
     return f"wh-{channel}-{digest}"
 
 
@@ -258,17 +310,49 @@ async def handle_webhook(
     extra_task_metadata: dict[str, str] | None = None,
     wait: bool = False,
     fetch: ParamsFetcher | None = None,
+    verify: WebhookVerification | None = None,
+    trust_caller: bool = False,
 ) -> WebhookResult:
     """Drive an agent turn from a webhook payload, agent-side, via the ADK client.
 
+    - Proves provenance FIRST: ``verify`` carries the raw body, headers and shared
+      secret, and nothing else runs until its signature checks out.
     - Shapes the payload (generic or GitHub PR) into a prompt + conversation scope.
     - Resolves task params: inline ``params``, or fetched from ``params_source``
       (config-by-id). The platform never interprets params — they're forwarded to the
       agent as ``task/create`` params.
     - Get-or-creates a task keyed on a stable session key, so repeat events fold in.
     - Sends the turn (sync → message/send returns the reply inline; async → event/send,
       with optional ``wait`` to poll for the reply).
+
+    Provenance is mandatory (VULN-3137). The route this runs in is served on the
+    agent's own public listener, so "it reached my route" is not evidence that the
+    control plane's signature-verifying ingress let it through. Pass ``verify`` with
+    the raw request bytes, or — only if the route proved provenance by some other
+    means — pass ``trust_caller=True`` to say so explicitly. With neither, this
+    raises :class:`WebhookSignatureError` rather than driving a turn.
+
+    Raises:
+        WebhookSignatureError: no provenance was established, or the signature failed.
+        WebhookError: the turn could not be driven (e.g. params resolution failed).
     """
+    if verify is not None:
+        verify.check()
+        session_secret: str | None = verify.secret
+    elif trust_caller:
+        # The route says it established provenance itself. Nothing to key the session
+        # name with, so it falls back to an unkeyed name.
+        session_secret = None
+    else:
+        raise WebhookSignatureError(
+            "handle_webhook refuses to drive a turn without provenance: pass "
+            "verify=WebhookVerification(source=..., secret=..., body=<raw request "
+            "bytes>, headers=request.headers), or trust_caller=True if this route "
+            "has already proved the request came from the webhook source. The "
+            "agent's HTTP route is reachable directly, so an unverified payload can "
+            "name and read back any existing webhook conversation (VULN-3137)."
+        )
+
     channel = channel or shaper
     if shaper == "github_pr":
         text, derived_peer, sender = shape_github_pr(payload)
@@ -292,7 +376,7 @@ async def handle_webhook(
         for key, value in extra_task_metadata.items():
             task_metadata.setdefault(key, str(value))
 
-    name = session_key(agent_name, channel, peer_id or "")
+    name = session_key(agent_name, channel, peer_id or "", secret=session_secret)
     # task/create carries only name/params (CreateTaskParams has no task_metadata field),
     # so we create first, then stamp task_metadata via a follow-up update below.
     task = await adk.acp.create_task(