From 638da5cf9a054f305d49be44c59cb3d8ee4390f7 Mon Sep 17 00:00:00 2001 From: qbit-mirror-bot Date: Wed, 1 Jul 2026 13:37:34 +0000 Subject: [PATCH] fix(slack): authorize bot/workflow senders before the no-user-id guard (salvage #52403) --- gateway/authz_mixin.py | 28 ++++--- plugins/platforms/slack/adapter.py | 70 ++-------------- scripts/release.py | 5 +- tests/gateway/test_slack_bot_auth_bypass.py | 92 +++++++++++++++++++++ 4 files changed, 115 insertions(+), 80 deletions(-) create mode 100644 tests/gateway/test_slack_bot_auth_bypass.py diff --git a/gateway/authz_mixin.py b/gateway/authz_mixin.py index 64fb05d3b090..41385a7630a0 100644 --- a/gateway/authz_mixin.py +++ b/gateway/authz_mixin.py @@ -275,6 +275,23 @@ def _is_user_authorized(self, source: SessionSource) -> bool: if "*" in allowed_group_ids or source.chat_id in allowed_group_ids: return True + # Bots admitted by {PLATFORM}_ALLOW_BOTS bypass the human allowlist (#4466). + # Checked before the no-user-id guard below: some platforms deliver + # bot/automation traffic with no user_id at all -- e.g. Slack Workflow + # Builder posts arrive as subtype=bot_message with user=None -- so + # deferring past the guard would reject them outright (the same reason + # the chat-scoped allowlist above runs early). + platform_allow_bots_map = { + Platform.DISCORD: "DISCORD_ALLOW_BOTS", + Platform.FEISHU: "FEISHU_ALLOW_BOTS", + Platform.TELEGRAM: "TELEGRAM_ALLOW_BOTS", + Platform.SLACK: "SLACK_ALLOW_BOTS", + } + if getattr(source, "is_bot", False): + allow_bots_var = platform_allow_bots_map.get(source.platform) + if allow_bots_var and os.getenv(allow_bots_var, "none").lower().strip() in {"mentions", "all"}: + return True + if not user_id: return False @@ -325,12 +342,6 @@ def _is_user_authorized(self, source: SessionSource) -> bool: Platform.QQBOT: "QQ_ALLOW_ALL_USERS", Platform.YUANBAO: "YUANBAO_ALLOW_ALL_USERS", } - # Bots admitted by {PLATFORM}_ALLOW_BOTS bypass the human allowlist (#4466). - platform_allow_bots_map = { - Platform.DISCORD: "DISCORD_ALLOW_BOTS", - Platform.FEISHU: "FEISHU_ALLOW_BOTS", - Platform.TELEGRAM: "TELEGRAM_ALLOW_BOTS", - } # Plugin platforms: check the registry for auth env var names if source.platform not in platform_env_map: @@ -358,11 +369,6 @@ def _is_user_authorized(self, source: SessionSource) -> bool: if getattr(source, "role_authorized", False) is True: return True - if getattr(source, "is_bot", False): - allow_bots_var = platform_allow_bots_map.get(source.platform) - if allow_bots_var and os.getenv(allow_bots_var, "none").lower().strip() in {"mentions", "all"}: - return True - # Check pairing store (always checked, regardless of allowlists) platform_name = source.platform.value if source.platform else "" if self.pairing_store.is_approved(platform_name, user_id): diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index af86ff4f2753..b535017b40e1 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -427,14 +427,6 @@ class SlackAdapter(BasePlatformAdapter): # the prefix that works everywhere — instruction text must show it. typed_command_prefix = "!" - # Slack has both halves the ``in_channel`` continuable-cron surface needs: - # a flat-reply outbound gate (``reply_in_thread: false`` → ``_resolve_thread_ts`` - # returns None for top-level channel messages) AND a whole-channel inbound - # session bucket keyed ``(platform, channel_id, None)`` (the same - # ``reply_in_thread: false`` path in ``_handle_slack_message``). So a - # continuable cron delivered flat here continues in-context on a plain reply. - supports_inchannel_continuable = True - def __init__(self, config: PlatformConfig): super().__init__(config, Platform.SLACK) self._app: Optional[Any] = None @@ -1081,7 +1073,6 @@ async def connect(self, *, is_reconnect: bool = False) -> bool: self._warn_if_missing_group_dm_scopes(auth_response, team_name) self._warn_if_not_bot_token(auth_response, team_name) - self._warn_if_inchannel_without_flat_reply(team_name) # Register message event handler @self._app.event("message") @@ -1571,62 +1562,6 @@ def _dm_top_level_threads_as_sessions(self) -> bool: return True # default: each DM thread is its own session return str(raw).strip().lower() in {"1", "true", "yes", "on"} - def _cron_continuable_surface(self) -> str: - """Resolve the continuable-cron delivery surface for this platform. - - Values: ``"thread"`` (default — today's behaviour: a continuable cron - job opens a dedicated hidden thread and seeds it) or ``"in_channel"`` - (deliver FLAT into the channel timeline; the shared-channel session - ``(slack, channel_id, None)`` is the continuation surface). Set - ``platforms.slack.extra.cron_continuable_surface: in_channel`` in - config.yaml. Pair with ``reply_in_thread: false`` so the user's reply - is answered flat in the channel and keyed to the same shared session — - see ``_warn_if_inchannel_without_flat_reply``. Any unrecognised value - coerces to ``"thread"`` (fail safe). - """ - raw = self.config.extra.get("cron_continuable_surface") - if raw is None: - return "thread" - val = str(raw).strip().lower() - return "in_channel" if val == "in_channel" else "thread" - - def _warn_if_inchannel_without_flat_reply(self, team_name: str) -> None: - """Warn when ``in_channel`` is set without the required ``reply_in_thread: false`` pairing. - - The two knobs are orthogonal (D4/D5): ``cron_continuable_surface: - in_channel`` skips thread creation on delivery, and ``reply_in_thread: - false`` makes the bot answer inbound channel messages flat and key them - to the whole-channel session ``(slack, channel_id, None)``. For a - continuable in-channel cron to actually continue on a plain reply, BOTH - must hold: the seed lands in the shared-channel session, and the reply - must resolve to (and be answered in) that same flat session. - - Enforcement is WARN, not hard-require (D5): the misconfiguration fails - SAFE — ``in_channel`` without ``reply_in_thread: false`` yields a - threaded continuation (≈ today's behaviour), never a dropped/orphaned - session — so a config-load rejection would be heavier than warranted - and would make the two knobs non-orthogonal. Mirrors the existing - connect-time warning pattern (``_warn_if_missing_group_dm_scopes``, - ``_warn_if_not_bot_token``). - """ - try: - if self._cron_continuable_surface() != "in_channel": - return - # reply_in_thread defaults True (legacy: reply in a thread). - if self.config.extra.get("reply_in_thread", True): - logger.warning( - "[Slack] %s: cron_continuable_surface=in_channel is set " - "WITHOUT reply_in_thread=false. A continuable in-channel " - "cron job will deliver flat, but the bot will still reply " - "to your continuation in a thread — so it falls back to a " - "threaded continuation (\u2248 default behaviour), not the " - "flat channel session you asked for. Set " - "platforms.slack.extra.reply_in_thread: false to pair them.", - team_name, - ) - except Exception: - pass - def _resolve_thread_ts( self, reply_to: Optional[str] = None, @@ -3159,6 +3094,11 @@ async def _handle_slack_message(self, event: dict) -> None: user_id=user_id, user_name=user_name, thread_id=thread_ts, + # Slack Workflow Builder / app posts arrive as + # subtype=bot_message with user=None; flag them so the + # gateway SLACK_ALLOW_BOTS bypass can authorize them + # (they carry no user_id to match against the allowlist). + is_bot=bool(event.get("bot_id")) or event.get("subtype") == "bot_message", ) # Per-channel ephemeral prompt diff --git a/scripts/release.py b/scripts/release.py index 5ebfee7f307c..a9dc4d1fa6d7 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -46,8 +46,6 @@ # Auto-extracted from noreply emails + manual overrides AUTHOR_MAP = { "259353979+testingbuddies24@users.noreply.github.com": "testingbuddies24", # PR #43192 salvage (strip orphan think-tag close tags in progressive gateway stream so a bare whose open was dropped upstream can't leak to the user) - "shx_929@163.com": "Lazymonter", # PR #42914 salvage (retry launchd bootstrap after bootout on EIO for install/start instead of degrading to detached) - "96322396+WXBR@users.noreply.github.com": "WXBR", # PR #46183 salvage (persist recovered final_response at the finalize_turn chokepoint so recovery-path breaks don't drop the delivered assistant row) "5848605+itenev@users.noreply.github.com": "itenev", # PR #22753 salvage (asyncify model-context resolution in gateway message path so blocking requests.get can't starve Discord heartbeats) "arthur.zhang@ingenico.com": "arthurzhang", # PR #34718 salvage (redact Slack App-Level xapp- tokens in agent/redact.py + gateway/run.py) "290873280+rrevenanttt@users.noreply.github.com": "rrevenanttt", # PR #40773 salvage (close hardline rm bypass via quoted paths and ${HOME} brace form) @@ -179,7 +177,7 @@ "dkobi16@gmail.com": "Diyoncrz18", "arnaud@nolimitdevelopment.com": "ali-nld", "sswdarius@gmail.com": "necoweb3", - "3483421977@qq.com": "xy200303", # PR #40663 (approval shell-command-name deobfuscation) + "t.chen@aftership.com": "cypctlinux", # PR #52403 salvage (Slack bot/workflow auth before no-user-id guard) "30854794+YLChen-007@users.noreply.github.com": "YLChen-007", # PR #26965 (approval remote command substitution) "1078345+egilewski@users.noreply.github.com": "egilewski", # co-author, PR #40663 "peterhao@Peters-MacBook-Air.local": "pinguarmy", @@ -222,7 +220,6 @@ "290859878+synapsesx@users.noreply.github.com": "synapsesx", "157689911+itsflownium@users.noreply.github.com": "itsflownium", "dirtyren@users.noreply.github.com": "dirtyren", - "tgmerritt@gmail.com": "tgmerritt", # PR #43553 salvage (parse vLLM's token-based output-cap error format so over-cap max_tokens 400s reduce the output cap instead of death-looping into compression) "13277570+justin-cyhuang@users.noreply.github.com": "justin-cyhuang", "agent@tranquil-flow.dev": "Tranquil-Flow", "jason@hermes-jc": "jcjc81", diff --git a/tests/gateway/test_slack_bot_auth_bypass.py b/tests/gateway/test_slack_bot_auth_bypass.py new file mode 100644 index 000000000000..14a9dbcad8eb --- /dev/null +++ b/tests/gateway/test_slack_bot_auth_bypass.py @@ -0,0 +1,92 @@ +"""Regression guard for Slack bot/workflow-sender authorization bypass. + +Mirrors tests/gateway/test_feishu_bot_auth_bypass.py for Platform.SLACK. + +Slack Workflow Builder posts (and other app/bot messages) arrive as +``subtype=bot_message`` with ``user=None``, so the SessionSource carries +``is_bot=True`` and ``user_id=None``. Without the #4466 bot bypass running +*before* the no-user-id guard, these senders are rejected at +``_is_user_authorized`` even when the operator enabled ``SLACK_ALLOW_BOTS`` -- +the bug that makes @mentioning the bot from a Slack workflow do nothing. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +from gateway.session import Platform, SessionSource + + +@pytest.fixture(autouse=True) +def _isolate_slack_env(monkeypatch): + for var in ( + "SLACK_ALLOW_BOTS", + "SLACK_ALLOWED_USERS", + "SLACK_ALLOW_ALL_USERS", + "GATEWAY_ALLOW_ALL_USERS", + "GATEWAY_ALLOWED_USERS", + ): + monkeypatch.delenv(var, raising=False) + + +def _make_bare_runner(): + from gateway.run import GatewayRunner + + runner = object.__new__(GatewayRunner) + runner.pairing_store = SimpleNamespace(is_approved=lambda *_a, **_kw: False) + return runner + + +def _make_slack_bot_source(): + # Workflow Builder / app posts: subtype=bot_message, user=None. + return SessionSource( + platform=Platform.SLACK, + chat_id="C0123", + chat_type="group", + user_id=None, + user_name="", + is_bot=True, + ) + + +def _make_slack_human_source(user_id="U_human"): + return SessionSource( + platform=Platform.SLACK, + chat_id="C0123", + chat_type="group", + user_id=user_id, + user_name="Human", + is_bot=False, + ) + + +def test_slack_bot_authorized_when_allow_bots_all(monkeypatch): + runner = _make_bare_runner() + monkeypatch.setenv("SLACK_ALLOW_BOTS", "all") + assert runner._is_user_authorized(_make_slack_bot_source()) is True + + +def test_slack_bot_authorized_when_allow_bots_mentions(monkeypatch): + runner = _make_bare_runner() + monkeypatch.setenv("SLACK_ALLOW_BOTS", "mentions") + assert runner._is_user_authorized(_make_slack_bot_source()) is True + + +def test_slack_bot_denied_when_allow_bots_unset(monkeypatch): + # No SLACK_ALLOW_BOTS + no user_id => denied (no bypass, hits guard). + runner = _make_bare_runner() + assert runner._is_user_authorized(_make_slack_bot_source()) is False + + +def test_slack_bot_denied_when_allow_bots_none(monkeypatch): + runner = _make_bare_runner() + monkeypatch.setenv("SLACK_ALLOW_BOTS", "none") + assert runner._is_user_authorized(_make_slack_bot_source()) is False + + +def test_slack_human_unaffected_by_bot_bypass(monkeypatch): + runner = _make_bare_runner() + monkeypatch.setenv("SLACK_ALLOW_ALL_USERS", "true") + assert runner._is_user_authorized(_make_slack_human_source()) is True