Skip to content

Fix/slack pre auth media download - #55272

Closed
necoweb3 wants to merge 2 commits into
NousResearch:mainfrom
necoweb3:fix/slack-pre-auth-media-download
Closed

Fix/slack pre auth media download#55272
necoweb3 wants to merge 2 commits into
NousResearch:mainfrom
necoweb3:fix/slack-pre-auth-media-download

Conversation

@necoweb3

Copy link
Copy Markdown
Contributor

Summary

This rejects unauthorized Slack message events before the adapter performs file metadata fetches or media processing.

The gateway runner performs the final _is_user_authorized check after the adapter has already constructed a MessageEvent. For Slack messages with files, that meant an unauthorized sender could still trigger Slack files_info calls and file-processing work before the runner rejected the event.

Changes

  • Reuse the bound gateway runner auth check at the start of _handle_slack_message().
  • Return before thread lookup, user resolution, files_info, or file download work when the sender is unauthorized.
  • Preserve existing behavior when the adapter is used without a bound gateway runner.
  • Add a regression test proving unauthorized Slack file messages do not call files_info.

Tests

python -m pytest tests/gateway/test_slack.py -k "unauthorized_message_does_not_fetch_file_info or file_shared_video_fallback_fetches_file_info" -q --timeout-method=thread
2 passed

necoweb3 added 2 commits June 30, 2026 02:34
Add early auth check in _handle_slack_message() that runs BEFORE any
API calls (thread context fetch, user name resolution, file downloads)
or file processing. Unauthorized users could previously trigger Slack
API calls and file downloads before the runner's _is_user_authorized
gate rejected them.

Same pattern as Telegram fix NousResearch#54164: build a SessionSource and check
the runner's _is_user_authorized at the adapter level before event
construction consumes resources.

Fixes the gap where Slack has no adapter-level auth gate while Discord
and Telegram have adapter-level allowlists.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jun 29, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #54213 (Teams) and #52353 (Telegram) apply the same pre-auth side-effect hardening pattern — reject the unauthorized sender before attachment metadata fetch / media download — to their respective platform adapters. This is the Slack counterpart (plugins/platforms/slack/adapter.py); not a duplicate (different adapter, different file).

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: LGTM

Same pattern as Discord early-auth fix (PR #55273). Rejects unauthorized Slack message events before file metadata fetches or media processing.

  • Consistent with Discord adapter pattern
  • Clean early-auth gate
  • Well-scoped: 2 files, 65 additions

Reviewed 2 files, 65 additions. Approved.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting a real pre-auth side-effect boundary. The normal Slack message path still fetches file metadata/media before the runner receives the constructed event (plugins/platforms/slack/adapter.py:2923, :2963, :3214, :3236; runner auth is at gateway/run.py:8958).

Problems

  • The proposed __self__ lookup is not reliable on current main. Profile multiplexing installs a nested handler closure at gateway/run.py:8604-8606, created at :8629-8638; it has no bound runner __self__, so the early gate would be skipped.
  • file_shared remains ungated: its registered handler reaches plugins/platforms/slack/adapter.py:2511, which calls files.info at :2528 before it delegates at :2587.

Suggested changes

  • Use main's existing BasePlatformAdapter._is_sender_authorized() (gateway/platforms/base.py:2852-2874), which GatewayRunner wires in all startup paths (gateway/run.py:7068, :8611). Reject only an explicit False result.
  • Apply the same gate before file_shared calls files.info, with regression coverage for that ingress and for a wrapped profile handler.

Automated hermes-sweeper review.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #69482 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your pre-auth media gating was cherry-picked directly.

Thanks for the contribution!

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

Labels

area/auth Authentication, OAuth, credential pools comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants