From 86fd6da1dc64423ea134f7ad41c7862cbd77c172 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 2 Aug 2026 14:34:04 -0700 Subject: [PATCH] fix(hooks): single delivery_id across header+body, never follow redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - delivery_id is now generated once per firing and used for both the X-Hermes-Delivery header and the signed body's delivery_id field — previously they were two different uuid4s, breaking receiver-side dedupe as documented. - 3xx responses are no longer followed: urllib's default redirect handler converts a redirected POST into a body-less GET, silently dropping the signed payload. Redirects now log a misconfiguration warning and count as delivery failure (no retry). - Docs: receiver-side replay-protection guidance (dedupe on delivery_id, timestamp freshness window) + redirect semantics. - Tests: 5xx retry count, redirect-not-followed (sabotage-verified), header/body delivery_id equality. --- agent/outbound_webhooks.py | 51 ++++++++++++++--- tests/agent/test_outbound_webhooks.py | 67 +++++++++++++++++++++-- website/docs/user-guide/features/hooks.md | 8 ++- 3 files changed, 112 insertions(+), 14 deletions(-) diff --git a/agent/outbound_webhooks.py b/agent/outbound_webhooks.py index c0f7ccbad28f0..5b81ede34a56f 100644 --- a/agent/outbound_webhooks.py +++ b/agent/outbound_webhooks.py @@ -383,15 +383,16 @@ def _make_callback(event: str, target: WebhookTarget): if event in _TOOL_SCOPED_EVENTS: if not target.matches_tool(kwargs.get("tool_name")): return None + delivery_id = uuid.uuid4().hex try: - body = _serialize_payload(event, kwargs) + body = _serialize_payload(event, kwargs, delivery_id) except Exception: # defensive — a bad payload must not hurt the loop logger.warning( "outbound webhook payload serialization failed (event=%s " "target=%s)", event, target.label, exc_info=True, ) return None - _enqueue(_build_delivery(event, target, body)) + _enqueue(_build_delivery(event, target, body, delivery_id)) return None _callback.__name__ = f"outbound_webhook[{event}:{target.label}]" @@ -399,9 +400,16 @@ def _make_callback(event: str, target: WebhookTarget): return _callback -def _serialize_payload(event: str, kwargs: Dict[str, Any]) -> bytes: +def _serialize_payload( + event: str, kwargs: Dict[str, Any], delivery_id: str, +) -> bytes: """Render the POST body. Same top-level shape as shell hooks' stdin - (documented in :mod:`agent.shell_hooks`), plus delivery metadata.""" + (documented in :mod:`agent.shell_hooks`), plus delivery metadata. + + ``delivery_id`` is shared with the ``X-Hermes-Delivery`` header so + receivers can dedupe on either — and since it (plus ``timestamp``) + lives inside the HMAC-signed body, it doubles as replay protection. + """ extras = {k: v for k, v in kwargs.items() if k not in _TOP_LEVEL_PAYLOAD_KEYS} try: cwd = str(Path.cwd()) @@ -414,7 +422,7 @@ def _serialize_payload(event: str, kwargs: Dict[str, Any]) -> bytes: "session_id": kwargs.get("session_id") or kwargs.get("parent_session_id") or "", "cwd": cwd, "extra": extras, - "delivery_id": uuid.uuid4().hex, + "delivery_id": delivery_id, "timestamp": datetime.now(tz=timezone.utc) .isoformat() .replace("+00:00", "Z"), @@ -423,13 +431,13 @@ def _serialize_payload(event: str, kwargs: Dict[str, Any]) -> bytes: def _build_delivery( - event: str, target: WebhookTarget, body: bytes, + event: str, target: WebhookTarget, body: bytes, delivery_id: str, ) -> Dict[str, Any]: headers = { "Content-Type": "application/json", "User-Agent": "Hermes-Agent-Outbound-Webhook", "X-Hermes-Event": event, - "X-Hermes-Delivery": uuid.uuid4().hex, + "X-Hermes-Delivery": delivery_id, } if target.secret: digest = hmac.new( @@ -486,9 +494,26 @@ def _worker_loop() -> None: _delivery_queue.task_done() +class _NoRedirectHandler(urlrequest.HTTPRedirectHandler): + """Refuse to follow redirects. + + urllib's default handler converts a redirected POST into a body-less + GET — the signed payload would be silently dropped and the headers + re-sent to a location the user never configured. Treat any 3xx as a + delivery failure instead (surfaced as HTTPError by returning None). + """ + + def redirect_request(self, req, fp, code, msg, headers, newurl): # noqa: D102 + return None + + +_opener = urlrequest.build_opener(_NoRedirectHandler) + + def _deliver(delivery: Dict[str, Any]) -> None: """POST with bounded retries. Retries on connection errors and 5xx; - 4xx is the receiver telling us the request itself is wrong — no retry.""" + 4xx is the receiver telling us the request itself is wrong — no retry. + 3xx redirects are never followed (misconfiguration — fix the URL).""" last_error = "" for attempt in range(1, MAX_DELIVERY_ATTEMPTS + 1): req = urlrequest.Request( @@ -498,7 +523,7 @@ def _deliver(delivery: Dict[str, Any]) -> None: method="POST", ) try: - with urlrequest.urlopen(req, timeout=delivery["timeout"]) as resp: + with _opener.open(req, timeout=delivery["timeout"]) as resp: status = getattr(resp, "status", 200) if 200 <= status < 300: logger.debug( @@ -509,6 +534,14 @@ def _deliver(delivery: Dict[str, Any]) -> None: last_error = f"HTTP {status}" except urlerror.HTTPError as exc: last_error = f"HTTP {exc.code}" + if 300 <= exc.code < 400: + logger.warning( + "outbound webhook target redirected (event=%s target=%s): " + "%s -> %s — redirects are not followed; update the " + "configured url", delivery["event"], delivery["label"], + last_error, exc.headers.get("Location", "?"), + ) + return if 400 <= exc.code < 500: logger.warning( "outbound webhook rejected (event=%s target=%s): %s — " diff --git a/tests/agent/test_outbound_webhooks.py b/tests/agent/test_outbound_webhooks.py index 1d1e12e999495..001f91c4a305b 100644 --- a/tests/agent/test_outbound_webhooks.py +++ b/tests/agent/test_outbound_webhooks.py @@ -56,12 +56,24 @@ class _CapturingHandler(BaseHTTPRequestHandler): self.server.captured.append( # type: ignore[attr-defined] { "path": self.path, + "method": "POST", "headers": dict(self.headers), "body": body, } ) status = getattr(self.server, "respond_status", 200) self.send_response(status) + location = getattr(self.server, "respond_location", None) + if location: + self.send_header("Location", location) + self.end_headers() + + def do_GET(self): # noqa: N802 — records redirect follow-ups + self.server.captured.append( # type: ignore[attr-defined] + {"path": self.path, "method": "GET", "headers": dict(self.headers), + "body": b""} + ) + self.send_response(200) self.end_headers() def log_message(self, format, *args): # noqa: A002 — http.server naming @@ -275,6 +287,7 @@ class TestPayload: "status": "ok", "duration_ms": 42, }, + "did_1234", ) payload = json.loads(body) assert payload["hook_event_name"] == "post_tool_call" @@ -283,12 +296,12 @@ class TestPayload: assert payload["session_id"] == "sess_1" assert payload["extra"]["status"] == "ok" assert payload["extra"]["duration_ms"] == 42 - assert payload["delivery_id"] + assert payload["delivery_id"] == "did_1234" assert payload["timestamp"].endswith("Z") def test_unserialisable_values_stringified(self): body = outbound_webhooks._serialize_payload( - "on_session_end", {"weird": object()} + "on_session_end", {"weird": object()}, "did_1" ) payload = json.loads(body) assert isinstance(payload["extra"]["weird"], str) @@ -418,11 +431,57 @@ class TestDelivery: url=_url(http_server), events=["on_session_end"], ) delivery = outbound_webhooks._build_delivery( - "on_session_end", target, b"{}", + "on_session_end", target, b"{}", "did_4xx", ) outbound_webhooks._deliver(delivery) assert len(http_server.captured) == 1 + def test_5xx_retried_once(self, http_server): + http_server.respond_status = 500 + target = outbound_webhooks.WebhookTarget( + url=_url(http_server), events=["on_session_end"], + ) + delivery = outbound_webhooks._build_delivery( + "on_session_end", target, b"{}", "did_5xx", + ) + outbound_webhooks._deliver(delivery) + assert len(http_server.captured) == outbound_webhooks.MAX_DELIVERY_ATTEMPTS + + def test_redirect_not_followed(self, http_server): + """3xx must be treated as failure — urllib's default handler would + convert a 302'd POST into a body-less GET at the redirect target.""" + http_server.respond_status = 302 + http_server.respond_location = _url(http_server, "/redirected") + target = outbound_webhooks.WebhookTarget( + url=_url(http_server), events=["on_session_end"], + ) + delivery = outbound_webhooks._build_delivery( + "on_session_end", target, b"{}", "did_3xx", + ) + outbound_webhooks._deliver(delivery) + # One POST hit the server (the redirect response), nothing followed + # (no GET to /redirected), no retry (misconfiguration, not transient). + assert [c["method"] for c in http_server.captured] == ["POST"] + assert http_server.captured[0]["path"] == "/hook" + + def test_delivery_id_matches_header_and_body(self, http_server): + """The X-Hermes-Delivery header and the signed body's delivery_id + must be the same value, or receiver-side dedupe breaks.""" + cfg = _cfg( + {"url": _url(http_server), "events": ["on_session_end"], + "secret": "s"} + ) + outbound_webhooks.register_from_config(cfg) + + from hermes_cli.plugins import get_plugin_manager + + get_plugin_manager().invoke_hook("on_session_end", session_id="s1") + assert outbound_webhooks.flush() + + req = http_server.captured[0] + payload = json.loads(req["body"]) + assert payload["delivery_id"] == req["headers"]["X-Hermes-Delivery"] + def test_connection_error_does_not_raise(self): target = outbound_webhooks.WebhookTarget( # Port 9 (discard) — nothing listening. @@ -431,7 +490,7 @@ class TestDelivery: timeout=1, ) delivery = outbound_webhooks._build_delivery( - "on_session_end", target, b"{}", + "on_session_end", target, b"{}", "did_conn", ) # Must swallow the failure (logged), never raise into the agent loop. outbound_webhooks._deliver(delivery) diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index 29625056ebfd9..be8b9c0caa279 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -1566,7 +1566,7 @@ Headers: |--------|-------| | `Content-Type` | `application/json` | | `X-Hermes-Event` | The hook event name | -| `X-Hermes-Delivery` | Unique id per delivery (for idempotency on the receiver) | +| `X-Hermes-Delivery` | Unique id per delivery — same value as `delivery_id` in the body | | `X-Hermes-Signature-256` | `sha256=` — HMAC-SHA256 of the raw body, GitHub-style; only present when a secret is configured | Verify the signature exactly as you would a GitHub webhook: @@ -1579,11 +1579,17 @@ def verify(body: bytes, header: str, secret: str) -> bool: return hmac.compare_digest(expected, header) ``` +Because `delivery_id` and `timestamp` live **inside the signed body**, a verified receiver also gets replay protection for free: + +- **Dedupe** on `delivery_id` (or the matching `X-Hermes-Delivery` header) — remember recently seen ids and skip duplicates. Hermes retries failed deliveries once, so the same id can legitimately arrive twice. +- **Reject stale events** by checking `timestamp` against your clock with a tolerance window (5 minutes is the common default). An attacker replaying a captured request can't forge a fresh timestamp without the secret. + ### Delivery semantics - **Fire-and-forget, off the hot path.** Events are serialized and queued instantly; a single background thread performs the HTTP POSTs. A slow or dead endpoint can never stall a tool call or an agent turn. - **Notify-only.** Unlike shell hooks, outbound webhooks cannot block tool calls or inject context — the response body is ignored. They observe, never steer. - **Bounded retries.** Connection errors and 5xx responses are retried once with backoff; 4xx responses are not retried (the receiver said the request itself is wrong). Failures are logged and dropped — delivery is best-effort, not guaranteed. +- **Redirects are never followed.** A 3xx response is treated as a misconfiguration and logged — following a redirected POST would silently drop the signed payload. Point the `url` at the final endpoint. - **Bounded queue.** If the queue backs up (dead endpoint, event storm), new events are dropped with a warning rather than consuming unbounded memory. - **No consent prompt.** Outbound targets execute no code on your machine — they receive data at a URL you configured. `HERMES_SAFE_MODE=1` still skips registration, same as plugins and shell hooks. Note that payloads include tool inputs and event metadata, so only point targets at endpoints you trust, and prefer `https://`.