diff --git a/tests/tools/test_image_source.py b/tests/tools/test_image_source.py index 56d07d9fe42..ca93abb64d7 100644 --- a/tests/tools/test_image_source.py +++ b/tests/tools/test_image_source.py @@ -69,6 +69,38 @@ class TestLocalBackend: res = await isrc.resolve_image_source(f"file://{img}", isrc.ResolveContext()) assert res.mime == "image/jpeg" + @pytest.mark.asyncio + async def test_bare_relative_path_resolves(self, tmp_path, monkeypatch): + """A cwd-relative bare filename ('pic.png') is a valid local source — + main accepted it; the resolver must not regress it (PR review).""" + isrc = _reload(monkeypatch, tmp_path / "hermes") + monkeypatch.setenv("TERMINAL_ENV", "local") + img = tmp_path / "pic.png" + img.write_bytes(PNG) + monkeypatch.chdir(tmp_path) + res = await isrc.resolve_image_source("pic.png", isrc.ResolveContext()) + assert res.data == PNG + assert res.origin == "file" + + @pytest.mark.asyncio + async def test_unknown_url_scheme_rejected(self, tmp_path, monkeypatch): + isrc = _reload(monkeypatch, tmp_path / "hermes") + monkeypatch.setenv("TERMINAL_ENV", "local") + with pytest.raises(isrc.UnsupportedScheme): + await isrc.resolve_image_source( + "ftp://example.com/pic.png", isrc.ResolveContext()) + + @pytest.mark.asyncio + async def test_svg_rejected_with_conversion_hint(self, tmp_path, monkeypatch): + """SVG has no raster magic bytes; the resolver rejects it with a + dedicated, actionable message (review note W3).""" + isrc = _reload(monkeypatch, tmp_path / "hermes") + monkeypatch.setenv("TERMINAL_ENV", "local") + svg = tmp_path / "art.svg" + svg.write_bytes(b'') + with pytest.raises(isrc.NotAnImage, match="SVG is not supported"): + await isrc.resolve_image_source(str(svg), isrc.ResolveContext()) + class TestNonLocalBackendConfinement: """The security model: under a sandbox backend, host reads are confined to diff --git a/tests/tools/test_vision_native_fast_path.py b/tests/tools/test_vision_native_fast_path.py index 4423e9c9cb3..d66b52aec36 100644 --- a/tests/tools/test_vision_native_fast_path.py +++ b/tests/tools/test_vision_native_fast_path.py @@ -119,9 +119,9 @@ class TestVisionAnalyzeNative: assert isinstance(result, str) parsed = json.loads(result) assert parsed.get("success") is False - # Unified resolver: unrecognized/non-existent local source. + # Unified resolver: local backend reports a clean not-found. err = parsed.get("error", "").lower() - assert "not reachable" in err or "unrecognized image source" in err or "no active sandbox" in err + assert "image file not found" in err or "no active sandbox" in err def test_empty_image_url_returns_error(self): result = asyncio.get_event_loop().run_until_complete( diff --git a/tools/image_source.py b/tools/image_source.py index 770ffc4419e..9046577359c 100644 --- a/tools/image_source.py +++ b/tools/image_source.py @@ -26,11 +26,13 @@ mechanism that fixes "vision can't see container files" also closes the escape. """ from __future__ import annotations +import asyncio import base64 import os -from dataclasses import dataclass, field +import re +from dataclasses import dataclass from pathlib import Path -from typing import Any, Optional +from typing import Optional # Raw-bytes INGEST budget — what the resolver will load before handing off. # This is deliberately the 50MB download cap (tools/vision_tools._VISION_MAX_DOWNLOAD_BYTES), @@ -70,8 +72,6 @@ class NotAnImage(ImageResolutionError): @dataclass class ResolveContext: task_id: Optional[str] = None - cfg: Optional[dict[str, Any]] = None - extra_roots: tuple = field(default_factory=tuple) @dataclass @@ -81,6 +81,11 @@ class ResolvedImage: origin: str # one of: data | http | file | local | container +# Explicit URL scheme, e.g. "ftp://", "s3://". Bare Windows drive paths +# ("C:\x.png") don't match because they lack the "//". +_SCHEME_RE = re.compile(r"^[A-Za-z][A-Za-z0-9+.\-]*://") + + async def resolve_image_source(src: str, ctx: ResolveContext) -> ResolvedImage: if not isinstance(src, str) or not src.strip(): raise SourceNotFound("image_url is required", src=str(src)) @@ -94,27 +99,34 @@ async def resolve_image_source(src: str, ctx: ResolveContext) -> ResolvedImage: raise SourceUnsafe(reason, src=s) return _finalize(await _download_to_bytes(s), "", "http", s) - candidate = s[len("file://"):] if s.startswith("file://") else s - if s.startswith("file://") or _looks_like_path(candidate): - p = Path(os.path.expanduser(candidate)) - # Confinement decision (see module docstring). Under a non-local backend - # a path is host-readable ONLY if it lands in a media cache (after - # translating a container-visible cache path back to its host mount); - # every other path is read inside the sandbox via exec-read, so a host - # path outside the caches never yields the host's bytes. - host_target = _permitted_host_read_target(p, ctx) - if host_target is not None and host_target.is_file(): - return _finalize(host_target.read_bytes(), "", "file", s) - # Not a permitted host read (or the host file is absent) -> read the - # bytes inside the sandbox. On the local backend this reaches the same - # host file; under a sandbox it reads the container's filesystem. - return await _resolve_container_fallback(p, ctx, s) + if _SCHEME_RE.match(s) and not s.lower().startswith("file://"): + raise UnsupportedScheme( + "Unrecognized image source scheme. Use an http(s) URL, a local " + "file path, a file:// URI, or a data: URL.", + src=s, + ) - raise UnsupportedScheme( - "Unrecognized image source. Use an http(s) URL, a local file path, " - "a file:// URI, or a data: URL.", - src=s, - ) + # Everything else is a filesystem path — including bare relative names + # like "pic.png" (accepted on main; a path-shape gate here regressed them). + candidate = s[len("file://"):] if s.lower().startswith("file://") else s + p = Path(os.path.expanduser(candidate)) + # Confinement decision (see module docstring). Under a non-local backend + # a path is host-readable ONLY if it lands in a media cache (after + # translating a container-visible cache path back to its host mount); + # every other path is read inside the sandbox via exec-read, so a host + # path outside the caches never yields the host's bytes. + host_target = _permitted_host_read_target(p, ctx) + if host_target is not None and host_target.is_file(): + data = await asyncio.to_thread(host_target.read_bytes) + return _finalize(data, "", "file", s) + if _is_local_terminal_backend(): + # Local backend: any path was host-readable, so a miss simply means + # the file doesn't exist — no sandbox to fall back to. + raise SourceNotFound(f"image file not found: '{p}'", src=s, origin="file") + # Not a permitted host read (or the host file is absent) -> read the + # bytes inside the sandbox. Under a sandbox this reads the container's + # filesystem, never the host's. + return await _resolve_container_fallback(p, ctx, s) def _resolve_data_url(s: str) -> tuple[bytes, str]: @@ -135,8 +147,12 @@ def _resolve_data_url(s: str) -> tuple[bytes, str]: def _http_block_reason(url: str) -> Optional[str]: """Return a human-readable block reason, or None when the URL is allowed. - Preserves the specific website-policy message (rather than collapsing it to - a generic string) so the agent sees *why* a fetch was refused. + Pre-flight short-circuit: policy-blocked URLs are refused BEFORE any + network I/O. ``_download_image`` re-checks policy internally (per attempt + and against the final redirect target) — that second evaluation is + intentional, not redundant: this one guarantees no bytes move for a + blocked URL; the inner one covers redirects and non-resolver callers. + Preserves the specific website-policy message so the agent sees *why*. """ from tools.url_safety import is_safe_url from tools.website_policy import check_website_access @@ -157,16 +173,15 @@ async def _download_to_bytes(url: str) -> bytes: with tempfile.NamedTemporaryFile(suffix=".img", delete=False) as tf: tmp = Path(tf.name) try: - await _download_image(url, tmp) # enforces the 50MB stream cap + redirect SSRF guard - return tmp.read_bytes() + # Enforces the 50MB stream cap, redirect SSRF guard, and website policy. + await _download_image(url, tmp) + return await asyncio.to_thread(tmp.read_bytes) + except PermissionError as exc: # website policy block + raise SourceUnsafe(str(exc), src=url, origin="http") finally: tmp.unlink(missing_ok=True) -def _looks_like_path(s: str) -> bool: - return s.startswith(("/", "~", "./", "../")) or (len(s) > 1 and s[1] == ":") - - def _is_local_terminal_backend() -> bool: """True when the terminal backend runs directly on the host. @@ -288,5 +303,12 @@ def _finalize(data: bytes, declared_mime: str, origin: str, src: str) -> Resolve raise SourceTooLarge("image exceeds size limit", src=src, origin=origin) sniffed = _detect_image_mime_type_from_bytes(data) if sniffed is None: + if b" Optional[str]: """Magic-byte MIME sniff on raw bytes (authoritative; no extension trust). Returns ``None`` for anything without a recognized image header — including - SVG, which has no magic bytes and is not ingestible as vision by any - provider. Callers that accept SVG-by-text (path-based) layer that on top. + SVG, which has no magic bytes and cannot be ingested as raster vision input + by any provider. The resolver surfaces a dedicated error for SVG sources. """ header = data[:64] if header.startswith(b"\x89PNG\r\n\x1a\n"): @@ -242,17 +242,6 @@ def _detect_image_mime_type_from_bytes(data: bytes) -> Optional[str]: return None -def _detect_image_mime_type(image_path: Path) -> Optional[str]: - """Return a MIME type when the file looks like a supported image.""" - with image_path.open("rb") as f: - mime = _detect_image_mime_type_from_bytes(f.read(64)) - if mime is None and image_path.suffix.lower() == ".svg": - head = image_path.read_text(encoding="utf-8", errors="ignore")[:4096].lower() - if " bool: """Return True only for transient image-download failures worth retrying. @@ -871,7 +860,7 @@ async def _vision_analyze_native( temp_dir = get_hermes_dir("cache/vision", "temp_vision_images") temp_dir.mkdir(parents=True, exist_ok=True) temp_image_path = temp_dir / f"temp_image_{uuid.uuid4()}.img" - temp_image_path.write_bytes(resolved.data) + await asyncio.to_thread(temp_image_path.write_bytes, resolved.data) should_cleanup = True image_data_url = await _run_encode_on_cpu_executor( @@ -1018,7 +1007,7 @@ async def vision_analyze_tool( temp_dir = get_hermes_dir("cache/vision", "temp_vision_images") temp_dir.mkdir(parents=True, exist_ok=True) temp_image_path = temp_dir / f"temp_image_{uuid.uuid4()}.img" - temp_image_path.write_bytes(resolved.data) + await asyncio.to_thread(temp_image_path.write_bytes, resolved.data) should_cleanup = True # Get image file size for logging