From c3e01c753dcbb0e881e693df89fe4141b79b2b03 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:05:39 -0700 Subject: [PATCH] fix(gateway): /p// webhook delivery and api_server callbacks stay on the routed profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under gateway.multiplex_profiles a /p// webhook route (or one with `profile: `) executed under that profile but delivered its reply as the FIRST profile owning the target platform: `_find_adapter` took `runner.adapters` then iterated `_profile_adapters` in dict order, and the home-channel fallback read `runner.config` (the default profile's). The gh leg for `github_comment` inherited the process environ, i.e. the default profile's GH_TOKEN. Both directions leaked: a secondary route posted through the default bot, and a default route borrowed a platform parked only on a secondary. The api_server `/p//api/platforms//events` callback had the same shape — `_get_platform_callback_adapter` read `runner.adapters` regardless of `_api_request_profile`, so a secondary's Google Chat / Teams events were verified and dispatched by the default adapter. Now: - webhook: `_delivery_info` / the deliver_only dict carry the resolved profile; `_find_adapter(platform, profile)` resolves through the runner's shared fail-closed `_authorization_adapter`; the delivery leg runs inside `_profile_scope(profile)` and takes the home channel from that profile's `load_gateway_config()`; `gh` gets GH_TOKEN/GITHUB_TOKEN from the profile secret scope (default's values dropped from the child env when the profile has none). - api_server: the callback adapter resolves via `_authorization_adapter(platform, _api_request_profile.get())`; a named profile without the adapter is a 503, never the primary's adapter. A profile without the target platform fails closed ("not connected" → 502 / 503) instead of a silent cross-profile send. Fixes #65939 Fixes #84266 Co-authored-by: 604maestro <604maestro@protonmail.com> Co-authored-by: mjshorty Co-authored-by: StellarisW --- gateway/platforms/api_server.py | 14 +- gateway/platforms/webhook.py | 73 +++++++--- tests/gateway/test_webhook_adapter.py | 2 + tests/gateway/test_webhook_deliver_only.py | 1 + tests/gateway/test_webhook_integration.py | 2 + tests/gateway/test_webhook_profile_egress.py | 125 ++++++++++++++++++ .../docs/user-guide/multi-profile-gateways.md | 10 ++ 7 files changed, 199 insertions(+), 28 deletions(-) create mode 100644 tests/gateway/test_webhook_profile_egress.py diff --git a/gateway/platforms/api_server.py b/gateway/platforms/api_server.py index c7d2c05cec..d72b9d6325 100644 --- a/gateway/platforms/api_server.py +++ b/gateway/platforms/api_server.py @@ -1387,16 +1387,16 @@ class APIServerAdapter(OpenAICompatRoutesMixin, BasePlatformAdapter): if adapter is not None: return adapter runner = self.gateway_runner or request.app.get("gateway_runner") - adapters = getattr(runner, "adapters", None) - if not adapters: + if runner is None: return None + # ``/p//`` binds the callback to that profile's adapter map; a missing adapter there is a + # 503, never the primary profile's adapter (verifying/dispatching a secondary's events under the + # default bot's credentials, #84266). ``_authorization_adapter`` is the shared fail-closed resolver. try: - return adapters.get(Platform(platform_name)) + platform = Platform(platform_name) except Exception: - for platform, candidate in adapters.items(): - if getattr(platform, "value", platform) == platform_name: - return candidate - return None + return None + return runner._authorization_adapter(platform, _api_request_profile.get()) async def _handle_platform_event_callback(self, request: "web.Request") -> "web.Response": platform_name = self._normalize_callback_platform(request.match_info.get("platform", "")) diff --git a/gateway/platforms/webhook.py b/gateway/platforms/webhook.py index 69317f90bd..1a34f58a5d 100644 --- a/gateway/platforms/webhook.py +++ b/gateway/platforms/webhook.py @@ -13,6 +13,7 @@ import hashlib import hmac import json import logging +import os import re import subprocess import sys @@ -60,6 +61,8 @@ _LOOPBACK_HOSTS = frozenset({"127.0.0.1", "localhost", "::1", "ip6-localhost", " _V2_REPLAY_WINDOW_SECONDS = 300 _TEMPLATE_KEY_RE = re.compile(r"\{([a-zA-Z0-9_.]+)\}") _REPO_RE = re.compile(r"[A-Za-z0-9._-]+/[A-Za-z0-9._-]+") +# Credentials `gh` reads; a routed profile's github_comment must use its own, never the process env's. +_GH_TOKEN_VARS = ("GH_TOKEN", "GITHUB_TOKEN") def _is_loopback_host(host: Optional[str]) -> bool: @@ -448,10 +451,10 @@ class WebhookAdapter(BasePlatformAdapter): return _UNPARSEABLE async def _handle_deliver_only(self, prompt: str, payload: Any, route_config: dict, route_name: str, - event_type: str, delivery_id: str) -> "web.Response": + event_type: str, delivery_id: str, profile: Optional[str] = None) -> "web.Response": """deliver_only: the rendered prompt IS the message — skip the agent, reuse the same auth/rate-limit/idempotency/template pipeline.""" - delivery = {"deliver": route_config.get("deliver", "log"), "payload": payload, + delivery = {"deliver": route_config.get("deliver", "log"), "payload": payload, "profile": profile, "deliver_extra": self._render_delivery_extra(route_config.get("deliver_extra", {}), payload)} logger.info("[webhook] direct-deliver event=%s route=%s target=%s msg_len=%d delivery=%s", event_type, route_name, delivery["deliver"], len(prompt), delivery_id) @@ -556,7 +559,8 @@ class WebhookAdapter(BasePlatformAdapter): logger.info("[webhook] Skipping duplicate delivery %s", delivery_id) return web.json_response({"status": "duplicate", "delivery_id": delivery_id}, status=200) if route_config.get("deliver_only"): - return await self._handle_deliver_only(prompt, payload, route_config, route_name, event_type, delivery_id) + return await self._handle_deliver_only(prompt, payload, route_config, route_name, event_type, delivery_id, + profile) return self._dispatch_agent_run(request, route_config, route_name, profile, payload, prompt, event_type, delivery_id, now) @@ -565,8 +569,10 @@ class WebhookAdapter(BasePlatformAdapter): """Record delivery info, spawn the agent run, and return 202 immediately.""" # delivery_id in the session key → concurrent webhooks on one route get independent runs. session_chat_id = f"webhook:{route_name}:{delivery_id}" + # ``profile`` rides along so the reply leg (``send`` → ``_deliver_cross_platform``) egresses through + # THIS profile's adapter, home channel and secrets — not the first profile that has the platform. self._delivery_info[session_chat_id] = { - "deliver": route_config.get("deliver", "log"), + "deliver": route_config.get("deliver", "log"), "profile": profile, "deliver_extra": self._render_delivery_extra(route_config.get("deliver_extra", {}), payload)} self._delivery_info_created[session_chat_id] = now self._delivery_info_order.append((now, session_chat_id)) @@ -735,7 +741,8 @@ class WebhookAdapter(BasePlatformAdapter): # the worker thread is bounded by the subprocess timeout below. result = await asyncio.to_thread( subprocess.run, ["gh", "pr", "comment", str(pr_int), "--repo", repo, "--body", content], - capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=30) + capture_output=True, text=True, encoding='utf-8', errors='replace', timeout=30, + env=self._github_env(delivery.get("profile"))) if result.returncode == 0: logger.info("[webhook] Posted comment on %s#%s", repo, pr_number) return SendResult(success=True) @@ -748,14 +755,26 @@ class WebhookAdapter(BasePlatformAdapter): logger.error("[webhook] github_comment delivery error: %s", e) return SendResult(success=False, error=str(e)) - def _find_adapter(self, target_platform: Platform): - """Default adapters first; multiplex may park a platform only on a secondary profile (_profile_adapters).""" - if adapter := self.gateway_runner.adapters.get(target_platform): - return adapter - for amap in (getattr(self.gateway_runner, "_profile_adapters", None) or {}).values(): - if isinstance(amap, dict) and amap.get(target_platform) is not None: - return amap[target_platform] - return None + def _github_env(self, profile: Optional[str]) -> Optional[dict]: + """``gh`` environment for a delivery: a routed profile authenticates with ITS ``GH_TOKEN`` / + ``GITHUB_TOKEN`` from the profile secret scope; under multiplex ``os.environ`` carries the default + profile's, so those keys are dropped when the profile has none (fail closed, ``gh`` then falls to + its own stored login). ``None`` (inherit) for bare/default-bound routes.""" + if not profile or not isinstance(profile, str) or profile == "default": + return None + from agent.secret_scope import get_secret + env = {k: v for k, v in os.environ.items() if k not in _GH_TOKEN_VARS} + with self._profile_scope(profile): + for name in _GH_TOKEN_VARS: + if value := get_secret(name): + env[name] = value + return env + + def _find_adapter(self, target_platform: Platform, profile: Optional[str]): + """The routed profile's own adapter, fail-closed. A ``/p//`` route must never post as + another profile's bot, and a bare (default-bound) route must not borrow a platform parked only on + a secondary profile — both directions leaked before #65939.""" + return self.gateway_runner._authorization_adapter(target_platform, profile) async def _deliver_cross_platform(self, platform_name: str, content: str, delivery: dict) -> SendResult: """Route response to another platform (telegram, discord, etc.).""" @@ -765,14 +784,26 @@ class WebhookAdapter(BasePlatformAdapter): target_platform = Platform(platform_name) except ValueError: return SendResult(success=False, error=f"Unknown platform: {platform_name}") - if not (adapter := self._find_adapter(target_platform)): + profile = delivery.get("profile") + if not (adapter := self._find_adapter(target_platform, profile)): return SendResult(success=False, error=f"Platform {platform_name} not connected") extra = delivery.get("deliver_extra", {}) chat_id = extra.get("chat_id", "") - if not chat_id: - home = self.gateway_runner.config.get_home_channel(target_platform) - if not home: - return SendResult(success=False, error=f"No chat_id or home channel for {platform_name}") - chat_id = home.chat_id - thread_id = extra.get("message_thread_id") or extra.get("thread_id") # Telegram forum topics - return await adapter.send(chat_id, content, metadata={"thread_id": thread_id} if thread_id else None) + # Whole leg under the routed profile's scope: the home channel comes from THAT profile's config + # (``self.gateway_runner.config`` is the default profile's), and the adapter's send reads its + # credentials through the profile secret scope. + with self._profile_scope(profile): + if not chat_id: + home = self._delivery_config(profile).get_home_channel(target_platform) + if not home: + return SendResult(success=False, error=f"No chat_id or home channel for {platform_name}") + chat_id = home.chat_id + thread_id = extra.get("message_thread_id") or extra.get("thread_id") # Telegram forum topics + return await adapter.send(chat_id, content, metadata={"thread_id": thread_id} if thread_id else None) + + def _delivery_config(self, profile: Optional[str]): + """Gateway config of the profile a delivery is bound to (call inside ``_profile_scope``).""" + if not profile or not isinstance(profile, str): + return self.gateway_runner.config + from gateway.config import load_gateway_config + return load_gateway_config() diff --git a/tests/gateway/test_webhook_adapter.py b/tests/gateway/test_webhook_adapter.py index 7aee89d9b4..95d5398ad9 100644 --- a/tests/gateway/test_webhook_adapter.py +++ b/tests/gateway/test_webhook_adapter.py @@ -714,6 +714,7 @@ class TestWebhookSilenceSuppression: mock_target.send = AsyncMock(return_value=SendResult(success=True)) mock_runner = MagicMock() mock_runner.adapters = {Platform("telegram"): mock_target} + mock_runner._authorization_adapter = lambda platform, profile=None: mock_runner.adapters.get(platform) mock_runner.config.get_home_channel.return_value = None adapter.gateway_runner = mock_runner @@ -841,6 +842,7 @@ class TestDeliverCrossPlatformThreadId: mock_runner = MagicMock() mock_runner.adapters = {Platform("telegram"): mock_target} + mock_runner._authorization_adapter = lambda platform, profile=None: mock_runner.adapters.get(platform) mock_runner.config.get_home_channel.return_value = None adapter.gateway_runner = mock_runner diff --git a/tests/gateway/test_webhook_deliver_only.py b/tests/gateway/test_webhook_deliver_only.py index 0bd5a190d0..a63f310dcf 100644 --- a/tests/gateway/test_webhook_deliver_only.py +++ b/tests/gateway/test_webhook_deliver_only.py @@ -52,6 +52,7 @@ def _wire_mock_target(adapter: WebhookAdapter, platform_name: str = "telegram"): mock_runner = MagicMock() mock_runner.adapters = {Platform(platform_name): mock_target} + mock_runner._authorization_adapter = lambda platform, profile=None: mock_runner.adapters.get(platform) mock_runner.config.get_home_channel.return_value = None adapter.gateway_runner = mock_runner diff --git a/tests/gateway/test_webhook_integration.py b/tests/gateway/test_webhook_integration.py index 2fae386b6a..0f3adc0d1a 100644 --- a/tests/gateway/test_webhook_integration.py +++ b/tests/gateway/test_webhook_integration.py @@ -233,6 +233,7 @@ class TestCrossPlatformDelivery: mock_runner = MagicMock() mock_runner.adapters = {Platform.TELEGRAM: mock_tg_adapter} + mock_runner._authorization_adapter = lambda platform, profile=None: mock_runner.adapters.get(platform) mock_runner.config = GatewayConfig( platforms={Platform.TELEGRAM: PlatformConfig(enabled=True, token="fake")} ) @@ -335,6 +336,7 @@ class TestGitHubCommentDelivery: encoding="utf-8", errors="replace", timeout=30, + env=None, ) # Delivery info is retained after send() so interim status messages # don't strand the final response (TTL-based cleanup happens on POST). diff --git a/tests/gateway/test_webhook_profile_egress.py b/tests/gateway/test_webhook_profile_egress.py new file mode 100644 index 0000000000..a0a5c70189 --- /dev/null +++ b/tests/gateway/test_webhook_profile_egress.py @@ -0,0 +1,125 @@ +"""Webhook egress under multiplex stays bound to the routed profile (#65939, #84266). + +A ``/p//`` route's reply, deliver_only message, home-channel fallback and +``github_comment`` credential all belong to THAT profile; a default-bound route never +borrows a secondary's adapter. Real ``GatewayAuthorizationMixin`` resolver, real +``load_gateway_config`` against a temp HERMES_HOME — no patched predicates. +""" +import os +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from gateway.authz_mixin import GatewayAuthorizationMixin +from gateway.config import GatewayConfig, HomeChannel, Platform, PlatformConfig +from gateway.platforms.api_server import APIServerAdapter, _api_request_profile +from gateway.platforms.base import SendResult +from gateway.platforms.webhook import WebhookAdapter + + +class _Runner(GatewayAuthorizationMixin): + def __init__(self, adapters, profile_adapters, config=None): + self.adapters = adapters + self._profile_adapters = profile_adapters + self._primary_profile_name = "default" + self.config = config or GatewayConfig() + + +def _target(): + t = MagicMock() + t.send = AsyncMock(return_value=SendResult(success=True)) + return t + + +def _webhook(runner) -> WebhookAdapter: + adapter = WebhookAdapter(PlatformConfig(enabled=True, extra={"host": "127.0.0.1", "port": 0, "routes": {}})) + adapter.gateway_runner = runner + return adapter + + +@pytest.fixture +def profile_homes(tmp_path, monkeypatch): + """Default home with a DEFAULT-HOME Slack home channel; ``profiles/sec`` with SEC-HOME + its own GH_TOKEN.""" + home = tmp_path / ".hermes" + sec = home / "profiles" / "sec" + sec.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr("hermes_cli.profiles._get_default_hermes_home", lambda: home) + monkeypatch.setattr("hermes_cli.profiles._get_profiles_root", lambda: home / "profiles") + (sec / "config.yaml").write_text( + "gateway:\n multiplex_profiles: true\nplatforms:\n slack:\n enabled: true\n" + " home_channel:\n platform: slack\n chat_id: SEC-HOME\n") + (sec / ".env").write_text("GH_TOKEN=sec-token\n") + monkeypatch.setenv("GH_TOKEN", "default-token") # multiplex: os.environ == the default profile + default_cfg = GatewayConfig() + default_cfg.platforms[Platform.SLACK] = PlatformConfig( + enabled=True, home_channel=HomeChannel(platform=Platform.SLACK, chat_id="DEFAULT-HOME", name="Home")) + return default_cfg + + +@pytest.mark.asyncio +async def test_routed_profile_delivers_via_its_own_adapter_and_home_channel(profile_homes): + default, secondary = _target(), _target() + adapter = _webhook(_Runner({Platform.SLACK: default}, {"sec": {Platform.SLACK: secondary}}, profile_homes)) + + result = await adapter._deliver_cross_platform( + "slack", "hi", {"deliver": "slack", "deliver_extra": {}, "profile": "sec"}) + + assert result.success + default.send.assert_not_awaited() + assert secondary.send.await_args.args[0] == "SEC-HOME" + + +@pytest.mark.asyncio +async def test_delivery_fails_closed_instead_of_crossing_profiles(profile_homes): + """Neither direction may borrow: a secondary route without the platform must not use the default + bot, and a default-bound route must not use a platform parked only on a secondary.""" + default, secondary = _target(), _target() + + sec_without_slack = _webhook(_Runner({Platform.SLACK: default}, {"sec": {}}, profile_homes)) + res = await sec_without_slack._deliver_cross_platform( + "slack", "hi", {"deliver": "slack", "deliver_extra": {"chat_id": "C1"}, "profile": "sec"}) + assert not res.success and "not connected" in res.error + default.send.assert_not_awaited() + + default_without_slack = _webhook(_Runner({}, {"sec": {Platform.SLACK: secondary}}, profile_homes)) + res = await default_without_slack._deliver_cross_platform( + "slack", "hi", {"deliver": "slack", "deliver_extra": {"chat_id": "C1"}, "profile": None}) + assert not res.success and "not connected" in res.error + secondary.send.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_github_comment_authenticates_with_routed_profile_token(profile_homes, monkeypatch): + seen = {} + + def fake_run(cmd, **kw): + seen["GH_TOKEN"] = kw["env"].get("GH_TOKEN") if kw.get("env") is not None else os.environ.get("GH_TOKEN") + return MagicMock(returncode=0, stderr="") + + monkeypatch.setattr("gateway.platforms.webhook.subprocess.run", fake_run) + adapter = _webhook(_Runner({}, {"sec": {}}, profile_homes)) + + res = await adapter._deliver_github_comment( + "body", {"deliver": "github_comment", "profile": "sec", "deliver_extra": {"repo": "o/r", "pr_number": "7"}}) + + assert res.success + assert seen["GH_TOKEN"] == "sec-token" + + +def test_api_server_profile_callback_resolves_routed_profile_adapter_fail_closed(): + default, secondary = object(), object() + api = APIServerAdapter(PlatformConfig(enabled=True, extra={"port": 0})) + request = MagicMock() + request.app = {} + + api.gateway_runner = _Runner({Platform("google_chat"): default}, {"sec": {Platform("google_chat"): secondary}}) + token = _api_request_profile.set("sec") + try: + assert api._get_platform_callback_adapter(request, "google_chat") is secondary + api.gateway_runner = _Runner({Platform("google_chat"): default}, {"sec": {}}) + assert api._get_platform_callback_adapter(request, "google_chat") is None + finally: + _api_request_profile.reset(token) + # No prefix: the primary map, unchanged. + assert api._get_platform_callback_adapter(request, "google_chat") is default diff --git a/website/docs/user-guide/multi-profile-gateways.md b/website/docs/user-guide/multi-profile-gateways.md index 7cc7b976d8..79636a2173 100644 --- a/website/docs/user-guide/multi-profile-gateways.md +++ b/website/docs/user-guide/multi-profile-gateways.md @@ -171,6 +171,16 @@ using the default listener's existing credentials. `/p/coder/webhooks/` and is rejected on every other profile prefix. - Webhook routes without `profile` remain default-profile routes and are not reachable through a named profile prefix. +- Delivery follows the same binding. A `profile: coder` route's reply (or + `deliver_only` message) goes out through **coder's** adapter for the + `deliver` platform, falls back to **coder's** home channel when + `deliver_extra.chat_id` is unset, and a `github_comment` delivery runs `gh` + with `GH_TOKEN` / `GITHUB_TOKEN` from `profiles/coder/.env`. If coder has no + adapter for that platform the delivery fails (502) rather than posting as + another profile's bot; a default route likewise never borrows a platform that + is enabled only on a secondary profile. +- `/p/coder/api/platforms//events` callbacks are verified and + dispatched by coder's adapter; when coder has none the callback is a 503. Keep port-binding platforms disabled in secondary profile configs. The shared listener and its route definitions stay on the default profile; profile