mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
fix(discord): harden DISCORD_ALLOWED_ROLES and cover gateway layer
Two follow-ups to the cherry-picked PR #9873 (`e3bcc819`): 1. `_is_allowed_user` now uses `getattr(self, '_allowed_*_ids', set())` so test fixtures that build the adapter via `object.__new__` (skipping __init__) don't crash with AttributeError. See AGENTS.md pitfall #17 — same pattern as gateway.run. 2. New 3-case regression coverage in test_discord_bot_auth_bypass.py: - role-only config bypasses the gateway 'no allowlists' branch - roles + users combined still authorizes user-allowlist matches - the role bypass does NOT leak to other platforms (Telegram, etc.) 3. Autouse fixture in test_discord_bot_auth_bypass.py clears all Discord auth env vars before each test so DISCORD_ALLOWED_ROLES leakage from a previous test in the session can't flip later 'should-reject' tests into false-pass. Required because the bare cherry-pick of #9873 only added the adapter- level role check — it didn't cover the gateway-level _is_user_authorized, which still rejected role-only setups via the 'no allowlists configured' branch.
This commit is contained in:
parent
541a3e27d7
commit
e5b880264b
2 changed files with 82 additions and 5 deletions
|
|
@ -1386,19 +1386,24 @@ class DiscordAdapter(BasePlatformAdapter):
|
|||
When author is a Member, checks .roles directly; otherwise falls back
|
||||
to scanning the bot's mutual guilds for a Member record.
|
||||
"""
|
||||
has_users = bool(self._allowed_user_ids)
|
||||
has_roles = bool(self._allowed_role_ids)
|
||||
# ``getattr`` fallbacks here guard against test fixtures that build
|
||||
# an adapter via ``object.__new__(DiscordAdapter)`` and skip __init__
|
||||
# (see AGENTS.md pitfall #17 — same pattern as gateway.run).
|
||||
allowed_users = getattr(self, "_allowed_user_ids", set())
|
||||
allowed_roles = getattr(self, "_allowed_role_ids", set())
|
||||
has_users = bool(allowed_users)
|
||||
has_roles = bool(allowed_roles)
|
||||
if not has_users and not has_roles:
|
||||
return True
|
||||
# Check user ID allowlist
|
||||
if has_users and user_id in self._allowed_user_ids:
|
||||
if has_users and user_id in allowed_users:
|
||||
return True
|
||||
# Check role allowlist
|
||||
if has_roles:
|
||||
# Try direct role check from Member object
|
||||
direct_roles = getattr(author, "roles", None) if author is not None else None
|
||||
if direct_roles:
|
||||
if any(getattr(r, "id", None) in self._allowed_role_ids for r in direct_roles):
|
||||
if any(getattr(r, "id", None) in allowed_roles for r in direct_roles):
|
||||
return True
|
||||
# Fallback: scan mutual guilds for member's roles
|
||||
if self._client is not None:
|
||||
|
|
@ -1412,7 +1417,7 @@ class DiscordAdapter(BasePlatformAdapter):
|
|||
if m is None:
|
||||
continue
|
||||
m_roles = getattr(m, "roles", None) or []
|
||||
if any(getattr(r, "id", None) in self._allowed_role_ids for r in m_roles):
|
||||
if any(getattr(r, "id", None) in allowed_roles for r in m_roles):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
|
|
|||
|
|
@ -22,6 +22,23 @@ import pytest
|
|||
from gateway.session import Platform, SessionSource
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _isolate_discord_env(monkeypatch):
|
||||
"""Make every test start with a clean Discord env so prior tests in the
|
||||
session (or CI setups) can't leak DISCORD_ALLOWED_ROLES / DISCORD_ALLOWED_USERS
|
||||
/ DISCORD_ALLOW_BOTS and silently flip the auth result.
|
||||
"""
|
||||
for var in (
|
||||
"DISCORD_ALLOW_BOTS",
|
||||
"DISCORD_ALLOWED_USERS",
|
||||
"DISCORD_ALLOWED_ROLES",
|
||||
"DISCORD_ALLOW_ALL_USERS",
|
||||
"GATEWAY_ALLOW_ALL_USERS",
|
||||
"GATEWAY_ALLOWED_USERS",
|
||||
):
|
||||
monkeypatch.delenv(var, raising=False)
|
||||
|
||||
|
||||
# -----------------------------------------------------------------------------
|
||||
# Gate 2: _is_user_authorized bypasses allowlist for permitted bots
|
||||
# -----------------------------------------------------------------------------
|
||||
|
|
@ -152,3 +169,58 @@ def test_bot_bypass_does_not_leak_to_other_platforms(monkeypatch):
|
|||
is_bot=True,
|
||||
)
|
||||
assert runner._is_user_authorized(telegram_bot) is False
|
||||
|
||||
|
||||
# -----------------------------------------------------------------------------
|
||||
# DISCORD_ALLOWED_ROLES gateway-layer bypass (#7871)
|
||||
# -----------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_discord_role_config_bypasses_gateway_allowlist(monkeypatch):
|
||||
"""When DISCORD_ALLOWED_ROLES is set, _is_user_authorized must trust
|
||||
the adapter's pre-filter and authorize. Without this, role-only setups
|
||||
(DISCORD_ALLOWED_ROLES populated, DISCORD_ALLOWED_USERS empty) would
|
||||
hit the 'no allowlists configured' branch and get rejected.
|
||||
"""
|
||||
runner = _make_bare_runner()
|
||||
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674")
|
||||
# Note: DISCORD_ALLOWED_USERS is NOT set — the entire point.
|
||||
|
||||
source = _make_discord_human_source(user_id="999888777")
|
||||
assert runner._is_user_authorized(source) is True
|
||||
|
||||
|
||||
def test_discord_role_config_still_authorizes_alongside_users(monkeypatch):
|
||||
"""Sanity: setting both DISCORD_ALLOWED_ROLES and DISCORD_ALLOWED_USERS
|
||||
doesn't break the user-id path. Users in the allowlist should still be
|
||||
authorized even if they don't have a role. (OR semantics.)
|
||||
"""
|
||||
runner = _make_bare_runner()
|
||||
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674")
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_USERS", "100200300")
|
||||
|
||||
# User on the user allowlist, no role → still authorized at gateway
|
||||
# level via the role bypass (adapter already approved them).
|
||||
source = _make_discord_human_source(user_id="100200300")
|
||||
assert runner._is_user_authorized(source) is True
|
||||
|
||||
|
||||
def test_discord_role_bypass_does_not_leak_to_other_platforms(monkeypatch):
|
||||
"""DISCORD_ALLOWED_ROLES must only affect Discord. Setting it should
|
||||
not suddenly start authorizing Telegram users whose platform has its
|
||||
own empty allowlist.
|
||||
"""
|
||||
runner = _make_bare_runner()
|
||||
|
||||
monkeypatch.setenv("DISCORD_ALLOWED_ROLES", "1493705176387948674")
|
||||
# Telegram has its own empty allowlist and no allow-all flag.
|
||||
|
||||
telegram_user = SessionSource(
|
||||
platform=Platform.TELEGRAM,
|
||||
chat_id="123",
|
||||
chat_type="channel",
|
||||
user_id="999888777",
|
||||
)
|
||||
assert runner._is_user_authorized(telegram_user) is False
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue