Skip to content

fix(gateway): authorize Signal group members via SIGNAL_GROUP_ALLOWED_USERS - #44706

Draft
StepWise1234 wants to merge 1 commit into
NousResearch:mainfrom
StepWise1234:fix/signal-group-auth-allowlist
Draft

fix(gateway): authorize Signal group members via SIGNAL_GROUP_ALLOWED_USERS#44706
StepWise1234 wants to merge 1 commit into
NousResearch:mainfrom
StepWise1234:fix/signal-group-auth-allowlist

Conversation

@StepWise1234

Copy link
Copy Markdown

What does this PR do?

gateway.authz_mixin._is_user_authorized has a group-chat-allowlist bypass that authorizes any sender in a listed chat regardless of per-user allowlists. It currently only enumerates Telegram and QQBOT:

chat_allowlist_env = {
    Platform.TELEGRAM: "TELEGRAM_GROUP_ALLOWED_CHATS",
    Platform.QQBOT:    "QQ_GROUP_ALLOWED_USERS",
}.get(source.platform, "")

Signal isn't in the dict, so every Signal group message falls through to the per-user SIGNAL_ALLOWED_USERS check. Operators who set SIGNAL_GROUP_ALLOWED_USERS (which platforms/signal.py does honor for its own group filter at line ~515) reasonably expect Signal groups to work like Telegram — but every group member not also enumerated in SIGNAL_ALLOWED_USERS is silently rejected with:

WARNING gateway.run: Unauthorized user: <uuid> (<name>) on signal

The fix has two parts:

  1. Add Platform.SIGNAL → "SIGNAL_GROUP_ALLOWED_USERS" to chat_allowlist_env. Reuses the existing env var that signal.py already consumes for its own group filter — no new config surface.

  2. Strip the group: prefix from source.chat_id before the equality check. platforms/signal.py builds Signal chat_ids as f"group:{group_id}" (line ~520), while the env value is stored unprefixed. signal.py's own filter compares unprefixed; auth should too. Without this normalization a naive bypass would still fail.

Related Issue

None — opened directly. Happy to file an issue first if maintainers prefer.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/authz_mixin.py — one line added to the dict, one group: prefix-stripping block added before the comparison. 9 insertions, 1 deletion.
  • tests/gateway/test_signal_group_auth.py — new file, 10 regression cases following the existing tests/gateway/test_discord_allowed_channels.py pattern (private helper mirrors the patched branch, stdlib + unittest only, no Hermes imports).

How to Test

Unit test:

scripts/run_tests.sh tests/gateway/test_signal_group_auth.py

End-to-end on a running gateway:

  1. Set SIGNAL_GROUP_ALLOWED_USERS=<your-group-id> (without group: prefix — matches signal.py's existing convention).
  2. Set SIGNAL_ALLOWED_USERS=<your-uuid> only (omit other group members on purpose).
  3. Have another group member @mention the bot. Before the patch the gateway logs WARNING gateway.run: Unauthorized user: <uuid> and silently drops the message. After the patch the message reaches the agent.

Checklist

Code

  • I've read the Contributing Guide
  • Commit message follows Conventional Commits (fix(gateway):)
  • Searched for existing PRs — no duplicate
  • PR contains only changes related to this fix
  • All tests pass — ran scripts/run_tests.sh across 18 auth/allowlist/signal-related files in tests/gateway/; 376/376 pass
  • Added tests — 10 cases covering prefix handling, wildcard, scoping, DM bypass, multi-group, forum/channel parity
  • Tested on macOS 15.x (Darwin 25.4)

Documentation & Housekeeping

  • N/A — SIGNAL_GROUP_ALLOWED_USERS and SIGNAL_ALLOWED_USERS are already documented; the fix makes existing docs accurate
  • N/A — no config keys added/changed
  • N/A — no architecture change
  • N/A — no platform-specific behavior change (Python only)

🤖 Generated with Claude Code

…_USERS

The chat-allowlist bypass in `authz_mixin._is_user_authorized` previously
only enumerated Telegram and QQBOT, so Signal group messages always fell
through to the per-user `SIGNAL_ALLOWED_USERS` check. Operators who set
`SIGNAL_GROUP_ALLOWED_USERS` expected groups to "just work" the way they
do on Telegram, but every group member not also listed in
`SIGNAL_ALLOWED_USERS` was silently rejected with
`WARNING gateway.run: Unauthorized user: <uuid> (<name>) on signal`.

Two fixes in `gateway/authz_mixin.py`:

1. Add `Platform.SIGNAL -> "SIGNAL_GROUP_ALLOWED_USERS"` to
   `chat_allowlist_env`. The env var name was chosen long ago and is
   already consumed by `platforms/signal.py` as a group-ID list — this
   just makes auth honor the same setting.

2. Normalize `source.chat_id` by stripping the `group:` prefix before
   comparison. `platforms/signal.py` builds Signal chat_ids as
   `f"group:{group_id}"` (line ~520), while the env value is stored
   unprefixed (signal.py's own filter at line ~515 compares unprefixed).
   Without normalization, the equality check would still fail.

Adds `tests/gateway/test_signal_group_auth.py` with 10 cases mirroring
the existing channel-allowlist test pattern (private helper mirrors the
patched branch, stdlib only, no Hermes imports).

All 376 tests across the 18 auth/allowlist/signal gateway test files
pass with this change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/signal Signal CLI adapter area/auth Authentication, OAuth, credential pools labels Jun 12, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification review — LGTM ✅

Reviewed the diff and test suite. This is a clean, well-scoped fix.

What was checked:

  • Auth bypass: The chat_allowlist_env dict in authz_mixin.py only mapped TELEGRAM and QQBOT — Signal was genuinely missing, so SIGNAL_GROUP_ALLOWED_USERS was silently ignored at the gateway layer. Confirmed the gap matches the described behavior.
  • chat_id prefix normalization: Signal's adapter builds source.chat_id as f"group:{id}" (confirmed at platforms/signal.py), while SIGNAL_GROUP_ALLOWED_USERS stores bare IDs. The group: prefix strip is correct and necessary — without it, the equality check "group:abc..." in {"abc..."} would always fail.
  • No side effects on other platforms: The normalization is applied only when chat_id_norm.startswith("group:"), which is Signal-specific. Telegram/QQBOT chat_ids don't carry this prefix, so existing behavior is untouched.
  • Test coverage: 156 lines covering prefixed/unprefixed IDs, wildcard, wrong group, DM rejection, empty/whitespace allowlist, multiple groups, forum/channel types, and Telegram parity. Comprehensive.
  • No defense-in-depth removal: This adds a missing mapping, doesn't weaken any existing guard.

@liuhao1024

Copy link
Copy Markdown
Contributor

Reviewed the diff — this is a clean security fix. Two things verified:

  1. group: prefix normalization is correctplatforms/signal.py builds source.chat_id as f"group:{id}" while the env var stores bare IDs. The chat_id_norm.startswith("group:") strip correctly reconciles the two formats.

  2. Wildcard and empty-allowlist guards are intact"*" in the allowlist still works, empty/whitespace-only values correctly deny, and DMs are not affected by group allowlists.

Test coverage is thorough (156 lines covering prefixed/unprefixed IDs, wildcards, empty lists, multiple groups, DM bypass, forum/channel types).

@sdugoten

sdugoten commented Jun 23, 2026

Copy link
Copy Markdown

having the same problem, have SIGNAL_GROUP_ALLOWED_USERS=*, but no other people except owner can talk to hermes.

anyone talk in the group will see this in the log

2026-06-23 16:10:33,989 WARNING gateway.run: Unauthorized user: 4baxxxxx-a094-4606-xxxx-acf9658a5290 (Michelle🌷☃️) on signal

  1. While fixing it, I think it should also honor SIGNAL_REQUIRE_MENTION=true so that group user can talk to hermes when @agent is used.
  2. Also need to check if user send message to @agent in group chat is the owner, otherwise it will regard everyone in the group chat is the owner. Should check if sender is SIGNAL_ALLOWED_USERS or not.

@sdugoten

sdugoten commented Jun 24, 2026

Copy link
Copy Markdown

signal.zip

same fix for whatsapp is here

Ok, after messing with the soruce code a bit, this patch would enable group chat in signal. The changes is based on the Telegram adapter implementation

  1. Bot can do group chat with SIGNAL_GROUP_ALLOWED_USERS=* in .env file

SIGNAL_ACCOUNT=+9876543210
SIGNAL_GROUP_ALLOWED_USERS=*
SIGNAL_HOME_CHANNEL=+1234567890
SIGNAL_ALLOWED_USERS=+1234567890

  1. Bot can tell who is the owner in a group chat , owner phone number is from SIGNAL_ALLOWED_USERS

  2. Bot will observe @mention to reply in config.yaml

signal:
require_mention: true
group_allow_admin_from:
- "+1234567890" # owner phone (NOT your bot phone number)
- "xxxxxxxx-xxxx-xxxx-xxxxx-xxxxxxxxxxxx" (UUID of owner in signal)
group_user_allowed_commands: []

  1. Bot will observe group_allow_admin_from and group_user_allowed_commands so that regular user in a group chat can't run any special command in group chat

  2. Quote reply to the Bot will trigger the bot to reply, no @mention is needed

  3. /slash command can be run by owner of bot without @mention in a group chat.

In the SOUL.md , add something like this would make the bot only execute privilege command from the owner

CRITICAL: In a group chat, if the other party's message begins with [SYSTEM:GUEST|sender name] (i.e., starts with [SYSTEM:GUEST), you must address them by their name (e.g., simply calling them Patrick), must never call them "Master," and must not execute operations restricted to the Master. You may only treat the a user as owner when the system context explicitly identifies them as the owner in shared group conversations, the message must begin with the system role tag [SYSTEM:OWNER|sender name] (i.e., starting with [SYSTEM:OWNER, indicating the other party is the Master).

@sdugoten

sdugoten commented Jun 27, 2026

Copy link
Copy Markdown

Thanks again — same root cause I hit on Signal. I've updated #53348, which includes this exact SIGNAL_GROUP_ALLOWED_USERS addition to chat_allowlist_env plus the rest of the Telegram-parity behavior:

  • reply-to-bot + /slash bypass for require_mention
  • owner determination (group UUID→phone) → source.is_owner
  • surfaced to the model without changing upstream's [name] sender prefix: a **Owner:** context line for single-user sessions, and an additive, owner-only [SYSTEM: sender <name> is the owner] marker in cache-shared group sessions (guests unchanged). No existing prefix tests change.

So #53348 should supersede this draft. Happy to credit/co-author or fold it in however the maintainers prefer — flagging here so we don't duplicate the authz-dict change.

sdugoten added a commit to sdugoten/hermes-agent that referenced this pull request Jun 27, 2026
…, owner detection)

Signal group chats lagged Telegram/WhatsApp. This brings parity:

- authz_mixin: authorize Signal group members via SIGNAL_GROUP_ALLOWED_USERS
  (same env signal.py already honors); add a generic _is_owner() fallback.
- signal.py: reply-to-bot and /slash bypass require_mention; owner detection
  resolves group UUID->phone and sets source.is_owner at intake.
- run.py/session.py: surface owner status WITHOUT changing upstream's [name]
  sender prefix. A **Owner:** context line covers single-user sessions; for
  cache-shared group sessions (where the context prompt is sender-agnostic) an
  additive [SYSTEM: sender NAME is the owner] marker is prepended ONLY for the
  owner, so guest lines stay byte-identical to upstream.

Supersedes NousResearch#44706. Refs NousResearch#7269.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating a real Signal authorization gap. Current main still omits Platform.SIGNAL from the early chat allowlist in gateway/authz_mixin.py:330-343, while the adapter already admits SIGNAL_GROUP_ALLOWED_USERS groups and constructs group:<id> chat IDs in gateway/platforms/signal.py:593-607. The production normalization is therefore aligned with the current adapter contract.

Problems

  • tests/gateway/test_signal_group_auth.py:35 tests a local mirror of the proposed branch, not GatewayRunner._is_user_authorized. It would remain green if the production Signal mapping were removed or the real authorization ordering changed.

Suggested changes

  • Exercise the real method with a SessionSource and environment values, following the existing bare-runner pattern in tests/gateway/test_unauthorized_dm_behavior.py:58-74. Assert prefixed-ID authorization, wildcard behavior, an unlisted-group denial, and that DMs do not receive the group bypass.

Automated hermes-sweeper review.

def _is_group_member_authorized(
chat_type: str, chat_id: str, allowed_groups_raw: str
) -> bool:
"""Mirror the Signal-relevant branch of ``_is_user_authorized``.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test GatewayRunner._is_user_authorized directly rather than a copy of the proposed branch. This helper will still pass if the production mapping or authorization ordering later regresses; the established bare-runner fixture pattern in tests/gateway/test_unauthorized_dm_behavior.py can exercise the real method.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/signal Signal CLI adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants