fix(slack): stop interactive-caller auth falling open on multiplexed profiles

[MilaArtyNew]

What does this PR do?

Closes a fail-open in Slack interactive-caller authorization on multiplexed 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. On a secondary profile it stopped consulting the gateway auth chain entirely and authorized callers from the default profile's process environment instead.

Bug Cause

Two defects compose:

1. The method resolved the gateway auth chain by introspecting _message_handler.__self__. Under gateway.multiplex_profiles the handler is the closure built by GatewayRunner._make_profile_message_handler, which has no __self__, so the introspection silently yielded nothing and control fell through to the env-only fallback. This is the same introspection gap fix(telegram): button-caller authorization breaks on multiplexed profiles (handler introspection) #65589 describes for Telegram.

2. That fallback read the process environment — the default profile's .env. Its first line was a raw os.getenv("SLACK_ALLOW_ALL_USERS"), and its _env() helper fell through to os.getenv whenever the profile secret scope returned empty or raised. agent/secret_scope.py deliberately refuses that read (returning the default under multiplex, raising UnscopedSecretError when no scope is installed); the helper undid it.

So a secondary profile inherited another profile's SLACK_ALLOWED_USERS / GATEWAY_ALLOWED_USERS and, worse, its SLACK_ALLOW_ALL_USERS / GATEWAY_ALLOW_ALL_USERS flags. On Telegram (#65589) this fallback happens to fail closed, so it is a denial bug. On Slack it fails open: a caller the profile never allowlisted can resolve its approvals.

Meanwhile the correct authority was already available and unused — the multiplexer registers a profile-bound callback via adapter.set_authorization_check(self._make_adapter_auth_check(platform, profile_name=profile_name)), which delegates to the full _is_user_authorized chain (env allowlists, config allowlists, group allowlists, pairing store, allow-all flags) under that profile's own secret scope.

This surface has been hardened before — #36848, #41226, #33844 — and the multiplex path reintroduced the same failure direction.

Reproduction Steps

1. Enable gateway.multiplex_profiles with a default profile and a secondary profile running its own Slack app.
2. Default profile's .env sets SLACK_ALLOW_ALL_USERS=true (or simply lists its own users in SLACK_ALLOWED_USERS).
3. Secondary profile's .env sets only its bot token plus SLACK_ALLOWED_USERS=<its owner>.
4. From a Slack user that is not in the secondary profile's allowlist, click an exec-approval button posted by the secondary profile's bot.

Expected: ignored, [Slack] Unauthorized approval click by ….
Before this PR: the click is honored.

Equivalent unit-level repro:

adapter = SlackAdapter(PlatformConfig(enabled=True, token="xoxb-test"))
adapter.set_authorization_check(lambda uid, ct=None, cid=None: uid == "U_OWNER")
adapter._message_handler = _profile_closure()      # no __self__, as multiplex builds it

os.environ["SLACK_ALLOW_ALL_USERS"] = "true"       # default profile's .env
set_multiplex_active(True)
set_secret_scope({"SLACK_BOT_TOKEN": "xoxb-b"})    # secondary profile's .env

adapter._is_interactive_user_authorized("U_STRANGER", channel_id="C1")
# before: True, and the registered check is never called

Fix

• Prefer the injected self._is_sender_authorized(...) check (registered for primary and multiplexed adapters alike) before any other resolution. Returns True/False when a check is wired, None when it isn't.
• Keep the __self__ introspection as the next step, for adapters wired without the injected check (bare-adapter embedding, existing tests).
• Make the env fallback multiplex-safe: on a scope miss or UnscopedSecretError, fail closed rather than 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 existing precedence over the allowlists.

No behavior change for single-profile deployments; test_single_profile_env_fallback_unchanged and test_handler_introspection_still_honored_without_injected_check pin that.

Related Issue

No issue filed for the Slack instance. Related: #65589 (same introspection gap on Telegram, fail-closed there), #70122 (the sibling _auth_env fallthrough in gateway/authz_mixin.py, still open — this PR does not touch that file).

Found via an adversarial scan of the external-surface authorization paths named in SECURITY.md §2.6.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• 🔒 Security fix
• ✅ Tests (adding or improving test coverage)

Changes Made

• plugins/platforms/slack/adapter.py — prefer the injected profile-bound auth check; multiplex-safe, fail-closed env fallback
• tests/gateway/test_slack_interactive_authz_multiplex.py — new regression tests

How to Test

scripts/run_tests.sh \
  tests/gateway/test_slack_interactive_authz_multiplex.py \
  tests/gateway/test_slack.py \
  tests/gateway/test_slack_approval_buttons.py \
  tests/gateway/test_slack_clarify_buttons.py \
  tests/gateway/test_slack_bot_auth_bypass.py \
  tests/gateway/test_multiplex_profile_authz.py \
  -q

523 passed, 0 failed locally. 5 of the 8 new tests fail against main without the adapter change; the other 3 guard behavior this PR deliberately leaves alone.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run scripts/run_tests.sh on relevant tests and they pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Linux

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — N/A
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A (pure authorization / env resolution)
• I've updated tool descriptions/schemas if I changed tool behavior — N/A

[teknium1]

Thanks for the focused Slack authorization fix. The premise is confirmed on current main: plugins/platforms/slack/adapter.py:6650-6700 can miss the gateway chain for multiplex closures and then read allow-all/allowlist values from process environment. The three interactive paths use this gate at plugins/platforms/slack/adapter.py:6714, 6858, and 7018.

The proposed callback-first resolution uses the existing profile-bound callback configured for secondary adapters at gateway/run.py:12661-12663; its factory stamps profile_name before delegating to the shared authorization chain at gateway/run.py:13073-13109. The fallback update also aligns with the fail-closed multiplex contract in agent/secret_scope.py:123-177.

No blocking issue found. The PR's adapter preimage still matches current main (a34c995cc81f...), so salvage should be mechanical.

Automated hermes-sweeper review.

[egilewski]

suggesting changes

The PR improves Slack's adapter-local fallback, but the production path now prefers GatewayRunner's injected authorization callback, whose shared _auth_env helper still falls through from an authoritative multiplex secret-scope miss to the default profile's process environment. A focused callback-chain probe reproduced the original allow-all cross-profile authorization on both current main and the reviewed head, including when the Slack socket task retains the secondary profile's secret scope. Consequently a user excluded by the secondary profile can still pass the interactive gate and reach approval, slash-confirm, or clarify resolution whenever the default profile enables SLACK_ALLOW_ALL_USERS or GATEWAY_ALLOW_ALL_USERS. The new tests substitute a hand-written callback and therefore do not exercise this production callback chain.

• [P1] Injected auth callback still inherits the default profile's allow-all flags (plugins/platforms/slack/adapter.py:6660)
  At this new preferred branch, _is_interactive_user_authorized calls the GatewayRunner callback. That callback delegates to GatewayAuthorizationMixin._is_user_authorized, where _auth_env calls get_secret(name), receives None for a key absent from the secondary profile's authoritative scope, and then falls through to os.getenv(name). If the default profile has SLACK_ALLOW_ALL_USERS=true or GATEWAY_ALLOW_ALL_USERS=true, an arbitrary secondary-profile Slack caller is therefore authorized. The action handlers subsequently resolve command approvals, slash confirmations, or clarifications. The focused probe used a captured secondary scope containing only SLACK_ALLOWED_USERS=U_SECONDARY_OWNER and a process-level SLACK_ALLOW_ALL_USERS=true; U_DEFAULT_STRANGER returned true on both current main and ec7e96a. This remains true without a scope as well. The added injected-callback test hides the defect by replacing the real callback with a lambda that directly returns false for the stranger.
  Remediation: Make gateway/authz_mixin.py::_auth_env treat an installed multiplex secret scope as authoritative: when get_secret returns None, return the supplied default instead of reading os.environ, and fail closed on UnscopedSecretError while multiplexing. Add a regression test that wires the real _make_adapter_auth_check callback to SlackAdapter under multiplexing, with the secondary scope omitting allow-all and the process environment setting each default-profile allow-all flag; assert the secondary stranger is denied and its configured owner is allowed.

Security evidence:

• trust boundary: Slack Block Kit action bodies supply user.id, channel.id, team.id, action id, and approval/session identifiers across an external workspace boundary. SlackAdapter._is_interactive_user_authorized is the sole caller-authorization gate before _handle_approval_action invokes resolve_gateway_approval, _handle_slash_confirm_action resolves a confirmation, or _handle_clarify_action resolves/captures clarification. Multiplex profile secret scopes and per-profile pairing/config data must remain isolated from the default profile's process environment.
• source/sink/invariant: Source: attacker-controlled Slack interactive user identity on a secondary profile. Validators: nonempty user id, then the injected BasePlatformAdapter authorization callback, ultimately GatewayAuthorizationMixin._is_user_authorized and its allowlists, pairing store, and explicit allow-all flags. Sinks: approval/confirmation/clarification state resolution. Required invariant: only grants belonging to the receiving adapter's profile may authorize the caller; an absent secondary-profile key must never inherit a default-profile allow-all value. The reviewed branch violates that invariant through _auth_env's scope-miss fallback.
• current-main reproduction: A local stdlib probe loaded e444d16's method and modeled the secondary Slack socket task with a captured secret scope {SLACK_ALLOWED_USERS: U_SECONDARY_OWNER} while process SLACK_ALLOW_ALL_USERS was true. U_DEFAULT_STRANGER was authorized (true), reproducing the cross-profile failure on current main. The old method reaches the process allow-all check directly.
• PR-head or patch-replay validation: The same probe ran the checked-out ec7e96a implementation with the production-shaped injected callback from _make_adapter_auth_check into GatewayAuthorizationMixin._is_user_authorized. Despite the captured secondary scope, U_DEFAULT_STRANGER remained authorized (true), while U_SECONDARY_OWNER was also true. A second unscoped multiplex probe likewise returned true. Thus the reviewed head does not change the vulnerable allow-all outcome.
• positive/negative cases: Positive control: the secondary owner in scoped SLACK_ALLOWED_USERS was accepted. Negative case expected by the security contract: a stranger absent from that scope should be rejected even if the default profile allows all; it was accepted on both revisions. The PR's adapter-local fallback does reject stranger and accept owner when no injected callback is installed, showing the added tests cover only the non-production branch. Empty user IDs remain denied by the unchanged initial validator.
• residual bypass search: Reviewed all three gated Slack action call sites, BasePlatformAdapter._is_sender_authorized, primary/secondary adapter callback installation and reconnect paths, _make_profile_message_handler, _make_adapter_auth_check, agent.secret_scope.get_secret, and GatewayAuthorizationMixin._auth_env/_is_user_authorized. The residual bypass is shared by both SLACK_ALLOW_ALL_USERS and GATEWAY_ALLOW_ALL_USERS because _auth_env falls through on a scoped missing key. Searches found no new test combining SlackAdapter with the real callback and a conflicting process-level allow-all flag.
• reviewer validation: The checkout matched reviewed head ec7e96a and current-main object e444d16. git diff --check passed. The focused Python probes completed successfully and asserted the vulnerable outcomes for current main and PR head plus the owner control. Python compileall completed successfully for gateway/authz_mixin.py, plugins/platforms/slack/adapter.py, and the new regression test. Pytest execution was attempted but unavailable in this leased checkout: pytest was not on PATH and /usr/bin/python reported No module named pytest.

Uncertainty: The Slack SDK and pytest development environment were not installed in the leased checkout, so no live Socket Mode event or repository pytest case was executed.; No network or GitHub state was consulted; review scope and revisions are those bound by the work order.

Signed: GPT-5.6-sol-xhigh in Codex

[teknium1]

Thanks @MilaArtyNew for this PR — Merged via #101250 (11f932c) on current main.

#101250 won because it was built on the earlier #65589 fix and covers the same behavior across all affected call paths in one change. You're credited via Co-authored-by on the merge and in the PR body of #101250.

Closing this PR as superseded by the merged work.