From ddfc6342ad153751f734ae86dfaa3104c2e56b1b Mon Sep 17 00:00:00 2001 From: fangliquanflq Date: Sat, 1 Aug 2026 04:54:38 +0000 Subject: [PATCH] fix(gateway): revoke WhatsApp sole allowlist entry without restart Clear live adapter _allow_from on pairing revoke and re-check DM allowlist authz so sole-entry removal takes effect without restart. --- gateway/authz_mixin.py | 20 +++ gateway/pairing.py | 156 +++++++++++++++--- gateway/platforms/whatsapp_cloud.py | 7 +- gateway/platforms/whatsapp_common.py | 27 ++- plugins/platforms/whatsapp/adapter.py | 9 +- .../gateway/test_pairing_allowlist_bypass.py | 150 +++++++++++++++++ 6 files changed, 346 insertions(+), 23 deletions(-) diff --git a/gateway/authz_mixin.py b/gateway/authz_mixin.py index af366033c9bc8..49972eee67c8c 100644 --- a/gateway/authz_mixin.py +++ b/gateway/authz_mixin.py @@ -651,6 +651,26 @@ class GatewayAuthorizationMixin: profile=adapter_profile, ) if effective_policy == "allowlist": + # Trust allowlist intake only when the live adapter still + # allowlists this sender. Pairing revoke can clear + # WHATSAPP_ALLOWED_USERS while a construction-time + # ``_allow_from`` snapshot would otherwise keep authorizing + # until restart; re-check when the adapter exposes a DM + # allowlist helper. Adapters without that helper keep the + # historical "reached the gateway under allowlist policy" + # rubber-stamp (#34515). + if source.chat_type not in {"group", "forum", "channel"}: + adapter = self._authorization_adapter( + source.platform, + profile=adapter_profile, + ) + dm_check = ( + getattr(adapter, "_is_dm_allowed", None) + if adapter is not None + else None + ) + if callable(dm_check): + return bool(dm_check(user_id)) return True # Some adapters (e.g. Telegram) gate access via config.extra.allow_from / # group_allow_from at intake but do not override enforces_own_access_policy. diff --git a/gateway/pairing.py b/gateway/pairing.py index a7c872f951461..4ac42688fb3d5 100644 --- a/gateway/pairing.py +++ b/gateway/pairing.py @@ -113,6 +113,34 @@ def _split_allowlist(raw: str) -> list: return [uid.strip() for uid in raw.split(",") if uid.strip()] +def _normalize_user_id(platform: str, user_id: str) -> str: + """Normalize platform-specific user IDs before persisting / comparing them.""" + raw_user_id = str(user_id or "").strip() + if platform == "whatsapp": + return normalize_whatsapp_identifier(raw_user_id) or raw_user_id + return raw_user_id + + +def _user_id_aliases(platform: str, user_id: str) -> set[str]: + """Return all known equivalent user IDs for auth / allowlist matching.""" + raw_user_id = str(user_id or "").strip() + if not raw_user_id: + return set() + + aliases = {raw_user_id, _normalize_user_id(platform, raw_user_id)} + if platform == "whatsapp": + aliases.update(expand_whatsapp_aliases(raw_user_id)) + aliases.discard("") + return aliases + + +def _user_ids_match(platform: str, left: str, right: str) -> bool: + """Return True when two user IDs represent the same principal.""" + left_aliases = _user_id_aliases(platform, left) + right_aliases = _user_id_aliases(platform, right) + return bool(left_aliases and right_aliases and (left_aliases & right_aliases)) + + def _sync_allowlist_add(platform: str, user_id: str) -> None: """Add ``user_id`` to the platform allowlist env var IF one is configured. @@ -142,16 +170,118 @@ def _sync_allowlist_add(platform: str, user_id: str) -> None: pass +def _iter_live_gateway_adapters(): + """Yield adapters from the in-process GatewayRunner, if one is running.""" + try: + from gateway.run import _gateway_runner_ref + + runner = _gateway_runner_ref() + except Exception: + return + if runner is None: + return + adapters = getattr(runner, "adapters", None) or {} + for adapter in adapters.values(): + if adapter is not None: + yield adapter + profile_adapters = getattr(runner, "_profile_adapters", None) or {} + for mapping in profile_adapters.values(): + for adapter in (mapping or {}).values(): + if adapter is not None: + yield adapter + + +def _adapter_platform_name(adapter) -> str: + platform = getattr(adapter, "platform", None) + if platform is not None: + value = getattr(platform, "value", None) + if value: + return str(value).strip().lower() + name = getattr(adapter, "name", None) + return str(name or "").strip().lower() + + +def _purge_allowlist_entries(entries, platform: str, user_id: str): + """Drop alias-equivalent allowlist entries while preserving ``*``.""" + if entries is None: + return entries + if isinstance(entries, str): + parts = _split_allowlist(entries) + remaining = [ + part for part in parts + if part == "*" or not _user_ids_match(platform, part, str(user_id)) + ] + return ",".join(remaining) + if isinstance(entries, (set, frozenset)): + return { + entry for entry in entries + if str(entry).strip() == "*" + or not _user_ids_match(platform, str(entry), str(user_id)) + } + if isinstance(entries, (list, tuple)): + return [ + entry for entry in entries + if str(entry).strip() == "*" + or not _user_ids_match(platform, str(entry), str(user_id)) + ] + return entries + + +def _sync_live_adapter_allowlist_remove(platform: str, user_id: str) -> None: + """Clear revoked principals from in-process adapter allowlist snapshots. + + ``WhatsAppAdapter`` (and Cloud) snapshot ``_allow_from`` at construction. + Pairing revoke updates ``WHATSAPP_ALLOWED_USERS`` / cloud env, but when the + revoked principal was the sole entry the env key is removed entirely. + Intake must not keep authorizing from the stale snapshot until restart. + """ + platform_name = (platform or "").strip().lower() + if not platform_name or not str(user_id or "").strip(): + return + for adapter in _iter_live_gateway_adapters(): + if _adapter_platform_name(adapter) != platform_name: + continue + if hasattr(adapter, "_allow_from"): + try: + adapter._allow_from = _purge_allowlist_entries( + set(adapter._allow_from or ()), platform_name, user_id + ) + except Exception: + pass + extra = getattr(getattr(adapter, "config", None), "extra", None) + if isinstance(extra, dict) and "allow_from" in extra: + try: + extra["allow_from"] = _purge_allowlist_entries( + extra.get("allow_from"), platform_name, user_id + ) + except Exception: + pass + + def _sync_allowlist_remove(platform: str, user_id: str) -> None: - """Remove ``user_id`` from the platform allowlist env var if present.""" + """Remove ``user_id`` (and WhatsApp alias equivalents) from the allowlist. + + Matching must mirror PairingStore / authz WhatsApp alias rules: approve + mirrors a normalized phone into ``WHATSAPP_ALLOWED_USERS``, while revoke + is often invoked with a JID or device-suffix form. Exact-string delete + would leave the allowlist entry and keep the sender authorized. + + Also clears matching entries from any in-process platform adapter + ``_allow_from`` snapshot so sole-entry revocation is effective without a + gateway restart. + """ env_var = _allowlist_env_for_platform(platform) if not env_var: return current = os.getenv(env_var, "").strip() if not current: - return + return # No allowlist configured — do not touch config-only snapshots. ids = _split_allowlist(current) - remaining = [i for i in ids if i != str(user_id)] + # Never strip a wildcard grant; drop every entry that aliases-matches. + remaining = [ + i for i in ids + if i == "*" or not _user_ids_match(platform, i, str(user_id)) + ] if len(remaining) == len(ids): return # Not present. try: @@ -163,6 +293,7 @@ def _sync_allowlist_remove(platform: str, user_id: str) -> None: remove_env_value(env_var) except Exception: pass + _sync_live_adapter_allowlist_remove(platform, user_id) def _load_json_file(path: Path) -> dict: @@ -339,28 +470,15 @@ class PairingStore: def _normalize_user_id(self, platform: str, user_id: str) -> str: """Normalize platform-specific user IDs before persisting them.""" - raw_user_id = str(user_id or "").strip() - if platform == "whatsapp": - return normalize_whatsapp_identifier(raw_user_id) or raw_user_id - return raw_user_id + return _normalize_user_id(platform, user_id) def _user_id_aliases(self, platform: str, user_id: str) -> set[str]: """Return all known equivalent user IDs for auth/rate-limit checks.""" - raw_user_id = str(user_id or "").strip() - if not raw_user_id: - return set() - - aliases = {raw_user_id, self._normalize_user_id(platform, raw_user_id)} - if platform == "whatsapp": - aliases.update(expand_whatsapp_aliases(raw_user_id)) - aliases.discard("") - return aliases + return _user_id_aliases(platform, user_id) def _user_ids_match(self, platform: str, left: str, right: str) -> bool: """Return True when two user IDs represent the same principal.""" - left_aliases = self._user_id_aliases(platform, left) - right_aliases = self._user_id_aliases(platform, right) - return bool(left_aliases and right_aliases and (left_aliases & right_aliases)) + return _user_ids_match(platform, left, right) # ----- Approved users ----- diff --git a/gateway/platforms/whatsapp_cloud.py b/gateway/platforms/whatsapp_cloud.py index 074ad1afa2ffe..8659ed696940d 100644 --- a/gateway/platforms/whatsapp_cloud.py +++ b/gateway/platforms/whatsapp_cloud.py @@ -385,11 +385,16 @@ class WhatsAppCloudAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): normalized.add(digits or entry) return normalized + def _dm_allowlist_env_keys(self) -> tuple[str, ...]: + """Cloud pairing mirrors WHATSAPP_CLOUD_ALLOWED_USERS (then legacy ALLOW_FROM).""" + return ("WHATSAPP_CLOUD_ALLOWED_USERS", "WHATSAPP_CLOUD_ALLOW_FROM") + def _is_dm_allowed(self, sender_id: str) -> bool: """Allowlist check against the normalized bare wa_id.""" if self._dm_policy == "allowlist": bare = re.sub(r"\D", "", str(sender_id).split("@", 1)[0]) - return (bare or sender_id) in self._allow_from + allow_from = self._normalize_allow_ids(self._live_dm_allow_from()) + return (bare or sender_id) in allow_from return super()._is_dm_allowed(sender_id) def _open_dm_opted_in(self) -> bool: diff --git a/gateway/platforms/whatsapp_common.py b/gateway/platforms/whatsapp_common.py index 70f77b2973aca..9032bff1d7afe 100644 --- a/gateway/platforms/whatsapp_common.py +++ b/gateway/platforms/whatsapp_common.py @@ -134,6 +134,29 @@ class WhatsAppBehaviorMixin: return {str(part).strip() for part in raw if str(part).strip()} return {part.strip() for part in str(raw).split(",") if part.strip()} + def _dm_allowlist_env_keys(self) -> tuple[str, ...]: + """Env vars that carry the live DM allowlist for this adapter. + + Pairing approve/revoke mutates the documented ``*_ALLOWED_USERS`` var + in-process. Intake must prefer that live carrier over the construction + snapshot in ``_allow_from`` so sole-entry revocation takes effect + without a gateway restart. + """ + return ("WHATSAPP_ALLOWED_USERS",) + + def _live_dm_allow_from(self) -> set[str]: + """Allowlist currently enforced for DM intake / strict DM auth. + + When a pairing-synced env allowlist key is present in ``os.environ``, + that value is authoritative (including the empty set after the last + entry is cleared but before the key is removed). Otherwise fall back + to the adapter snapshot seeded at construction from config/env. + """ + for key in self._dm_allowlist_env_keys(): + if key in os.environ: + return self._coerce_allow_list(os.environ.get(key, "")) + return set(self._allow_from or ()) + # ------------------------------------------------------------------ JID helpers @staticmethod def _normalize_whatsapp_id(value: Optional[str]) -> str: @@ -214,7 +237,7 @@ class WhatsAppBehaviorMixin: if self._dm_policy == "disabled": return False if self._dm_policy == "allowlist": - return self._matches_whatsapp_allowlist(sender_id, self._allow_from) + return self._matches_whatsapp_allowlist(sender_id, self._live_dm_allow_from()) if self._dm_policy == "open": return self._open_dm_opted_in() return False @@ -227,7 +250,7 @@ class WhatsAppBehaviorMixin: if self._dm_policy == "disabled": return False if self._dm_policy == "allowlist": - return self._matches_whatsapp_allowlist(principal, self._allow_from) + return self._matches_whatsapp_allowlist(principal, self._live_dm_allow_from()) if self._dm_policy == "pairing": return True if self._dm_policy == "open": diff --git a/plugins/platforms/whatsapp/adapter.py b/plugins/platforms/whatsapp/adapter.py index 3f6c9faa8b9f2..512dc93262781 100644 --- a/plugins/platforms/whatsapp/adapter.py +++ b/plugins/platforms/whatsapp/adapter.py @@ -408,7 +408,14 @@ class WhatsAppAdapter(WhatsAppBehaviorMixin, BasePlatformAdapter): )) self._reply_prefix: Optional[str] = config.extra.get("reply_prefix") self._dm_policy = str(config.extra.get("dm_policy") or os.getenv("WHATSAPP_DM_POLICY", "pairing")).strip().lower() - self._allow_from = self._coerce_allow_list(config.extra.get("allow_from") or config.extra.get("allowFrom")) + # Prefer config.extra, then the documented WHATSAPP_ALLOWED_USERS env + # (setup wizard / pairing mirror). Construction seeds the snapshot; + # intake re-reads the live env via ``_live_dm_allow_from``. + self._allow_from = self._coerce_allow_list( + config.extra.get("allow_from") + or config.extra.get("allowFrom") + or os.getenv("WHATSAPP_ALLOWED_USERS") + ) self._group_policy = str(config.extra.get("group_policy") or os.getenv("WHATSAPP_GROUP_POLICY", "pairing")).strip().lower() self._group_allow_from = self._coerce_allow_list(config.extra.get("group_allow_from") or config.extra.get("groupAllowFrom")) read_receipts = config.extra.get("send_read_receipts", False) diff --git a/tests/gateway/test_pairing_allowlist_bypass.py b/tests/gateway/test_pairing_allowlist_bypass.py index 2f343a0d8783f..62b1df5dbc7c8 100644 --- a/tests/gateway/test_pairing_allowlist_bypass.py +++ b/tests/gateway/test_pairing_allowlist_bypass.py @@ -28,6 +28,7 @@ def _isolate_env(monkeypatch): "TELEGRAM_ALLOW_ALL_USERS", "TELEGRAM_GROUP_ALLOWED_USERS", "TELEGRAM_GROUP_ALLOWED_CHATS", + "WHATSAPP_ALLOWED_USERS", "GATEWAY_ALLOW_ALL_USERS", "GATEWAY_ALLOWED_USERS", ): @@ -127,3 +128,152 @@ def test_revoke_removes_from_allowlist(store, monkeypatch): assert saved.get("TELEGRAM_ALLOWED_USERS") == "owner1" +def test_revoke_whatsapp_device_jid_removes_bare_allowlist_entry(store, monkeypatch): + """Revoke with a device-suffix JID must clear the normalized phone allowlist entry. + + Approve persists/mirrors the bare phone; operators often revoke with the + bridge's JID form. Exact-string allowlist remove left the user authorized. + """ + monkeypatch.setenv("WHATSAPP_ALLOWED_USERS", "already,15551234567") + saved = {} + import hermes_cli.config as cfg + + monkeypatch.setattr( + cfg, + "save_env_value", + lambda k, v: (saved.__setitem__(k, v), os.environ.__setitem__(k, v)), + ) + monkeypatch.setattr(cfg, "remove_env_value", lambda k: os.environ.pop(k, None)) + + store._approve_user("whatsapp", "15551234567@s.whatsapp.net", "") + assert store.is_approved("whatsapp", "15551234567@s.whatsapp.net") is True + + assert store.revoke("whatsapp", "15551234567:47@s.whatsapp.net") is True + assert store.is_approved("whatsapp", "15551234567@s.whatsapp.net") is False + assert saved.get("WHATSAPP_ALLOWED_USERS") == "already" + assert os.environ.get("WHATSAPP_ALLOWED_USERS") == "already" + + +def test_revoke_whatsapp_removes_all_alias_forms_from_allowlist(store, monkeypatch): + """Allowlist may hold both bare phone and JID; revoke must drop every alias.""" + monkeypatch.setenv( + "WHATSAPP_ALLOWED_USERS", + "keeper,15551234567,15551234567@s.whatsapp.net", + ) + saved = {} + import hermes_cli.config as cfg + + monkeypatch.setattr( + cfg, + "save_env_value", + lambda k, v: (saved.__setitem__(k, v), os.environ.__setitem__(k, v)), + ) + store._approve_user("whatsapp", "15551234567", "") + + assert store.revoke("whatsapp", "15551234567@s.whatsapp.net") is True + assert saved.get("WHATSAPP_ALLOWED_USERS") == "keeper" + + +def test_revoke_whatsapp_preserves_wildcard_allowlist_entry(store, monkeypatch): + monkeypatch.setenv("WHATSAPP_ALLOWED_USERS", "*,15551234567") + saved = {} + import hermes_cli.config as cfg + + monkeypatch.setattr( + cfg, + "save_env_value", + lambda k, v: (saved.__setitem__(k, v), os.environ.__setitem__(k, v)), + ) + store._approve_user("whatsapp", "15551234567", "") + + assert store.revoke("whatsapp", "15551234567:47@s.whatsapp.net") is True + assert saved.get("WHATSAPP_ALLOWED_USERS") == "*" + + +def test_revoke_whatsapp_sole_entry_denies_live_adapter_without_restart( + store, monkeypatch, +): + """Sole allowlist entry revoke must deny immediately on a live gateway. + + Persistence alone is not enough: WhatsAppAdapter snapshots ``_allow_from`` + at construction, and authz trusts ``dm_policy=allowlist`` when the env + allowlist is gone. After revoke, intake and ``_is_user_authorized`` must + both deny the device-suffix JID without restarting the gateway. + """ + from types import SimpleNamespace + + from gateway.config import GatewayConfig, Platform, PlatformConfig + from gateway.platforms.whatsapp_common import WhatsAppBehaviorMixin + from gateway.run import GatewayRunner + import gateway.run as gateway_run + import hermes_cli.config as cfg + + monkeypatch.setenv("WHATSAPP_ALLOWED_USERS", "15551234567") + monkeypatch.setattr( + cfg, + "save_env_value", + lambda k, v: os.environ.__setitem__(k, v), + ) + monkeypatch.setattr( + cfg, + "remove_env_value", + lambda k: (os.environ.pop(k, None), True)[1], + ) + + class LiveWhatsAppAdapter(WhatsAppBehaviorMixin): + def __init__(self): + self.config = SimpleNamespace( + extra={ + "dm_policy": "allowlist", + "allow_from": ["15551234567"], + } + ) + self.platform = Platform.WHATSAPP + self._dm_policy = "allowlist" + self._allow_from = {"15551234567"} + self._group_policy = "pairing" + self._group_allow_from = set() + + adapter = LiveWhatsAppAdapter() + runner = object.__new__(GatewayRunner) + runner.config = GatewayConfig( + platforms={ + Platform.WHATSAPP: PlatformConfig( + enabled=True, + extra={"dm_policy": "allowlist", "allow_from": ["15551234567"]}, + ) + } + ) + runner.adapters = {Platform.WHATSAPP: adapter} + runner.pairing_store = store + runner.pairing_stores = {} + monkeypatch.setattr(gateway_run, "_gateway_runner_ref", lambda: runner) + + store._approve_user("whatsapp", "15551234567@s.whatsapp.net", "") + sender = "15551234567:47@s.whatsapp.net" + assert adapter._is_dm_intake_allowed(sender) is True + assert runner._is_user_authorized( + SessionSource( + platform=Platform.WHATSAPP, + user_id=sender, + chat_id=sender, + user_name="revoked", + chat_type="dm", + ) + ) is True + + assert store.revoke("whatsapp", sender) is True + assert store.is_approved("whatsapp", "15551234567@s.whatsapp.net") is False + assert os.environ.get("WHATSAPP_ALLOWED_USERS") in (None, "") + assert "15551234567" not in (adapter._allow_from or set()) + assert adapter._is_dm_intake_allowed(sender) is False + assert adapter._is_dm_allowed(sender) is False + assert runner._is_user_authorized( + SessionSource( + platform=Platform.WHATSAPP, + user_id=sender, + chat_id=sender, + user_name="revoked", + chat_type="dm", + ) + ) is False