Skip to content

fix(platforms): block image upload redirects to private URLs - #62932

Closed
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/platform-image-redirect-ssrf
Closed

fix(platforms): block image upload redirects to private URLs#62932
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/platform-image-redirect-ssrf

Conversation

@necoweb3

Copy link
Copy Markdown
Contributor

Summary

This closes redirect-based SSRF gaps in Slack and Discord outbound image upload paths.

Slack and Discord already preflight remote image URLs with is_safe_url(), but the platform upload paths then allowed the HTTP client to follow redirects without revalidating the final target. A public URL could therefore redirect to a private/internal address and have Hermes fetch those bytes before uploading them back to Slack or Discord.

Why

These paths run on behalf of messaging-platform output and perform host-side network fetches:

  • Slack send_multiple_images() downloads URL images with httpx.AsyncClient(follow_redirects=True) before files_upload_v2.
  • Discord send_image() and send_multiple_images() download URL images with aiohttp before sending attachments.

Without redirect revalidation, the initial public URL check does not protect the final fetch target.

Changes

  • Wire Slack batch image downloads through the existing HTTPX _ssrf_redirect_guard.
  • Add a Discord aiohttp image-read helper that disables automatic redirects and validates every redirect target before reading response bytes.
  • Use the Discord helper for both single-image and multi-image upload paths.
  • Add regression tests proving private metadata redirects are blocked before Slack upload or Discord send.

Tests

python -m pytest --basetemp=C:\tmp\hermes-p1-hunt-20260712\tmp_pytest tests/gateway/test_send_multiple_images.py -q --timeout-method=thread
21 passed

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery platform/discord Discord bot adapter platform/slack Slack app adapter sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data needs-repro Bug needs reproduction steps labels Jul 12, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to the open SSRF redirect-guard cluster in the platform image-upload paths: #24831 (Mattermost media redirects), #43938 (Yuanbao download_url), following the merged pattern from #7151 (single-image Slack SSRF). This PR adds the Slack-batch + Discord redirect revalidation combo (the closed #35940 was the Slack-batch-only version). Defense-in-depth against attacker-influenceable model-output URLs — no external-attacker PoC, so it stays capped as hardening rather than a boundary crossing. Cross-linking for maintainer review.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing a real redirect-based SSRF gap. Current main still follows redirects after only an initial URL preflight in Slack batch uploads (plugins/platforms/slack/adapter.py:1751-1779) and Discord URL-image downloads (plugins/platforms/discord/adapter.py:2534-2543).

Problems

  • The new Discord helper converts aiohttp's case-insensitive headers to dict and then checks only headers.get("location") (plugins/platforms/discord/adapter.py:150 in this PR). A normal Location header can be missed, so safe redirects fail as HTTP 302 rather than being followed.
  • Discord GIF uploads still preflight only their initial URL and then use redirect-following session.get(animation_url) (plugins/platforms/discord/adapter.py:3748); this sibling SSRF path is unchanged.
  • The new tests exercise only send_multiple_images, although this PR also changes send_image; neither send_image nor send_animation gets direct coverage. AGENTS.md:84-87 also requires real-path validation for security-sensitive network I/O.

Suggested changes

  • Preserve case-insensitive response-header lookup or normalize keys, and test both public Location redirects and private targets.
  • Reuse the guard for send_animation, then add direct send_image and animation regressions.

Automated hermes-sweeper review.

async with session.get(
current_url,
timeout=timeout,
allow_redirects=False,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

aiohttp headers are case-insensitive, but converting them to dict makes this subsequent headers.get("location") lookup case-sensitive. Preserve the original header mapping or normalize keys; otherwise normal Location headers can make safe redirects fail as HTTP 302.

@necoweb3
necoweb3 force-pushed the fix/platform-image-redirect-ssrf branch from 5ec182d to 90ab530 Compare July 12, 2026 01:30
@necoweb3

Copy link
Copy Markdown
Contributor Author

Updated the branch to address the review feedback.

Changes:

  • Preserved aiohttp's case-insensitive redirect header behavior by normalizing response header keys before reading Location.
  • Reused the Discord redirect guard for send_animation() so GIF uploads no longer follow only-preflighted redirects.
  • Added direct regressions for Discord send_image() and send_animation().
  • Added a safe public redirect regression to ensure normal Location redirects still work.
  • Kept the Slack batch upload guard using the existing HTTPX redirect hook.

Validation:

python -m pytest --basetemp=C:\tmp\hermes-p1-hunt-20260712\tmp_pytest tests/gateway/test_send_multiple_images.py -q --timeout-method=thread
24 passed

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
@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 SSRF redirect guard was cherry-picked — redirect re-validation on every hop.

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

comp/gateway Gateway runner, session dispatch, delivery needs-repro Bug needs reproduction steps P3 Low — cosmetic, nice to have platform/discord Discord bot adapter platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

3 participants