mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-27 11:22:03 +00:00
fix(relay): authorize relay-delivered events by delivery, not source.platform (#52306)
* fix(relay): authorize relay-delivered events by delivery, not source.platform The #52190 upstream-authz fix keyed _is_user_authorized off source.platform via _adapter_authorization_is_upstream(source.platform). But a relay *message* inbound carries the UNDERLYING platform (source.platform == discord/telegram/...), NOT Platform.RELAY, because ws_transport._event_from_wire maps the connector's wire payload (platform="discord") straight onto SessionSource for session-keying and egress. The relay adapter is registered only under Platform.RELAY, so adapters.get(Platform.DISCORD) misses, the trusted-upstream branch is skipped, and the user hits the env-allowlist default-deny: WARNING gateway.run: Unauthorized user: <id> (<name>) on discord (Live staging bug: alpha tester linked successfully, then every follow-up DM was silently dropped.) Fix: the authentic trust signal is that the event was delivered over the per-instance-authenticated relay WS, not which platform it underlies. Add a wire-INVISIBLE SessionSource.delivered_via_upstream_relay flag, stamped by the relay transport in _event_from_wire, and authorize on it. The flag is excluded from to_dict/from_dict so a peer can neither forge it across the wire nor have it restored from persistence. The existing adapter-flag check is retained for events whose source.platform IS Platform.RELAY (interaction-passthrough). A direct Discord event on a multiplexing gateway (direct + relay adapters) is unmarked and still default-denies. * fix(relay): use identity check on delivery marker to avoid MagicMock fail-open A MagicMock() source (used by test_signal.py and other gateway tests) auto- vivifies source.delivered_via_upstream_relay as a truthy Mock, which a bare truthiness check would treat as authorized — flipping test_signal_in_allowlist_maps from False to True. The marker is a real bool on SessionSource, so check 'is True' explicitly: refuses to authorize any non-bool stand-in, defensive against accidental fail-open.
This commit is contained in:
parent
0c442fa1d3
commit
72ae163250
4 changed files with 143 additions and 6 deletions
|
|
@ -223,11 +223,28 @@ class GatewayAuthorizationMixin:
|
|||
# connector observed, never gateway-asserted). There is no local
|
||||
# RELAY_ALLOWED_USERS env allowlist to consult, and default-denying for
|
||||
# its absence is the bug this branch fixes. This is delegation to a
|
||||
# trusted upstream, NOT a fail-open: it fires only for an adapter that
|
||||
# explicitly sets authorization_is_upstream=True; every direct
|
||||
# network-exposed adapter leaves it False and the env-allowlist
|
||||
# default-deny below still applies unchanged.
|
||||
if self._adapter_authorization_is_upstream(source.platform):
|
||||
# trusted upstream, NOT a fail-open: it fires only for an event that was
|
||||
# actually delivered over the authenticated relay WS (the transport
|
||||
# stamps ``delivered_via_upstream_relay``), or whose platform's adapter
|
||||
# explicitly declares ``authorization_is_upstream=True``; every direct
|
||||
# network-exposed adapter leaves the flag False and its events unmarked,
|
||||
# so the env-allowlist default-deny below still applies unchanged.
|
||||
#
|
||||
# The delivery marker is the PRIMARY signal: a relay *message* inbound
|
||||
# carries the UNDERLYING platform (``source.platform`` == discord/…),
|
||||
# NOT ``Platform.RELAY``, because that's what session-keying and egress
|
||||
# need — so keying authz off ``source.platform`` would miss (the relay
|
||||
# adapter is registered under ``Platform.RELAY``) and default-deny the
|
||||
# user ("Unauthorized user <id> on discord"). The adapter-flag check is
|
||||
# retained for events whose ``source.platform`` IS ``Platform.RELAY``
|
||||
# (e.g. the interaction-passthrough path).
|
||||
# ``is True`` (not just truthiness): the marker is a real bool on a
|
||||
# SessionSource, and an explicit identity check refuses to authorize a
|
||||
# non-bool stand-in (e.g. a MagicMock attribute auto-vivifies truthy in
|
||||
# tests) — defensive against accidental fail-open.
|
||||
if source.delivered_via_upstream_relay is True or self._adapter_authorization_is_upstream(
|
||||
source.platform
|
||||
):
|
||||
return True
|
||||
|
||||
user_id = source.user_id
|
||||
|
|
|
|||
|
|
@ -120,6 +120,14 @@ def _event_from_wire(raw: Dict[str, Any]) -> MessageEvent:
|
|||
guild_id=src.get("guild_id"),
|
||||
parent_chat_id=src.get("parent_chat_id"),
|
||||
message_id=src.get("message_id"),
|
||||
# Authentic upstream-trust signal: this event arrived over the
|
||||
# per-instance-authenticated relay WS, so the connector already resolved
|
||||
# it to this instance's owner-bound author. ``platform`` is the
|
||||
# UNDERLYING platform (e.g. discord), not ``relay`` — authz keys the
|
||||
# upstream-trust decision off THIS flag, not off ``platform`` (which
|
||||
# would miss because the relay adapter is registered under
|
||||
# ``Platform.RELAY``). Stamped here, never read off the wire.
|
||||
delivered_via_upstream_relay=True,
|
||||
)
|
||||
try:
|
||||
msg_type = MessageType(raw.get("message_type", "text"))
|
||||
|
|
|
|||
|
|
@ -119,7 +119,20 @@ class SessionSource:
|
|||
# None => the gateway's active/default profile. Drives both session-key
|
||||
# namespacing and the per-turn config/credential scope.
|
||||
profile: Optional[str] = None
|
||||
|
||||
|
||||
# Internal, wire-INVISIBLE trust signal: True when this event was delivered
|
||||
# to the gateway over the per-instance-authenticated relay WebSocket (the
|
||||
# Team Gateway connector). The connector authenticates the gateway's socket
|
||||
# with a per-instance secret and resolves owner-only author bindings BEFORE
|
||||
# delivering, so a relay-delivered event is already authorized as this
|
||||
# instance's bound user. ``platform`` carries the UNDERLYING platform
|
||||
# (e.g. ``discord``) for session-keying/egress, NOT ``relay`` — so authz
|
||||
# must key the upstream-trust decision off THIS flag, not off ``platform``.
|
||||
# Set locally by the relay transport (``ws_transport._event_from_wire``);
|
||||
# deliberately excluded from ``to_dict``/``from_dict`` so a peer can never
|
||||
# forge it across the wire or have it restored from persistence.
|
||||
delivered_via_upstream_relay: bool = False
|
||||
|
||||
@property
|
||||
def description(self) -> str:
|
||||
"""Human-readable description of the source."""
|
||||
|
|
|
|||
|
|
@ -144,3 +144,102 @@ def test_upstream_authz_helper_false_for_unknown_platform(monkeypatch):
|
|||
# A platform with no registered adapter must not be treated as upstream-authz.
|
||||
assert runner._adapter_authorization_is_upstream(Platform.DISCORD) is False
|
||||
assert runner._adapter_authorization_is_upstream(None) is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The underlying-platform regression: a relay *message* inbound carries the
|
||||
# UNDERLYING platform (source.platform == Platform.DISCORD), not Platform.RELAY,
|
||||
# because the connector's wire payload sets platform="discord" and
|
||||
# ws_transport._event_from_wire maps it straight onto SessionSource. The relay
|
||||
# adapter is registered ONLY under Platform.RELAY, so keying upstream-authz off
|
||||
# source.platform misses and the user hits default-deny ("Unauthorized user
|
||||
# <id> (<name>) on discord"). The authentic trust signal is that the event was
|
||||
# delivered over the per-instance-authenticated relay WS — carried by
|
||||
# source.delivered_via_upstream_relay, set by the relay transport, NOT by
|
||||
# source.platform.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_relay_message_with_underlying_discord_platform_authorized(monkeypatch):
|
||||
"""The live message path: source.platform=DISCORD + relay-delivered marker.
|
||||
|
||||
Reproduces the staging bug exactly — a relay-delivered Discord message whose
|
||||
source.platform is the underlying "discord" (not "relay"). The relay adapter
|
||||
is registered only under Platform.RELAY, so the OLD source.platform-keyed
|
||||
check missed and default-denied. Authorization must come from the
|
||||
relay-delivery marker instead.
|
||||
"""
|
||||
_clear_auth_env(monkeypatch)
|
||||
runner, _ = _make_runner(platform=Platform.RELAY, authorization_is_upstream=True)
|
||||
src = SessionSource(
|
||||
platform=Platform.DISCORD, # underlying platform off the wire
|
||||
user_id="267171776755269633",
|
||||
chat_id="1400724139874058314",
|
||||
user_name="rewbs",
|
||||
chat_type="dm",
|
||||
delivered_via_upstream_relay=True,
|
||||
)
|
||||
assert runner._is_user_authorized(src) is True
|
||||
|
||||
|
||||
def test_direct_discord_event_not_authorized_by_relay_presence(monkeypatch):
|
||||
"""A DIRECT Discord event must NOT be authorized just because a relay adapter
|
||||
is registered (multiplexing gateway: direct Discord adapter + relay adapter).
|
||||
|
||||
Without the delivery marker, the relay's upstream-authz must not leak onto a
|
||||
direct Discord inbound — that would be a fail-open. Only events the relay
|
||||
transport actually delivered carry delivered_via_upstream_relay=True.
|
||||
"""
|
||||
_clear_auth_env(monkeypatch)
|
||||
runner, _ = _make_runner(platform=Platform.RELAY, authorization_is_upstream=True)
|
||||
src = SessionSource(
|
||||
platform=Platform.DISCORD,
|
||||
user_id="999",
|
||||
chat_id="456",
|
||||
user_name="direct_discord_user",
|
||||
chat_type="dm",
|
||||
# delivered_via_upstream_relay defaults to False (direct delivery)
|
||||
)
|
||||
assert runner._is_user_authorized(src) is False
|
||||
|
||||
|
||||
def test_relay_delivery_marker_is_wire_invisible():
|
||||
"""delivered_via_upstream_relay is an INTERNAL trust signal, never serialized.
|
||||
|
||||
It must not appear in to_dict() (the wire/persistence surface) — it is set
|
||||
locally by the relay transport from the authenticated socket, never trusted
|
||||
off the wire.
|
||||
"""
|
||||
src = SessionSource(
|
||||
platform=Platform.DISCORD,
|
||||
chat_id="1",
|
||||
user_id="2",
|
||||
delivered_via_upstream_relay=True,
|
||||
)
|
||||
assert "delivered_via_upstream_relay" not in src.to_dict()
|
||||
# And it does not survive a wire round-trip (a peer can't forge it).
|
||||
assert SessionSource.from_dict(src.to_dict()).delivered_via_upstream_relay is False
|
||||
|
||||
|
||||
def test_event_from_wire_sets_relay_delivery_marker():
|
||||
"""The relay transport stamps the marker on every event it rebuilds.
|
||||
|
||||
This is the authentic injection point: _event_from_wire only runs for frames
|
||||
that arrived over the per-instance-authenticated relay WS.
|
||||
"""
|
||||
from gateway.relay.ws_transport import _event_from_wire
|
||||
|
||||
event = _event_from_wire(
|
||||
{
|
||||
"text": "hello!",
|
||||
"source": {
|
||||
"platform": "discord",
|
||||
"chat_id": "123",
|
||||
"chat_type": "dm",
|
||||
"user_id": "267171776755269633",
|
||||
"user_name": "rewbs",
|
||||
},
|
||||
}
|
||||
)
|
||||
assert event.source.platform is Platform.DISCORD
|
||||
assert event.source.delivered_via_upstream_relay is True
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue