mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
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>
This commit is contained in:
parent
42626da1ce
commit
0cd4afeafd
9 changed files with 268 additions and 20 deletions
|
|
@ -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={
|
||||
|
|
|
|||
|
|
@ -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]},
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue