From 0d6717e5aa6423e62586ce9de6290900fedcde97 Mon Sep 17 00:00:00 2001 From: dsad Date: Sun, 12 Jul 2026 04:12:41 +0300 Subject: [PATCH] fix(platforms): block image upload redirects to private URLs --- plugins/platforms/discord/adapter.py | 172 +++++++++----- plugins/platforms/slack/adapter.py | 5 +- tests/gateway/test_send_multiple_images.py | 259 +++++++++++++++++++++ 3 files changed, 373 insertions(+), 63 deletions(-) diff --git a/plugins/platforms/discord/adapter.py b/plugins/platforms/discord/adapter.py index a64231ff7267..dd73971456ab 100644 --- a/plugins/platforms/discord/adapter.py +++ b/plugins/platforms/discord/adapter.py @@ -26,6 +26,7 @@ import time from collections import defaultdict from contextlib import suppress from typing import Callable, Dict, List, Optional, Any, Tuple +from urllib.parse import urljoin from agent.async_utils import ( consume_detached_task_result as _consume_background_task_result, @@ -67,6 +68,8 @@ _DISCORD_NONCONVERSATIONAL_METADATA_KEYS = frozenset({ "non_conversational", "non_conversational_history", }) +_DISCORD_IMAGE_REDIRECT_STATUSES = {301, 302, 303, 307, 308} +_DISCORD_IMAGE_MAX_REDIRECTS = 10 # Upgrade-bridge fallback only. The primary mechanism is the persisted # non-conversational message-ID set populated from explicitly marked sends # (metadata["non_conversational"]). These regexes exist solely to recognize @@ -135,6 +138,43 @@ from gateway.platforms.base import ( from tools.url_safety import is_safe_url +async def _read_url_image_with_redirect_guard( + session: Any, + url: str, + *, + timeout: Any, + request_kwargs: Dict[str, Any], +) -> Tuple[int, bytes, Dict[str, str]]: + """Read an image URL while re-checking every redirect target for SSRF.""" + current_url = url + for _ in range(_DISCORD_IMAGE_MAX_REDIRECTS + 1): + if not is_safe_url(current_url): + raise ValueError("Blocked unsafe image URL redirect") + + async with session.get( + current_url, + timeout=timeout, + allow_redirects=False, + **request_kwargs, + ) as resp: + raw_headers = getattr(resp, "headers", {}) or {} + headers = {str(key).lower(): value for key, value in dict(raw_headers).items()} + status = int(getattr(resp, "status", 0)) + if status in _DISCORD_IMAGE_REDIRECT_STATUSES: + location = headers.get("location") + if not location: + return status, b"", headers + next_url = urljoin(current_url, str(location)) + if not is_safe_url(next_url): + raise ValueError("Blocked redirect to private/internal address") + current_url = next_url + continue + + return status, await resp.read(), headers + + raise ValueError("Too many image URL redirects") + + def _truncate_discord_component_text(text: str, limit: int) -> str: """Return text within Discord's UTF-16 component field budget.""" return _prefix_within_utf16_limit(str(text or ""), max(0, limit)) @@ -3396,25 +3436,27 @@ class DiscordAdapter(BasePlatformAdapter): _sess_kw, _req_kw = proxy_kwargs_for_aiohttp(_proxy) if aiohttp_session is None: aiohttp_session = _aiohttp.ClientSession(**_sess_kw) - async with aiohttp_session.get( - image_url, timeout=_aiohttp.ClientTimeout(total=30), **_req_kw, - ) as resp: - if resp.status != 200: - logger.warning( - "[%s] Failed to download image (HTTP %d) in batch: %s", - self.name, resp.status, image_url[:80], - ) - continue - data = await resp.read() - ct = resp.headers.get("content-type", "image/png") - ext = "png" - if "jpeg" in ct or "jpg" in ct: - ext = "jpg" - elif "gif" in ct: - ext = "gif" - elif "webp" in ct: - ext = "webp" - files.append(_discord_mod.File(_io.BytesIO(data), filename=f"image_{len(files)}.{ext}")) + status, data, headers = await _read_url_image_with_redirect_guard( + aiohttp_session, + image_url, + timeout=_aiohttp.ClientTimeout(total=30), + request_kwargs=_req_kw, + ) + if status != 200: + logger.warning( + "[%s] Failed to download image (HTTP %d) in batch: %s", + self.name, status, image_url[:80], + ) + continue + ct = headers.get("content-type", "image/png") + ext = "png" + if "jpeg" in ct or "jpg" in ct: + ext = "jpg" + elif "gif" in ct: + ext = "gif" + elif "webp" in ct: + ext = "webp" + files.append(_discord_mod.File(_io.BytesIO(data), filename=f"image_{len(files)}.{ext}")) except Exception as dl_err: logger.warning("[%s] Download failed for %s: %s", self.name, image_url[:80], dl_err) continue @@ -4531,37 +4573,40 @@ class DiscordAdapter(BasePlatformAdapter): _proxy = resolve_proxy_url(platform_env_var="DISCORD_PROXY") _sess_kw, _req_kw = proxy_kwargs_for_aiohttp(_proxy) async with aiohttp.ClientSession(**_sess_kw) as session: - async with session.get(image_url, timeout=aiohttp.ClientTimeout(total=30), **_req_kw) as resp: - if resp.status != 200: - raise Exception(f"Failed to download image: HTTP {resp.status}") + status, image_data, headers = await _read_url_image_with_redirect_guard( + session, + image_url, + timeout=aiohttp.ClientTimeout(total=30), + request_kwargs=_req_kw, + ) + if status != 200: + raise Exception(f"Failed to download image: HTTP {status}") - image_data = await resp.read() + # Determine filename from URL or content type + content_type = headers.get("content-type", "image/png") + ext = "png" + if "jpeg" in content_type or "jpg" in content_type: + ext = "jpg" + elif "gif" in content_type: + ext = "gif" + elif "webp" in content_type: + ext = "webp" - # Determine filename from URL or content type - content_type = resp.headers.get("content-type", "image/png") - ext = "png" - if "jpeg" in content_type or "jpg" in content_type: - ext = "jpg" - elif "gif" in content_type: - ext = "gif" - elif "webp" in content_type: - ext = "webp" + import io + file = discord.File(io.BytesIO(image_data), filename=f"image.{ext}") - import io - file = discord.File(io.BytesIO(image_data), filename=f"image.{ext}") - - if self._is_forum_parent(channel): - return await self._forum_post_file( - channel, - content=(caption or "").strip(), - file=file, - ) - - msg = await channel.send( - content=caption if caption else None, + if self._is_forum_parent(channel): + return await self._forum_post_file( + channel, + content=(caption or "").strip(), file=file, ) - return SendResult(success=True, message_id=str(msg.id)) + + msg = await channel.send( + content=caption if caption else None, + file=file, + ) + return SendResult(success=True, message_id=str(msg.id)) except ImportError: logger.warning( @@ -4610,27 +4655,30 @@ class DiscordAdapter(BasePlatformAdapter): _proxy = resolve_proxy_url(platform_env_var="DISCORD_PROXY") _sess_kw, _req_kw = proxy_kwargs_for_aiohttp(_proxy) async with aiohttp.ClientSession(**_sess_kw) as session: - async with session.get(animation_url, timeout=aiohttp.ClientTimeout(total=30), **_req_kw) as resp: - if resp.status != 200: - raise Exception(f"Failed to download animation: HTTP {resp.status}") + status, animation_data, _headers = await _read_url_image_with_redirect_guard( + session, + animation_url, + timeout=aiohttp.ClientTimeout(total=30), + request_kwargs=_req_kw, + ) + if status != 200: + raise Exception(f"Failed to download animation: HTTP {status}") - animation_data = await resp.read() + import io + file = discord.File(io.BytesIO(animation_data), filename="animation.gif") - import io - file = discord.File(io.BytesIO(animation_data), filename="animation.gif") - - if self._is_forum_parent(channel): - return await self._forum_post_file( - channel, - content=(caption or "").strip(), - file=file, - ) - - msg = await channel.send( - content=caption if caption else None, + if self._is_forum_parent(channel): + return await self._forum_post_file( + channel, + content=(caption or "").strip(), file=file, ) - return SendResult(success=True, message_id=str(msg.id)) + + msg = await channel.send( + content=caption if caption else None, + file=file, + ) + return SendResult(success=True, message_id=str(msg.id)) except ImportError: logger.warning( diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 21e9e734df3d..51fe2c885d53 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -52,6 +52,7 @@ from gateway.platforms.base import ( is_host_excluded_by_no_proxy, resolve_proxy_url, safe_url_for_log, + _ssrf_redirect_guard, cache_document_from_bytes, cache_video_from_bytes, ) @@ -2297,7 +2298,9 @@ class SlackAdapter(BasePlatformAdapter): initial_comment_parts: List[str] = [] try: async with _httpx.AsyncClient( - timeout=30.0, follow_redirects=True + timeout=30.0, + follow_redirects=True, + event_hooks={"response": [_ssrf_redirect_guard]}, ) as http_client: for image_url, alt_text in chunk: if alt_text: diff --git a/tests/gateway/test_send_multiple_images.py b/tests/gateway/test_send_multiple_images.py index 77acf205296d..8b32be4e5c7b 100644 --- a/tests/gateway/test_send_multiple_images.py +++ b/tests/gateway/test_send_multiple_images.py @@ -14,6 +14,7 @@ Signal's native implementation is covered by test_signal.py. import asyncio import sys +import types from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -264,6 +265,221 @@ class TestDiscordMultiImage: adapter._client = MagicMock() _run(adapter.send_multiple_images("67890", [])) + def test_url_batch_blocks_private_redirect_before_send(self, adapter, monkeypatch): + """A public image URL must not redirect into private metadata and then upload.""" + import plugins.platforms.discord.adapter as discord_adapter + + public_url = "https://cdn.example.test/image.png" + private_url = "http://169.254.169.254/latest/meta-data/" + safe_calls = [] + + def fake_is_safe_url(url): + safe_calls.append(url) + return not str(url).startswith("http://169.254.169.254") + + class FakeResponse: + status = 302 + headers = {"location": private_url} + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def read(self): + return b"metadata-secret" + + class FakeSession: + def get(self, url, **kwargs): + assert kwargs.get("allow_redirects") is False + return FakeResponse() + + async def close(self): + return None + + fake_aiohttp = types.SimpleNamespace( + ClientSession=lambda **kwargs: FakeSession(), + ClientTimeout=lambda **kwargs: kwargs, + ) + monkeypatch.setitem(sys.modules, "aiohttp", fake_aiohttp) + monkeypatch.setattr(discord_adapter, "is_safe_url", fake_is_safe_url) + + mock_channel = MagicMock() + mock_channel.send = AsyncMock(return_value=MagicMock(id=1)) + adapter._client.get_channel = MagicMock(return_value=mock_channel) + adapter._is_forum_parent = MagicMock(return_value=False) + + _run(adapter.send_multiple_images("67890", [(public_url, "caption")])) + + mock_channel.send.assert_not_awaited() + assert private_url in safe_calls + + def test_url_batch_follows_safe_redirect_location_header(self, adapter, monkeypatch): + """Redirect handling preserves aiohttp's case-insensitive Location behavior.""" + import plugins.platforms.discord.adapter as discord_adapter + + public_url = "https://cdn.example.test/image.png" + redirected_url = "https://assets.example.test/image.png" + requested_urls = [] + + class RedirectResponse: + status = 302 + headers = {"Location": redirected_url} + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def read(self): + raise AssertionError("redirect responses must not be read") + + class ImageResponse: + status = 200 + headers = {"Content-Type": "image/png"} + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def read(self): + return b"\x89PNG" + + class FakeSession: + def get(self, url, **kwargs): + assert kwargs.get("allow_redirects") is False + requested_urls.append(url) + if url == public_url: + return RedirectResponse() + assert url == redirected_url + return ImageResponse() + + async def close(self): + return None + + fake_aiohttp = types.SimpleNamespace( + ClientSession=lambda **kwargs: FakeSession(), + ClientTimeout=lambda **kwargs: kwargs, + ) + monkeypatch.setitem(sys.modules, "aiohttp", fake_aiohttp) + monkeypatch.setattr(discord_adapter, "is_safe_url", lambda url: True) + + mock_channel = MagicMock() + mock_channel.send = AsyncMock(return_value=MagicMock(id=1)) + adapter._client.get_channel = MagicMock(return_value=mock_channel) + adapter._is_forum_parent = MagicMock(return_value=False) + + _run(adapter.send_multiple_images("67890", [(public_url, "caption")])) + + assert requested_urls == [public_url, redirected_url] + mock_channel.send.assert_awaited_once() + + def test_send_image_blocks_private_redirect_before_send(self, adapter, monkeypatch): + import plugins.platforms.discord.adapter as discord_adapter + + public_url = "https://cdn.example.test/image.png" + private_url = "http://169.254.169.254/latest/meta-data/" + + class FakeResponse: + status = 302 + headers = {"Location": private_url} + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def read(self): + return b"metadata-secret" + + class FakeSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + def get(self, url, **kwargs): + assert kwargs.get("allow_redirects") is False + return FakeResponse() + + fake_aiohttp = types.SimpleNamespace( + ClientSession=lambda **kwargs: FakeSession(), + ClientTimeout=lambda **kwargs: kwargs, + ) + monkeypatch.setitem(sys.modules, "aiohttp", fake_aiohttp) + monkeypatch.setattr( + discord_adapter, + "is_safe_url", + lambda url: not str(url).startswith("http://169.254.169.254"), + ) + adapter._is_forum_parent = MagicMock(return_value=False) + mock_channel = MagicMock() + mock_channel.send = AsyncMock(return_value=MagicMock(id=1)) + adapter._client.get_channel = MagicMock(return_value=mock_channel) + adapter._client.fetch_channel = AsyncMock(return_value=mock_channel) + adapter.send = AsyncMock() + + _run(adapter.send_image("67890", public_url, "caption")) + + mock_channel.send.assert_not_awaited() + + def test_send_animation_blocks_private_redirect_before_send(self, adapter, monkeypatch): + import plugins.platforms.discord.adapter as discord_adapter + + public_url = "https://cdn.example.test/animation.gif" + private_url = "http://169.254.169.254/latest/meta-data/" + + class FakeResponse: + status = 302 + headers = {"Location": private_url} + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def read(self): + return b"metadata-secret" + + class FakeSession: + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + def get(self, url, **kwargs): + assert kwargs.get("allow_redirects") is False + return FakeResponse() + + fake_aiohttp = types.SimpleNamespace( + ClientSession=lambda **kwargs: FakeSession(), + ClientTimeout=lambda **kwargs: kwargs, + ) + monkeypatch.setitem(sys.modules, "aiohttp", fake_aiohttp) + monkeypatch.setattr( + discord_adapter, + "is_safe_url", + lambda url: not str(url).startswith("http://169.254.169.254"), + ) + adapter._is_forum_parent = MagicMock(return_value=False) + mock_channel = MagicMock() + mock_channel.send = AsyncMock(return_value=MagicMock(id=1)) + adapter._client.get_channel = MagicMock(return_value=mock_channel) + adapter._client.fetch_channel = AsyncMock(return_value=mock_channel) + adapter.send = AsyncMock() + + _run(adapter.send_animation("67890", public_url, "caption")) + + mock_channel.send.assert_not_awaited() + # --------------------------------------------------------------------------- # Slack @@ -337,6 +553,49 @@ class TestSlackMultiImage: client = adapter._get_client("C12345") client.files_upload_v2.assert_not_called() + def test_url_batch_blocks_private_redirect_before_upload(self, adapter, monkeypatch): + """HTTP redirects are rechecked before Slack batch uploads remote bytes.""" + import httpx + import tools.url_safety as url_safety + + public_url = "https://cdn.example.test/image.png" + private_url = "http://169.254.169.254/latest/meta-data/" + safe_calls = [] + + def fake_is_safe_url(url): + safe_calls.append(url) + return not str(url).startswith("http://169.254.169.254") + + class RedirectResponse: + is_redirect = True + url = public_url + headers = {"location": private_url} + next_request = None + + class FakeAsyncClient: + def __init__(self, **kwargs): + self.event_hooks = kwargs.get("event_hooks", {}) + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + async def get(self, url): + for hook in self.event_hooks.get("response", []): + await hook(RedirectResponse()) + raise AssertionError("private redirect was not blocked before fetch") + + monkeypatch.setattr(httpx, "AsyncClient", FakeAsyncClient) + monkeypatch.setattr(url_safety, "is_safe_url", fake_is_safe_url) + + _run(adapter.send_multiple_images("C12345", [(public_url, "caption")])) + + client = adapter._get_client("C12345") + client.files_upload_v2.assert_not_called() + assert private_url in safe_calls + # --------------------------------------------------------------------------- # Mattermost