diff --git a/agent/tools/http_request.py b/agent/tools/http_request.py index aeea541f..1adb6e75 100644 --- a/agent/tools/http_request.py +++ b/agent/tools/http_request.py @@ -1,13 +1,10 @@ from typing import Any -from urllib.parse import urljoin, urlparse, urlunparse import httpx -from ..utils.url_safety import resolve_and_validate as _resolve_and_validate - -_MAX_REDIRECTS = 5 - -_REDIRECT_CODES = {301, 302, 303, 307, 308} +from ..utils.url_safety import pinned_url as _pinned_url # noqa: F401 (kept for tests) +from ..utils.url_safety import request_with_safe_redirects +from ..utils.url_safety import resolve_and_validate as _resolve_and_validate # noqa: F401 def _blocked_response(url: str, reason: str) -> dict[str, Any]: @@ -20,79 +17,19 @@ def _blocked_response(url: str, reason: str) -> dict[str, Any]: } -def _pinned_url(url: str, ip: str) -> str: - """Rewrite ``url`` so the connection targets ``ip`` while keeping the path/query. - - The original hostname is preserved separately for the ``Host`` header and TLS - SNI/cert verification (via httpx's ``sni_hostname`` request extension). - """ - parsed = urlparse(url) - host_literal = f"[{ip}]" if ":" in ip else ip - netloc = f"{host_literal}:{parsed.port}" if parsed.port else host_literal - return urlunparse(parsed._replace(netloc=netloc)) - - async def _request_with_safe_redirects( client: httpx.AsyncClient, method: str, url: str, **kwargs: Any, ) -> tuple[httpx.Response | None, dict[str, Any] | None]: - """Issue a request while validating every redirect target before following it. - - The hostname is resolved once per hop and the connection is pinned to the - validated IP, closing the DNS-rebinding race where a controlled resolver - returns a public IP at validation time and a private IP at connect time. - """ - current_method = method.upper() - current_url = url - request_kwargs = dict(kwargs) - # Pop caller headers/extensions ONCE so they're reused on every redirect hop - # (the per-hop Host + SNI are layered on top each time). Popping inside the - # loop dropped the caller's Authorization/Accept/etc. on the first redirect. - caller_headers = dict(request_kwargs.pop("headers", None) or {}) - caller_extensions = dict(request_kwargs.pop("extensions", None) or {}) - - for redirect_count in range(_MAX_REDIRECTS + 1): - is_safe, reason, hostname, addr_infos = _resolve_and_validate(current_url) - if not is_safe or hostname is None or addr_infos is None: - return None, _blocked_response(current_url, reason) - - pinned_ip = addr_infos[0][4][0] - parsed = urlparse(current_url) - headers = {**caller_headers, "Host": parsed.netloc} - extensions = {**caller_extensions, "sni_hostname": hostname} - - response = await client.request( - current_method, - _pinned_url(current_url, pinned_ip), - follow_redirects=False, - headers=headers, - extensions=extensions, - **request_kwargs, - ) - - if response.status_code not in _REDIRECT_CODES: - return response, None - - location = response.headers.get("Location") - if not location: - return response, None - - if redirect_count == _MAX_REDIRECTS: - return None, _blocked_response(current_url, "Too many redirects") - - current_url = urljoin(current_url, location) - - if response.status_code == 303 or ( - response.status_code in {301, 302} and current_method not in {"GET", "HEAD"} - ): - current_method = "GET" - request_kwargs.pop("data", None) - request_kwargs.pop("content", None) - request_kwargs.pop("json", None) - - return None, _blocked_response(current_url, "Too many redirects") + """Thin wrapper over the shared SSRF-safe redirect loop that shapes a blocked + hop into the tool's error-response dict.""" + response, blocked = await request_with_safe_redirects(client, method, url, **kwargs) + if blocked is not None: + blocked_url, reason = blocked + return None, _blocked_response(blocked_url, reason) + return response, None async def http_request( diff --git a/agent/utils/multimodal.py b/agent/utils/multimodal.py index 67709dc5..0bd2b0fc 100644 --- a/agent/utils/multimodal.py +++ b/agent/utils/multimodal.py @@ -13,7 +13,7 @@ from urllib.parse import urlparse import httpx from langchain_core.messages.content import create_image_block -from .url_safety import is_url_safe +from .url_safety import request_with_safe_redirects logger = logging.getLogger(__name__) @@ -52,23 +52,23 @@ async def fetch_image_block( image_url: str, client: httpx.AsyncClient, ) -> dict[str, Any] | None: - """Fetch image bytes and build an image content block.""" + """Fetch image bytes and build an image content block. + + The fetch validates and pins every redirect hop (SSRF guard) and drops any + Authorization header once the URL redirects, so a per-host token is never + replayed to a redirect target the caller never chose to authenticate to. + URLs are logged host-only — a signed image URL can carry a bearer token. + """ + host = (urlparse(image_url).hostname or "").lower() try: - safe, reason = is_url_safe(image_url) - if not safe: - logger.warning("Refusing to fetch image (SSRF guard) %s: %s", image_url, reason) - return None - logger.debug("Fetching image from %s", image_url) headers = None - host = (urlparse(image_url).hostname or "").lower() if host == "uploads.linear.app" or host.endswith(".uploads.linear.app"): linear_api_key = os.environ.get("LINEAR_API_KEY", "") if linear_api_key: headers = {"Authorization": linear_api_key} else: logger.warning( - "LINEAR_API_KEY not set; cannot authenticate image fetch for %s", - image_url, + "LINEAR_API_KEY not set; cannot authenticate image fetch for %s", host ) elif host == "files.slack.com" or host.endswith(".files.slack.com"): slack_bot_token = os.environ.get("SLACK_BOT_TOKEN", "") @@ -76,41 +76,39 @@ async def fetch_image_block( headers = {"Authorization": f"Bearer {slack_bot_token}"} else: logger.warning( - "SLACK_BOT_TOKEN not set; cannot authenticate image fetch for %s", - image_url, + "SLACK_BOT_TOKEN not set; cannot authenticate image fetch for %s", host ) - response = await client.get(image_url, headers=headers, follow_redirects=True) + + response, blocked = await request_with_safe_redirects( + client, "GET", image_url, headers=headers, strip_auth_on_redirect=True + ) + if blocked is not None: + _, reason = blocked + logger.warning("Refusing to fetch image (SSRF guard) from %s: %s", host, reason) + return None response.raise_for_status() content_type = response.headers.get("Content-Type", "").split(";")[0].strip() if not content_type: guessed, _ = mimetypes.guess_type(image_url) if not guessed: - logger.warning( - "Could not determine content type for %s; skipping image", - image_url, - ) + logger.warning("Could not determine content type from %s; skipping image", host) return None content_type = guessed supported_types = {"image/jpeg", "image/png", "image/gif", "image/webp"} if content_type not in supported_types: logger.warning( - "Unsupported content type '%s' for %s; skipping image", - content_type, - image_url, + "Unsupported content type '%s' from %s; skipping image", content_type, host ) return None encoded = base64.b64encode(response.content).decode("ascii") logger.info( - "Fetched image %s (%s, %d bytes)", - image_url, - content_type, - len(response.content), + "Fetched image from %s (%s, %d bytes)", host, content_type, len(response.content) ) return create_image_block(base64=encoded, mime_type=content_type) except Exception: - logger.exception("Failed to fetch image from %s", image_url) + logger.exception("Failed to fetch image from %s", host) return None diff --git a/agent/utils/url_safety.py b/agent/utils/url_safety.py index 69330e7a..7eb2ecbf 100644 --- a/agent/utils/url_safety.py +++ b/agent/utils/url_safety.py @@ -9,7 +9,13 @@ from __future__ import annotations import ipaddress import socket -from urllib.parse import urlparse +from typing import Any +from urllib.parse import urljoin, urlparse, urlunparse + +import httpx + +DEFAULT_MAX_REDIRECTS = 5 +_REDIRECT_CODES = {301, 302, 303, 307, 308} def resolve_and_validate(url: str) -> tuple[bool, str, str | None, list | None]: @@ -61,3 +67,92 @@ def is_url_safe(url: str) -> tuple[bool, str]: """Check if a URL is safe to request (not targeting private/internal networks).""" is_safe, reason, _, _ = resolve_and_validate(url) return is_safe, reason + + +def pinned_url(url: str, ip: str) -> str: + """Rewrite ``url`` so the connection targets ``ip`` while keeping the path/query. + + The original hostname is preserved separately for the ``Host`` header and TLS + SNI/cert verification (via httpx's ``sni_hostname`` request extension). + """ + parsed = urlparse(url) + host_literal = f"[{ip}]" if ":" in ip else ip + netloc = f"{host_literal}:{parsed.port}" if parsed.port else host_literal + return urlunparse(parsed._replace(netloc=netloc)) + + +async def request_with_safe_redirects( + client: httpx.AsyncClient, + method: str, + url: str, + *, + max_redirects: int = DEFAULT_MAX_REDIRECTS, + strip_auth_on_redirect: bool = False, + **kwargs: Any, +) -> tuple[httpx.Response | None, tuple[str, str] | None]: + """Issue a request, validating every redirect target before following it. + + The hostname is resolved once per hop and the connection is pinned to the + validated IP, closing the DNS-rebinding race where a controlled resolver + returns a public IP at validation time and a private IP at connect time. + + Returns ``(response, None)`` on success, or ``(None, (blocked_url, reason))`` + when a hop fails validation or the redirect budget is exhausted. When + ``strip_auth_on_redirect`` is set, the caller's ``Authorization`` header is + dropped once the request leaves the original URL, so a bearer token can't be + replayed to a redirect target the caller never chose to authenticate to. + """ + current_method = method.upper() + current_url = url + request_kwargs = dict(kwargs) + # Pop caller headers/extensions ONCE so they're reused on every redirect hop + # (the per-hop Host + SNI are layered on top each time). Popping inside the + # loop dropped the caller's Authorization/Accept/etc. on the first redirect. + caller_headers = dict(request_kwargs.pop("headers", None) or {}) + caller_extensions = dict(request_kwargs.pop("extensions", None) or {}) + + for redirect_count in range(max_redirects + 1): + is_safe, reason, hostname, addr_infos = resolve_and_validate(current_url) + if not is_safe or hostname is None or addr_infos is None: + return None, (current_url, reason) + + pinned_ip = addr_infos[0][4][0] + parsed = urlparse(current_url) + headers = {**caller_headers, "Host": parsed.netloc} + extensions = {**caller_extensions, "sni_hostname": hostname} + + response = await client.request( + current_method, + pinned_url(current_url, pinned_ip), + follow_redirects=False, + headers=headers, + extensions=extensions, + **request_kwargs, + ) + + if response.status_code not in _REDIRECT_CODES: + return response, None + + location = response.headers.get("Location") + if not location: + return response, None + + if redirect_count == max_redirects: + return None, (current_url, "Too many redirects") + + current_url = urljoin(current_url, location) + + if strip_auth_on_redirect: + caller_headers = { + k: v for k, v in caller_headers.items() if k.lower() != "authorization" + } + + if response.status_code == 303 or ( + response.status_code in {301, 302} and current_method not in {"GET", "HEAD"} + ): + current_method = "GET" + request_kwargs.pop("data", None) + request_kwargs.pop("content", None) + request_kwargs.pop("json", None) + + return None, (current_url, "Too many redirects") diff --git a/agent/webhooks/linear.py b/agent/webhooks/linear.py index f897bb1c..104b1b51 100644 --- a/agent/webhooks/linear.py +++ b/agent/webhooks/linear.py @@ -5,6 +5,7 @@ object (``webapp.X``) so tests that monkeypatch them keep working. """ from typing import Any +from urllib.parse import urlparse import httpx from langchain_core.messages.content import create_text_block @@ -182,7 +183,7 @@ async def process_linear_issue( # noqa: PLR0912, PLR0915 resolved_model_id = fallback_model_id image_model_override = (fallback_model_id, fallback_effort) webapp.logger.info("Preparing %d image(s) for multimodal content", len(image_urls)) - webapp.logger.debug("Image URLs: %s", image_urls) + webapp.logger.debug("Image hosts: %s", [urlparse(u).hostname for u in image_urls]) async with httpx.AsyncClient(timeout=webapp.DEFAULT_HTTP_TIMEOUT) as client: for image_url in image_urls: diff --git a/tests/test_http_security.py b/tests/test_http_security.py index 45266413..9771c4c2 100644 --- a/tests/test_http_security.py +++ b/tests/test_http_security.py @@ -20,6 +20,7 @@ fetch_url_tool = sys.modules["agent.tools.fetch_url"] http_request_tool = sys.modules["agent.tools.http_request"] # SSRF resolution now lives in the shared validator; patch DNS there. url_safety = importlib.import_module("agent.utils.url_safety") +multimodal = importlib.import_module("agent.utils.multimodal") _NO_JSON = object() @@ -310,3 +311,67 @@ async def test_http_request_returns_timeout_result(monkeypatch) -> None: assert result["success"] is False assert result["status_code"] == 0 assert "timed out after 7 seconds" in result["content"] + + +# --- fetch_image_block SSRF (INJ-01 regression) ------------------------------ + + +def _image_dns(monkeypatch, ip_map: dict[str, str]) -> None: + monkeypatch.setattr( + url_safety.socket, + "getaddrinfo", + lambda host, port, *a, **k: [_addr_info(ip_map[host], port)], + ) + + +async def test_fetch_image_block_blocks_redirect_to_internal_host(monkeypatch) -> None: + """A public image URL that 302-redirects to the metadata endpoint must not be + followed: the redirect target is re-validated and the internal hop blocked.""" + _image_dns( + monkeypatch, + {"public.example": "93.184.216.34", "internal.evil": "169.254.169.254"}, + ) + + def responder(method, url, **kwargs): + return FakeResponse( + status_code=302, + url=url, + headers={"Location": "http://internal.evil/latest/meta-data/"}, + ) + + client = FakeAsyncClient(responder) + result = await multimodal.fetch_image_block("http://public.example/x.png", client) + + assert result is None + # Exactly one hop was issued (to the validated public host); the redirect to + # the internal host was blocked before any request left the process. + assert len(client.calls) == 1 + assert all("169.254.169.254" not in call["url"] for call in client.calls) + + +async def test_fetch_image_block_strips_auth_header_on_redirect(monkeypatch) -> None: + """The per-host Slack bearer token must not be replayed to a redirect target.""" + monkeypatch.setenv("SLACK_BOT_TOKEN", "xoxb-secret") + _image_dns( + monkeypatch, + {"files.slack.com": "93.184.216.34", "cdn.public": "93.184.216.35"}, + ) + + def responder(method, url, **kwargs): + if len(client.calls) == 1: + return FakeResponse( + status_code=302, url=url, headers={"Location": "https://cdn.public/x.png"} + ) + resp = FakeResponse(status_code=200, url=url, headers={"Content-Type": "image/png"}) + resp.content = b"\x89PNG\r\n" + return resp + + client = FakeAsyncClient(responder) + result = await multimodal.fetch_image_block("https://files.slack.com/x.png", client) + + assert result is not None + assert len(client.calls) == 2 + first_auth = client.calls[0]["headers"].get("Authorization") + second_auth = client.calls[1]["headers"].get("Authorization") + assert first_auth == "Bearer xoxb-secret" + assert second_auth is None