Skip to content

[security] fix(slack): preserve per-user session isolation for slash commands in shared channels#9361

Closed
Hinotoi-agent wants to merge 1 commit into
NousResearch:mainfrom
Hinotoi-agent:fix/slack-slash-session-isolation
Closed

[security] fix(slack): preserve per-user session isolation for slash commands in shared channels#9361
Hinotoi-agent wants to merge 1 commit into
NousResearch:mainfrom
Hinotoi-agent:fix/slack-slash-session-isolation

Conversation

@Hinotoi-agent
Copy link
Copy Markdown
Contributor

Summary

This PR fixes a Slack session-isolation issue affecting slash commands in shared channels.

Before this change, /hermes slash commands invoked by different users in the same Slack channel could resolve to the same session key. That allowed cross-user session/context bleed and could cause one user to inherit or affect another user’s active conversation state.

This patch preserves per-user isolation for shared-channel slash commands and adds regression coverage to lock in the expected behavior.

Security impact

Issue:
Slack slash-command session collision in shared channels

Impact:
Cross-user context bleed and session manipulation

CVSS v3.1:
8.3 High

Vector:
CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:L

Problem

Slack slash commands currently construct their session source using DM-style semantics even when invoked from a shared channel.

Because DM session keys do not include user_id, two different users running /hermes in the same channel can land in the same session.

This can cause:

  • cross-user conversation context bleed,
  • one user inheriting another user’s active agent state,
  • session-level actions affecting the wrong user context.

Root cause

In gateway/platforms/slack.py, slash-command handling forced channel slash commands into DM semantics.

In gateway/session.py, DM session keys are keyed by channel/thread and do not include user identity.

Together, this allowed different users in the same Slack channel to collide into the same session.

Fix

This PR updates Slack slash-command session construction so shared-channel slash commands preserve per-user isolation instead of forcing DM session semantics.

Example

Before

User A -> agent:main:slack:dm:C123
User B -> agent:main:slack:dm:C123

After

User A -> distinct session key
User B -> distinct session key

Files changed

  • gateway/platforms/slack.py
  • tests/gateway/test_slack.py

Test plan

  • Added regression coverage for two different Slack users invoking /hermes in the same shared channel
  • Verified channel slash commands use group semantics
  • Verified DM slash commands continue using DM semantics
  • Ran: source venv/bin/activate && python -m pytest tests/gateway/test_slack.py -q

Scope

This is a narrow fix limited to Slack slash-command session handling in shared channels. It does not change normal DM behavior or unrelated gateway logic.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround platform/slack Slack app adapter comp/gateway Gateway runner, session dispatch, delivery labels Apr 27, 2026
@alt-glitch
Copy link
Copy Markdown
Collaborator

Related to #10875 — both address Slack slash-command session key issues. This PR focuses on cross-user isolation (security), while #10875 addresses session scope drift.

@Hinotoi-agent
Copy link
Copy Markdown
Contributor Author

Thanks for linking this. I rechecked the current PR against #10875 and did not make a code change here: this branch is still intentionally scoped to the cross-user isolation issue for Slack slash commands in shared channels, while #10875 carries the broader session-scope/thread-context fix.

Validation on the current head (7e1d4440e92c9f7dba27bd40246f401f4351e6ce):

  • uv run pytest tests/gateway/test_slack.py -q → 127 passed
  • uv run ruff check gateway/platforms/slack.py tests/gateway/test_slack.py → still blocked by pre-existing lint findings in tests/gateway/test_slack.py unrelated to this PR's diff

@Hinotoi-agent
Copy link
Copy Markdown
Contributor Author

Thanks for flagging this. I checked the overlap with #10875 and I agree they are related but not duplicate:

Keeping this PR scoped to the cross-user isolation boundary should make it safe to review/merge independently while #10875 handles the broader session-key behavior.

@kshitijk4poor
Copy link
Copy Markdown
Collaborator

Merged via PR #18553. Your commit was cherry-picked onto current main with your authorship preserved (rebase merge). Thanks for the contribution! 🎉

kshitijk4poor added a commit that referenced this pull request May 1, 2026
Adds email→username mappings for:
- priveperfumes (PR #18456)
- amroessam (PR #17798)
- Hinotoi-agent (PR #9361)
- valda (PR #14932)
donald131 pushed a commit to donald131/hermes-agent that referenced this pull request May 2, 2026
Adds email→username mappings for:
- priveperfumes (PR NousResearch#18456)
- amroessam (PR NousResearch#17798)
- Hinotoi-agent (PR NousResearch#9361)
- valda (PR NousResearch#14932)
nickdlkk pushed a commit to nickdlkk/hermes-agent that referenced this pull request May 11, 2026
Adds email→username mappings for:
- priveperfumes (PR NousResearch#18456)
- amroessam (PR NousResearch#17798)
- Hinotoi-agent (PR NousResearch#9361)
- valda (PR NousResearch#14932)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround platform/slack Slack app adapter type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants