From 4fd9151e9e122ef9466410dbb607ce23b52455eb Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Fri, 31 Jul 2026 00:40:36 -0400 Subject: [PATCH 1/2] fix(web-search): resolve the SearXNG base URL host instead of prefix-matching it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#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 #929 --- crates/genie-core/src/security/sandbox.rs | 14 ++++- crates/genie-core/src/tools/web_search.rs | 68 ++++++++++++++++++++--- 2 files changed, 73 insertions(+), 9 deletions(-) diff --git a/crates/genie-core/src/security/sandbox.rs b/crates/genie-core/src/security/sandbox.rs index aa1455de..270ad0a7 100644 --- a/crates/genie-core/src/security/sandbox.rs +++ b/crates/genie-core/src/security/sandbox.rs @@ -226,7 +226,14 @@ fn find_secret_matches(text: &str, pattern: &SecretPattern) -> Vec { matches } -fn extract_host(url: &str) -> String { +/// Extract the lowercased host from a URL: strip the scheme, take the +/// authority, drop any `user[:pass]@` userinfo, and keep IPv6 literals bracketed. +/// +/// `pub(crate)` so every on-device-only guard in this crate classifies a URL the +/// same way. `tools::web_search` used to prefix-match its base URL instead, which +/// let `http://localhost.attacker.example` and `http://127.0.0.1@evil.com` pass +/// as loopback. +pub(crate) fn extract_host(url: &str) -> String { let url = url.trim(); let stripped = url .strip_prefix("http://") @@ -247,7 +254,10 @@ fn extract_host(url: &str) -> String { /// True when `host` is a literal loopback target (not a hostname that merely /// starts with a loopback-looking prefix). -fn is_loopback_host(host: &str) -> bool { +/// +/// `pub(crate)` alongside [`extract_host`] so sibling guards reuse this parse +/// rather than growing another prefix-matching copy of it. +pub(crate) fn is_loopback_host(host: &str) -> bool { let host = host.trim(); if host.is_empty() { return false; diff --git a/crates/genie-core/src/tools/web_search.rs b/crates/genie-core/src/tools/web_search.rs index 4e7069aa..23f11909 100644 --- a/crates/genie-core/src/tools/web_search.rs +++ b/crates/genie-core/src/tools/web_search.rs @@ -399,14 +399,31 @@ fn searxng_search_url(base_url: &str) -> String { } } +/// True when the configured SearXNG base URL points at this device. +/// +/// This gates `allow_remote_base_url`: when the user has not opted in, every +/// household search query must stay on-device. The check therefore has to +/// resolve the URL's *host*, not the shape of its first few characters. +/// +/// It used to prefix-match (`starts_with("http://localhost")`, `"http://127."`), +/// 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`, `http://127.evil.example`, +/// `https://127.0.0.1.nip.io`, and `http://127.0.0.1@evil.com`, where the +/// loopback text is userinfo and the real host is `evil.com`. +/// - Genuine loopback URLs were rejected, so a legitimate local SearXNG behind +/// credentials (`http://user@localhost:8888`, `http://user:pass@[::1]:8888`) +/// could not be used at all. +/// +/// `security::sandbox` already parses hosts correctly for `validate_inference_route` +/// and documents itself as matching "a literal loopback target (not a hostname +/// that merely starts with a loopback-looking prefix)". Delegate to it instead of +/// keeping a third, weaker copy of the same decision — `genie-common::config` +/// hardened its own copy after this exact class regressed once already (#327). fn is_local_base_url(base_url: &str) -> bool { - let lower = base_url.trim().to_lowercase(); - lower.starts_with("http://127.") - || lower.starts_with("http://localhost") - || lower.starts_with("http://[::1]") - || lower.starts_with("https://127.") - || lower.starts_with("https://localhost") - || lower.starts_with("https://[::1]") + let host = crate::security::sandbox::extract_host(base_url); + crate::security::sandbox::is_loopback_host(&host) } pub(crate) fn format_results(query: &str, body: &str, limit: usize) -> Result { @@ -724,6 +741,43 @@ mod tests { assert!(!is_local_base_url("https://searx.example.com")); } + #[test] + fn local_base_url_rejects_hosts_that_merely_look_loopback() { + // The prefix match classified all of these as on-device, so household + // search queries were sent to a remote SearXNG with + // allow_remote_base_url still false. The last one is the userinfo case: + // the loopback text is credentials, the real host is evil.com. + for url in [ + "http://localhost.attacker.example/search", + "http://localhost-evil.example", + "http://127.evil.example/", + "https://127.0.0.1.nip.io/", + "http://127.0.0.1@evil.com/search", + "http://127.0.0.1:8888@evil.com", + "https://searx.example.com", + ] { + assert!(!is_local_base_url(url), "{url:?} must not count as local"); + } + } + + #[test] + fn local_base_url_accepts_loopback_behind_userinfo_and_the_whole_127_range() { + // The prefix match rejected these, so a legitimate on-device SearXNG + // behind credentials could not be used at all — the guard failed closed + // on the very configuration it exists to permit. + for url in [ + "http://user@localhost:8888", + "http://user:pass@[::1]:8888", + // 127.0.0.0/8 is entirely loopback, not just 127.0.0.1 (#327). + "http://127.0.0.2:8888", + "http://127.1.2.3:9090/search", + // Casing and a trailing path must not matter. + "http://LocalHost:8888/search", + ] { + assert!(is_local_base_url(url), "{url:?} must count as local"); + } + } + #[test] fn cache_query_normalization_is_stable() { assert_eq!( From ce6b175dfc484263c6ec893f4254728a743f9886 Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Fri, 31 Jul 2026 06:00:48 -0400 Subject: [PATCH 2/2] fix(security): end the authority at a query, fragment, or backslash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review on #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. --- crates/genie-core/src/security/sandbox.rs | 37 ++++++++++++++++++++++- crates/genie-core/src/tools/web_search.rs | 5 +++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/crates/genie-core/src/security/sandbox.rs b/crates/genie-core/src/security/sandbox.rs index 270ad0a7..f93af431 100644 --- a/crates/genie-core/src/security/sandbox.rs +++ b/crates/genie-core/src/security/sandbox.rs @@ -240,7 +240,18 @@ pub(crate) fn extract_host(url: &str) -> String { .or_else(|| url.strip_prefix("https://")) .unwrap_or(url); - let authority = stripped.split('/').next().unwrap_or(stripped); + // The authority ends at the first '/', '?', '#', or '\'. Splitting on '/' + // alone left a query or fragment attached, and the userinfo strip below then + // read *into* it: "http://evil.com#@127.0.0.1" and "http://evil.com?@127.0.0.1" + // both yielded the host "127.0.0.1", so a remote endpoint passed both the + // inference-route SSRF check and the web-search egress guard. A backslash is + // included because WHATWG treats it as a path separator for http(s), which is + // how reqwest's URL parser resolves it — "http://127.0.0.1\@evil.com" really + // does connect to 127.0.0.1, and the old split reported "evil.com". + let authority = stripped + .split(['/', '?', '#', '\\']) + .next() + .unwrap_or(stripped); let host_port = authority.rsplit('@').next().unwrap_or(authority); let host = if let Some(rest) = host_port.strip_prefix('[') { rest.find(']') @@ -295,6 +306,30 @@ mod tests { assert!(validate_inference_route("http://localhost.evil.com:8080/v1").is_err()); } + #[test] + fn reject_remote_host_hidden_behind_a_query_fragment_or_backslash() { + // The authority used to be split on '/' only, so a query or fragment + // stayed attached and the userinfo strip read into it — the '@' inside + // the fragment made "127.0.0.1" look like the host. Both the SSRF check + // here and the web-search egress guard accepted these. + assert!(validate_inference_route("http://evil.com#@127.0.0.1").is_err()); + assert!(validate_inference_route("http://evil.com?@127.0.0.1").is_err()); + assert!(validate_inference_route("http://evil.com\\@127.0.0.1").is_err()); + assert!(validate_inference_route("http://evil.com?next=@localhost").is_err()); + assert_eq!(extract_host("http://evil.com#@127.0.0.1"), "evil.com"); + + // A backslash is a path separator for http(s) under WHATWG, which is how + // reqwest resolves it: this really does connect to 127.0.0.1, so it must + // be accepted rather than misread as "evil.com" and rejected. + assert!(validate_inference_route("http://127.0.0.1\\@evil.com").is_ok()); + assert_eq!(extract_host("http://127.0.0.1\\@evil.com"), "127.0.0.1"); + + // Genuine userinfo in the authority still resolves to the real host. + assert!(validate_inference_route("http://user:pass@[::1]:8080").is_ok()); + assert!(validate_inference_route("http://user@localhost").is_ok()); + assert!(validate_inference_route("http://localhost:1@evil.com/v1").is_err()); + } + #[test] fn reject_remote_routes() { assert!(validate_inference_route("http://api.openai.com/v1").is_err()); diff --git a/crates/genie-core/src/tools/web_search.rs b/crates/genie-core/src/tools/web_search.rs index 23f11909..270f6c00 100644 --- a/crates/genie-core/src/tools/web_search.rs +++ b/crates/genie-core/src/tools/web_search.rs @@ -755,6 +755,11 @@ mod tests { "http://127.0.0.1@evil.com/search", "http://127.0.0.1:8888@evil.com", "https://searx.example.com", + // The loopback text sits in a query, fragment, or backslash-delimited + // path — the authority is still evil.com. + "http://evil.com#@127.0.0.1", + "http://evil.com?@127.0.0.1", + "http://evil.com\\@127.0.0.1", ] { assert!(!is_local_base_url(url), "{url:?} must not count as local"); }