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.
This commit is contained in:
Teknium
GitHub
parent
04b4310643
commit
1bfe08145c
3 changed files with 279 additions and 69 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue