fix(web-search): resolve the SearXNG base URL host instead of prefix-matching it - #931
Conversation
…matching it is_local_base_url gates web_search.allow_remote_base_url: with the opt-in off, every household search query must stay on-device. It decided that by prefix-matching the URL text, which was wrong in both directions. Remote hosts passed as local, so queries left the device with the opt-in still off: http://localhost.attacker.example -> local http://localhost-evil.example -> local http://127.evil.example -> local https://127.0.0.1.nip.io -> local http://127.0.0.1@evil.com -> local (userinfo; real host evil.com) And genuine loopback URLs were rejected, so a legitimate on-device SearXNG behind credentials could not be used at all — the guard failed closed on the configuration it exists to permit: http://user@localhost:8888 -> remote http://user:pass@[::1]:8888 -> remote security::sandbox already parses hosts correctly for validate_inference_route, and its helper documents itself as matching "a literal loopback target (not a hostname that merely starts with a loopback-looking prefix)" — exactly the property missing here. Make extract_host/is_loopback_host pub(crate) and delegate, so this is one fewer independent copy of the decision rather than one more. That matters because the copies drift: genie-common::config hardened its own duplicate after this class regressed once already (GeniePod#327, the 127.0.0.0/8 range), and its test comment names the genie-core check as the drifted sibling. This was the remaining weak copy. Behavior for the configurations already covered by tests is unchanged: 127.0.0.1, localhost, and [::1] stay local, searx.example.com stays remote. Newly correct: the whole 127.0.0.0/8 range, userinfo handling, and mixed-case hosts. Closes GeniePod#929
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change makes sandbox URL helpers crate-visible and updates ChangesLoopback URL guard
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/genie-core/src/security/sandbox.rs`:
- Around line 229-236: The URL host extraction used by the egress guard must use
standards-compliant parsing instead of manual authority parsing. In
crates/genie-core/src/security/sandbox.rs:229-236, update extract_host to parse
with reqwest::Url, return host_str() in the existing normalized form, and fail
closed on parse errors; add regressions for query-, fragment-, and
backslash-delimited authority cases. In
crates/genie-core/src/tools/web_search.rs:424-426 and 744-779, preserve the
existing call sites and ensure their egress decisions use the corrected
extract_host behavior; no separate parsing or prefix matching should remain
there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e02a7e63-454f-48ee-8ebb-815b808a579e
📒 Files selected for processing (2)
crates/genie-core/src/security/sandbox.rscrates/genie-core/src/tools/web_search.rs
Follow-up to the review on GeniePod#931. extract_host split the authority on '/' only, so a query or fragment stayed attached and the userinfo strip below then read *into* it. The '@' inside the query/fragment made the trailing text look like the host: extract_host("http://evil.com#@127.0.0.1") -> "127.0.0.1" (loopback) extract_host("http://evil.com?@127.0.0.1") -> "127.0.0.1" (loopback) extract_host("http://evil.com\@127.0.0.1") -> "127.0.0.1" (loopback) That is fail-open, and it is not confined to the web-search guard this PR touches: extract_host also backs validate_inference_route, the SSRF check on LLM endpoints. Both accepted those URLs. End the authority at the first '/', '?', '#', or '\'. The backslash belongs in that set rather than being left to the userinfo strip: WHATWG treats '\' as a path separator for http(s), which is how reqwest's URL parser resolves it, so "http://127.0.0.1\@evil.com" really does connect to 127.0.0.1 — the old split reported "evil.com" and rejected a valid local endpoint. Kept the hand-rolled parse rather than switching to reqwest::Url as the review suggested. validate_inference_route is also fed scheme-less authorities, and Url::parse reads "127.0.0.1:8080" as scheme "127.0.0.1" — adopting it would need a synthesized scheme and a fail-closed error path, which is a larger behavioral change than this bug warrants. The delimiter set is what was actually wrong. Genuine userinfo is unaffected: "http://user:pass@[::1]:8080" and "http://user@localhost" stay local, "http://localhost:1@evil.com/v1" stays remote.
|
Good catch — the underlying finding is real, and it is worse than a hardening nit. Fixed in the latest commit.
That is fail-open, and it is not confined to the web-search guard this PR touches — The fix ends the authority at the first On the One related item I left alone to keep this PR focused: |
1 participant
Summary
Closes #929.
is_local_base_urlgatesweb_search.allow_remote_base_url: withthe opt-in off, every household search query must stay on-device. It decided
that by prefix-matching the URL text, which is wrong in both directions.
Remote hosts passed as local, so queries left the device with the opt-in
still off:
http://localhost.attacker.example/searchlocalhost.attacker.examplehttp://localhost-evil.examplelocalhost-evil.examplehttp://127.evil.example/127.evil.examplehttps://127.0.0.1.nip.io/127.0.0.1.nip.iohttp://127.0.0.1@evil.com/searchevil.comThe last row is the sharp one: the loopback text is userinfo, so the string
looks unambiguously local and the connection goes to
evil.com.Genuine loopback URLs were rejected, so a legitimate on-device SearXNG
behind credentials could not be used at all — the guard failed closed on the
configuration it exists to permit:
http://user@localhost:8888http://user:pass@[::1]:8888Changes
is_local_base_urlnow resolves the URL's host and tests it for loopback,instead of prefix-matching the string.
security::sandbox::extract_host/is_loopback_host, whichalready do this correctly for
validate_inference_route—extract_hoststrips userinfo and handles bracketed IPv6, and
is_loopback_hostdocumentsitself as matching "a literal loopback target (not a hostname that merely
starts with a loopback-looking prefix)". Those two are now
pub(crate).genie-common::config::is_remote_urlhardened its own duplicate after thisexact class regressed once already ([bug] llm/provider: remote_url() blocks 127.0.0.0/8 except 127.0.0.1 #327, the
127.0.0.0/8range), and itstest comment names the genie-core check as the drifted sibling. This was the
remaining weak copy, so the fix removes it rather than re-fixing it in place.
Behavior for the configurations already covered by tests is unchanged:
127.0.0.1,localhostand[::1]stay local;searx.example.comstaysremote. Newly correct: the whole
127.0.0.0/8range, userinfo handling, andmixed-case hosts.
Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
x86_64 Linux laptop (Ubuntu 22.04 LTS, kernel 6.8.0-136-generic,
rustc 1.96.0 / cargo 1.96.0). No Jetson and no SearXNG instance available to
me — see the validation gap below.
Plus a temporary probe calling
is_local_base_urldirectly onmainover thefull corpus, so the classification was read off the function rather than
reasoned about. The probe was removed before committing; only the permanent
tests remain.
What I observed
Probe output on
main— five remote hosts accepted as local, two real loopbackhosts rejected:
Every one of those is inverted correctly on this branch, and the two controls
(
127.0.0.1:8888local,searx.example.comremote) are unchanged. Thepre-existing
local_base_url_detection_allows_loopbacktest still passesunmodified.
Full suite: 1005 lib tests + all integration suites green on default
features and on
--no-default-features; clippy clean on both;cargo fmt --checkclean.Validation gap
I could not verify on Jetson hardware or against a live SearXNG. The equivalent
verification path: the change is confined to a pure
&str -> boolpredicate andthe visibility of two existing helpers. No I/O, no async, no hardware
dependency, no model involvement. The predicate's entire contract is the
classification it returns for a base URL, which the tests assert directly, and
the call site it feeds (
search_searxng'sanyhow::bail!) is untouched — a URLthat classifies remote is refused exactly as it is today.
I did not attempt to resolve any of the adversarial hostnames; they are inputs
to a string predicate here, and no network call is made in any test.
Test plan
mainalone — both fail.web_search.provider = "searxng"andallow_remote_base_url = false,set
base_url = "http://localhost.attacker.example"and confirm the searchis now refused with "base URL must be local", then set
base_url = "http://user@localhost:8888"and confirm it is accepted.Notes for reviewers
local_base_url_accepts_loopback_behind_userinfo_and_the_whole_127_rangeisworth a look alongside the rejection test: tightening a guard is only safe if
it does not also start blocking valid on-device configs, and the prefix
version was blocking two of them.
is untouched.
Summary by CodeRabbit