From ec7e96af60dfca8d40e9a3e5a23fc8a9edc937e6 Mon Sep 17 00:00:00 2001 From: Ludmila Date: Mon, 27 Jul 2026 12:43:08 +0000 Subject: [PATCH] fix(slack): stop interactive-caller auth falling open on multiplexed profiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SlackAdapter._is_interactive_user_authorized` gates approval-button, slash-confirm and clarify clicks. Button clicks bypass the normal message auth flow in `gateway/run.py`, so this is the only gate on that path. It resolved the gateway auth chain by introspecting `_message_handler.__self__`. On a multiplexed profile the handler is the closure built by `GatewayRunner._make_profile_message_handler`, which has no `__self__`, so the introspection silently yielded nothing and the method fell through to its env-only fallback. That fallback opened with a raw `os.getenv("SLACK_ALLOW_ALL_USERS")` read of the process environment — the DEFAULT profile's `.env` — and its `_env()` helper fell through to `os.getenv` on any scope miss. A secondary profile therefore inherited another profile's allowlist and allow-all flags, and the direction is fail-open: a caller that profile never allowlisted could resolve its approvals. The profile-bound callback the multiplexer already registers via `set_authorization_check` (`gateway/run.py`) was never consulted. Same introspection gap #65589 describes for Telegram, where the fallback happens to fail closed. On Slack it fails open. - Prefer the injected `_is_sender_authorized` check, which delegates to the full `_is_user_authorized` chain under this adapter's own profile. The `__self__` introspection stays as the next step for adapters wired without it (bare-adapter embedding, existing tests). - Make the env fallback multiplex-safe: on a scope miss or `UnscopedSecretError`, fail closed instead of reading `os.environ`. Single-profile deployments keep the plain env read — there is no other profile to leak from. - Route the early `SLACK_ALLOW_ALL_USERS` check through the same scoped helper, preserving its precedence over the allowlists. Co-Authored-By: Claude Fable 5 --- plugins/platforms/slack/adapter.py | 57 +++- .../test_slack_interactive_authz_multiplex.py | 258 ++++++++++++++++++ 2 files changed, 304 insertions(+), 11 deletions(-) create mode 100644 tests/gateway/test_slack_interactive_authz_multiplex.py diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index a34c995cc81f5..097d555a4a20b 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -6647,6 +6647,24 @@ def _is_interactive_user_authorized( if not normalized_user_id: return False + chat_type = "dm" if str(channel_id or "").startswith("D") else "group" + + # The authorization callback ``GatewayRunner`` injects at adapter-connect + # time (``set_authorization_check``) is the authoritative chain: env + # allowlists, config allowlists, group allowlists, pairing store, and + # allow-all flags, all resolved under THIS adapter's profile. It is + # registered for primary and multiplexed adapters alike, so prefer it + # over the ``__self__`` introspection below — which resolves to nothing + # on a multiplexed profile, whose message handler is the closure built + # by ``GatewayRunner._make_profile_message_handler`` (no ``__self__``). + injected = self._is_sender_authorized( + normalized_user_id, chat_type, str(channel_id or "") + ) + if injected is not None: + return injected + + # Legacy resolution for adapters wired without the injected check + # (bare-adapter embedding, existing tests). runner = getattr(getattr(self, "_message_handler", None), "__self__", None) auth_fn = getattr(runner, "_is_user_authorized", None) if callable(auth_fn): @@ -6656,7 +6674,7 @@ def _is_interactive_user_authorized( source = SessionSource( platform=Platform.SLACK, chat_id=str(channel_id or normalized_user_id), - chat_type="dm" if str(channel_id or "").startswith("D") else "group", + chat_type=chat_type, user_id=normalized_user_id, user_name=str(user_name).strip() if user_name else None, scope_id=str(team_id) if team_id else None, @@ -6669,21 +6687,40 @@ def _is_interactive_user_authorized( exc_info=True, ) - if os.getenv("SLACK_ALLOW_ALL_USERS", "").lower() in {"true", "1", "yes"}: - return True - def _env(name: str) -> str: - # Multiplex: profile .env is in secret_scope, not process environ. + """Read an authz env var, honoring the active profile secret scope. + + Under multiplexing a scope miss must NOT fall through to + ``os.environ``: that holds the DEFAULT profile's values, so falling + through lets one profile's allowlist / allow-all flag authorize + callers on another profile's bot. ``agent.secret_scope`` already + refuses that read (returning the default, or raising + ``UnscopedSecretError`` when no scope is installed); this helper + must not undo it. Fail closed instead, and keep the plain + ``os.environ`` read for single-profile deployments, where there is + no other profile to leak from. + """ try: - from agent.secret_scope import get_secret + from agent.secret_scope import get_secret, is_multiplex_active + except Exception: + return (os.getenv(name) or "").strip() + try: val = get_secret(name) - if val is not None and str(val).strip(): - return str(val).strip() except Exception: - pass + # UnscopedSecretError under multiplex is the deliberate + # fail-closed signal — never downgrade it to an os.environ read. + return "" if is_multiplex_active() else (os.getenv(name) or "").strip() + + if val is not None and str(val).strip(): + return str(val).strip() + if is_multiplex_active(): + return "" return (os.getenv(name) or "").strip() + if _env("SLACK_ALLOW_ALL_USERS").lower() in {"true", "1", "yes"}: + return True + allowed_ids = set() platform_allowlist = _env("SLACK_ALLOWED_USERS") if platform_allowlist: @@ -6695,8 +6732,6 @@ def _env(name: str) -> str: if allowed_ids: return "*" in allowed_ids or normalized_user_id in allowed_ids - if _env("SLACK_ALLOW_ALL_USERS").lower() in {"true", "1", "yes"}: - return True return _env("GATEWAY_ALLOW_ALL_USERS").lower() in {"true", "1", "yes"} async def _handle_slash_confirm_action(self, ack, body, action) -> None: diff --git a/tests/gateway/test_slack_interactive_authz_multiplex.py b/tests/gateway/test_slack_interactive_authz_multiplex.py new file mode 100644 index 0000000000000..dedf657b86eec --- /dev/null +++ b/tests/gateway/test_slack_interactive_authz_multiplex.py @@ -0,0 +1,258 @@ +"""Regression tests for Slack interactive-caller auth under multiplex profiles. + +``SlackAdapter._is_interactive_user_authorized`` gates approval-button, +slash-confirm and clarify clicks. Button clicks bypass the normal message auth +flow in ``gateway/run.py``, so this is the only gate on that path. + +Two failure modes are covered here: + +1. It used to resolve the gateway auth chain only by introspecting + ``_message_handler.__self__``. On a multiplexed profile the handler is the + closure built by ``GatewayRunner._make_profile_message_handler``, which has + no ``__self__``, so the introspection silently yielded nothing and the + method fell through to its env-only fallback. +2. That fallback read the process environment — the DEFAULT profile's ``.env`` + — so a secondary profile inherited another profile's allowlist and + allow-all flags. Direction is fail-OPEN: a caller the profile never + allowlisted could resolve its approvals. +""" + +import sys +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock + +import pytest + +# --------------------------------------------------------------------------- +# Ensure the repo root is importable +# --------------------------------------------------------------------------- +_repo = str(Path(__file__).resolve().parents[2]) +if _repo not in sys.path: + sys.path.insert(0, _repo) + + +# --------------------------------------------------------------------------- +# Minimal Slack SDK mock so SlackAdapter can be imported +# --------------------------------------------------------------------------- +def _ensure_slack_mock(): + if "slack_bolt" in sys.modules: + return + slack_bolt = MagicMock() + slack_bolt.async_app.AsyncApp = MagicMock + sys.modules["slack_bolt"] = slack_bolt + sys.modules["slack_bolt.async_app"] = slack_bolt.async_app + handler_mod = MagicMock() + handler_mod.AsyncSocketModeHandler = MagicMock + sys.modules["slack_bolt.adapter"] = MagicMock() + sys.modules["slack_bolt.adapter.socket_mode"] = MagicMock() + sys.modules["slack_bolt.adapter.socket_mode.async_handler"] = handler_mod + sdk_mod = MagicMock() + sdk_mod.web = MagicMock() + sdk_mod.web.async_client = MagicMock() + sdk_mod.web.async_client.AsyncWebClient = MagicMock + sys.modules["slack_sdk"] = sdk_mod + sys.modules["slack_sdk.web"] = sdk_mod.web + sys.modules["slack_sdk.web.async_client"] = sdk_mod.web.async_client + + +_ensure_slack_mock() + +from agent import secret_scope # noqa: E402 +from gateway.config import PlatformConfig # noqa: E402 +from plugins.platforms.slack.adapter import SlackAdapter # noqa: E402 + + +_AUTH_ENV_KEYS = ( + "SLACK_ALLOWED_USERS", + "SLACK_ALLOW_ALL_USERS", + "GATEWAY_ALLOWED_USERS", + "GATEWAY_ALLOW_ALL_USERS", +) + +OWNER = "U_OWNER_B" +STRANGER = "U_STRANGER" + + +@pytest.fixture +def multiplex(monkeypatch): + """Enable/disable multiplex without leaking the module global across tests.""" + previous = secret_scope.is_multiplex_active() + + def _set(active: bool): + secret_scope.set_multiplex_active(active) + + yield _set + secret_scope.set_multiplex_active(previous) + + +@pytest.fixture +def profile_scope(): + """Install a profile secret scope, always resetting it.""" + tokens = [] + + def _install(mapping): + tokens.append(secret_scope.set_secret_scope(mapping)) + + yield _install + for token in reversed(tokens): + secret_scope.reset_secret_scope(token) + + +@pytest.fixture +def clean_env(monkeypatch): + for key in _AUTH_ENV_KEYS: + monkeypatch.delenv(key, raising=False) + return monkeypatch + + +def _make_adapter(): + adapter = SlackAdapter(PlatformConfig(enabled=True, token="xoxb-test-token")) + adapter._app = MagicMock() + adapter._bot_user_id = "U_BOT" + return adapter + + +def _multiplexed_handler(): + """A per-profile message handler closure — no ``__self__``, like the real one.""" + + async def _handler(event): + return None + + return _handler + + +def test_injected_authorization_check_is_preferred(clean_env, multiplex, profile_scope): + """The profile-bound callback wins over process env and handler introspection.""" + clean_env.setenv("SLACK_ALLOW_ALL_USERS", "true") + multiplex(True) + profile_scope({"SLACK_BOT_TOKEN": "xoxb-b"}) + + adapter = _make_adapter() + adapter._message_handler = _multiplexed_handler() + + seen = [] + + def check(user_id, chat_type=None, chat_id=None): + seen.append((user_id, chat_type, chat_id)) + return user_id == OWNER + + adapter.set_authorization_check(check) + + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is False + assert adapter._is_interactive_user_authorized(OWNER, channel_id="C1") is True + assert seen == [(STRANGER, "group", "C1"), (OWNER, "group", "C1")] + + +def test_dm_channel_passes_dm_chat_type(clean_env, multiplex): + """A ``D``-prefixed channel is reported to the auth chain as a DM.""" + multiplex(False) + adapter = _make_adapter() + seen = [] + + def check(user_id, chat_type=None, chat_id=None): + seen.append(chat_type) + return True + + adapter.set_authorization_check(check) + + adapter._is_interactive_user_authorized(OWNER, channel_id="D123") + assert seen == ["dm"] + + +def test_multiplex_does_not_inherit_process_env_allow_all( + clean_env, multiplex, profile_scope +): + """A secondary profile must not inherit the default profile's allow-all flag.""" + clean_env.setenv("SLACK_ALLOW_ALL_USERS", "true") + clean_env.setenv("GATEWAY_ALLOW_ALL_USERS", "true") + multiplex(True) + profile_scope({"SLACK_BOT_TOKEN": "xoxb-b", "SLACK_ALLOWED_USERS": OWNER}) + + adapter = _make_adapter() + adapter._message_handler = _multiplexed_handler() + + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is False + assert adapter._is_interactive_user_authorized(OWNER, channel_id="C1") is True + + +def test_multiplex_does_not_inherit_process_env_allowlist( + clean_env, multiplex, profile_scope +): + """A secondary profile must not inherit the default profile's allowlists.""" + clean_env.setenv("SLACK_ALLOWED_USERS", STRANGER) + clean_env.setenv("GATEWAY_ALLOWED_USERS", STRANGER) + multiplex(True) + profile_scope({"SLACK_BOT_TOKEN": "xoxb-b", "SLACK_ALLOWED_USERS": OWNER}) + + adapter = _make_adapter() + adapter._message_handler = _multiplexed_handler() + + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is False + assert adapter._is_interactive_user_authorized(OWNER, channel_id="C1") is True + + +def test_multiplex_without_scope_fails_closed(clean_env, multiplex): + """No scope installed under multiplex is the fail-closed signal, not an env read.""" + clean_env.setenv("SLACK_ALLOW_ALL_USERS", "true") + clean_env.setenv("GATEWAY_ALLOWED_USERS", STRANGER) + multiplex(True) + + adapter = _make_adapter() + adapter._message_handler = _multiplexed_handler() + + # get_secret raises UnscopedSecretError here; it must not be downgraded + # to an os.environ read. + with pytest.raises(secret_scope.UnscopedSecretError): + secret_scope.get_secret("SLACK_ALLOW_ALL_USERS") + + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is False + + +def test_single_profile_env_fallback_unchanged(clean_env, multiplex): + """Single-profile deployments keep reading credentials from the process env.""" + multiplex(False) + adapter = _make_adapter() + adapter._message_handler = _multiplexed_handler() + + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is False + + clean_env.setenv("SLACK_ALLOWED_USERS", OWNER) + assert adapter._is_interactive_user_authorized(OWNER, channel_id="C1") is True + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is False + + clean_env.delenv("SLACK_ALLOWED_USERS") + clean_env.setenv("SLACK_ALLOW_ALL_USERS", "true") + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is True + + +def test_handler_introspection_still_honored_without_injected_check( + clean_env, multiplex +): + """Bare-adapter embedding keeps working via ``_message_handler.__self__``.""" + multiplex(False) + adapter = _make_adapter() + + class _Runner: + def __init__(self): + self.seen = [] + + def _is_user_authorized(self, source): + self.seen.append(source.user_id) + return source.user_id == OWNER + + async def handle(self, event): + return None + + runner = _Runner() + adapter._message_handler = runner.handle + + assert adapter._is_interactive_user_authorized(OWNER, channel_id="C1") is True + assert adapter._is_interactive_user_authorized(STRANGER, channel_id="C1") is False + assert runner.seen == [OWNER, STRANGER] + + +def test_empty_user_id_denied(multiplex): + multiplex(False) + adapter = _make_adapter() + assert adapter._is_interactive_user_authorized("") is False + assert adapter._is_interactive_user_authorized(" ") is False