fix(security): validate inbound Slack file URLs against SSRF - #44026
fix(security): validate inbound Slack file URLs against SSRF#44026zapabob wants to merge 1 commit into
Conversation
_download_slack_file and _download_slack_file_bytes fetched Slack-supplied URLs with the bot token attached and follow_redirects=True, but without the is_safe_url pre-flight check or per-redirect guard that the outbound send_image path already uses. A URL that resolves to (or 3xx-redirects into) a private/internal address could reach internal services and leak the bot token (CWE-918). Add the same pre-flight + _ssrf_redirect_guard hook to both inbound sibling paths.
|
Verification: LGTM ✅ Reviewed the diff and tests. The SSRF guard is correctly applied to both Checked:
Clean implementation, no issues found. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the authenticated inbound-file download path. The security premise remains valid on current main: plugins/platforms/slack/adapter.py:4059-4065 and :4115-4121 still follow redirects while attaching the bot token, without the shared URL preflight and redirect guard.
Problems
- The PR targets the removed
gateway/platforms/slack.py; commit5600105478ffde29d7566b45421b100eaa29c4efmoved it toplugins/platforms/slack/adapter.py. The new test'sgateway.platforms.slackimport is obsolete as well. plugins/platforms/slack/adapter.py:1751-1778is another redirect-following Slack URL fetch: it preflights the initial URL at:1772but registers no redirect hook.
Suggested changes
- Retarget the two helper changes and test import to
plugins/platforms/slack/adapter.py, using the existinggateway.platforms.base._ssrf_redirect_guard(gateway/platforms/base.py:541-554). - Cover the remaining
send_multiple_imagesredirect-following fetch or split it into a focused follow-up.
Automated hermes-sweeper review.
|
|
||
| import pytest | ||
|
|
||
| from gateway.platforms.base import _ssrf_redirect_guard |
There was a problem hiding this comment.
Current main moved SlackAdapter from gateway/platforms/slack.py to plugins/platforms/slack/adapter.py in 5600105478ffde29d7566b45421b100eaa29c4ef; this import must be retargeted along with the production change.
…ds, widen token-file perms warning Follow-up hardening on top of the C14 cherry-picks (#57860/#44026/#66742/#60009): - Slack file downloads (_download_slack_file/_download_slack_file_bytes) now require an https URL on a Slack CDN host (files.slack.com, *.slack.com Enterprise Grid, *.slack-files.com legacy shares) before attaching the bot token. url_private/url_private_download only ever point at the Slack CDN, so a forged file object from a malicious workspace app or compromised event stream pointing the Bearer-token download at an arbitrary PUBLIC host (token exfiltration) is now refused — a hole #44026's generic private-IP SSRF check alone could not close. - The same two download paths now use create_ssrf_safe_async_client (from #57860) so the preflight-validated hostname is resolved once, validated, and dialed by IP — closing the DNS-rebinding TOCTOU window for the token-bearing inbound fetches as well. - #60009's slack_tokens.json permission warning is generalized into utils.warn_if_credential_file_broadly_readable() (POSIX-only, fail-quiet) and wired into the other read path with the same gap: google_chat's load_user_credentials(). google_chat already writes 0o600 via _write_private_json; the read-time warning covers hand-provisioned/legacy files. Nothing in-repo writes slack_tokens.json (user/OAuth-provisioned), so there is no write path to chmod for Slack. Security tests both directions: non-CDN/lookalike/http URLs and connect-time DNS rebinds are blocked before any TCP connect; real files.slack.com, Enterprise Grid, and slack-files.com URLs still reach the network layer; 0o600 files stay silent while 0o644/0o640 warn with a chmod hint. A/B: all 10 new download-guard tests fail with the hardening reverted and pass with it applied.
…ds, widen token-file perms warning Follow-up hardening on top of the C14 cherry-picks (#57860/#44026/#66742/#60009): - Slack file downloads (_download_slack_file/_download_slack_file_bytes) now require an https URL on a Slack CDN host (files.slack.com, *.slack.com Enterprise Grid, *.slack-files.com legacy shares) before attaching the bot token. url_private/url_private_download only ever point at the Slack CDN, so a forged file object from a malicious workspace app or compromised event stream pointing the Bearer-token download at an arbitrary PUBLIC host (token exfiltration) is now refused — a hole #44026's generic private-IP SSRF check alone could not close. - The same two download paths now use create_ssrf_safe_async_client (from #57860) so the preflight-validated hostname is resolved once, validated, and dialed by IP — closing the DNS-rebinding TOCTOU window for the token-bearing inbound fetches as well. - #60009's slack_tokens.json permission warning is generalized into utils.warn_if_credential_file_broadly_readable() (POSIX-only, fail-quiet) and wired into the other read path with the same gap: google_chat's load_user_credentials(). google_chat already writes 0o600 via _write_private_json; the read-time warning covers hand-provisioned/legacy files. Nothing in-repo writes slack_tokens.json (user/OAuth-provisioned), so there is no write path to chmod for Slack. Security tests both directions: non-CDN/lookalike/http URLs and connect-time DNS rebinds are blocked before any TCP connect; real files.slack.com, Enterprise Grid, and slack-files.com URLs still reach the network layer; 0o600 files stay silent while 0o644/0o640 warn with a chmod hint. A/B: all 10 new download-guard tests fail with the hardening reverted and pass with it applied.
|
Merged via #70193 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your inbound file URL validation was hand-ported to the plugin adapter, tightened to a Slack CDN allowlist. Thanks for the contribution! |
…ds, widen token-file perms warning Follow-up hardening on top of the C14 cherry-picks (NousResearch#57860/NousResearch#44026/NousResearch#66742/NousResearch#60009): - Slack file downloads (_download_slack_file/_download_slack_file_bytes) now require an https URL on a Slack CDN host (files.slack.com, *.slack.com Enterprise Grid, *.slack-files.com legacy shares) before attaching the bot token. url_private/url_private_download only ever point at the Slack CDN, so a forged file object from a malicious workspace app or compromised event stream pointing the Bearer-token download at an arbitrary PUBLIC host (token exfiltration) is now refused — a hole NousResearch#44026's generic private-IP SSRF check alone could not close. - The same two download paths now use create_ssrf_safe_async_client (from NousResearch#57860) so the preflight-validated hostname is resolved once, validated, and dialed by IP — closing the DNS-rebinding TOCTOU window for the token-bearing inbound fetches as well. - NousResearch#60009's slack_tokens.json permission warning is generalized into utils.warn_if_credential_file_broadly_readable() (POSIX-only, fail-quiet) and wired into the other read path with the same gap: google_chat's load_user_credentials(). google_chat already writes 0o600 via _write_private_json; the read-time warning covers hand-provisioned/legacy files. Nothing in-repo writes slack_tokens.json (user/OAuth-provisioned), so there is no write path to chmod for Slack. Security tests both directions: non-CDN/lookalike/http URLs and connect-time DNS rebinds are blocked before any TCP connect; real files.slack.com, Enterprise Grid, and slack-files.com URLs still reach the network layer; 0o600 files stay silent while 0o644/0o640 warn with a chmod hint. A/B: all 10 new download-guard tests fail with the hardening reverted and pass with it applied.
Summary
When Hermes receives a file on Slack it downloads the bytes server-side using the bot token. The two inbound download helpers followed redirects without validating the destination, while the outbound
send_imagepath in the same adapter already validates URLs and re-checks every redirect hop. Because the request carries anAuthorization: Bearer <bot-token>header andfollow_redirects=True, a URL that resolves to — or 3xx-redirects into — a private/internal address could be used to reach internal services and/or egress the bot token to a non-Slack host (CWE-918, SSRF).Root cause
gateway/platforms/slack.py—_download_slack_fileand_download_slack_file_bytes:No
is_safe_urlpre-flight and no redirect guard, unlike the guarded outbound path andbase.cache_image_from_url.Fix
Apply the existing, canonical pattern to both inbound sibling paths:
is_safe_url(url)pre-flight (raisesValueErrorbefore any network I/O when the target is private/internal),event_hooks={"response": [_ssrf_redirect_guard]}so each redirect hop is re-validated.This reuses
tools.url_safety.is_safe_urlandgateway.platforms.base._ssrf_redirect_guard— the same helpers the outbound path uses — so behavior stays consistent across the adapter.Tests
tests/gateway/test_slack_download_ssrf.py(parametrized over both helpers):ValueErrorbefore any HTTP client call,AsyncClientis constructed with_ssrf_redirect_guardregistered on the response hook.Impact
Legitimate Slack file URLs (
files.slack.cometc.) are unaffected. Only requests targeting private/internal/metadata addresses or redirecting into them are blocked. No change to prompt cache, role alternation, or config.