From cc4665b09c337a95df18697ab76276c246452413 Mon Sep 17 00:00:00 2001 From: Mega Mind <146339422+M3gA-Mind@users.noreply.github.com> Date: Wed, 27 May 2026 16:54:27 +0530 Subject: [PATCH] fix(tools): make http_request web-fetch work with empty allowed_domains (#2738) --- src/openhuman/tools/impl/network/url_guard.rs | 154 +++++++++++++++--- 1 file changed, 131 insertions(+), 23 deletions(-) diff --git a/src/openhuman/tools/impl/network/url_guard.rs b/src/openhuman/tools/impl/network/url_guard.rs index 9bb827ced..c52d3d4bd 100644 --- a/src/openhuman/tools/impl/network/url_guard.rs +++ b/src/openhuman/tools/impl/network/url_guard.rs @@ -1,19 +1,22 @@ //! Shared URL validation + SSRF guards for outbound network tools. //! //! Used by `http_request`, `curl`, and any future tool that takes a -//! user-supplied URL. The contract is intentionally strict: +//! user-supplied URL. Two allowlist modes: //! -//! - http(s) only -//! - non-empty allowlist required (callers pass it in) -//! - no whitespace, no userinfo, no IPv6 hosts -//! - blocks loopback / RFC1918 / link-local / multicast / documentation / -//! shared-address / IPv4-mapped IPv6, including `localhost` / -//! `*.localhost` / `*.local` +//! - **Open allowlist** (`allowed_domains` is empty): any public non-private +//! host is permitted. All SSRF guards still apply (loopback / RFC1918 / +//! link-local / multicast / documentation / shared-address / +//! IPv4-mapped IPv6, `localhost` / `*.localhost` / `*.local`). +//! - **Strict allowlist** (`allowed_domains` is non-empty): only the listed +//! domains and their subdomains are permitted. //! -//! The blocklist deliberately does NOT cover alternate IP notations -//! (octal, hex, decimal) because Rust's `IpAddr::parse` rejects them — -//! they fall through and get rejected by the allowlist instead. See the -//! tests in `http_request.rs` for the documented behaviour. +//! Both modes enforce: http(s) only, no whitespace, no userinfo, no IPv6 hosts. +//! +//! **Alternate IP notations** (octal, hex, decimal): Rust's `IpAddr::parse` +//! rejects them so they are treated as plain hostnames. In strict-allowlist +//! mode they are rejected by the domain check. In open-allowlist mode they +//! pass `validate_url` but are caught by `validate_url_with_dns_check` +//! because they fail real-world DNS resolution. //! //! ## DNS Rebinding Protection //! @@ -43,21 +46,30 @@ pub(super) fn validate_url(raw_url: &str, allowed_domains: &[String]) -> anyhow: anyhow::bail!("Only http:// and https:// URLs are allowed"); } - if allowed_domains.is_empty() { - anyhow::bail!( - "I'm not allowed to open any website yet — the allowed websites list is empty. \ - Add the site you need (or turn on \"Allow all sites\") under \ - Settings → Advanced → Search engine → Allowed websites, then ask me again." - ); - } - let host = extract_host(url)?; if is_private_or_local_host(&host) { + log::debug!( + "[url_guard] ssrf block: host={host} mode={}", + if allowed_domains.is_empty() { + "open" + } else { + "strict" + } + ); anyhow::bail!("Blocked local/private host: {host}"); } - if !host_matches_allowlist(&host, allowed_domains) { + // Empty allowed_domains = open mode: any public non-private host is + // permitted (same as ["*"]). This ensures the http_request tool works + // out of the box regardless of whether the user configured an explicit + // domain list, and keeps web-fetch consistent across routing paths. + // A non-empty list = strict mode: only listed domains pass. (#2700) + if !allowed_domains.is_empty() && !host_matches_allowlist(&host, allowed_domains) { + log::debug!( + "[url_guard] strict-allowlist rejection: host={host} allowed={:?}", + allowed_domains + ); anyhow::bail!( "I'm not allowed to open '{host}' — it isn't in your allowed websites. \ Add it (or turn on \"Allow all sites\") under \ @@ -65,6 +77,15 @@ pub(super) fn validate_url(raw_url: &str, allowed_domains: &[String]) -> anyhow: ); } + log::debug!( + "[url_guard] validate_url ok: host={host} mode={}", + if allowed_domains.is_empty() { + "open" + } else { + "strict" + } + ); + Ok(url.to_string()) } @@ -144,12 +165,26 @@ async fn resolve_host_ips(host: String, port: u16) -> anyhow::Result } pub(super) fn normalize_allowed_domains(domains: Vec) -> Vec { + if domains.is_empty() { + return Vec::new(); + } let mut normalized = domains .into_iter() .filter_map(|d| normalize_domain(&d)) .collect::>(); normalized.sort_unstable(); normalized.dedup(); + if normalized.is_empty() { + // All entries were malformed (whitespace-only, scheme-only, etc.) and + // filtered out. Returning empty would silently enter open mode; instead + // return a sentinel that keeps the tool in strict mode and rejects every + // URL — fail-closed on misconfiguration. (#2738) + log::warn!( + "[url_guard] all configured allowed_domains entries are invalid — \ + treating as misconfigured allowlist (fail-closed)" + ); + return vec!["".to_string()]; + } normalized } @@ -420,12 +455,85 @@ mod tests { assert!(err.contains("userinfo")); } + // Empty allowed_domains = open mode: any public host is permitted. + // This keeps web-fetch working when no domain list is configured and + // makes behaviour consistent between default and external-LLM routing. + // (#2700) #[test] - fn validate_requires_allowlist() { - let err = validate_url("https://example.com", &[]) + fn validate_empty_allowlist_allows_public_host() { + assert!(validate_url("https://example.com", &[]).is_ok()); + assert!(validate_url("https://www.cnbc.com/markets", &[]).is_ok()); + } + + #[test] + fn validate_empty_allowlist_still_blocks_private_hosts() { + let err = validate_url("https://192.168.1.5", &[]) .unwrap_err() .to_string(); - assert!(err.contains("allowed websites")); + assert!(err.contains("local/private")); + + let err = validate_url("https://localhost", &[]) + .unwrap_err() + .to_string(); + assert!(err.contains("local/private")); + } + + // ── normalize_allowed_domains: fail-closed on malformed-only input ── + + #[test] + fn normalize_all_invalid_entries_stays_fail_closed() { + // A non-empty list that fully normalizes to nothing must NOT produce + // an empty slice (which would silently enter open mode). (#2738) + let got = normalize_allowed_domains(vec![" ".into(), "https://".into()]); + assert!( + !got.is_empty(), + "normalized result must be non-empty to stay in strict mode" + ); + // The sentinel must not match any real public host. + assert!( + !host_matches_allowlist("example.com", &got), + "sentinel must not grant access to real hosts" + ); + assert!( + !host_matches_allowlist("api.example.com", &got), + "sentinel must not grant access to subdomains" + ); + } + + #[test] + fn normalize_empty_input_stays_empty_for_open_mode() { + // Explicitly empty input should return empty (open mode is intentional). + assert!(normalize_allowed_domains(vec![]).is_empty()); + } + + #[tokio::test] + async fn dns_check_with_empty_allowlist_allows_public_resolved_host() { + // Open mode (empty allowlist) must still pass DNS check for public IPs. + let got = validate_url_with_dns_check_with_resolver( + "https://example.com", + &[], + |host, port| async move { + assert_eq!(host, "example.com"); + assert_eq!(port, 443); + Ok(vec!["93.184.216.34".parse().unwrap()]) + }, + ) + .await + .unwrap(); + assert_eq!(got, "https://example.com"); + } + + #[tokio::test] + async fn dns_check_with_empty_allowlist_blocks_private_resolved_ip() { + // Even in open mode, DNS rebinding to a private IP must be blocked. + let err = + validate_url_with_dns_check_with_resolver("https://example.com", &[], |_, _| async { + Ok(vec!["10.0.0.1".parse().unwrap()]) + }) + .await + .unwrap_err() + .to_string(); + assert!(err.contains("DNS rebinding blocked")); } #[test]