mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-08 03:01:47 +00:00
fix(discord): extend role-scope fix to slash surface + fixture update
Sibling-site fix: _evaluate_slash_authorization was the fourth _is_allowed_user caller and didn't pass guild/is_dm through, so slash interactions would take the DM branch regardless of whether they came from a guild channel. Now reads interaction.guild + in_dm and forwards. Also updates test_discord_slash_auth fixture (_make_interaction) so the SimpleNamespace guild mock has a get_member(uid)->None method — required by the new guild-scoped fallback path in _is_allowed_user. Tests exercising positive role paths still work via user.roles. Three new regression tests in test_discord_roles_dm_scope: - Slash DM + role in mutual public guild → rejected - Slash in guild B + role only in guild A → rejected - Slash in guild B + role in guild B → allowed (positive control) 368 Discord tests pass. test_discord_free_channel_skips_auto_thread also fails on clean main (pre-existing, unrelated to this fix).
This commit is contained in:
parent
ef1e565570
commit
5c045b8f6c
3 changed files with 105 additions and 2 deletions
|
|
@ -2305,7 +2305,16 @@ class DiscordAdapter(BasePlatformAdapter):
|
||||||
return (True, None)
|
return (True, None)
|
||||||
|
|
||||||
user_id = str(user.id)
|
user_id = str(user.id)
|
||||||
if not self._is_allowed_user(user_id, author=user):
|
# Pass guild + is_dm so role check is scoped to the originating
|
||||||
|
# guild and cross-guild DM bypass (#12136) can't land via the
|
||||||
|
# slash surface either.
|
||||||
|
interaction_guild = getattr(interaction, "guild", None)
|
||||||
|
if not self._is_allowed_user(
|
||||||
|
user_id,
|
||||||
|
author=user,
|
||||||
|
guild=interaction_guild,
|
||||||
|
is_dm=in_dm,
|
||||||
|
):
|
||||||
return (
|
return (
|
||||||
False,
|
False,
|
||||||
"user not in DISCORD_ALLOWED_USERS / DISCORD_ALLOWED_ROLES",
|
"user not in DISCORD_ALLOWED_USERS / DISCORD_ALLOWED_ROLES",
|
||||||
|
|
|
||||||
|
|
@ -252,3 +252,93 @@ def test_empty_allowlists_allow_everyone():
|
||||||
adapter._is_allowed_user("42", author=None, guild=None, is_dm=True)
|
adapter._is_allowed_user("42", author=None, guild=None, is_dm=True)
|
||||||
is True
|
is True
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Slash-surface sibling site: _evaluate_slash_authorization must pass
|
||||||
|
# guild/is_dm through so the cross-guild bypass can't land via slash either.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_slash_authorization_rejects_cross_guild_role_dm(monkeypatch):
|
||||||
|
"""Slash interaction in a DM must not be authorized by a role held in
|
||||||
|
any mutual guild (parallel to the on_message cross-guild bypass)."""
|
||||||
|
import discord as _discord # type: ignore
|
||||||
|
monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False)
|
||||||
|
|
||||||
|
public_guild, _ = _guild_with_member(
|
||||||
|
guild_id=111111,
|
||||||
|
member_id=42,
|
||||||
|
role_ids=[5555],
|
||||||
|
)
|
||||||
|
adapter = _make_adapter(
|
||||||
|
allowed_roles=[5555],
|
||||||
|
guilds=[public_guild],
|
||||||
|
)
|
||||||
|
|
||||||
|
# Fake a DM interaction: user is Member-like, channel is DMChannel,
|
||||||
|
# interaction.guild is None.
|
||||||
|
interaction = SimpleNamespace(
|
||||||
|
user=SimpleNamespace(id=42),
|
||||||
|
channel=MagicMock(spec=_discord.DMChannel),
|
||||||
|
channel_id=None,
|
||||||
|
guild=None,
|
||||||
|
)
|
||||||
|
|
||||||
|
allowed, reason = adapter._evaluate_slash_authorization(interaction)
|
||||||
|
assert allowed is False
|
||||||
|
assert "ALLOWED" in (reason or "")
|
||||||
|
|
||||||
|
|
||||||
|
def test_slash_authorization_rejects_cross_guild_role_in_guild(monkeypatch):
|
||||||
|
"""Slash in guild B must not be authorized by a role held in guild A."""
|
||||||
|
monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False)
|
||||||
|
|
||||||
|
public_guild, _ = _guild_with_member(
|
||||||
|
guild_id=111111,
|
||||||
|
member_id=42,
|
||||||
|
role_ids=[5555],
|
||||||
|
)
|
||||||
|
# Interaction arrives in trusted_guild where user 42 has no role
|
||||||
|
trusted_guild = SimpleNamespace(id=222222, get_member=lambda uid: None)
|
||||||
|
adapter = _make_adapter(
|
||||||
|
allowed_roles=[5555],
|
||||||
|
guilds=[public_guild, trusted_guild],
|
||||||
|
)
|
||||||
|
|
||||||
|
interaction = SimpleNamespace(
|
||||||
|
user=SimpleNamespace(id=42),
|
||||||
|
channel=SimpleNamespace(id=9999), # not a DMChannel instance
|
||||||
|
channel_id=9999,
|
||||||
|
guild=trusted_guild,
|
||||||
|
)
|
||||||
|
|
||||||
|
allowed, reason = adapter._evaluate_slash_authorization(interaction)
|
||||||
|
assert allowed is False
|
||||||
|
assert "ALLOWED" in (reason or "")
|
||||||
|
|
||||||
|
|
||||||
|
def test_slash_authorization_allows_in_scope_guild_role(monkeypatch):
|
||||||
|
"""Positive control: slash in guild B, user has role in guild B → allowed."""
|
||||||
|
monkeypatch.delenv("DISCORD_DM_ROLE_AUTH_GUILD", raising=False)
|
||||||
|
|
||||||
|
trusted_guild, _ = _guild_with_member(
|
||||||
|
guild_id=222222,
|
||||||
|
member_id=42,
|
||||||
|
role_ids=[5555],
|
||||||
|
)
|
||||||
|
adapter = _make_adapter(
|
||||||
|
allowed_roles=[5555],
|
||||||
|
guilds=[trusted_guild],
|
||||||
|
)
|
||||||
|
|
||||||
|
interaction = SimpleNamespace(
|
||||||
|
user=SimpleNamespace(id=42),
|
||||||
|
channel=SimpleNamespace(id=9999),
|
||||||
|
channel_id=9999,
|
||||||
|
guild=trusted_guild,
|
||||||
|
)
|
||||||
|
|
||||||
|
allowed, reason = adapter._evaluate_slash_authorization(interaction)
|
||||||
|
assert allowed is True
|
||||||
|
assert reason is None
|
||||||
|
|
|
||||||
|
|
@ -158,7 +158,11 @@ def _make_interaction(
|
||||||
|
|
||||||
return SimpleNamespace(
|
return SimpleNamespace(
|
||||||
user=user_obj,
|
user=user_obj,
|
||||||
guild=SimpleNamespace(owner_id=999),
|
# `get_member` needed for the guild-scoped role fallback path in
|
||||||
|
# _is_allowed_user after the #12136 cross-guild fix. Fixture guild
|
||||||
|
# has no members by default — tests exercising positive role paths
|
||||||
|
# assign their own Member via user.roles + matching allowed_role_ids.
|
||||||
|
guild=SimpleNamespace(owner_id=999, id=guild_id, get_member=lambda uid: None),
|
||||||
guild_id=guild_id,
|
guild_id=guild_id,
|
||||||
channel_id=channel_id,
|
channel_id=channel_id,
|
||||||
channel=channel,
|
channel=channel,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue