fix(web): run URL SSRF checks off the event loop in async paths
Add async_is_safe_url() wrapping is_safe_url via asyncio.to_thread, and route all async SSRF call sites through it: web_extract_tool, the vision/video preflight checks, and both download redirect guards. socket.getaddrinfo blocks; calling it inline from async tool paths froze the event loop for the duration of DNS resolution. vision_tools: split _validate_image_url into _image_url_shape_ok (no DNS) + sync _validate_image_url (for sync callers/tests) + async _validate_image_url_async. Widened beyond the original PR #3691 to sibling async sites that also blocked the loop (second redirect guard, video preflight). Salvage of #3691 by @Kewe63 — surgically re-applied onto current main because the original branch was too stale to cherry-pick cleanly (would have reverted the web_crawl_tool refactor). Co-authored-by: Kewe63 <kewe.3217@gmail.com>
This commit is contained in:
@@ -372,7 +372,8 @@ class TestVisionDispatchLoopSafety:
|
||||
side_effect=lambda url, dest, **kw: _write_fake_image(dest),
|
||||
),
|
||||
patch(
|
||||
"tools.vision_tools._validate_image_url",
|
||||
"tools.vision_tools._validate_image_url_async",
|
||||
new_callable=AsyncMock,
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
@@ -416,7 +417,8 @@ class TestVisionDispatchLoopSafety:
|
||||
side_effect=lambda url, dest, **kw: _write_fake_image(dest),
|
||||
),
|
||||
patch(
|
||||
"tools.vision_tools._validate_image_url",
|
||||
"tools.vision_tools._validate_image_url_async",
|
||||
new_callable=AsyncMock,
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
|
||||
@@ -5,6 +5,7 @@ from unittest.mock import patch
|
||||
|
||||
from tools.url_safety import (
|
||||
is_safe_url,
|
||||
async_is_safe_url,
|
||||
is_always_blocked_url,
|
||||
_is_blocked_ip,
|
||||
_global_allow_private_urls,
|
||||
@@ -195,6 +196,24 @@ class TestIsSafeUrl:
|
||||
assert is_safe_url("https://multimedia.nt.qq.com.cn/download?id=123") is False
|
||||
|
||||
|
||||
class TestAsyncIsSafeUrl:
|
||||
"""async_is_safe_url must match is_safe_url (runs DNS in a thread pool)."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_public_url_allowed(self):
|
||||
with patch("socket.getaddrinfo", return_value=[
|
||||
(2, 1, 6, "", ("93.184.216.34", 0)),
|
||||
]):
|
||||
assert await async_is_safe_url("https://example.com/x") is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_localhost_blocked(self):
|
||||
with patch("socket.getaddrinfo", return_value=[
|
||||
(2, 1, 6, "", ("127.0.0.1", 0)),
|
||||
]):
|
||||
assert await async_is_safe_url("http://localhost:8080/") is False
|
||||
|
||||
|
||||
class TestIsBlockedIp:
|
||||
"""Direct tests for the _is_blocked_ip helper."""
|
||||
|
||||
|
||||
@@ -297,7 +297,7 @@ class TestErrorLoggingExcInfo:
|
||||
async def test_analysis_error_logs_exc_info(self, caplog):
|
||||
"""When vision_analyze_tool encounters an error, it should log with exc_info."""
|
||||
with (
|
||||
patch("tools.vision_tools._validate_image_url", return_value=True),
|
||||
patch("tools.vision_tools._validate_image_url_async", new_callable=AsyncMock, return_value=True),
|
||||
patch(
|
||||
"tools.vision_tools._download_image",
|
||||
new_callable=AsyncMock,
|
||||
@@ -329,7 +329,7 @@ class TestErrorLoggingExcInfo:
|
||||
return dest
|
||||
|
||||
with (
|
||||
patch("tools.vision_tools._validate_image_url", return_value=True),
|
||||
patch("tools.vision_tools._validate_image_url_async", new_callable=AsyncMock, return_value=True),
|
||||
patch("tools.vision_tools._download_image", side_effect=fake_download),
|
||||
patch(
|
||||
"tools.vision_tools._image_to_base64_data_url",
|
||||
@@ -451,7 +451,7 @@ class TestVisionSafetyGuards:
|
||||
|
||||
with (
|
||||
patch("tools.vision_tools.check_website_access", return_value=blocked),
|
||||
patch("tools.vision_tools._validate_image_url", return_value=True),
|
||||
patch("tools.vision_tools._validate_image_url_async", new_callable=AsyncMock, return_value=True),
|
||||
patch("tools.vision_tools._download_image", new_callable=AsyncMock) as mock_download,
|
||||
):
|
||||
result = json.loads(await vision_analyze_tool("https://blocked.test/cat.png", "describe"))
|
||||
@@ -549,7 +549,9 @@ class TestTildeExpansion:
|
||||
img = fake_home / "test_image.png"
|
||||
img.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 8)
|
||||
|
||||
# Windows expanduser() prefers USERPROFILE over HOME; POSIX uses HOME.
|
||||
monkeypatch.setenv("HOME", str(fake_home))
|
||||
monkeypatch.setenv("USERPROFILE", str(fake_home))
|
||||
|
||||
mock_response = MagicMock()
|
||||
mock_choice = MagicMock()
|
||||
@@ -580,6 +582,7 @@ class TestTildeExpansion:
|
||||
fake_home = tmp_path / "fakehome"
|
||||
fake_home.mkdir()
|
||||
monkeypatch.setenv("HOME", str(fake_home))
|
||||
monkeypatch.setenv("USERPROFILE", str(fake_home))
|
||||
|
||||
result = await vision_analyze_tool(
|
||||
"~/nonexistent.png", "describe this", "test/model"
|
||||
|
||||
@@ -372,7 +372,10 @@ class TestWebToolPolicy:
|
||||
from plugins.web.firecrawl import provider as firecrawl_provider
|
||||
|
||||
# Allow test URLs past SSRF check so website policy is what gets tested
|
||||
monkeypatch.setattr(web_tools, "is_safe_url", lambda url: True)
|
||||
async def _allow_ssrf(_url: str) -> bool:
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(web_tools, "async_is_safe_url", _allow_ssrf)
|
||||
# The per-URL website-policy gate moved into the firecrawl plugin's
|
||||
# extract() during the web-provider migration. Patch it at the new
|
||||
# location.
|
||||
@@ -406,7 +409,10 @@ class TestWebToolPolicy:
|
||||
from plugins.web.firecrawl import provider as firecrawl_provider
|
||||
|
||||
# Allow test URLs past SSRF check so website policy is what gets tested
|
||||
monkeypatch.setattr(web_tools, "is_safe_url", lambda url: True)
|
||||
async def _allow_ssrf(_url: str) -> bool:
|
||||
return True
|
||||
|
||||
monkeypatch.setattr(web_tools, "async_is_safe_url", _allow_ssrf)
|
||||
|
||||
def fake_check(url):
|
||||
if url == "https://allowed.test":
|
||||
|
||||
Reference in New Issue
Block a user