fix(crawler): refuse crawl targets that are not publicly routable - #1728
fix(crawler): refuse crawl targets that are not publicly routable#1728ntdat812 wants to merge 1 commit into
Conversation
|
@ntdat812 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`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 MODSetter#1709
1 participant
WebCrawlerConnector.crawl_urlvalidated its input withvalidators.urlalone. That answers a syntactic question and says nothing about where the URL points, so loopback, private, link-local and cloud-metadata targets passed and were fetched from inside the backend's own network namespace.Description
Resolve the target host before any tier runs, and refuse it unless every address it answers with is publicly routable.
The guard is a new
app/utils/crawl/net_guard.py(is_publicly_routable), placed beside the existing generic crawler helpers rather than inurl_policy.py, whose docstring states it holds pure functions with no I/O.crawl_urlis the single choke point, so one gate covers both entry paths: the spider reaches it through_ConnectorSession.fetch, which matters because a public page linking to an internal address would otherwise be followed.Two details the obvious implementation gets wrong, both covered by tests:
::ffff:127.0.0.1reportsis_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_private/is_loopback/is_link_local/is_reserved/is_multicastadmits carrier-grade NAT (100.64.0.0/10, RFC 6598), which is not publicly routable.is_globalcovers that case, but reportsTruefor multicast, so multicast is excluded separately.Resolution failures refuse, so an unreachable resolver is not a way past the guard. A host answering with both a public and a private address is refused, because the fetcher resolves independently and may pick either.
The malformed-URL and restricted-URL refusals carry distinct messages so the two diagnoses stay apart, which also keeps the existing
test_invalid_url_is_failedassertion meaningful.DNS blocks, so it is offloaded with
asyncio.to_thread, the same way the browser tiers already are.Scope
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 any resolve-then-fetch guard. Worth stating plainly rather than implying the hole is closed.
Motivation and Context
FIX #1709
Reproduced on
devbefore the change. With the tier stubs removed, all three tiers actually attempt the connection:and
http://169.254.169.254/latest/meta-data/reached the tiers and returnedEMPTY— it was fetched, not refused:Both new connector tests fail on the parent commit and pass with the change.
API Changes
CrawlOutcomefor a restricted target isFAILEDwith a newRestricted URL (host is not publicly routable): <url>message. No signature changes.Change Type
Testing Performed
New coverage.
tests/unit/utils/crawl/test_net_guard.py— 20 cases. The literal-address cases need no resolver, so they exercise the real code end to end; only the hostname cases stub_resolve_host, since a unit test cannot depend on what DNS answers. Includes the IPv4-mapped-IPv6, CGNAT and multicast cases named above, plus fail-closed on resolution error, empty answer, and mixed public/private answers.tests/unit/proprietary/web_crawler/test_connector.py— two cases: the restricted target isFAILEDand reaches no tier, and the two refusal messages stay distinct.Test hygiene.
crawl_urlnow resolves, so the 13 existing tests calling it withhttps://example.comwould have started doing live DNS. An autouse fixture in that module stubs the resolver, keeping them hermetic.Results. Crawler + capability suites:
110 passed. Full unit suite:2757 passed, 10 failed. Those 10 are pre-existing on a cleandevcheckout — same 10, inknowledge_store/engines/test_git.py,knowledge_store/test_import_boundary.py,middleware/test_git_tree_backend.pyandtest_pat_fail_closed_static.py— and are unrelated to this change (verified by running them onupstream/dev).Verified against the real resolver as a sanity check:
example.com,github.comandwww.google.comare allowed;127.0.0.1,localhost,169.254.169.254,[::1],[::ffff:127.0.0.1],100.64.0.1and224.0.0.1are refused.ruff checkandruff format --checkare clean on every touched file.Checklist
High-level PR Summary
This PR fixes a security vulnerability where the web crawler could be tricked into accessing internal network resources like loopback addresses (
127.0.0.1), private networks, link-local addresses, and cloud metadata endpoints (169.254.169.254). The fix adds a DNS resolution check that validates every IP address a target URL resolves to is publicly routable before any crawling tier executes. The implementation handles edge cases like IPv4-mapped IPv6 addresses and carrier-grade NAT, and fails closed when DNS resolution fails to prevent bypassing the guard.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
surfsense_backend/app/utils/crawl/net_guard.pysurfsense_backend/tests/unit/utils/crawl/test_net_guard.pysurfsense_backend/app/utils/crawl/__init__.pysurfsense_backend/app/proprietary/web_crawler/connector.pysurfsense_backend/tests/unit/proprietary/web_crawler/test_connector.py