fix(security): fail closed when an own-policy gateway adapter has no allowlist
Own-policy adapters (WhatsApp, WeCom, Weixin, QQBot, Yuanbao) default dm_policy/group_policy to "open", which forwards every sender. The gateway's adapter-trust shortcut in _is_user_authorized blanket-trusted those platforms when no env allowlist was set, so an operator who enabled one with only credentials authorized the entire external network -- the fail-open SECURITY.md section 2.6 forbids ("an allowlist is required for every enabled network-exposed adapter").
Trust the adapter only when its effective policy for the chat type is an actual "allowlist" restriction (the case #34515 was protecting). "open"/"pairing"/anything else falls through to default-deny, where {PLATFORM}_ALLOW_ALL_USERS / GATEWAY_ALLOW_ALL_USERS and the pairing flow remain the explicit opt-ins.
This commit is contained in:
+69
-27
@@ -37,10 +37,12 @@ class GatewayAuthorizationMixin:
|
|||||||
Mirrors ``BasePlatformAdapter.enforces_own_access_policy``. Adapters
|
Mirrors ``BasePlatformAdapter.enforces_own_access_policy``. Adapters
|
||||||
such as WeCom, Weixin, Yuanbao, QQBot, and WhatsApp evaluate their
|
such as WeCom, Weixin, Yuanbao, QQBot, and WhatsApp evaluate their
|
||||||
documented ``dm_policy`` / ``group_policy`` / ``allow_from`` config before a
|
documented ``dm_policy`` / ``group_policy`` / ``allow_from`` config before a
|
||||||
message is dispatched to the gateway, so a message that reaches
|
message is dispatched to the gateway. The flag alone is NOT "already
|
||||||
``_is_user_authorized`` has already been authorized by the adapter.
|
authorized": these adapters default to ``open``, which forwards every
|
||||||
Defaults to ``False`` when the adapter is unknown or doesn't expose
|
sender, so ``_is_user_authorized`` only trusts the adapter when its
|
||||||
the flag.
|
effective policy for the chat type is an actual ``allowlist`` restriction
|
||||||
|
(see that method). Defaults to ``False`` when the adapter is unknown or
|
||||||
|
doesn't expose the flag.
|
||||||
"""
|
"""
|
||||||
if not platform:
|
if not platform:
|
||||||
return False
|
return False
|
||||||
@@ -65,10 +67,11 @@ class GatewayAuthorizationMixin:
|
|||||||
env var is not always bridged back into ``config.extra``) — and falls
|
env var is not always bridged back into ``config.extra``) — and falls
|
||||||
back to ``config.extra`` for bare runners built without a live adapter.
|
back to ``config.extra`` for bare runners built without a live adapter.
|
||||||
|
|
||||||
Used by ``_is_user_authorized`` to carve ``dm_policy: pairing`` out of
|
Used by ``_is_user_authorized`` to decide whether an own-policy adapter
|
||||||
the adapter-trust shortcut: in pairing mode the adapter forwards the DM
|
actually restricted DM senders to a configured allowlist (trustworthy)
|
||||||
so the gateway can run its pairing handshake, so "reached the gateway"
|
or merely forwarded everyone under ``dm_policy: open`` / for a pairing
|
||||||
must not be read as "authorized".
|
handshake (not authorization). "Reached the gateway" only carries an
|
||||||
|
authorization signal in the ``allowlist`` case.
|
||||||
"""
|
"""
|
||||||
if not platform:
|
if not platform:
|
||||||
return ""
|
return ""
|
||||||
@@ -87,6 +90,37 @@ class GatewayAuthorizationMixin:
|
|||||||
policy = extra.get("dm_policy")
|
policy = extra.get("dm_policy")
|
||||||
return str(policy or "").strip().lower()
|
return str(policy or "").strip().lower()
|
||||||
|
|
||||||
|
def _adapter_group_policy(self, platform: Optional[Platform]) -> str:
|
||||||
|
"""Best-effort read of an own-policy adapter's effective group policy.
|
||||||
|
|
||||||
|
Mirror of ``_adapter_dm_policy`` for group / forum / channel traffic:
|
||||||
|
returns the lowercased ``group_policy`` (``"open"`` / ``"allowlist"`` /
|
||||||
|
``"disabled"``) for *platform*, or ``""`` when unknown. Prefers the live
|
||||||
|
adapter's resolved ``_group_policy`` and falls back to ``config.extra``
|
||||||
|
for bare runners built without a live adapter.
|
||||||
|
|
||||||
|
Used by ``_is_user_authorized`` to decide whether an own-policy adapter
|
||||||
|
restricted group senders to a configured allowlist (trustworthy) or
|
||||||
|
forwarded the whole channel under ``group_policy: open`` (not
|
||||||
|
authorization).
|
||||||
|
"""
|
||||||
|
if not platform:
|
||||||
|
return ""
|
||||||
|
adapters = getattr(self, "adapters", None) or {}
|
||||||
|
adapter = adapters.get(platform)
|
||||||
|
policy = getattr(adapter, "_group_policy", None) if adapter is not None else None
|
||||||
|
if policy is None:
|
||||||
|
config = getattr(self, "config", None)
|
||||||
|
platform_cfg = (
|
||||||
|
config.platforms.get(platform)
|
||||||
|
if config is not None and hasattr(config, "platforms")
|
||||||
|
else None
|
||||||
|
)
|
||||||
|
extra = getattr(platform_cfg, "extra", None) if platform_cfg else None
|
||||||
|
if isinstance(extra, dict):
|
||||||
|
policy = extra.get("group_policy")
|
||||||
|
return str(policy or "").strip().lower()
|
||||||
|
|
||||||
def _is_user_authorized(self, source: SessionSource) -> bool:
|
def _is_user_authorized(self, source: SessionSource) -> bool:
|
||||||
"""
|
"""
|
||||||
Check if a user is authorized to use the bot.
|
Check if a user is authorized to use the bot.
|
||||||
@@ -237,27 +271,35 @@ class GatewayAuthorizationMixin:
|
|||||||
global_allowlist = os.getenv("GATEWAY_ALLOWED_USERS", "").strip()
|
global_allowlist = os.getenv("GATEWAY_ALLOWED_USERS", "").strip()
|
||||||
|
|
||||||
if not platform_allowlist and not group_user_allowlist and not group_chat_allowlist and not global_allowlist:
|
if not platform_allowlist and not group_user_allowlist and not group_chat_allowlist and not global_allowlist:
|
||||||
# No env allowlists configured. Adapters that own their own
|
# No env allowlist configured. Adapters that own their own
|
||||||
# config-driven access policy (dm_policy / group_policy /
|
# config-driven access policy (dm_policy / group_policy /
|
||||||
# allow_from / group_allow_from) already gated this message at
|
# allow_from / group_allow_from) gate access at intake, so for those
|
||||||
# intake — it would not have reached the gateway otherwise — so
|
# platforms we can honor the adapter's decision instead of the
|
||||||
# honor that decision instead of falling through to the
|
# env-only default-deny below -- but ONLY when that decision was an
|
||||||
# env-only default-deny below, which would silently break
|
# actual allowlist restriction.
|
||||||
# `dm_policy: open` and config-only allowlists. (#34515)
|
#
|
||||||
|
# The adapters default dm_policy / group_policy to "open", which
|
||||||
|
# forwards EVERY sender. Reading "reached the gateway" as
|
||||||
|
# authorization in that case would admit the whole external network
|
||||||
|
# with no operator-configured allowlist -- the fail-open SECURITY.md
|
||||||
|
# §2.6 forbids ("an allowlist is required for every enabled
|
||||||
|
# network-exposed adapter ... code paths that fail open when no
|
||||||
|
# allowlist is configured are code bugs"). "disabled" never
|
||||||
|
# forwards, and "pairing" forwards unpaired DMs only so the gateway
|
||||||
|
# can run its pairing handshake (the pairing-store check above
|
||||||
|
# already denied this sender). So trust the adapter only when its
|
||||||
|
# effective policy for THIS chat type is "allowlist"; for "open" /
|
||||||
|
# "pairing" / anything else, fall through to default-deny, where
|
||||||
|
# GATEWAY_ALLOW_ALL_USERS, the per-platform {PLATFORM}_ALLOW_ALL_USERS
|
||||||
|
# flag (checked above), and the pairing flow remain the explicit
|
||||||
|
# opt-ins to broader access. (#34515 follow-up: trusting "open" was a
|
||||||
|
# fail-open.)
|
||||||
if self._adapter_enforces_own_access_policy(source.platform):
|
if self._adapter_enforces_own_access_policy(source.platform):
|
||||||
# Exception: `dm_policy: pairing` does NOT authorize at intake.
|
if source.chat_type in {"group", "forum", "channel"}:
|
||||||
# The adapter forwards the DM precisely so the gateway can run
|
effective_policy = self._adapter_group_policy(source.platform)
|
||||||
# its pairing handshake (issue a code, consult the pairing
|
else:
|
||||||
# store). The pairing-store approval check above already ran and
|
effective_policy = self._adapter_dm_policy(source.platform)
|
||||||
# returned False for this sender, so blanket-trusting the
|
if effective_policy == "allowlist":
|
||||||
# adapter here would silently turn pairing mode into open
|
|
||||||
# access. Fall through to default-deny so the unpaired sender is
|
|
||||||
# offered a pairing code instead. (Pairing is DM-only; group
|
|
||||||
# traffic keeps the adapter-trust path.)
|
|
||||||
if not (
|
|
||||||
source.chat_type == "dm"
|
|
||||||
and self._adapter_dm_policy(source.platform) == "pairing"
|
|
||||||
):
|
|
||||||
return True
|
return True
|
||||||
# No allowlists configured -- check global allow-all flag
|
# No allowlists configured -- check global allow-all flag
|
||||||
return os.getenv("GATEWAY_ALLOW_ALL_USERS", "").lower() in {"true", "1", "yes"}
|
return os.getenv("GATEWAY_ALLOW_ALL_USERS", "").lower() in {"true", "1", "yes"}
|
||||||
|
|||||||
@@ -1916,16 +1916,21 @@ class BasePlatformAdapter(ABC):
|
|||||||
enforce it at intake: a message is dropped inside the adapter and never
|
enforce it at intake: a message is dropped inside the adapter and never
|
||||||
reaches the gateway unless it already passed that policy.
|
reaches the gateway unless it already passed that policy.
|
||||||
|
|
||||||
The gateway's env-based allowlist check runs *after* the adapter, so for
|
The gateway's env-based allowlist check runs *after* the adapter. When
|
||||||
these platforms a message arriving at ``_is_user_authorized`` has, by
|
no env allowlist is configured, the gateway consults this flag so it can
|
||||||
definition, already been authorized by the adapter. Without this flag the
|
honor a config-only ``dm_policy: allowlist`` / ``allow_from`` (which the
|
||||||
gateway would then deny it again (no env allowlist → default deny),
|
adapter already enforced) instead of double-denying it. Crucially, the
|
||||||
silently breaking ``dm_policy: open`` and config-only allowlists.
|
flag alone is NOT "already authorized": these adapters default
|
||||||
|
``dm_policy`` / ``group_policy`` to ``"open"``, which forwards every
|
||||||
|
sender, so the gateway trusts the adapter only when its effective policy
|
||||||
|
for the chat type is an actual ``"allowlist"`` restriction — never for
|
||||||
|
``"open"`` (that would be the network-exposed fail-open SECURITY.md §2.6
|
||||||
|
forbids). Open access still requires an explicit
|
||||||
|
``{PLATFORM}_ALLOW_ALL_USERS`` / ``GATEWAY_ALLOW_ALL_USERS`` opt-in.
|
||||||
|
|
||||||
Adapters that own their access policy override this to return ``True``.
|
Adapters that own their access policy override this to return ``True``.
|
||||||
The gateway treats that as "already authorized at intake" and skips the
|
Adapters that delegate access control to the gateway leave it ``False``
|
||||||
env-allowlist default-deny. Adapters that delegate access control to the
|
(the default).
|
||||||
gateway leave it ``False`` (the default).
|
|
||||||
"""
|
"""
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
|||||||
@@ -8,16 +8,21 @@ a message is dropped inside the adapter and never reaches the gateway unless it
|
|||||||
already passed that policy.
|
already passed that policy.
|
||||||
|
|
||||||
The gateway's env-based allowlist check (``_is_user_authorized``) runs *after*
|
The gateway's env-based allowlist check (``_is_user_authorized``) runs *after*
|
||||||
the adapter. Before the fix it fell through to an env-only default-deny when no
|
the adapter. Adapters that own their access policy declare
|
||||||
``PLATFORM_ALLOWED_USERS`` env var was set, silently rejecting ``dm_policy:
|
``enforces_own_access_policy`` (a ``BasePlatformAdapter`` property, default
|
||||||
open`` and config-only allowlists even though the adapter had already
|
``False``) so the gateway can honor a config-only ``dm_policy: allowlist`` /
|
||||||
authorized the sender.
|
``allow_from`` (which the adapter already enforced) instead of double-denying it
|
||||||
|
when no ``PLATFORM_ALLOWED_USERS`` env var is set.
|
||||||
|
|
||||||
The fix is a single drift-proof contract: adapters that own their access policy
|
Crucially, the flag is NOT a blanket "already authorized" pass. These adapters
|
||||||
declare ``enforces_own_access_policy`` (a ``BasePlatformAdapter`` property,
|
default ``dm_policy`` / ``group_policy`` to ``"open"``, which forwards *every*
|
||||||
default ``False``). The gateway trusts that flag and skips the env-only
|
sender, so the gateway trusts the adapter only when its effective policy for the
|
||||||
default-deny for those platforms, rather than re-implementing each adapter's
|
chat type is an actual ``"allowlist"`` restriction. Trusting ``"open"`` here
|
||||||
policy logic a second time.
|
admitted the whole external network with no operator-configured allowlist — the
|
||||||
|
fail-open SECURITY.md §2.6 forbids for network-exposed adapters ("an allowlist
|
||||||
|
is required for every enabled network-exposed adapter ... code paths that fail
|
||||||
|
open when no allowlist is configured are code bugs"). Open access requires an
|
||||||
|
explicit ``{PLATFORM}_ALLOW_ALL_USERS`` / ``GATEWAY_ALLOW_ALL_USERS`` opt-in.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from types import SimpleNamespace
|
from types import SimpleNamespace
|
||||||
@@ -128,15 +133,16 @@ def test_own_policy_adapters_declare_the_flag(module_path, class_name):
|
|||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
||||||
def test_own_policy_platform_authorized_without_env_allowlist(monkeypatch, platform):
|
def test_own_policy_allowlist_authorized_without_env_allowlist(monkeypatch, platform):
|
||||||
"""A message reaching the gateway from an own-policy adapter is trusted.
|
"""A config-only ``dm_policy: allowlist`` is trusted without an env allowlist.
|
||||||
|
|
||||||
With no env allowlist set, the gateway must NOT default-deny — the adapter
|
The adapter only forwards an allowlisted sender under ``allowlist`` policy,
|
||||||
already authorized the sender at intake (e.g. ``dm_policy: open``).
|
so a message reaching the gateway *was* authorized for this specific sender.
|
||||||
|
The gateway must honor that instead of double-denying (the #34515 case).
|
||||||
"""
|
"""
|
||||||
_clear_auth_env(monkeypatch)
|
_clear_auth_env(monkeypatch)
|
||||||
config = GatewayConfig(
|
config = GatewayConfig(
|
||||||
platforms={platform: PlatformConfig(enabled=True, extra={"dm_policy": "open"})}
|
platforms={platform: PlatformConfig(enabled=True, extra={"dm_policy": "allowlist"})}
|
||||||
)
|
)
|
||||||
runner, _adapter = _make_runner(platform, config, enforces=True)
|
runner, _adapter = _make_runner(platform, config, enforces=True)
|
||||||
|
|
||||||
@@ -144,15 +150,61 @@ def test_own_policy_platform_authorized_without_env_allowlist(monkeypatch, platf
|
|||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
||||||
def test_own_policy_platform_authorized_for_group_chat(monkeypatch, platform):
|
def test_own_policy_open_dm_not_authorized_without_allowlist(monkeypatch, platform):
|
||||||
"""Group traffic from an own-policy adapter is trusted the same way."""
|
"""``dm_policy: open`` forwards everyone → NOT authorization (SECURITY.md §2.6).
|
||||||
|
|
||||||
|
With no env allowlist and no per-platform allow-all flag, an own-policy
|
||||||
|
adapter running ``open`` (the default) must NOT fail open: the gateway falls
|
||||||
|
through to default-deny so the whole external network can't reach the agent.
|
||||||
|
"""
|
||||||
|
_clear_auth_env(monkeypatch)
|
||||||
|
config = GatewayConfig(
|
||||||
|
platforms={platform: PlatformConfig(enabled=True, extra={"dm_policy": "open"})}
|
||||||
|
)
|
||||||
|
runner, _adapter = _make_runner(platform, config, enforces=True)
|
||||||
|
|
||||||
|
assert runner._is_user_authorized(_source(platform)) is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
||||||
|
def test_own_policy_default_open_dm_is_fail_closed(monkeypatch, platform):
|
||||||
|
"""The adapters' *default* ``open`` policy (no config at all) fails closed.
|
||||||
|
|
||||||
|
Operators who enable an own-policy adapter with only credentials get
|
||||||
|
``dm_policy = "open"`` resolved on the live adapter. Simulate that resolved
|
||||||
|
state (empty config.extra, adapter ``_dm_policy = "open"``) and confirm the
|
||||||
|
gateway denies — the do-nothing default must not be open to the world.
|
||||||
|
"""
|
||||||
|
_clear_auth_env(monkeypatch)
|
||||||
|
config = GatewayConfig(platforms={platform: PlatformConfig(enabled=True, extra={})})
|
||||||
|
runner, adapter = _make_runner(platform, config, enforces=True)
|
||||||
|
adapter._dm_policy = "open" # as the live adapter resolves the default
|
||||||
|
|
||||||
|
assert runner._is_user_authorized(_source(platform)) is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
||||||
|
def test_own_policy_allowlist_authorized_for_group_chat(monkeypatch, platform):
|
||||||
|
"""A config-only ``group_policy: allowlist`` is trusted for group traffic."""
|
||||||
|
_clear_auth_env(monkeypatch)
|
||||||
|
config = GatewayConfig(
|
||||||
|
platforms={platform: PlatformConfig(enabled=True, extra={"group_policy": "allowlist"})}
|
||||||
|
)
|
||||||
|
runner, _adapter = _make_runner(platform, config, enforces=True)
|
||||||
|
|
||||||
|
assert runner._is_user_authorized(_source(platform, chat_type="group")) is True
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("platform", _OWN_POLICY_PLATFORMS)
|
||||||
|
def test_own_policy_open_group_not_authorized_without_allowlist(monkeypatch, platform):
|
||||||
|
"""``group_policy: open`` is the same fail-open class as DM open → deny."""
|
||||||
_clear_auth_env(monkeypatch)
|
_clear_auth_env(monkeypatch)
|
||||||
config = GatewayConfig(
|
config = GatewayConfig(
|
||||||
platforms={platform: PlatformConfig(enabled=True, extra={"group_policy": "open"})}
|
platforms={platform: PlatformConfig(enabled=True, extra={"group_policy": "open"})}
|
||||||
)
|
)
|
||||||
runner, _adapter = _make_runner(platform, config, enforces=True)
|
runner, _adapter = _make_runner(platform, config, enforces=True)
|
||||||
|
|
||||||
assert runner._is_user_authorized(_source(platform, chat_type="group")) is True
|
assert runner._is_user_authorized(_source(platform, chat_type="group")) is False
|
||||||
|
|
||||||
|
|
||||||
def test_non_owning_platform_still_default_denies(monkeypatch):
|
def test_non_owning_platform_still_default_denies(monkeypatch):
|
||||||
@@ -259,12 +311,16 @@ def test_pairing_carveout_reads_adapter_when_env_set(monkeypatch):
|
|||||||
|
|
||||||
|
|
||||||
def test_pairing_dm_policy_group_chat_still_trusted(monkeypatch):
|
def test_pairing_dm_policy_group_chat_still_trusted(monkeypatch):
|
||||||
"""Pairing is DM-only — group traffic keeps the adapter-trust path."""
|
"""Pairing is DM-only — the DM pairing carve-out doesn't gate group traffic.
|
||||||
|
|
||||||
|
Group access is governed by ``group_policy``, so an allowlisted group is
|
||||||
|
still trusted even while DMs are in ``pairing`` mode.
|
||||||
|
"""
|
||||||
_clear_auth_env(monkeypatch)
|
_clear_auth_env(monkeypatch)
|
||||||
config = GatewayConfig(
|
config = GatewayConfig(
|
||||||
platforms={
|
platforms={
|
||||||
Platform.WECOM: PlatformConfig(
|
Platform.WECOM: PlatformConfig(
|
||||||
enabled=True, extra={"dm_policy": "pairing", "group_policy": "open"}
|
enabled=True, extra={"dm_policy": "pairing", "group_policy": "allowlist"}
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
)
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user