Skip to content

fix(security): reuse auth chain when tagging unverified senders in Slack threads - #55979

Merged
teknium1 merged 1 commit into
mainfrom
hermes/hermes-1d157289
Jul 1, 2026
Merged

fix(security): reuse auth chain when tagging unverified senders in Slack threads#55979
teknium1 merged 1 commit into
mainfrom
hermes/hermes-1d157289

Conversation

@teknium1

@teknium1 teknium1 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

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 via conversations.replies and 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. Returns None when 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_authorized chain — 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, forwarded chat_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

Result
tests/gateway/test_slack.py 216/216 pass (7 new)
E2E auth-chain closure allowlisted → True, non-allowlisted → False, empty user short-circuits; SessionSource built with correct fields

Relation to #10035

#10035 fixes the same bug (submitted earlier) but re-parses SLACK_ALLOWED_USERS / *_ALLOW_ALL_USERS env vars directly inside the adapter — missing the pairing store, the global GATEWAY_ALLOWED_USERS allowlist, and group allowlists, and duplicating auth logic. This PR routes through the single _is_user_authorized source of truth. Closing #10035 in favour of this, with credit.

Salvaged from #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

Slack unverified-sender tagging

…ack threads

Mitigates indirect prompt injection (CWE-863) in Slack thread context.
When the bot is mentioned mid-thread for the first time, _fetch_thread_context
pulls the full thread via conversations.replies and prepends every reply to
the LLM prompt. Replies from senders not on the allowlist were rendered
identically to authorised senders, letting a third party in a shared channel
inject instructions the model might act on when answering the next authorised
message.

- BasePlatformAdapter.set_authorization_check / _is_sender_authorized, registered
  by GatewayRunner._make_adapter_auth_check() with a closure over the existing
  _is_user_authorized chain (platform/global/group allowlists, allow-all flags,
  pairing store all stay the single source of truth — no env-var re-parsing).
- Tags non-bot thread messages whose sender fails the auth check with an
  [unverified] prefix; strengthens the header with soft guidance only when at
  least one unverified message is present, so setups without an allowlist see
  no behaviour change.
- Wired into all three adapter-init sites in run.py (start, reconnect watcher,
  restart) so the reconnect path is covered too.

Softened wording: adapted from the original [untrusted] tag to [unverified]
and non-accusatory header framing — the label reflects allowlist status, not
a judgment about the person. Adapter relocated to plugins/platforms/slack/
since the PR was authored.

Salvaged from #17059.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jul 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this is the salvage of #17059 (authorship preserved by @syahidfrd, adapter relocated to plugins/platforms/slack/) and supersedes #10035 (which re-parsed env vars in the adapter, missing the pairing store + global/group allowlists). This PR routes through the single _is_user_authorized chain. #17059 and #10035 are the earlier attempts at the same CWE-863 fix — a maintainer should pick this canonical version and close the predecessors.

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/slack Slack app adapter sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants