fix(security): reuse auth chain when tagging unverified senders in Slack threads - #79
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
A latent privilege-escalation path in the new Files Reviewed (2 files) |
There was a problem hiding this comment.
Risk: 🟡 Medium (42/100) — 1 medium finding · 41 LOC across 2 files
Summary
This PR adds a _make_adapter_auth_check helper to gateway/run.py that centralizes adapter authorization checks via _is_sender_authorized. The method constructs a SessionSource with chat_type defaulting to "group" when the caller does not provide one.
Finding: Latent allowlist-scope bypass
gateway/run.py:8018 — chat_type=chat_type or "group" is a latent privilege-escalation risk (medium severity, 85 confidence). When chat_type is omitted, the default "group" causes _is_user_authorized in authz_mixin.py to consult group-scoped platform allowlists (TELEGRAM_GROUP_ALLOWED_USERS, etc.) and merge them into the authorized set. The authz code's own comment explicitly states that group allowlists "should not imply DM access" (authz_mixin.py:459-462). The sole current caller (SlackAdapter._fetch_thread_context) always passes chat_type='thread', so this is not exploitable today, but any future adapter or code path that calls _is_sender_authorized without an explicit chat_type would inherit the unsafe default.
Recommendation
Change the fallback to chat_type or "dm" (matching SessionSource's own default at session.py:105). This is a one-line fix that eliminates the latent risk without affecting the current Slack caller.
| source = SessionSource( | ||
| platform=platform, | ||
| chat_id=chat_id or "", | ||
| chat_type=chat_type or "group", |
There was a problem hiding this comment.
🟡 _make_adapter_auth_check defaults chat_type to 'group', creating latent allowlist-scope bypass (security)
In gateway/run.py, the _make_adapter_auth_check method (introduced in this PR) constructs a SessionSource with chat_type=chat_type or 'group' (line 8018). When chat_type is not provided by the caller, this defaults to 'group', which causes _is_user_authorized (authz_mixin.py:375-377) to consult group-scoped allowlists (TELEGRAM_GROUP_ALLOWED_USERS and similar platform-specific group user allowlists) and merge their entries into allowed_ids (line 466-467). This means a user who is only on the group allowlist could be authorized for a non-group context. The code's own comment at authz_mixin.py:459-462 explicitly states: 'TELEGRAM_GROUP_ALLOWED_USERS is the scoped allowlist and should not imply DM access.' The default contradicts this intent. Currently, the sole caller (SlackAdapter._fetch_thread_context at adapter.py:3675) always passes chat_type='thread', so the unsafe default is never exercised. However, it creates a latent privilege-escalation path for any future adapter or code path that calls _is_sender_authorized without an explicit chat_type.
💡 Suggestion: Change the default from chat_type or 'group' to chat_type or 'dm' to match SessionSource's own default (session.py:105) and prevent future auth decisions from accidentally using group-scoped allowlists for non-group messages. Alternatively, log a warning when chat_type is not provided to catch callers that should be explicit.
| chat_type=chat_type or "group", | |
| chat_type=chat_type or "dm", |
📋 Prompt for AI Agents
In gateway/run.py, line 8018, change chat_type=chat_type or "group" to chat_type=chat_type or "dm" so the fallback matches SessionSource's own default ('dm') and does not incorrectly trigger group-scoped authorization paths when a caller omits chat_type.
Summary
Slack thread context now marks messages from senders not on the allowlist as
[unverified], so the LLM treats them as background reference rather than authoritative input — closing an indirect prompt-injection vector (CWE-863).Root cause: When the bot is @-mentioned mid-thread for the first time,
_fetch_thread_context()pulls the full thread viaconversations.repliesand prepends every reply to the LLM prompt. Replies from non-allowlisted senders were rendered identically to authorised ones, so a third party in a shared channel could plant instructions the model might act on when answering the next authorised message.Changes
gateway/platforms/base.py:BasePlatformAdapter.set_authorization_check()/_is_sender_authorized()— an optional, adapter-agnostic auth hook. ReturnsNonewhen no check is registered (legacy behaviour preserved).gateway/run.py:GatewayRunner._make_adapter_auth_check()builds a platform-bound callback that closes over the existing_is_user_authorizedchain — platform + global + group allowlists, allow-all flags, and the pairing store stay the single source of truth. No env-var re-parsing inside adapters. Wired into all three adapter-init sites (start, reconnect watcher, restart).plugins/platforms/slack/adapter.py: tags non-bot thread replies whose sender fails the auth check with[unverified]; strengthens the context header with soft guidance only when at least one unverified message is present, so allowlist-free setups see no behaviour change.tests/gateway/test_slack.py: 7 tests (legacy format when no callback, no tags when all authorised, tagging when any unauthorised, strong vs. legacy header switch, callback-exception safety, forwardedchat_type/chat_id).Wording
Softened from the original
[untrusted]tag to[unverified]with non-accusatory header framing. The label reflects the sender's allowlist status (identity not confirmed) — not a judgment about the person. Same security behaviour; the model still won't act on injected instructions.Validation
tests/gateway/test_slack.pySessionSourcebuilt with correct fieldsRelation to NousResearch#10035
NousResearch#10035 fixes the same bug (submitted earlier) but re-parses
SLACK_ALLOWED_USERS/*_ALLOW_ALL_USERSenv vars directly inside the adapter — missing the pairing store, the globalGATEWAY_ALLOWED_USERSallowlist, and group allowlists, and duplicating auth logic. This PR routes through the single_is_user_authorizedsource of truth. Closing NousResearch#10035 in favour of this, with credit.Salvaged from NousResearch#17059 by @syahidfrd — authorship preserved. Adapter relocated to
plugins/platforms/slack/and wired into the third (reconnect) init site since the PR was authored.Infographic
Mirror-of: NousResearch#55979
NousResearch#55979