fix(gateway): Slack approval handler must fail closed when SLACK_ALLOWED_USERS unset#36861
fix(gateway): Slack approval handler must fail closed when SLACK_ALLOWED_USERS unset#36861dhyabi2 wants to merge 2 commits into
Conversation
…WED_USERS unset _handle_approval_action gated approval-button clicks behind 'if allowed_csv:', so when SLACK_ALLOWED_USERS was unset (the default) the auth check was skipped entirely and any workspace member in the channel could click 'Approve always', persisting a dangerous-command allowlist entry. Fail closed by default and require an explicit opt-in (SLACK_ALLOWED_USERS=* or GATEWAY_ALLOW_ALL_USERS), mirroring the merged Telegram fix (NousResearch#24457). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Covers _handle_approval_action: denies when SLACK_ALLOWED_USERS unset and GATEWAY_ALLOW_ALL_USERS unset; permits on wildcard, listed user, or allow-all; denies unlisted user. Mirrors the Matrix/Telegram fail-closed test style. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Good security fix. The fail-closed logic is correct: when SLACK_ALLOWED_USERS is unset, approvals are denied unless GATEWAY_ALLOW_ALL_USERS is explicitly set to true/1/yes. Test coverage is comprehensive and mirrors the Telegram pattern (#24457). No issues found. |
1 similar comment
|
Good security fix. The fail-closed logic is correct: when SLACK_ALLOWED_USERS is unset, approvals are denied unless GATEWAY_ALLOW_ALL_USERS is explicitly set to true/1/yes. Test coverage is comprehensive and mirrors the Telegram pattern (#24457). No issues found. |
|
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 Your version is clean and well-tested, but it gates only on Holding this open until #41226 is reviewed/merged rather than closing it now — the credit stands either way. |
|
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. |
What & why
_handle_approval_action(Slack Block-Kit) gates approval-button clicks behindif allowed_csv:. WhenSLACK_ALLOWED_USERSis unset (the default — it's not in the example configs), the entire auth block is skipped and execution falls through to processing the approval. Any workspace member who can see the message can click Approve always, persisting a dangerous-command allowlist entry to the operator'sconfig.yaml. The in-code comment even notes button clicks bypass the normal message-auth flow "so we must check here as well" — but that check is disabled by default.The analogous Telegram handler was already fixed to fail closed in #24457; this brings Slack in line.
The fix
Fail closed when
SLACK_ALLOWED_USERSis unset: deny by default and require an explicit opt-in (SLACK_ALLOWED_USERS=*orGATEWAY_ALLOW_ALL_USERS) for no-restriction mode — mirroring the merged Telegram fix.How to test
SLACK_ALLOWED_USERSunset,GATEWAY_ALLOW_ALL_USERSunset → approval click is rejected with the existing "Unauthorized approval click" warning.SLACK_ALLOWED_USERS=*(or the clicking user listed) → click authorized, as before.GATEWAY_ALLOW_ALL_USERS=true→ opt-in no-restriction mode, click authorized.Platforms
Logic-only change in
gateway/platforms/slack.py.Fixes #36848. Reported privately as
GHSA-jg5x-hmg9-2q4c(still in triage); mirrors the already-merged Telegram fix #24457.