From 0cd4afeafdf10c80384786a8bda2678e376f5ccd Mon Sep 17 00:00:00 2001 From: Eugeniusz Gilewski Date: Wed, 15 Jul 2026 20:16:40 +0200 Subject: [PATCH] fix(security): guard remaining preflighted HTTP fetches Several platform fetch paths called is_safe_url before constructing ordinary httpx clients, leaving a second DNS lookup at connection time. This preserved the rebinding window for Slack batch images, Feishu documents, Telegram URL-photo fallback, and WeCom remote media. Route each path through create_ssrf_safe_async_client and the shared redirect guard so direct connections validate and dial vetted IPs while configured proxies remain an explicit trusted egress boundary. Add per-path regressions that change DNS from public at preflight to metadata at connect time. The Skills Hub provenance fixture intentionally serves content over loopback. Opt that test-scoped server into private-address access so it keeps exercising the real HTTP transport without weakening production blocking. Related #8033 Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com> --- plugins/platforms/feishu/adapter.py | 12 ++-- plugins/platforms/slack/adapter.py | 9 ++- plugins/platforms/telegram/adapter.py | 9 ++- plugins/platforms/wecom/adapter.py | 20 ++++-- tests/gateway/test_feishu.py | 62 ++++++++++++++++- tests/gateway/test_slack.py | 54 +++++++++++++++ .../gateway/test_telegram_thread_fallback.py | 66 +++++++++++++++++-- tests/gateway/test_wecom.py | 50 ++++++++++++++ tests/tools/test_skill_bundle_provenance.py | 6 +- 9 files changed, 268 insertions(+), 20 deletions(-) diff --git a/plugins/platforms/feishu/adapter.py b/plugins/platforms/feishu/adapter.py index 882b695b90c5..da15ddfb3ad8 100644 --- a/plugins/platforms/feishu/adapter.py +++ b/plugins/platforms/feishu/adapter.py @@ -3446,13 +3446,17 @@ class FeishuAdapter(BasePlatformAdapter): default_ext: str, preferred_name: str, ) -> tuple[str, str]: - from tools.url_safety import is_safe_url + from gateway.platforms.base import _ssrf_redirect_guard + from tools.url_safety import create_ssrf_safe_async_client, is_safe_url + if not is_safe_url(file_url): raise ValueError(f"Blocked unsafe URL (SSRF protection): {file_url[:80]}") - import httpx - - async with httpx.AsyncClient(timeout=30.0, follow_redirects=True) as client: + async with create_ssrf_safe_async_client( + timeout=30.0, + follow_redirects=True, + event_hooks={"response": [_ssrf_redirect_guard]}, + ) as client: response = await client.get( file_url, headers={ diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index c9bb1b1f3081..20726599cd41 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -2906,9 +2906,12 @@ class SlackAdapter(BasePlatformAdapter): chat_id, team_id=self._metadata_team_id(metadata) ) try: - import httpx as _httpx from urllib.parse import unquote as _unquote - from tools.url_safety import is_safe_url as _is_safe_url + from gateway.platforms.base import _ssrf_redirect_guard + from tools.url_safety import ( + create_ssrf_safe_async_client, + is_safe_url as _is_safe_url, + ) except Exception: await super().send_multiple_images(chat_id, images, metadata, human_delay) return @@ -2925,7 +2928,7 @@ class SlackAdapter(BasePlatformAdapter): file_uploads: List[Dict[str, Any]] = [] initial_comment_parts: List[str] = [] try: - async with _httpx.AsyncClient( + async with create_ssrf_safe_async_client( timeout=30.0, follow_redirects=True, event_hooks={"response": [_ssrf_redirect_guard]}, diff --git a/plugins/platforms/telegram/adapter.py b/plugins/platforms/telegram/adapter.py index 43fc073c4e72..dd19d2551a8d 100644 --- a/plugins/platforms/telegram/adapter.py +++ b/plugins/platforms/telegram/adapter.py @@ -6925,8 +6925,13 @@ class TelegramAdapter(BasePlatformAdapter): ) # Fallback: download and upload as file (supports up to 10MB) try: - import httpx - async with httpx.AsyncClient(timeout=30.0) as client: + from gateway.platforms.base import _ssrf_redirect_guard + from tools.url_safety import create_ssrf_safe_async_client + + async with create_ssrf_safe_async_client( + timeout=30.0, + event_hooks={"response": [_ssrf_redirect_guard]}, + ) as client: resp = await client.get(image_url) resp.raise_for_status() image_data = resp.content diff --git a/plugins/platforms/wecom/adapter.py b/plugins/platforms/wecom/adapter.py index 4fc743aa6e23..89eeecfde021 100644 --- a/plugins/platforms/wecom/adapter.py +++ b/plugins/platforms/wecom/adapter.py @@ -219,8 +219,14 @@ class WeComAdapter(BasePlatformAdapter): try: # Tighter keepalive so idle CLOSE_WAIT drains promptly (#18451). from gateway.platforms._http_client_limits import platform_httpx_limits - self._http_client = httpx.AsyncClient( - timeout=30.0, follow_redirects=True, limits=platform_httpx_limits(), + from gateway.platforms.base import _ssrf_redirect_guard + from tools.url_safety import create_ssrf_safe_async_client + + self._http_client = create_ssrf_safe_async_client( + timeout=30.0, + follow_redirects=True, + event_hooks={"response": [_ssrf_redirect_guard]}, + limits=platform_httpx_limits(), ) await self._open_connection() self._mark_connected() @@ -1095,14 +1101,20 @@ class WeComAdapter(BasePlatformAdapter): url: str, max_bytes: int, ) -> Tuple[bytes, Dict[str, str]]: - from tools.url_safety import is_safe_url + from gateway.platforms.base import _ssrf_redirect_guard + from tools.url_safety import create_ssrf_safe_async_client, is_safe_url + if not is_safe_url(url): raise ValueError(f"Blocked unsafe URL (SSRF protection): {url[:80]}") if not HTTPX_AVAILABLE: raise RuntimeError("httpx is required for WeCom media download") - client = self._http_client or httpx.AsyncClient(timeout=30.0, follow_redirects=True) + client = self._http_client or create_ssrf_safe_async_client( + timeout=30.0, + follow_redirects=True, + event_hooks={"response": [_ssrf_redirect_guard]}, + ) created_client = client is not self._http_client try: async with client.stream( diff --git a/tests/gateway/test_feishu.py b/tests/gateway/test_feishu.py index bf972f7ada9f..0e411b8cf600 100644 --- a/tests/gateway/test_feishu.py +++ b/tests/gateway/test_feishu.py @@ -3,6 +3,7 @@ import asyncio import json import os +import socket import tempfile import time import unittest @@ -1997,7 +1998,10 @@ class TestAdapterBehavior(unittest.TestCase): async def _run() -> tuple[str, str]: with patch("tools.url_safety.is_safe_url", return_value=True): - with patch("httpx.AsyncClient", _FakeAsyncClient): + with patch( + "tools.url_safety.create_ssrf_safe_async_client", + side_effect=lambda **_kwargs: _FakeAsyncClient(), + ): with patch( "plugins.platforms.feishu.adapter.cache_document_from_bytes", return_value="/tmp/cached-doc.bin", @@ -2017,6 +2021,62 @@ class TestAdapterBehavior(unittest.TestCase): # down, which only works by accident (httpx's eager buffering). self.assertLess(events.index("content_read"), events.index("client_exit")) + def test_download_remote_document_blocks_connect_time_rebind(self): + import httpcore + from httpcore._backends.auto import AutoBackend + from gateway.config import PlatformConfig + from plugins.platforms.feishu.adapter import FeishuAdapter + from tools.url_safety import SSRFConnectionBlocked + + adapter = FeishuAdapter(PlatformConfig()) + answers = iter(("93.184.216.34", "169.254.169.254")) + + def fake_getaddrinfo(_host, port, *_args, **_kwargs): + ip = next(answers) + return [ + (socket.AF_INET, socket.SOCK_STREAM, 6, "", (ip, port or 0)) + ] + + connect_attempts = [] + + async def fake_connect_tcp( + _self, + host, + port, + timeout=None, + local_address=None, + socket_options=None, + ): + connect_attempts.append((host, port)) + raise httpcore.ConnectError("stop before network") + + proxy_vars = { + name: "" + for name in ( + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "http_proxy", + "https_proxy", + "all_proxy", + ) + } + with ( + patch.dict(os.environ, proxy_vars, clear=False), + patch("socket.getaddrinfo", side_effect=fake_getaddrinfo), + patch.object(AutoBackend, "connect_tcp", new=fake_connect_tcp), + self.assertRaises(SSRFConnectionBlocked), + ): + asyncio.run( + adapter._download_remote_document( + "http://rebind.example/doc.bin", + default_ext=".bin", + preferred_name="doc", + ) + ) + + self.assertEqual(connect_attempts, []) + def test_dedup_state_persists_across_adapter_restart(self): from gateway.config import PlatformConfig from plugins.platforms.feishu.adapter import FeishuAdapter diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index 80c28b53376f..965f251728da 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -13,6 +13,7 @@ import contextlib import importlib from importlib.machinery import PathFinder import os +import socket import sys import time from types import ModuleType @@ -5991,6 +5992,59 @@ class TestSendImageSSRFGuards: assert call_kwargs.get("thread_ts") == "parent_ts_789" +class TestSendMultipleImagesSSRFGuards: + """Batch image downloads must revalidate DNS at TCP connect time.""" + + @pytest.mark.asyncio + async def test_batch_download_blocks_connect_time_rebind( + self, adapter, monkeypatch + ): + import httpcore + from httpcore._backends.auto import AutoBackend + + for proxy_var in ( + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "http_proxy", + "https_proxy", + "all_proxy", + ): + monkeypatch.delenv(proxy_var, raising=False) + + answers = iter(("93.184.216.34", "169.254.169.254")) + + def fake_getaddrinfo(_host, port, *_args, **_kwargs): + ip = next(answers) + return [ + (socket.AF_INET, socket.SOCK_STREAM, 6, "", (ip, port or 0)) + ] + + connect_attempts = [] + + async def fake_connect_tcp( + _self, + host, + port, + timeout=None, + local_address=None, + socket_options=None, + ): + connect_attempts.append((host, port)) + raise httpcore.ConnectError("stop before network") + + monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo) + monkeypatch.setattr(AutoBackend, "connect_tcp", fake_connect_tcp) + adapter._app.client.files_upload_v2 = AsyncMock(return_value={"ok": True}) + + await adapter.send_multiple_images( + "C123", [("http://rebind.example/image.png", "image")] + ) + + assert connect_attempts == [] + adapter._app.client.files_upload_v2.assert_not_awaited() + + # --------------------------------------------------------------------------- # TestProgressMessageThread # --------------------------------------------------------------------------- diff --git a/tests/gateway/test_telegram_thread_fallback.py b/tests/gateway/test_telegram_thread_fallback.py index 41dbcc001d3c..df02dedfb578 100644 --- a/tests/gateway/test_telegram_thread_fallback.py +++ b/tests/gateway/test_telegram_thread_fallback.py @@ -9,6 +9,7 @@ avoid retrying with a partial topic route that can render outside the lane. """ import sys +import socket import types from types import SimpleNamespace from unittest.mock import AsyncMock @@ -1082,15 +1083,15 @@ async def test_send_image_upload_dm_topic_reply_not_found_retry_drops_thread_id( async def get(self, _url): return _FakeResponse() - monkeypatch.setitem( - sys.modules, - "httpx", - SimpleNamespace(AsyncClient=_FakeAsyncClient), - ) adapter._bot = SimpleNamespace(send_photo=mock_send_photo) import tools.url_safety as url_safety monkeypatch.setattr(url_safety, "is_safe_url", lambda _url: True) + monkeypatch.setattr( + url_safety, + "create_ssrf_safe_async_client", + lambda **_kwargs: _FakeAsyncClient(), + ) result = await adapter.send_image( chat_id="123", @@ -1112,6 +1113,61 @@ async def test_send_image_upload_dm_topic_reply_not_found_retry_drops_thread_id( assert "direct_messages_topic_id" not in call_log[2] +@pytest.mark.asyncio +async def test_send_image_upload_fallback_blocks_connect_time_rebind(monkeypatch): + import httpcore + from httpcore._backends.auto import AutoBackend + from gateway.platforms.base import BasePlatformAdapter + + adapter = _make_adapter() + adapter._bot = SimpleNamespace( + send_photo=AsyncMock(side_effect=RuntimeError("force URL upload fallback")) + ) + + for proxy_var in ( + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "http_proxy", + "https_proxy", + "all_proxy", + ): + monkeypatch.delenv(proxy_var, raising=False) + + answers = iter(("93.184.216.34", "169.254.169.254")) + + def fake_getaddrinfo(_host, port, *_args, **_kwargs): + ip = next(answers) + return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (ip, port or 0))] + + connect_attempts = [] + + async def fake_connect_tcp( + _self, + host, + port, + timeout=None, + local_address=None, + socket_options=None, + ): + connect_attempts.append((host, port)) + raise httpcore.ConnectError("stop before network") + + async def fake_base_send_image(*_args, **_kwargs): + return SendResult(success=False, error="fallback") + + monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo) + monkeypatch.setattr(AutoBackend, "connect_tcp", fake_connect_tcp) + monkeypatch.setattr(BasePlatformAdapter, "send_image", fake_base_send_image) + + await adapter.send_image( + chat_id="123", + image_url="http://rebind.example/photo.png", + ) + + assert connect_attempts == [] + + @pytest.mark.asyncio async def test_slash_confirm_private_topic_callback_followup_sends_thread_and_reply(monkeypatch): adapter = _make_adapter() diff --git a/tests/gateway/test_wecom.py b/tests/gateway/test_wecom.py index 949851e4d969..1e9d9c759ed8 100644 --- a/tests/gateway/test_wecom.py +++ b/tests/gateway/test_wecom.py @@ -3,6 +3,7 @@ import asyncio import base64 import os +import socket from pathlib import Path from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -481,6 +482,55 @@ class TestMediaUpload: with pytest.raises(ValueError, match="exceeds WeCom limit"): await adapter._download_remote_bytes("https://example.com/file.bin", max_bytes=4) + @pytest.mark.asyncio + async def test_download_remote_bytes_blocks_connect_time_rebind(self, monkeypatch): + import httpcore + from httpcore._backends.auto import AutoBackend + from plugins.platforms.wecom.adapter import WeComAdapter + from tools.url_safety import SSRFConnectionBlocked + + for proxy_var in ( + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "http_proxy", + "https_proxy", + "all_proxy", + ): + monkeypatch.delenv(proxy_var, raising=False) + + answers = iter(("93.184.216.34", "169.254.169.254")) + + def fake_getaddrinfo(_host, port, *_args, **_kwargs): + ip = next(answers) + return [ + (socket.AF_INET, socket.SOCK_STREAM, 6, "", (ip, port or 0)) + ] + + connect_attempts = [] + + async def fake_connect_tcp( + _self, + host, + port, + timeout=None, + local_address=None, + socket_options=None, + ): + connect_attempts.append((host, port)) + raise httpcore.ConnectError("stop before network") + + monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo) + monkeypatch.setattr(AutoBackend, "connect_tcp", fake_connect_tcp) + + adapter = WeComAdapter(PlatformConfig(enabled=True)) + with pytest.raises(SSRFConnectionBlocked): + await adapter._download_remote_bytes( + "http://rebind.example/file.bin", max_bytes=1024 + ) + + assert connect_attempts == [] + @pytest.mark.asyncio async def test_cache_media_decrypts_url_payload_before_writing(self): from plugins.platforms.wecom.adapter import WeComAdapter diff --git a/tests/tools/test_skill_bundle_provenance.py b/tests/tools/test_skill_bundle_provenance.py index 33878b3f9caf..3bd9f77602e0 100644 --- a/tests/tools/test_skill_bundle_provenance.py +++ b/tests/tools/test_skill_bundle_provenance.py @@ -32,7 +32,11 @@ class _QuietHandler(SimpleHTTPRequestHandler): @pytest.fixture -def served_repo(tmp_path): +def served_repo(tmp_path, monkeypatch): + # The fixture intentionally serves over loopback. Keep exercising the real + # HTTP transport while opting this test server into private-address access. + monkeypatch.setattr("tools.url_safety._global_allow_private_urls", lambda: True) + repo = tmp_path / "upstream" repo.mkdir() (repo / "SKILL.md").write_text(SKILL_MD)