From bdbd691c24b7e6a12c4aaab1de4bebc634d8866d Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 31 Aug 2026 16:33:24 +0700 Subject: [PATCH] fix(crawler): refuse crawl targets that are not publicly routable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `crawl_url` validated its input with `validators.url` alone, which answers a syntactic question and says nothing about where the URL points. Both `http://127.0.0.1:8000/` and `http://169.254.169.254/latest/meta-data/` — the cloud metadata endpoint — pass it and are then fetched from inside the backend's own network namespace, by all three tiers in turn. Resolve the target host before any tier runs and refuse it unless every address it answers with is publicly routable. `crawl_url` is the single choke point: the spider reaches it through `_ConnectorSession.fetch`, so the same gate covers link-following, where a public page linking to an internal address would otherwise be followed. Two details the obvious implementation gets wrong: - `::ffff:127.0.0.1` reports `is_loopback == False`. The flags only read true on the mapped IPv4 form, so the mapping is unwrapped before the address is judged; reading them off the IPv6 object admits loopback under its v6 spelling. - A union of `is_private`/`is_loopback`/`is_link_local`/`is_reserved`/ `is_multicast` admits carrier-grade NAT (100.64.0.0/10, RFC 6598), which is not publicly routable. `is_global` covers that, but reports True for multicast, so multicast is excluded separately. Resolution failures refuse, so an unreachable resolver is not a way past the guard, and a host answering with both a public and a private address is refused because the fetcher resolves independently and may pick either. This narrows the reachable surface; it is not a defence against DNS rebinding, which a resolve-then-fetch guard cannot address. The malformed-URL and restricted-URL refusals carry distinct messages so the two diagnoses stay apart. DNS blocks, so it is offloaded with `asyncio.to_thread`, as the browser tiers already are. Closes #1709 --- .../app/proprietary/web_crawler/connector.py | 19 ++- surfsense_backend/app/utils/crawl/__init__.py | 2 + .../app/utils/crawl/net_guard.py | 94 +++++++++++++ .../proprietary/web_crawler/test_connector.py | 58 +++++++- .../tests/unit/utils/crawl/test_net_guard.py | 125 ++++++++++++++++++ 5 files changed, 296 insertions(+), 2 deletions(-) create mode 100644 surfsense_backend/app/utils/crawl/net_guard.py create mode 100644 surfsense_backend/tests/unit/utils/crawl/test_net_guard.py diff --git a/surfsense_backend/app/proprietary/web_crawler/connector.py b/surfsense_backend/app/proprietary/web_crawler/connector.py index 76118b1181..acae052a3d 100644 --- a/surfsense_backend/app/proprietary/web_crawler/connector.py +++ b/surfsense_backend/app/proprietary/web_crawler/connector.py @@ -43,7 +43,12 @@ ) from app.proprietary.web_crawler.url_policy import extract_link_records from app.utils.captcha import captcha_enabled, get_captcha_config -from app.utils.crawl import BlockType, classify_block, extract_contacts +from app.utils.crawl import ( + BlockType, + classify_block, + extract_contacts, + is_publicly_routable, +) from app.utils.proxy import get_proxy_url, is_pool_backed logger = logging.getLogger(__name__) @@ -240,6 +245,18 @@ async def crawl_url(self, url: str) -> CrawlOutcome: block_type=block_state["block_type"], ) + # ``validators.url`` only judges the spelling; it passes + # ``http://127.0.0.1:8000/`` and the cloud metadata endpoint alike. + # Resolving before any tier runs is what keeps those from being + # fetched from inside the backend's own network. DNS blocks, so it + # is offloaded the same way the browser tiers below are. + if not await asyncio.to_thread(is_publicly_routable, url): + return CrawlOutcome( + status=CrawlOutcomeStatus.FAILED, + error=f"Restricted URL (host is not publicly routable): {url}", + block_type=block_state["block_type"], + ) + errors: list[str] = [] # True once any tier fetched the page but extraction yielded nothing # (distinguishes EMPTY from FAILED, where every tier raised/was diff --git a/surfsense_backend/app/utils/crawl/__init__.py b/surfsense_backend/app/utils/crawl/__init__.py index 3f4762414a..b64d0a3a2d 100644 --- a/surfsense_backend/app/utils/crawl/__init__.py +++ b/surfsense_backend/app/utils/crawl/__init__.py @@ -12,11 +12,13 @@ from app.utils.crawl.classifier import BlockType, classify_block from app.utils.crawl.contacts import Contacts, extract_contacts, is_social_host +from app.utils.crawl.net_guard import is_publicly_routable __all__ = [ "BlockType", "Contacts", "classify_block", "extract_contacts", + "is_publicly_routable", "is_social_host", ] diff --git a/surfsense_backend/app/utils/crawl/net_guard.py b/surfsense_backend/app/utils/crawl/net_guard.py new file mode 100644 index 0000000000..a599bd70af --- /dev/null +++ b/surfsense_backend/app/utils/crawl/net_guard.py @@ -0,0 +1,94 @@ +"""Destination guard — refuses crawl targets that are not publicly routable. + +``validators.url`` answers a syntactic question: is this a well-formed URL. It +says nothing about *where* the URL points, so ``http://127.0.0.1:8000/`` and +``http://169.254.169.254/latest/meta-data/`` (the cloud metadata endpoint) both +pass it and are then fetched from inside the backend's network namespace. This +module answers the other question — does the host resolve only to addresses the +public internet can reach — and is the gate the crawler consults before any +tier runs. + +Two details decide correctness here: + +* ``::ffff:127.0.0.1`` reports ``is_loopback == False``. The address flags only + read true on the mapped IPv4 form, so the mapping is unwrapped before the + address is judged; reading the flags off the IPv6 object admits loopback + under its v6 spelling. +* ``is_global`` is the right primitive rather than a union of + ``is_private``/``is_loopback``/``is_link_local``/``is_reserved``: that union + admits carrier-grade NAT (``100.64.0.0/10``, RFC 6598), which is not + publicly routable. ``is_global`` does report ``True`` for multicast, which is + therefore excluded separately. + +Resolution failures refuse. A name that cannot be resolved is not a name that +can be cleared, and failing open here would make an unreachable DNS server a +way past the guard. + +This narrows the reachable surface; it is not a defence against DNS rebinding. +The fetcher resolves the host again on its own, so a name that answers +differently between this check and that fetch is out of scope for a +resolve-then-fetch guard. +""" + +from __future__ import annotations + +import ipaddress +import socket +from urllib.parse import urlsplit + +_ALLOWED_SCHEMES = frozenset({"http", "https"}) + + +def _resolve_host(host: str) -> list[str]: + """Every address ``host`` resolves to, across both families.""" + return sorted( + { + info[4][0] + for info in socket.getaddrinfo(host, None, proto=socket.IPPROTO_TCP) + } + ) + + +def _is_public_address(raw: str) -> bool: + """True when ``raw`` parses as an address the public internet can route to.""" + try: + address = ipaddress.ip_address(raw) + except ValueError: + # Includes scoped forms such as ``fe80::1%eth0``, which are link-local + # anyway; anything unparseable is refused rather than guessed at. + return False + mapped = getattr(address, "ipv4_mapped", None) + if mapped is not None: + address = mapped + return address.is_global and not address.is_multicast + + +def is_publicly_routable(url: str) -> bool: + """True only when every address ``url``'s host resolves to is public. + + A host with both a public and a private answer is refused: the fetcher + resolves independently and may pick either one. + """ + parts = urlsplit(url) + if parts.scheme not in _ALLOWED_SCHEMES: + return False + host = parts.hostname + if not host: + return False + + # An address literal needs no resolver, and asking one would let a + # nameserver answer for a host the URL already pinned. + try: + ipaddress.ip_address(host) + except ValueError: + pass + else: + return _is_public_address(host) + + try: + addresses = _resolve_host(host) + except (OSError, UnicodeError): + return False + if not addresses: + return False + return all(_is_public_address(address) for address in addresses) diff --git a/surfsense_backend/tests/unit/proprietary/web_crawler/test_connector.py b/surfsense_backend/tests/unit/proprietary/web_crawler/test_connector.py index 8db7c1ed1b..760ef71526 100644 --- a/surfsense_backend/tests/unit/proprietary/web_crawler/test_connector.py +++ b/surfsense_backend/tests/unit/proprietary/web_crawler/test_connector.py @@ -12,11 +12,22 @@ WebCrawlerConnector, connector as connector_module, ) -from app.utils.crawl import BlockType +from app.utils.crawl import BlockType, net_guard pytestmark = pytest.mark.unit +@pytest.fixture(autouse=True) +def _stub_destination_resolver(monkeypatch: pytest.MonkeyPatch) -> None: + """Keep the destination guard hermetic. + + ``crawl_url`` now resolves the target host before running a tier. These + are unit tests, so the resolver answers with a fixed public address + instead of whatever DNS says about ``example.com`` today. + """ + monkeypatch.setattr(net_guard, "_resolve_host", lambda _host: ["93.184.216.34"]) + + def _result(tier: str) -> dict: return { "content": "hello world", @@ -497,3 +508,48 @@ def test_build_result_ok_on_real_content() -> None: ) assert block_state["block_type"] is BlockType.OK + + +async def test_restricted_url_is_failed_before_any_tier( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A host that is not publicly routable never reaches a fetch tier. + + ``validators.url`` passes the cloud metadata endpoint, so without the + destination guard this URL is fetched from inside the backend's network. + """ + crawler = WebCrawlerConnector() + tiers: list[str] = [] + + async def _record_static(_url: str, *_args) -> None: + tiers.append("static") + return None + + async def _record_dynamic(_url: str, *_args) -> None: + tiers.append("dynamic") + return None + + async def _record_stealthy(_url: str, *_args) -> None: + tiers.append("stealthy") + return None + + monkeypatch.setattr(crawler, "_crawl_with_async_fetcher", _record_static) + monkeypatch.setattr(crawler, "_crawl_with_dynamic", _record_dynamic) + monkeypatch.setattr(crawler, "_crawl_with_stealthy", _record_stealthy) + + outcome = await crawler.crawl_url("http://169.254.169.254/latest/meta-data/") + + assert outcome.status is CrawlOutcomeStatus.FAILED + assert outcome.result is None + assert "Restricted URL" in (outcome.error or "") + assert tiers == [] + + +async def test_restricted_url_is_reported_apart_from_a_malformed_one() -> None: + """The two refusals are different diagnoses and must not share a message.""" + restricted = await WebCrawlerConnector().crawl_url("http://127.0.0.1:8000/health") + malformed = await WebCrawlerConnector().crawl_url("not a url") + + assert "Restricted URL" in (restricted.error or "") + assert "Invalid URL" in (malformed.error or "") + assert (malformed.error or "") != (restricted.error or "") diff --git a/surfsense_backend/tests/unit/utils/crawl/test_net_guard.py b/surfsense_backend/tests/unit/utils/crawl/test_net_guard.py new file mode 100644 index 0000000000..4d7ac4688c --- /dev/null +++ b/surfsense_backend/tests/unit/utils/crawl/test_net_guard.py @@ -0,0 +1,125 @@ +"""``is_publicly_routable`` behavior: refuse URLs whose host is not public. + +The literal-address cases need no resolver at all, so they exercise the real +code end to end; only the hostname cases stub ``_resolve_host``, because a unit +test cannot depend on what DNS answers. +""" + +from __future__ import annotations + +import socket + +import pytest + +from app.utils.crawl import is_publicly_routable, net_guard + +pytestmark = pytest.mark.unit + + +def test_public_literal_is_allowed() -> None: + assert is_publicly_routable("https://8.8.8.8/") is True + + +def test_loopback_literal_is_refused() -> None: + assert is_publicly_routable("http://127.0.0.1:8000/health") is False + + +def test_cloud_metadata_literal_is_refused() -> None: + """169.254.169.254 is the AWS/GCP/Azure credential endpoint.""" + assert is_publicly_routable("http://169.254.169.254/latest/meta-data/") is False + + +@pytest.mark.parametrize( + "url", + [ + "http://10.0.0.1/", + "http://172.16.0.1/", + "http://192.168.1.1/", + "http://0.0.0.0/", + ], +) +def test_private_and_unspecified_literals_are_refused(url: str) -> None: + assert is_publicly_routable(url) is False + + +def test_ipv6_loopback_literal_is_refused() -> None: + assert is_publicly_routable("http://[::1]:8000/") is False + + +def test_ipv4_mapped_ipv6_literal_is_refused() -> None: + """``::ffff:127.0.0.1`` reports ``is_loopback == False``; only the mapped + IPv4 form does, so a guard that reads the flags off the IPv6 object alone + lets loopback through under its v6 spelling.""" + assert is_publicly_routable("http://[::ffff:127.0.0.1]/") is False + + +def test_carrier_grade_nat_literal_is_refused() -> None: + """100.64.0.0/10 (RFC 6598) is none of private/loopback/link-local/reserved + /multicast, so a guard built from those five flags admits it — but it is + carrier-internal and not publicly routable.""" + assert is_publicly_routable("http://100.64.0.1/") is False + + +def test_multicast_literal_is_refused() -> None: + """224.0.0.1 reports ``is_global == True``, so the routability check alone + is not enough to exclude it.""" + assert is_publicly_routable("http://224.0.0.1/") is False + + +@pytest.mark.parametrize( + "url", + ["file:///etc/passwd", "gopher://127.0.0.1/", "ftp://example.com/", "not a url"], +) +def test_non_http_scheme_is_refused(url: str) -> None: + assert is_publicly_routable(url) is False + + +def test_hostname_resolving_to_a_private_address_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(net_guard, "_resolve_host", lambda _host: ["10.1.2.3"]) + + assert is_publicly_routable("https://internal.example.com/") is False + + +def test_hostname_is_refused_when_any_answer_is_private( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A host with both a public and a private answer must not be crawled: the + fetcher resolves independently and may pick either.""" + monkeypatch.setattr( + net_guard, "_resolve_host", lambda _host: ["93.184.216.34", "192.168.0.7"] + ) + + assert is_publicly_routable("https://split-horizon.example.com/") is False + + +def test_hostname_resolving_only_to_public_addresses_is_allowed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr( + net_guard, + "_resolve_host", + lambda _host: ["93.184.216.34", "2606:2800:220:1:248:1893:25c8:1946"], + ) + + assert is_publicly_routable("https://example.com/page") is True + + +def test_unresolvable_hostname_is_refused(monkeypatch: pytest.MonkeyPatch) -> None: + """Fail closed: a name we cannot resolve is not a name we can clear.""" + + def _boom(_host: str) -> list[str]: + raise socket.gaierror("Name or service not known") + + monkeypatch.setattr(net_guard, "_resolve_host", _boom) + + assert is_publicly_routable("https://does-not-exist.example/") is False + + +def test_hostname_resolving_to_nothing_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(net_guard, "_resolve_host", lambda _host: []) + + assert is_publicly_routable("https://empty.example/") is False