fix(websearch): harden SSRF guard in ContentFetcher (#383)

openrag/components/websearch/content_fetcher.py

@@ -1,6 +1,6 @@
 import asyncio
 import ipaddress
-from urllib.parse import urlparse
+from urllib.parse import urljoin, urlparse
 
 import httpx
 import lxml.html
@@ -18,6 +18,72 @@
 
 _USER_AGENT = "Mozilla/5.0 (compatible; OpenRAG/1.0; +https://github.com/linagora/openrag)"
 
+_MAX_REDIRECTS = 10
+
+
+def _is_blocked_address(addr: ipaddress.IPv4Address | ipaddress.IPv6Address) -> bool:
+    """True for any IP that a server-side fetcher must not contact.
+
+    Checks all private/reserved/non-global flags explicitly so the guard is
+    correct across Python minor releases (``is_global`` semantics changed
+    between 3.10 and 3.11 for CGNAT and some multicast ranges).
+    """
+    return (
+        addr.is_loopback
+        or addr.is_private
+        or addr.is_link_local
+        or addr.is_reserved
+        or addr.is_unspecified
+        or addr.is_multicast
+        or not addr.is_global
+    )
+
+
+def _is_safe_url(url: str) -> bool:
+    """Return True only if *url* is safe for a server-side fetch.
+
+    Blocks:
+    - Non-HTTP(S) schemes
+    - ``localhost`` hostname
+    - IPv4/IPv6 literals in private, loopback, link-local, or reserved ranges
+    - Decimal-integer-encoded IPv4 addresses (e.g. ``2130706433`` == ``127.0.0.1``)
+
+    Regular hostnames pass through; per-hop redirect validation (in
+    :meth:`ContentFetcher._fetch_single`) re-checks every redirect target,
+    covering the case where a public hostname redirects to a private address.
+    """
+    try:
+        parsed = urlparse(url)
+    except Exception:
+        return False
+
+    if parsed.scheme not in ("http", "https"):
+        return False
+
+    host = parsed.hostname
+    if not host:
+        return False
+
+    if host.lower() == "localhost":
+        return False
+
+    # Dotted-decimal or IPv6 literal (e.g. "127.0.0.1", "::1", "10.0.0.1")
+    try:
+        return not _is_blocked_address(ipaddress.ip_address(host))
+    except ValueError:
+        pass
+
+    # Decimal-integer form (e.g. 2130706433 → 127.0.0.1).
+    # ipaddress.ip_address(int) interprets the value as a packed IPv4 address,
+    # matching how glibc's resolver (and therefore httpx) handles such hostnames.
+    try:
+        return not _is_blocked_address(ipaddress.ip_address(int(host)))
+    except (ValueError, TypeError):
+        pass
+
+    # Regular hostname — passes initial check; every redirect hop is re-validated.
+    return True
+
 
 class ContentFetcher:
     """Fetch and extract text content from web search result URLs."""
@@ -46,29 +112,44 @@ def _truncate(self, text: str) -> str:
             truncated = truncated[:last_space]
         return truncated.rstrip() + " [...]"
 
-    @staticmethod
-    def _is_loopback_url(url: str) -> bool:
-        host = urlparse(url).hostname or ""
-        if host == "localhost":
-            return True
-        try:
-            return not ipaddress.ip_address(host).is_global
-        except ValueError:
-            return False  # Regular hostname, let it through
-
     async def _fetch_single(self, client: httpx.AsyncClient, url: str) -> str | None:
-        """Fetch a single URL and extract text. Returns None on any failure."""
-        # Guard against SSRF: URLs come from the search provider, but a compromised
-        # or misbehaving provider could return loopback addresses targeting internal services.
-        if self._is_loopback_url(url):
-            logger.warning("Blocked loopback URL in web search results", url=url)
-            return None
-        try:
-            response = await asyncio.wait_for(
-                client.get(url, follow_redirects=True),
-                timeout=self.timeout,
-            )
-            response.raise_for_status()
+        """Fetch a single URL and extract text. Returns None on any failure.
+
+        Redirects are followed manually (``follow_redirects=False``) so every
+        hop is validated by :func:`_is_safe_url` before the request is sent.
+        This prevents a legitimate initial URL from redirecting to a private
+        address after the initial check passes.
+        """
+        current_url = url
+        for _ in range(_MAX_REDIRECTS + 1):
+            if not _is_safe_url(current_url):
+                logger.warning("Blocked unsafe URL in web search results", url=current_url)
+                return None
+
+            try:
+                response = await asyncio.wait_for(
+                    client.get(current_url, follow_redirects=False),
+                    timeout=self.timeout,
+                )
+            except TimeoutError:
+                logger.debug("Content fetch timed out", url=current_url)
+                return None
+            except Exception as e:
+                logger.debug("Content fetch failed", url=current_url, error=str(e))
+                return None
+
+            if response.is_redirect:
+                location = response.headers.get("location", "")
+                if not location:
+                    return None
+                current_url = urljoin(current_url, location)
+                continue
+
+            try:
+                response.raise_for_status()
+            except Exception as e:
+                logger.debug("Content fetch failed", url=current_url, error=str(e))
+                return None
 
             content_type = response.headers.get("content-type", "")
             if "text/html" not in content_type and "text/plain" not in content_type:
@@ -78,7 +159,6 @@ async def _fetch_single(self, client: httpx.AsyncClient, url: str) -> str | None
             if not html.strip():
                 return None
 
-            # Strip boilerplate elements (nav, footer, etc.) before conversion
             try:
                 tree = lxml.html.fromstring(html)
                 for tag in _BOILERPLATE_TAGS:
@@ -95,12 +175,8 @@ async def _fetch_single(self, client: httpx.AsyncClient, url: str) -> str | None
 
             return self._truncate(text)
 
-        except TimeoutError:
-            logger.debug("Content fetch timed out", url=url)
-            return None
-        except Exception as e:
-            logger.debug("Content fetch failed", url=url, error=str(e))
-            return None
+        logger.debug("Too many redirects, giving up", url=url)
+        return None
 
     async def enrich(self, results: list[WebResult]) -> list[WebResult]:
         """Fetch content for the top N results in parallel. Mutates results in place."""

openrag/components/websearch/test_content_fetcher.py

@@ -3,7 +3,7 @@
 import httpx
 import pytest
 from components.websearch.base import WebResult
-from components.websearch.content_fetcher import ContentFetcher
+from components.websearch.content_fetcher import ContentFetcher, _is_safe_url
 
 
 @pytest.fixture
@@ -15,6 +15,63 @@ def _make_result(url="https://example.com", snippet="short snippet"):
     return WebResult(title="Test", url=url, snippet=snippet)
 
 
+# ---------------------------------------------------------------------------
+# _is_safe_url — unit tests (no network, no client)
+# ---------------------------------------------------------------------------
+
+
+class TestIsSafeUrl:
+    @pytest.mark.parametrize(
+        "url",
+        [
+            "http://localhost/secret",
+            "http://127.0.0.1/admin",
+            "http://127.0.0.42/x",
+            "http://[::1]/admin",
+            "http://10.0.0.1/internal",
+            "http://192.168.1.1/router",
+            "http://169.254.169.254/metadata",  # AWS/cloud metadata service
+            "http://0.0.0.0/x",
+            "http://100.64.0.1/cgnat",  # RFC 6598 shared address space
+            "http://198.18.0.1/bench",  # Benchmarking range
+        ],
+    )
+    def test_blocks_private_and_reserved_addresses(self, url):
+        assert _is_safe_url(url) is False
+
+    def test_blocks_decimal_encoded_loopback(self):
+        """2130706433 is 127.0.0.1 in decimal integer form."""
+        assert _is_safe_url("http://2130706433/secret") is False
+
+    def test_blocks_decimal_encoded_private(self):
+        """167772161 is 10.0.0.1 in decimal integer form."""
+        assert _is_safe_url("http://167772161/secret") is False
+
+    @pytest.mark.parametrize(
+        "url",
+        [
+            "file:///etc/passwd",
+            "ftp://example.com/x",
+            "data:text/html,<h1>hi</h1>",
+            "javascript:alert(1)",
+        ],
+    )
+    def test_blocks_non_http_schemes(self, url):
+        assert _is_safe_url(url) is False
+
+    def test_allows_public_ip(self):
+        # 93.184.216.34 is example.com — globally routable
+        assert _is_safe_url("http://93.184.216.34/page") is True
+
+    def test_allows_regular_hostname(self):
+        assert _is_safe_url("https://example.com/page") is True
+
+
+# ---------------------------------------------------------------------------
+# _fetch_single — integration tests with mock transport
+# ---------------------------------------------------------------------------
+
+
 class TestFetchSingleURL:
     @pytest.mark.asyncio
     async def test_extracts_text_from_html(self, fetcher):
@@ -78,6 +135,73 @@ async def mock_handler(request):
 
         assert text is None
 
+    @pytest.mark.asyncio
+    async def test_blocks_redirect_to_private_ip(self, fetcher):
+        """A response that redirects to a private IP must be blocked after the initial fetch."""
+        calls: list[str] = []
+
+        async def redirecting_handler(request):
+            calls.append(str(request.url))
+            if len(calls) == 1:
+                return httpx.Response(
+                    302,
+                    headers={"location": "http://192.168.1.1/secret"},
+                )
+            return httpx.Response(200, text="<html><body>SSRF data</body></html>")
+
+        transport = httpx.MockTransport(redirecting_handler)
+        async with httpx.AsyncClient(transport=transport) as client:
+            # 93.184.216.34 is example.com — passes the initial check
+            text = await fetcher._fetch_single(client, "http://93.184.216.34/page")
+
+        assert text is None
+        # The redirect target (192.168.1.1) must never have been contacted
+        assert len(calls) == 1
+
+    @pytest.mark.asyncio
+    async def test_follows_safe_redirect(self, fetcher):
+        """A redirect to another public URL must be followed and its content returned."""
+        calls: list[str] = []
+
+        async def handler(request):
+            calls.append(str(request.url))
+            if len(calls) == 1:
+                return httpx.Response(
+                    301,
+                    headers={"location": "http://93.184.216.34/final"},
+                )
+            return httpx.Response(
+                200,
+                text="<html><body><p>Final page content.</p></body></html>",
+            )
+
+        transport = httpx.MockTransport(handler)
+        async with httpx.AsyncClient(transport=transport) as client:
+            text = await fetcher._fetch_single(client, "http://93.184.216.34/start")
+
+        assert text is not None
+        assert "Final page content" in text
+        assert len(calls) == 2
+
+    @pytest.mark.asyncio
+    async def test_stops_after_max_redirects(self, fetcher):
+        """Redirect chains that exceed _MAX_REDIRECTS are aborted."""
+        hop = 0
+
+        async def infinite_redirect(request):
+            nonlocal hop
+            hop += 1
+            return httpx.Response(
+                302,
+                headers={"location": f"http://93.184.216.34/hop{hop}"},
+            )
+
+        transport = httpx.MockTransport(infinite_redirect)
+        async with httpx.AsyncClient(transport=transport) as client:
+            text = await fetcher._fetch_single(client, "http://93.184.216.34/start")
+
+        assert text is None
+
     @pytest.mark.asyncio
     async def test_strips_boilerplate_html(self, fetcher):
         html = """<html><body>