diff --git a/src/client/common.rs b/src/client/common.rs index 8b4fe84..d927440 100644 --- a/src/client/common.rs +++ b/src/client/common.rs @@ -56,7 +56,9 @@ pub trait Client: Sync + Send { let mut builder = ReqwestClient::builder(); let extra = self.extra_config(); let timeout = extra.and_then(|v| v.connect_timeout).unwrap_or(10); - builder = apply_proxy(builder, extra.and_then(|v| v.proxy.as_deref()))?; + if let Some(proxy) = extra.and_then(|v| v.proxy.as_deref()) { + builder = set_proxy(builder, proxy)?; + } if let Some(user_agent) = self.app_config().user_agent.as_ref() { builder = builder.user_agent(user_agent); } diff --git a/src/rag/providers/qdrant.rs b/src/rag/providers/qdrant.rs index 29f5690..fd1e0ad 100644 --- a/src/rag/providers/qdrant.rs +++ b/src/rag/providers/qdrant.rs @@ -1,6 +1,5 @@ use crate::rag::provider::RagProvider; use crate::rag::{DocumentId, RagData}; -use crate::utils::apply_proxy; use anyhow::{Context, Result, bail}; use async_trait::async_trait; @@ -10,6 +9,7 @@ use reqwest::{Client, Response, StatusCode}; use serde_json::Value; use std::collections::HashMap; use std::sync::Arc; +use url::{Host, Url}; /// Marks a `DocumentId` that stands in for a point id Coyote cannot carry /// directly. Qdrant accepts UUID strings as point ids, and that is what @@ -184,7 +184,25 @@ pub struct QdrantProvider { } impl QdrantProvider { - fn make_client(api_key: Option<&str>) -> Result { + /// A proxy cannot usefully forward to an address that means something different + /// on its side, and anything that intercepts proxied traffic answers for a store + /// that is running fine, so the failure names the proxy rather than Qdrant. + fn skips_proxy(base_url: &str) -> bool { + let Ok(url) = Url::parse(base_url) else { + return false; + }; + match url.host() { + Some(Host::Domain(name)) => { + name == "localhost" || name.ends_with(".localhost") || name.ends_with(".local") + } + Some(Host::Ipv4(ip)) => ip.is_loopback() || ip.is_private() || ip.is_link_local(), + // No stable is_unique_local, so fc00::/7 is matched directly. + Some(Host::Ipv6(ip)) => ip.is_loopback() || ip.segments()[0] & 0xfe00 == 0xfc00, + None => false, + } + } + + fn make_client(base_url: &str, api_key: Option<&str>) -> Result { let mut headers = HeaderMap::new(); if let Some(key) = api_key { let mut value = @@ -192,9 +210,11 @@ impl QdrantProvider { value.set_sensitive(true); headers.insert("api-key", value); } - apply_proxy(Client::builder().default_headers(headers), None)? - .build() - .context("Failed to build reqwest client") + let mut builder = Client::builder().default_headers(headers); + if Self::skips_proxy(base_url) { + builder = builder.no_proxy(); + } + builder.build().context("Failed to build reqwest client") } pub(crate) fn normalize_base_url(host: &str) -> String { @@ -219,7 +239,7 @@ impl QdrantProvider { api_key: Option<&str>, ) -> Result { let base_url = Self::normalize_base_url(host); - let client = Self::make_client(api_key)?; + let client = Self::make_client(&base_url, api_key)?; let resp = client .get(format!("{base_url}/collections/{collection}")) .send() @@ -237,7 +257,7 @@ impl QdrantProvider { pub async fn new(host: &str, collection: &str, api_key: Option<&str>) -> Result { let base_url = Self::normalize_base_url(host); - let client = Self::make_client(api_key)?; + let client = Self::make_client(&base_url, api_key)?; let resp = client .get(format!("{base_url}/collections/{collection}")) .send() @@ -260,7 +280,7 @@ impl QdrantProvider { pub async fn list_collections(host: &str, api_key: Option<&str>) -> Result> { let base_url = Self::normalize_base_url(host); - let client = Self::make_client(api_key)?; + let client = Self::make_client(&base_url, api_key)?; let resp = client .get(format!("{base_url}/collections")) .send() @@ -310,7 +330,7 @@ impl QdrantProvider { api_key: Option<&str>, ) -> Result> { let base_url = Self::normalize_base_url(host); - let client = Self::make_client(api_key)?; + let client = Self::make_client(&base_url, api_key)?; let url = format!("{base_url}/collections/{collection}/points/scroll"); let body = serde_json::json!({ "limit": 1, "with_payload": false }); @@ -577,6 +597,39 @@ mod tests { assert!(provider.fetch_content(&[]).await.unwrap().is_empty()); } + #[test] + fn local_and_private_hosts_skip_the_proxy() { + for host in [ + "http://localhost:6333", + "http://127.0.0.1:6333", + "http://192.168.0.56:6333", + "http://10.1.2.3:6333", + "http://172.16.4.5:6333", + "http://qdrant.local:6333", + "http://[::1]:6333", + ] { + assert!( + QdrantProvider::skips_proxy(host), + "{host} should not be proxied" + ); + } + } + + #[test] + fn public_hosts_still_honour_the_environment() { + for host in [ + "https://qdrant.example.com", + "http://8.8.8.8:6333", + "https://xyz.eu-central.aws.cloud.qdrant.io:6333", + "http://172.32.0.1:6333", + ] { + assert!( + !QdrantProvider::skips_proxy(host), + "{host} must keep the environment's proxy" + ); + } + } + /// Euclid collections score by NEGATIVE distance, so the 0.0 the caller /// passes must mean "no floor". Filtering on it drops every hit — the exact /// bug that keeps Qdrant's own `score_threshold` off the wire. diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 2e8dc5b..428e882 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -295,83 +295,18 @@ pub fn is_url(path: &str) -> bool { path.starts_with("http://") || path.starts_with("https://") } -/// 127.0.0.1 means something different to a proxy than it does to us, so a tool -/// that intercepts proxied traffic answers for a local service that is running -/// fine, and the failure reads as a fault in Coyote or in that service. -const LOCAL_NO_PROXY: &str = - "localhost,127.0.0.0/8,::1,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,.local"; - -/// Applies proxy settings, keeping local traffic direct. `configured` is a -/// client's own `extra.proxy`, where `"-"` means no proxy at all; with nothing -/// configured an ambient `*_PROXY` is still honoured for public hosts. -pub fn apply_proxy( +pub fn set_proxy( mut builder: reqwest::ClientBuilder, - configured: Option<&str>, + proxy: &str, ) -> Result { - // reqwest offers no way to add rules to the proxies it auto-detects, so - // detection is disabled and redone below. builder = builder.no_proxy(); - - let configured = configured.map(str::trim).filter(|p| !p.is_empty()); - if configured == Some("-") { - return Ok(builder); - } - let exempt = no_proxy_rules(); - if let Some(url) = configured { - let proxy = reqwest::Proxy::all(url) - .with_context(|| format!("Invalid proxy `{url}`"))? - .no_proxy(reqwest::NoProxy::from_string(&exempt)); - return Ok(builder.proxy(proxy)); - } - - // Split per scheme, because HTTP_PROXY and HTTPS_PROXY are allowed to differ. - for (is_https, keys) in [ - ( - true, - ["HTTPS_PROXY", "https_proxy", "ALL_PROXY", "all_proxy"], - ), - ( - false, - ["HTTP_PROXY", "http_proxy", "ALL_PROXY", "all_proxy"], - ), - ] { - let Some(url) = first_env(&keys) else { - continue; - }; - let proxy = if is_https { - reqwest::Proxy::https(&url) - } else { - reqwest::Proxy::http(&url) - }; - let proxy = proxy - .with_context(|| format!("Invalid proxy `{url}`"))? - .no_proxy(reqwest::NoProxy::from_string(&exempt)); - builder = builder.proxy(proxy); - } + if !proxy.is_empty() && proxy != "-" { + builder = builder + .proxy(reqwest::Proxy::all(proxy).with_context(|| format!("Invalid proxy `{proxy}`"))?); + }; Ok(builder) } -fn first_env(keys: &[&str]) -> Option { - keys.iter() - .find_map(|key| env::var(key).ok()) - .map(|value| value.trim().to_string()) - .filter(|value| !value.is_empty()) -} - -/// Replacing reqwest's auto-detection loses its `NO_PROXY` handling, so that is -/// merged back in here. -fn no_proxy_rules() -> String { - let mut rules = LOCAL_NO_PROXY.to_string(); - let extra = env::var("NO_PROXY") - .or_else(|_| env::var("no_proxy")) - .unwrap_or_default(); - if !extra.trim().is_empty() { - rules.push(','); - rules.push_str(extra.trim()); - } - rules -} - pub fn decode_bin(data: &[u8]) -> Result { let (v, _) = bincode::serde::decode_from_slice(data, bincode::config::legacy())?; Ok(v) @@ -381,47 +316,6 @@ pub fn decode_bin(data: &[u8]) -> Result { mod tests { use super::*; - /// Invalid rule syntax makes `from_string` return `None`, which silently drops - /// every exemption and sends local traffic back through the proxy. - #[test] - fn the_local_no_proxy_rules_are_valid() { - assert!( - reqwest::NoProxy::from_string(LOCAL_NO_PROXY).is_some(), - "reqwest rejected LOCAL_NO_PROXY, so nothing would be exempt" - ); - } - - #[test] - fn local_rules_cover_loopback_and_private_ranges() { - for host in [ - "localhost", - "127.0.0.0/8", - "10.0.0.0/8", - "172.16.0.0/12", - "192.168.0.0/16", - ] { - assert!( - LOCAL_NO_PROXY.contains(host), - "{host} must stay exempt from proxying" - ); - } - } - - #[test] - fn a_dash_means_no_proxy_at_all() { - assert!(apply_proxy(reqwest::ClientBuilder::new(), Some("-")).is_ok()); - assert!(apply_proxy(reqwest::ClientBuilder::new(), Some(" - ")).is_ok()); - } - - #[test] - fn an_unparseable_proxy_is_reported() { - let err = apply_proxy(reqwest::ClientBuilder::new(), Some("not a url")) - .unwrap_err() - .to_string(); - - assert!(err.contains("Invalid proxy"), "got: {err}"); - } - #[test] #[cfg(not(target_os = "windows"))] fn test_safe_join_path() {