Skip to content

fix(slack): fail closed for interactive approval callbacks#33280

Closed
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/slack-block-kit-interactive-callback-auth
Closed

fix(slack): fail closed for interactive approval callbacks#33280
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/slack-block-kit-interactive-callback-auth

Conversation

@Dusk1e

@Dusk1e Dusk1e commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Fix Slack interactive approval authorization so Block Kit callbacks cannot resolve gated prompts unless the clicking user is authorized.

Slack approval and slash-confirm button handlers bypass the normal gateway message dispatch path, so they must enforce authorization locally. The previous callback checks only consulted SLACK_ALLOWED_USERS when it was non-empty, which made an empty platform allowlist behave like allow-all for button clicks.

This PR adds a shared Slack interactive auth helper that:

  • delegates to GatewayRunner._is_user_authorized() when available
  • preserves approved runner behavior such as global allowlists and pairing
  • falls back to SLACK_ALLOWED_USERS / GATEWAY_ALLOWED_USERS
  • honors explicit allow-all flags
  • fails closed when no allowlist or allow-all is configured

Security Impact

This closes an external-surface auth gap where an unauthorized Slack user who could see a Block Kit approval prompt could click an approval or slash-confirm button and resolve the pending action.

Affected surfaces:

  • dangerous command approval buttons
  • destructive slash-confirm buttons

This aligns Slack callbacks with the gateway trust model: session IDs and callback payloads are routing data, not authorization.

Tests

uv --cache-dir .uv-cache run --with pytest --with pytest-asyncio pytest -o addopts= tests/gateway/test_slack_approval_buttons.py -q
uv --cache-dir .uv-cache run --with pytest --with pytest-asyncio pytest -o addopts= tests/gateway/test_slack.py tests/gateway/test_slack_mention.py tests/gateway/test_slack_channel_skills.py tests/gateway/test_slack_approval_buttons.py -q
uv --cache-dir .uv-cache run --with pytest --with pytest-asyncio pytest -o addopts= tests/gateway/test_unauthorized_dm_behavior.py tests/gateway/test_allowlist_startup_check.py -q
uv --cache-dir .uv-cache run --with ruff ruff check gateway/platforms/slack.py tests/gateway/test_slack_approval_buttons.py
git diff --check

Results:

Slack approval tests: 29 passed
Full Slack gateway test set: 281 passed
Gateway allowlist tests: 34 passed
Ruff: passed
Diff check: passed

@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 P2 Medium — degraded but workaround exists labels May 27, 2026
@alt-glitch

alt-glitch commented May 27, 2026

Copy link
Copy Markdown
Collaborator

Competes with #29627 (Slack-only Block Kit auth) and #30964 (cross-platform auth consolidation for both Slack and Discord). #30964 is the broadest fix.

@liuhao1024

liuhao1024 commented May 27, 2026

Copy link
Copy Markdown
Contributor

Verified: this correctly consolidates the authorization checks from both _handle_slash_confirm_action and _handle_approval_action into a single _is_interactive_user_authorized helper. All 2 inline SLACK_ALLOWED_USERS env var checks are replaced.

Key correctness observations:

  1. Fail-closed on empty allowlists: When neither SLACK_ALLOWED_USERS nor GATEWAY_ALLOWED_USERS is set, and GATEWAY_ALLOW_ALL_USERS is not "true"/"1"/"yes", the method returns False. This is the correct security posture — the previous code silently allowed all users when SLACK_ALLOWED_USERS was empty.

  2. Runner auth chain is correct: adapter._message_handler is set to the bound GatewayRunner._handle_message method (base.py:1522, run.py:3507), so __self__ correctly yields the GatewayRunner instance, and _is_user_authorized (run.py:5381) is the right fallback.

  3. Both interactive paths covered: _handle_slash_confirm_action (line 2386) and _handle_approval_action (line 2484) are the only button-click handlers in slack.py — no other interactive paths are missed.

  4. GATEWAY_ALLOW_ALL_USERS precedence is safe: The new env var only takes effect when both per-platform and global allowlists are empty, so it doesn't weaken existing explicit allowlist configurations.

Test coverage is thorough — 8 new tests covering: no-allowlist denial, global allowlist permit/mismatch, slash-confirm auth, and runner-auth-first fallback. LGTM.

This was referenced May 28, 2026
Closed
Closed
Closed
@teknium1 teknium1 mentioned this pull request Jun 7, 2026
Merged
@teknium1

teknium1 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for catching this on a real exploit path — independent confirmation from several contributors is what got it prioritized.

The fail-open is being fixed in #41226 (open, pending review), a cross-adapter salvage that closes the same bug on Slack, Feishu, and Discord in one PR. For the Slack side we picked #33844 by @Dusk1e as canonical because it mirrors Telegram's _is_callback_user_authorized precedent most closely — it delegates to the gateway runner's central _is_user_authorized() so GATEWAY_ALLOWED_USERS, DM pairing, and GATEWAY_ALLOW_ALL_USERS are all honored before the env-only fail-closed fallback.

Your fix is correct on the security logic, but _is_interactive_user_authorized calls self.build_source(...), which isn't defined on the Slack adapter — #33844 constructs the SessionSource directly, matching how Telegram does it, so it works without that method.

Holding this open until #41226 is reviewed/merged rather than closing it now — the credit stands either way.

@teknium1

teknium1 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Closing — the fail-open is now fixed on main via PR #41226 (merged: commits a317e54 / 410cb74 (Slack), 3fa15b3 (Feishu), f6f3636 (Discord)). The Slack side landed @Dusk1e's #33844 as canonical (delegates to the gateway runner's central auth, matching the Telegram precedent). Thanks again for the report and the fix — your contribution is credited in #41226.

@teknium1 teknium1 closed this Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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 type/security Security vulnerability or hardening

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Dusk1e @alt-glitch @liuhao1024 @teknium1