fix(gateway): /p/<profile>/ webhook delivery and api_server callbacks stay on the routed profile
Under gateway.multiplex_profiles a /p/<profile>/ webhook route (or one with
`profile: <name>`) 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/<profile>/api/platforms/<platform>/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 <mjshorty@users.noreply.github.com>
Co-authored-by: StellarisW <stellarisw@users.noreply.github.com>
This commit is contained in:
@@ -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/<profile>/`` 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", ""))
|
||||
|
||||
@@ -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/<profile>/`` 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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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).
|
||||
|
||||
125
tests/gateway/test_webhook_profile_egress.py
Normal file
125
tests/gateway/test_webhook_profile_egress.py
Normal file
@@ -0,0 +1,125 @@
|
||||
"""Webhook egress under multiplex stays bound to the routed profile (#65939, #84266).
|
||||
|
||||
A ``/p/<profile>/`` 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
|
||||
@@ -171,6 +171,16 @@ using the default listener's existing credentials.
|
||||
`/p/coder/webhooks/<route>` 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/<platform>/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
|
||||
|
||||
Reference in New Issue
Block a user