From 1bfe08145c8e01ab22ad8a9ebe8c62e8faa348de Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 1 Jul 2026 05:31:15 -0700 Subject: [PATCH] fix(gateway): pairing is a grant that syncs to the allowlist (#23778) (#56381) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates the pairing/allowlist authorization model. Reverses the read-side AND-ing from #56346 (which made a paired user require ALSO being in the allowlist) and restores pairing as a first-class grant: - authz_mixin: a pairing-store entry authorizes regardless of the allowlist (union). approve_code is reachable only by the trusted operator (CLI / authenticated dashboard), never by an inbound sender, so it is not an attacker-controlled path — the #23778 bypass was the inbound message/approval-button gate, fixed separately. - pairing: when an allowlist IS already configured for the platform, operator approval also appends the user to that allowlist env var (option i) and revoke removes them, keeping a single operator-visible, editable source of truth instead of an opaque approved.json. On an open gateway (no allowlist) approval is a no-op on the env var so we never silently lock an open gateway; the pairing store remains the grant record, honored by the union. - auto-resume authz (0de67ad60) now honors paired users automatically via the same union — a legitimately-paired session survives restart. Replaces the now-incorrect AND-ing tests with union + mirror + revoke coverage. E2E verified: locked-gateway approve/revoke round-trips through the allowlist; open-gateway approval stays open. --- gateway/authz_mixin.py | 28 ++- gateway/pairing.py | 115 ++++++++++ .../gateway/test_pairing_allowlist_bypass.py | 205 +++++++++++++----- 3 files changed, 279 insertions(+), 69 deletions(-) diff --git a/gateway/authz_mixin.py b/gateway/authz_mixin.py index 37807ac94..869849133 100644 --- a/gateway/authz_mixin.py +++ b/gateway/authz_mixin.py @@ -417,17 +417,20 @@ class GatewayAuthorizationMixin: if getattr(source, "role_authorized", False) is True: return True - # Pairing store membership. Recorded here but NOT returned yet: if an - # allowlist is configured, a previously-paired user must STILL be in - # that allowlist to be honored. Otherwise a user who tapped "Always" on - # an approval button could permanently bypass TELEGRAM_ALLOWED_USERS (or - # equivalent) via the pairing store even after being removed from the - # allowlist (issue #23778). When no allowlist is configured, pairing is - # the intended access path and is honored in the no-allowlist branch - # below; when an allowlist IS configured, the pairing entry only counts - # if the user is also in it (folded into the final membership check). + # Check pairing store. A pairing entry is a first-class authorization + # grant, created only by a trusted operator approving a pairing code + # (hermes gateway pairing approve / the authenticated dashboard) — an + # inbound sender can never reach approve_code, so this is not an + # attacker-controlled path. Honored as a UNION with the allowlist: a + # paired user is authorized regardless of the allowlist, and when an + # allowlist IS configured, operator approval also writes the user into + # that allowlist (see PairingStore._approve_user), keeping a single + # operator-visible source of truth. (#23778: the original bypass was the + # inbound message/approval-button gate, not this grant; that gate is + # fixed separately.) platform_name = source.platform.value if source.platform else "" - is_paired = self.pairing_store.is_approved(platform_name, user_id) + if self.pairing_store.is_approved(platform_name, user_id): + return True # Check platform-specific and global allowlists platform_allowlist = os.getenv(platform_env_map.get(source.platform, ""), "").strip() @@ -439,11 +442,6 @@ class GatewayAuthorizationMixin: 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: - # No env allowlist configured. A pairing-store entry is the intended - # access path here (the user completed the pairing handshake), so - # honor it — there is no allowlist to re-validate against. - if is_paired: - return True # No env allowlist configured. Adapters that own their own # config-driven access policy (dm_policy / group_policy / # allow_from / group_allow_from) gate access at intake, so for those diff --git a/gateway/pairing.py b/gateway/pairing.py index b8bfe46a9..278823d6e 100644 --- a/gateway/pairing.py +++ b/gateway/pairing.py @@ -52,6 +52,112 @@ MAX_FAILED_ATTEMPTS = 5 # Failed approvals before lockout PAIRING_DIR = get_hermes_dir("platforms/pairing", "pairing") +# Platform value -> its per-platform allowlist env var. When an operator has +# already configured an allowlist for a platform, approving a pairing code also +# writes the user into that allowlist (and revoking removes them), so the +# operator's own list stays the single visible/editable source of truth instead +# of drifting from an opaque approved.json (#23778 consolidation, option i). +# Platforms absent from this map (or with no allowlist configured) keep the +# pairing store as the sole grant record, honored by the authz union. +_PLATFORM_ALLOWLIST_ENV = { + "telegram": "TELEGRAM_ALLOWED_USERS", + "discord": "DISCORD_ALLOWED_USERS", + "whatsapp": "WHATSAPP_ALLOWED_USERS", + "whatsapp_cloud": "WHATSAPP_CLOUD_ALLOWED_USERS", + "slack": "SLACK_ALLOWED_USERS", + "signal": "SIGNAL_ALLOWED_USERS", + "email": "EMAIL_ALLOWED_USERS", + "sms": "SMS_ALLOWED_USERS", + "mattermost": "MATTERMOST_ALLOWED_USERS", + "matrix": "MATRIX_ALLOWED_USERS", + "dingtalk": "DINGTALK_ALLOWED_USERS", + "feishu": "FEISHU_ALLOWED_USERS", + "wecom": "WECOM_ALLOWED_USERS", + "wecom_callback": "WECOM_CALLBACK_ALLOWED_USERS", + "weixin": "WEIXIN_ALLOWED_USERS", + "bluebubbles": "BLUEBUBBLES_ALLOWED_USERS", + "qqbot": "QQ_ALLOWED_USERS", + "yuanbao": "YUANBAO_ALLOWED_USERS", +} + + +def _allowlist_env_for_platform(platform: str) -> Optional[str]: + """Return the per-platform allowlist env var name, or None. + + Falls back to the platform registry for plugin platforms so a plugin's + own ``allowed_users_env`` is honored too. + """ + platform = (platform or "").lower().strip() + env_var = _PLATFORM_ALLOWLIST_ENV.get(platform) + if env_var: + return env_var + try: + from gateway.platform_registry import platform_registry + + entry = platform_registry.get(platform) + if entry and entry.allowed_users_env: + return entry.allowed_users_env + except Exception: + pass + return None + + +def _split_allowlist(raw: str) -> list: + return [uid.strip() for uid in raw.split(",") if uid.strip()] + + +def _sync_allowlist_add(platform: str, user_id: str) -> None: + """Add ``user_id`` to the platform allowlist env var IF one is configured. + + Option (i): only materialize the grant into the allowlist when the operator + already runs an allowlist for this platform. On an open gateway (no + allowlist) we do nothing — the pairing store remains the grant record and + the authz union honors it, so we never silently convert an open gateway into + a locked one on first pairing. + """ + env_var = _allowlist_env_for_platform(platform) + if not env_var: + return + current = os.getenv(env_var, "").strip() + if not current: + return # No allowlist configured — leave the gateway open (option i). + ids = _split_allowlist(current) + if "*" in ids or str(user_id) in ids: + return # Already covered. + ids.append(str(user_id)) + try: + from hermes_cli.config import save_env_value + + save_env_value(env_var, ",".join(ids)) + except Exception: + # Best-effort: the pairing store grant still authorizes via the union, + # so a failure here degrades to "grant recorded but not mirrored". + pass + + +def _sync_allowlist_remove(platform: str, user_id: str) -> None: + """Remove ``user_id`` from the platform allowlist env var if present.""" + env_var = _allowlist_env_for_platform(platform) + if not env_var: + return + current = os.getenv(env_var, "").strip() + if not current: + return + ids = _split_allowlist(current) + remaining = [i for i in ids if i != str(user_id)] + if len(remaining) == len(ids): + return # Not present. + try: + from hermes_cli.config import save_env_value, remove_env_value + + if remaining: + save_env_value(env_var, ",".join(remaining)) + else: + remove_env_value(env_var) + except Exception: + pass + + def _secure_write(path: Path, data: str) -> None: """Write data to file with restrictive permissions (owner read/write only). @@ -177,6 +283,11 @@ class PairingStore: } self._save_json(self._approved_path(platform), approved) + # Mirror the grant into the operator's allowlist when one is configured + # (option i), so the pairing store and the allowlist stay a single + # visible source of truth. No-op on open gateways. + _sync_allowlist_add(platform, normalized_user_id) + def revoke(self, platform: str, user_id: str) -> bool: """Remove a user from the approved list. Returns True if found.""" path = self._approved_path(platform) @@ -191,6 +302,10 @@ class PairingStore: for approved_user_id in matching_ids: del approved[approved_user_id] self._save_json(path, approved) + # Keep the allowlist mirror in sync: revoking a paired user + # also removes the entry the approval added (option i). No-op if + # the user was added to the allowlist by other means. + _sync_allowlist_remove(platform, user_id) return True return False diff --git a/tests/gateway/test_pairing_allowlist_bypass.py b/tests/gateway/test_pairing_allowlist_bypass.py index da004cfaa..30cee312d 100644 --- a/tests/gateway/test_pairing_allowlist_bypass.py +++ b/tests/gateway/test_pairing_allowlist_bypass.py @@ -1,14 +1,19 @@ -"""Regression guard: pairing store must not bypass a configured allowlist (#23778). +"""Pairing store <-> allowlist consolidation (#23778). -A user who tapped "Always" on an approval button gets a pairing-store entry. -Before the fix, ``_is_user_authorized()`` returned True from the pairing store -BEFORE the allowlist was ever consulted, so a paired-but-not-allowed user -permanently bypassed ``TELEGRAM_ALLOWED_USERS`` (or equivalent) even after being -removed from the allowlist. The fix records pairing membership but only honors -it when no allowlist is configured; when an allowlist IS configured, the paired -user must still appear in it. +Design (union + option-i mirror): + * A pairing-store entry is a first-class authorization grant. A paired user + is authorized regardless of any configured allowlist (union), because + ``approve_code`` is reachable only by the trusted operator (CLI/dashboard), + never by an inbound sender. + * When an allowlist IS already configured for the platform, approving a + pairing code ALSO writes the user into that allowlist env var (and revoking + removes them), so the two stay a single operator-visible source of truth. + * On an open gateway (no allowlist configured) approval does NOT create an + allowlist — that would silently lock an open gateway. The pairing store + remains the grant record, honored by the authz union. """ +import os from types import SimpleNamespace import pytest @@ -29,6 +34,10 @@ def _isolate_env(monkeypatch): monkeypatch.delenv(var, raising=False) +# -------------------------------------------------------------------------- +# authz union: a paired user is authorized regardless of the allowlist +# -------------------------------------------------------------------------- + def _make_runner(*, paired: bool): from gateway.run import GatewayRunner @@ -37,7 +46,7 @@ def _make_runner(*, paired: bool): return runner -def _make_source(user_id: str = "attacker42", chat_type: str = "dm"): +def _make_source(user_id: str = "pairme", chat_type: str = "dm"): return SessionSource( platform=Platform.TELEGRAM, chat_id="123", @@ -48,61 +57,149 @@ def _make_source(user_id: str = "attacker42", chat_type: str = "dm"): ) -def test_paired_user_denied_when_not_in_platform_allowlist(monkeypatch): - """The core bypass: paired but absent from TELEGRAM_ALLOWED_USERS → denied.""" +def test_paired_user_authorized_even_when_not_in_allowlist(monkeypatch): + """Union semantics: pairing is a grant, honored alongside the allowlist.""" runner = _make_runner(paired=True) monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "owner1,owner2") - assert runner._is_user_authorized(_make_source("attacker42")) is False + assert runner._is_user_authorized(_make_source("pairme")) is True -def test_paired_user_denied_when_not_in_global_allowlist(monkeypatch): - runner = _make_runner(paired=True) - monkeypatch.setenv("GATEWAY_ALLOWED_USERS", "owner1,owner2") - - assert runner._is_user_authorized(_make_source("attacker42")) is False - - -def test_paired_user_allowed_when_still_in_platform_allowlist(monkeypatch): - """A paired user who is also in the allowlist stays authorized.""" - runner = _make_runner(paired=True) - monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "owner1,attacker42") - - assert runner._is_user_authorized(_make_source("attacker42")) is True - - -def test_paired_user_allowed_when_still_in_global_allowlist(monkeypatch): - runner = _make_runner(paired=True) - monkeypatch.setenv("GATEWAY_ALLOWED_USERS", "owner1,attacker42") - - assert runner._is_user_authorized(_make_source("attacker42")) is True - - -def test_paired_user_allowed_with_wildcard_allowlist(monkeypatch): - """A "*" allowlist means everyone; a paired user is honored.""" - runner = _make_runner(paired=True) - monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "*") - - assert runner._is_user_authorized(_make_source("attacker42")) is True - - -def test_paired_user_allowed_when_no_allowlist_configured(monkeypatch): - """No allowlist configured → pairing is the intended access path, honored.""" +def test_paired_user_authorized_with_no_allowlist(monkeypatch): runner = _make_runner(paired=True) - assert runner._is_user_authorized(_make_source("attacker42")) is True + assert runner._is_user_authorized(_make_source("pairme")) is True -def test_unpaired_user_denied_when_no_allowlist_configured(monkeypatch): - """No allowlist and not paired → default-deny (no fail-open).""" - runner = _make_runner(paired=False) - - assert runner._is_user_authorized(_make_source("randouser")) is False - - -def test_unpaired_user_in_allowlist_still_allowed(monkeypatch): - """Pairing changes did not regress the plain allowlist path.""" +def test_unpaired_user_in_allowlist_still_authorized(monkeypatch): runner = _make_runner(paired=False) monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "owner1") assert runner._is_user_authorized(_make_source("owner1")) is True + + +def test_unpaired_user_not_in_allowlist_denied(monkeypatch): + runner = _make_runner(paired=False) + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "owner1") + + assert runner._is_user_authorized(_make_source("stranger")) is False + + +def test_unpaired_user_no_allowlist_denied_no_failopen(monkeypatch): + runner = _make_runner(paired=False) + + assert runner._is_user_authorized(_make_source("stranger")) is False + + +# -------------------------------------------------------------------------- +# B2 mirror: approval writes into the allowlist iff one is configured +# -------------------------------------------------------------------------- + +@pytest.fixture +def store(tmp_path, monkeypatch): + """A real PairingStore backed by a temp pairing dir.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + (tmp_path / ".hermes").mkdir(parents=True, exist_ok=True) + import importlib + + import gateway.pairing as pairing_mod + importlib.reload(pairing_mod) + return pairing_mod.PairingStore() + + +def _approve_new_user(store, platform, user_id, user_name=""): + code = store.generate_code(platform, user_id, user_name) + assert code is not None + return store.approve_code(platform, code) + + +def test_approval_adds_to_configured_allowlist(store, monkeypatch): + """When an allowlist exists, approval appends the user to it (option i).""" + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "owner1") + # save_env_value writes to .env under HERMES_HOME; patch it to capture. + captured = {} + import hermes_cli.config as cfg + + monkeypatch.setattr(cfg, "save_env_value", + lambda k, v: (captured.__setitem__(k, v), + os.environ.__setitem__(k, v))) + + _approve_new_user(store, "telegram", "newuser99") + + assert captured.get("TELEGRAM_ALLOWED_USERS") == "owner1,newuser99" + + +def test_approval_no_allowlist_leaves_gateway_open(store, monkeypatch): + """Open gateway: approval must NOT create an allowlist (option i).""" + called = {} + import hermes_cli.config as cfg + + monkeypatch.setattr(cfg, "save_env_value", + lambda k, v: called.__setitem__(k, v)) + + _approve_new_user(store, "telegram", "newuser99") + + assert "TELEGRAM_ALLOWED_USERS" not in called + assert os.getenv("TELEGRAM_ALLOWED_USERS", "") == "" + # The pairing store still records the grant (union honors it). + assert store.is_approved("telegram", "newuser99") is True + + +def test_approval_idempotent_when_already_in_allowlist(store, monkeypatch): + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "owner1,newuser99") + called = {} + import hermes_cli.config as cfg + + monkeypatch.setattr(cfg, "save_env_value", + lambda k, v: called.__setitem__(k, v)) + + _approve_new_user(store, "telegram", "newuser99") + + # Already present — no rewrite. + assert "TELEGRAM_ALLOWED_USERS" not in called + + +def test_approval_skips_wildcard_allowlist(store, monkeypatch): + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "*") + called = {} + import hermes_cli.config as cfg + + monkeypatch.setattr(cfg, "save_env_value", + lambda k, v: called.__setitem__(k, v)) + + _approve_new_user(store, "telegram", "newuser99") + + assert "TELEGRAM_ALLOWED_USERS" not in called + + +def test_revoke_removes_from_allowlist(store, monkeypatch): + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "owner1,newuser99") + saved = {} + removed = [] + 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: removed.append(k)) + # Seed the approved list directly so revoke has something to remove. + store._approve_user("telegram", "newuser99", "") + + assert store.revoke("telegram", "newuser99") is True + assert saved.get("TELEGRAM_ALLOWED_USERS") == "owner1" + + +def test_revoke_removes_env_var_when_list_empties(store, monkeypatch): + monkeypatch.setenv("TELEGRAM_ALLOWED_USERS", "newuser99") + removed = [] + import hermes_cli.config as cfg + + monkeypatch.setattr(cfg, "save_env_value", + lambda k, v: os.environ.__setitem__(k, v)) + monkeypatch.setattr(cfg, "remove_env_value", lambda k: removed.append(k)) + store._approve_user("telegram", "newuser99", "") + # _approve_user's own add is a no-op (already present); reset for the revoke. + os.environ["TELEGRAM_ALLOWED_USERS"] = "newuser99" + + assert store.revoke("telegram", "newuser99") is True + assert "TELEGRAM_ALLOWED_USERS" in removed