From 2e08b778ab26c2f045900216b1d2e4d58d0ece6c Mon Sep 17 00:00:00 2001 From: zapabob Date: Thu, 11 Jun 2026 14:39:49 +0900 Subject: [PATCH] fix(security): validate inbound Slack file URLs against SSRF _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. --- plugins/platforms/slack/adapter.py | 34 +++++- tests/gateway/test_slack_download_ssrf.py | 129 ++++++++++++++++++++++ 2 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 tests/gateway/test_slack_download_ssrf.py diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 20726599cd41..c07bd2968e0d 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -7171,6 +7171,18 @@ class SlackAdapter(BasePlatformAdapter): ) -> str: """Download a Slack file using the bot token for auth, with retry.""" import httpx + from gateway.platforms.base import _ssrf_redirect_guard, safe_url_for_log + from tools.url_safety import is_safe_url + + # SSRF guard: the download attaches the bot token, so a URL that + # resolves to (or 3xx-redirects into) a private/internal address would + # both leak the token and let the server reach internal services + # (CWE-918). The outbound send_image() path is already guarded; this + # is the inbound sibling that was missing the same protection. + if not is_safe_url(url): + raise ValueError( + f"Blocked unsafe Slack file URL (SSRF protection): {safe_url_for_log(url)}" + ) bot_token = ( self._team_clients[team_id].token @@ -7178,7 +7190,11 @@ class SlackAdapter(BasePlatformAdapter): else self.config.token ) - async with httpx.AsyncClient(timeout=30.0, follow_redirects=True) as client: + async with httpx.AsyncClient( + timeout=30.0, + follow_redirects=True, + event_hooks={"response": [_ssrf_redirect_guard]}, + ) as client: for attempt in range(3): try: response = await client.get( @@ -7227,6 +7243,16 @@ class SlackAdapter(BasePlatformAdapter): async def _download_slack_file_bytes(self, url: str, team_id: str = "") -> bytes: """Download a Slack file and return raw bytes, with retry.""" import httpx + from gateway.platforms.base import _ssrf_redirect_guard, safe_url_for_log + from tools.url_safety import is_safe_url + + # SSRF guard (CWE-918): see _download_slack_file. This sibling path + # also attaches the bot token and must validate the destination plus + # every redirect hop. + if not is_safe_url(url): + raise ValueError( + f"Blocked unsafe Slack file URL (SSRF protection): {safe_url_for_log(url)}" + ) bot_token = ( self._team_clients[team_id].token @@ -7234,7 +7260,11 @@ class SlackAdapter(BasePlatformAdapter): else self.config.token ) - async with httpx.AsyncClient(timeout=30.0, follow_redirects=True) as client: + async with httpx.AsyncClient( + timeout=30.0, + follow_redirects=True, + event_hooks={"response": [_ssrf_redirect_guard]}, + ) as client: for attempt in range(3): try: response = await client.get( diff --git a/tests/gateway/test_slack_download_ssrf.py b/tests/gateway/test_slack_download_ssrf.py new file mode 100644 index 000000000000..47c0d282765c --- /dev/null +++ b/tests/gateway/test_slack_download_ssrf.py @@ -0,0 +1,129 @@ +"""SSRF regression tests for inbound Slack file downloads. + +``_download_slack_file`` / ``_download_slack_file_bytes`` attach the bot +token and follow redirects, so they must validate the destination (CWE-918) +exactly like the already-guarded outbound ``send_image`` path: a pre-flight +``is_safe_url`` check plus a per-redirect guard. +""" +import asyncio +import sys +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from gateway.platforms.base import _ssrf_redirect_guard + + +def _ensure_slack_mock(): + """Install mock slack modules so SlackAdapter can be imported.""" + if "slack_bolt" not in sys.modules: + for name in ( + "slack_bolt", + "slack_bolt.adapter", + "slack_bolt.adapter.socket_mode", + "slack_bolt.adapter.socket_mode.async_handler", + "slack_bolt.async_app", + "slack_sdk", + "slack_sdk.web", + "slack_sdk.web.async_client", + "slack_sdk.errors", + ): + sys.modules.setdefault(name, MagicMock()) + if "aiohttp" not in sys.modules: + sys.modules.setdefault("aiohttp", MagicMock()) + + +_ensure_slack_mock() + +from plugins.platforms.slack.adapter import SlackAdapter # noqa: E402 + + +def _fake_adapter(): + self = SlackAdapter.__new__(SlackAdapter) + self.config = SimpleNamespace(token="xoxb-test-token") + self._team_clients = {} + return self + + +class _NetworkTouched(RuntimeError): + pass + + +class _RecordingClient: + """Captures AsyncClient kwargs; refuses to perform real I/O.""" + + last_kwargs = None + + def __init__(self, **kwargs): + type(self).last_kwargs = kwargs + + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def get(self, *args, **kwargs): + raise _NetworkTouched("network access attempted") + + +@pytest.mark.parametrize( + "method_name", + ["_download_slack_file", "_download_slack_file_bytes"], +) +def test_unsafe_url_blocked_before_network(monkeypatch, method_name): + import tools.url_safety as url_safety + + calls = {"checked": []} + + def fake_is_safe_url(url, *a, **k): + calls["checked"].append(url) + return False + + monkeypatch.setattr(url_safety, "is_safe_url", fake_is_safe_url) + + # If the guard is bypassed, the fake client raises _NetworkTouched; a + # correct implementation raises ValueError *before* touching httpx. + monkeypatch.setattr("httpx.AsyncClient", _RecordingClient) + + self = _fake_adapter() + method = getattr(self, method_name) + args = ("http://169.254.169.254/latest/meta-data/", ".jpg") \ + if method_name == "_download_slack_file" \ + else ("http://169.254.169.254/latest/meta-data/",) + + with pytest.raises(ValueError): + asyncio.run(method(*args)) + + assert calls["checked"], "download must call is_safe_url before fetching" + + +@pytest.mark.parametrize( + "method_name", + ["_download_slack_file", "_download_slack_file_bytes"], +) +def test_redirect_guard_is_wired(monkeypatch, method_name): + import tools.url_safety as url_safety + + monkeypatch.setattr(url_safety, "is_safe_url", lambda *a, **k: True) + monkeypatch.setattr("httpx.AsyncClient", _RecordingClient) + + self = _fake_adapter() + method = getattr(self, method_name) + args = ("https://files.slack.com/x.jpg", ".jpg") \ + if method_name == "_download_slack_file" \ + else ("https://files.slack.com/x.jpg",) + + # The fake client raises when .get() is called; we only care that the + # client was constructed with the redirect guard hook. + with pytest.raises(_NetworkTouched): + asyncio.run(method(*args)) + + kwargs = _RecordingClient.last_kwargs + assert kwargs is not None + hooks = kwargs.get("event_hooks", {}) + assert _ssrf_redirect_guard in hooks.get("response", []), ( + "AsyncClient must register _ssrf_redirect_guard to block " + "redirect-based SSRF" + )