Skip to content

fix(slack): re-check gateway auth on approval and slash-confirm buttons#33844

Closed
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/security-slack-interactive-approval-auth
Closed

fix(slack): re-check gateway auth on approval and slash-confirm buttons#33844
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/security-slack-interactive-approval-auth

Conversation

@Dusk1e

@Dusk1e Dusk1e commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

This PR fixes an authorization gap in Slack interactive approval handling.

Slack approval buttons and slash-confirm buttons bypass the normal gateway message auth path, so they must re-check authorization before resolving a pending approval or confirmation. Before this change, those interactive callbacks only consulted SLACK_ALLOWED_USERS. If that env var was unset, a deployment restricted via GATEWAY_ALLOWED_USERS could still allow an unauthorized Slack user to click a button and resolve the prompt.

What changed

  • Added SlackAdapter._is_interactive_user_authorized() in gateway/platforms/slack.py
  • Reused the central gateway auth model when available via runner._is_user_authorized(...)
  • Added a fail-closed fallback that respects:
    • SLACK_ALLOW_ALL_USERS
    • SLACK_ALLOWED_USERS
    • GATEWAY_ALLOWED_USERS
    • GATEWAY_ALLOW_ALL_USERS
  • Applied the interactive auth check to:
    • Slack exec approval button callbacks
    • Slack slash-confirm button callbacks

Tests

Added coverage in tests/gateway/test_slack_approval_buttons.py for:

  • blocking an unauthorized approval click when only GATEWAY_ALLOWED_USERS is configured
  • allowing an authorized slash-confirm click when SLACK_ALLOWED_USERS is unset but GATEWAY_ALLOWED_USERS is set
  • delegating interactive auth to the gateway runner’s central _is_user_authorized() path

Test results

Ran:

ruff check gateway/platforms/slack.py tests/gateway/test_slack_approval_buttons.py
python -m pytest -o addopts="-m 'not integration'" tests/gateway/test_slack_approval_buttons.py
python -m pytest -o addopts="-m 'not integration'" tests/gateway/test_destructive_slash_confirm.py

ruff check gateway/platforms/slack.py tests/gateway/test_slack_approval_buttons.py
All checks passed!
tests/gateway/test_slack_approval_buttons.py
26 passed
tests/gateway/test_destructive_slash_confirm.py
6 passed

@liuhao1024

liuhao1024 commented May 28, 2026

Copy link
Copy Markdown
Contributor

Verified this security fix — the approach is correct and the test coverage is solid.

What this fixes: Slack interactive buttons (approval and slash-confirm) previously only checked SLACK_ALLOWED_USERS env var, bypassing the full auth chain (GATEWAY_ALLOWED_USERS, SLACK_ALLOW_ALL_USERS, GATEWAY_ALLOW_ALL_USERS, and the SessionSource-based _is_user_authorized from the gateway runner). A user allowed by GATEWAY_ALLOWED_USERS but not SLACK_ALLOWED_USERS could be blocked from clicking approval buttons, while conversely a user not in any allowlist but in a misconfigured env could potentially interact.

Verification:

  • _is_interactive_user_authorized() correctly chains through runner auth → env var fallback, matching the normal message auth flow
  • Both _handle_approval_action and _handle_slash_confirm_action now gate on the new method before proceeding
  • The old allowed_csv check is set to "" (dead code) — functionally correct since _is_interactive_user_authorized already covers all env vars; the dead code could be cleaned up in a follow-up but doesn't affect correctness
  • Test coverage includes: unauthorized click blocked, authorized click allowed via GATEWAY_ALLOWED_USERS, gateway runner delegation verified, and slash-confirm action flow tested end-to-end

No issues found — this is a clean security fix.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter area/auth Authentication, OAuth, credential pools labels May 28, 2026
@alt-glitch alt-glitch mentioned this pull request May 28, 2026
Open
@alt-glitch

alt-glitch commented May 28, 2026

Copy link
Copy Markdown
Collaborator

Competing with #33280 and #29627 — all three fix Slack interactive callback authorization bypass. #29627 (earliest) addresses config.extra.allow_from; #33280 focuses on fail-closed; this PR adds central gateway auth delegation with fail-closed fallback.

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

teknium1 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Consolidated into #41226 (open, pending review) — thank you @Dusk1e.

This is the canonical Slack fix in a cross-adapter salvage that closes the same fail-open on Slack, Feishu, and Discord in one PR. Your commit is cherry-picked with authorship preserved, so the per-commit author line stays yours. I picked this over the two sibling Slack PRs 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.

One fixup applied during salvage: the new test_delegates_to_gateway_runner_auth test referenced Platform without importing it (NameError) — added Platform to the gateway.config import. Verified green (295 passed across the affected adapter suites).

This was referenced Jun 7, 2026
Closed
Closed
Closed
teknium1 pushed a commit that referenced this pull request Jun 7, 2026
@LaPhilosophie @teknium1
fix(discord): fail closed for component button auth when no allowlist…
f6f3636 
… set

Salvage of the Discord half of PR #30964 by @LaPhilosophie. Discord
component button callbacks (ExecApprovalView, SlashConfirmView,
UpdatePromptView, ModelPickerView) bypass the normal message dispatch
authorization path. _component_check_auth previously returned True when
both the user and role allowlists were empty, so any guild member who
could see an approval prompt could click Approve on a dangerous command.

Fail closed instead: require DISCORD_ALLOWED_USERS / DISCORD_ALLOWED_ROLES
/ GATEWAY_ALLOWED_USERS membership, or an explicit DISCORD_ALLOW_ALL_USERS
/ GATEWAY_ALLOW_ALL_USERS opt-in for deliberately-open deployments.

Mirrors the Telegram (#24457) and Matrix fail-closed precedent.
The Slack half of #30964 is superseded by PR #33844's helper.

Reported via GHSA-mc26-p6fw-7pp6 (@whyiug).

Co-authored-by: LaPhilosophie <804436395@qq.com>
teknium1 added a commit that referenced this pull request Jun 7, 2026
@teknium1
chore(release): map Dusk1e and LaPhilosophie for approval fail-closed…
a317e54 
… salvage (#33844, #33866, #30964)
agogo233 added a commit to agogo233/hermes-agent that referenced this pull request Jun 8, 2026
@agogo233
Merge remote-tracking branch 'upstream/main'
9aeaf0d 
* upstream/main: (430 commits)
  fix(yuanbao): bound ws.close() so an idle server can't stall shutdown ~5s (NousResearch#40607)
  docs: add Urdu translation of README (NousResearch#40578)
  fix(hindsight): send only new-turn delta on append retains instead of whole session (NousResearch#40605)
  feat(gateway): render terminal tool calls as native bash code blocks on markdown platforms (NousResearch#41215)
  feat(desktop): stop the chat viewport from following streaming output (NousResearch#41414)
  chore(release): map AlchemistChaos co-author email for NousResearch#40135 salvage
  fix(desktop): recover chat after sleep/wake by revalidating a stale remote backend
  fix(web): make _has_env config-aware so SEARXNG_URL auto-detect honors Hermes config
  fix(web): honor Hermes config-aware SEARXNG_URL lookup
  install.sh: hint at root-owned npm cache when desktop npm install fails (NousResearch#39688)
  fix(tools): percent-encode non-ascii URL components
  fix(skills): browse shows full catalog, not first 5000 (NousResearch#41413)
  feat(desktop+gateway): remote media relay — attach images/PDFs and display gateway images over the network
  feat(desktop): full tool-backend config (pickers + per-backend settings) in Settings (NousResearch#41232)
  hardening(api-server): scan cron prompts on REST create/update for parity with the agent tool
  fix: skip MCP preflight content-type probe on reconnect when already ready (NousResearch#40604)
  fix(kanban): sweep deferred scratch parent on non-scratch child completion + tests
  fix: defer scratch workspace cleanup when task has active children (NousResearch#33774)
  feat(onboarding): opt-in structured profile-build path on first contact (NousResearch#41114)
  feat(compression): temporal anchoring in compaction summaries (NousResearch#41102)
  test(discord): align clarify/model-picker tests with fail-closed component auth (NousResearch#41338)
  chore(release): map Dusk1e and LaPhilosophie for approval fail-closed salvage (NousResearch#33844, NousResearch#33866, NousResearch#30964)
  fix(discord): fail closed for component button auth when no allowlist set
  fix(feishu): fail closed for update prompt card actions
  fix(slack): re-check gateway auth on approval and slash-confirm buttons
  fix: guard int(os.getenv()) casts against malformed env vars (NousResearch#40598)
  fix: respect Honcho env var fallback in doctor and honcho status
  chore(release): add synapsesx to AUTHOR_MAP for NousResearch#40495 salvage
  fix(research): keep tool_call/tool_response pairs intact when compressing trajectories
  fix(simplex): accept display name in SIMPLEX_ALLOWED_USERS
  fix(desktop): make the running-turn timer per-session (NousResearch#41182)
  test(approval): regression for shell-escape denylist bypass (NousResearch#36846, NousResearch#36847)
  fix(security): strip shell escapes in denylist normalizer; fail-closed on missing approval module
  fix(stream+output-cap): guard empty streams and parse OpenRouter output-cap errors (NousResearch#40589)
  fix(desktop): bootstrap falls back to installed agent install.sh on GitHub 404
  feat(dashboard): change UI font from the theme picker, independent of theme (NousResearch#41145)
  fix(cli): return bool (not None) when a destructive-slash confirmation is cancelled (NousResearch#40583)
  fix(desktop): preserve configured base_url on same-provider model switch (NousResearch#41121)
  fix(desktop): stop bare-URL autolinker swallowing trailing emphasis asterisks (NousResearch#41093)
  fix(cron): bound the desktop run-history query to one job (NousResearch#41088)
  fix(desktop): scope in-session /model switch per-session, stop process-env leak (NousResearch#41120)
  chore: map bmoore210 author email for PR NousResearch#40550 salvage
  fix(desktop): scope session list to active profile + longer timeout
  fix: harden gateway startup and turn persistence
  fix(computer_use): honor custom vision routing
  fix(aux): honor model.default_headers on auxiliary client too (NousResearch#40033)
  fix(agent): honor model.default_headers for custom OpenAI-compatible providers (NousResearch#40033)
  docs(i18n): port deep-audit corrections to zh-Hans mirror (NousResearch#41104)
  fix(compression): don't overwrite the -1 post-compression sentinel in preflight seed (NousResearch#36718)
  chore(release): map singhsanidhya741@gmail.com to sanidhyasin (NousResearch#41094)
  ...
changman pushed a commit to changman/hermes-agent that referenced this pull request Jun 10, 2026
@LaPhilosophie @changman
fix(discord): fail closed for component button auth when no allowlist…
e93ea2d 
… set

Salvage of the Discord half of PR NousResearch#30964 by @LaPhilosophie. Discord
component button callbacks (ExecApprovalView, SlashConfirmView,
UpdatePromptView, ModelPickerView) bypass the normal message dispatch
authorization path. _component_check_auth previously returned True when
both the user and role allowlists were empty, so any guild member who
could see an approval prompt could click Approve on a dangerous command.

Fail closed instead: require DISCORD_ALLOWED_USERS / DISCORD_ALLOWED_ROLES
/ GATEWAY_ALLOWED_USERS membership, or an explicit DISCORD_ALLOW_ALL_USERS
/ GATEWAY_ALLOW_ALL_USERS opt-in for deliberately-open deployments.

Mirrors the Telegram (NousResearch#24457) and Matrix fail-closed precedent.
The Slack half of NousResearch#30964 is superseded by PR NousResearch#33844's helper.

Reported via GHSA-mc26-p6fw-7pp6 (@whyiug).

Co-authored-by: LaPhilosophie <804436395@qq.com>
changman pushed a commit to changman/hermes-agent that referenced this pull request Jun 10, 2026
@teknium1 @changman
chore(release): map Dusk1e and LaPhilosophie for approval fail-closed…
bc0c588 
… salvage (NousResearch#33844, NousResearch#33866, NousResearch#30964)
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 @liuhao1024 @alt-glitch @teknium1