diff --git a/Cargo.lock b/Cargo.lock index 4fc58110..0007a86a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -55,6 +55,7 @@ dependencies = [ "tree-sitter-swift", "tree-sitter-typescript", "tui-test", + "url", "uuid", "which", "zip", diff --git a/Cargo.toml b/Cargo.toml index f3f0e443..abf81b1f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,6 +25,9 @@ tokio = { version = "1", features = ["full"] } # HTTP client reqwest = { version = "0.12", features = ["json", "rustls-tls", "stream"], default-features = false } +# URL parsing (reqwest's own dependency) — used to match loopback hosts exactly +# rather than by string prefix, so the HTTPS transport guard cannot be bypassed. +url = "2" # Serialization serde = { version = "1", features = ["derive"] } diff --git a/src/api/client.rs b/src/api/client.rs index 59571bf8..88a86e75 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -45,10 +45,10 @@ impl ActualApiClient { timeout: Duration, connect_timeout: Duration, ) -> Result { - // Enforce HTTPS for all non-loopback URLs to protect credentials in transit. - let is_localhost = base_url.starts_with("http://localhost") - || base_url.starts_with("http://127.0.0.1") - || base_url.starts_with("http://[::1]"); + // Enforce HTTPS for all non-loopback URLs to protect credentials in + // transit — see `crate::net::is_loopback_http_url` for why the loopback + // check must be an exact host match, not a raw string prefix. + let is_localhost = crate::net::is_loopback_http_url(base_url); if !base_url.starts_with("https://") && !is_localhost { return Err(ActualError::ConfigError( "API URL must use HTTPS (got a non-HTTPS URL). \ diff --git a/src/auth/oauth.rs b/src/auth/oauth.rs index 564518a6..e9eabf9a 100644 --- a/src/auth/oauth.rs +++ b/src/auth/oauth.rs @@ -140,12 +140,10 @@ pub fn build_authorize_url( /// Build an HTTP client for the auth server. Enforces HTTPS for non-loopback /// URLs so tokens are never sent in clear text (loopback `http://` is allowed -/// for the local mock). +/// for the local mock — see [`crate::net::is_loopback_http_url`]). fn build_http_client(base_url: &str) -> Result { - let is_localhost = base_url.starts_with("http://localhost") - || base_url.starts_with("http://127.0.0.1") - || base_url.starts_with("http://[::1]"); - if !base_url.starts_with("https://") && !is_localhost { + let is_loopback = crate::net::is_loopback_http_url(base_url); + if !base_url.starts_with("https://") && !is_loopback { return Err(ActualError::ConfigError( "Auth server URL must use HTTPS (got a non-HTTPS, non-loopback URL). \ Use https:// to protect your credentials." @@ -154,7 +152,7 @@ fn build_http_client(base_url: &str) -> Result { } reqwest::Client::builder() .user_agent(USER_AGENT_VALUE) - .https_only(!is_localhost) + .https_only(!is_loopback) .timeout(Duration::from_secs(30)) .connect_timeout(Duration::from_secs(10)) .build() @@ -439,6 +437,37 @@ mod tests { } } + // --- Transport guard: the auth client must refuse to relax HTTPS for any + // non-loopback host. The exact-host predicate itself is unit-tested in + // `crate::net`; these cover this client's use of it. --- + + #[test] + fn test_build_http_client_rejects_bypass_urls() { + // Each bypass URL must be refused outright — the HTTPS guard stays on, + // so no clear-text client is ever built for a non-loopback host. + for url in [ + "http://localhost.evil.com", + "http://127.0.0.1.evil.com", + "http://127.0.0.1@evil.com", + "http://evil.com", + ] { + let err = build_http_client(url).unwrap_err(); + assert!( + matches!(err, ActualError::ConfigError(ref m) if m.contains("HTTPS")), + "expected HTTPS-guard rejection for {url}, got: {err:?}" + ); + } + } + + #[test] + fn test_build_http_client_allows_loopback_and_https() { + // Genuine loopback dev endpoints and any https URL still build fine. + assert!(build_http_client("http://localhost:4000").is_ok()); + assert!(build_http_client("http://127.0.0.1:4000").is_ok()); + assert!(build_http_client("http://[::1]:4000").is_ok()); + assert!(build_http_client("https://app.actual.ai").is_ok()); + } + #[test] fn test_build_authorize_url_contains_required_params() { let cfg = test_cfg("http://localhost:4000"); @@ -921,10 +950,11 @@ mod tests { #[tokio::test] async fn test_login_with_authorize_url_error() { - // base_url passes the loopback HTTP-client guard (localhost) but fails - // URL parsing (invalid port) → exercises the build_authorize_url `?` + // base_url passes the HTTP-client guard (an `https://` URL is accepted + // by scheme prefix, without a full parse) but fails `build_authorize_url`'s + // URL parse (invalid port 99999) → exercises the build_authorize_url `?` // error path inside login_with. - let cfg = test_cfg("http://127.0.0.1:99999"); + let cfg = test_cfg("https://127.0.0.1:99999"); let server = LoopbackServer::bind().await.unwrap(); let opener = |_: &str| {}; let err = login_with( diff --git a/src/lib.rs b/src/lib.rs index 68bcb8ac..d1003826 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,6 +7,7 @@ pub mod config; pub mod error; pub mod generation; pub mod model_cache; +pub mod net; pub mod runner; pub mod tailoring; #[cfg(feature = "telemetry")] diff --git a/src/model_cache.rs b/src/model_cache.rs index cebbe44e..d69642fb 100644 --- a/src/model_cache.rs +++ b/src/model_cache.rs @@ -153,8 +153,7 @@ pub(crate) async fn fetch_openai_models_async( ) -> Result, crate::error::ActualError> { use crate::error::ActualError; - let is_local = - base_url.starts_with("http://localhost") || base_url.starts_with("http://127.0.0.1"); + let is_local = crate::net::is_loopback_http_url(base_url); let client = reqwest::Client::builder() .timeout(timeout) @@ -245,8 +244,7 @@ pub(crate) async fn fetch_anthropic_models_async( ) -> Result, crate::error::ActualError> { use crate::error::ActualError; - let is_local = - base_url.starts_with("http://localhost") || base_url.starts_with("http://127.0.0.1"); + let is_local = crate::net::is_loopback_http_url(base_url); let client = reqwest::Client::builder() .timeout(timeout) diff --git a/src/net.rs b/src/net.rs new file mode 100644 index 00000000..4e227c93 --- /dev/null +++ b/src/net.rs @@ -0,0 +1,89 @@ +//! Network transport helpers shared across the crate's HTTP clients. + +use std::net::{Ipv4Addr, Ipv6Addr}; + +use url::Host; + +/// Return `true` only when `base_url` is a genuine clear-text `http` loopback +/// endpoint for which it is safe to relax the HTTPS-only transport guard. +/// +/// Several HTTP clients in this crate allow clear-text `http` for a local dev +/// or mock server by disabling reqwest's `https_only`. The earlier checks did +/// this with `str::starts_with("http://localhost")` / `127.0.0.1` / `[::1]` on +/// the raw URL, which a non-loopback attacker host could satisfy: +/// `http://localhost.evil.com`, `http://127.0.0.1.evil.com`, and the userinfo +/// trick `http://127.0.0.1@evil.com` (the connection's real host is `evil.com`) +/// all matched, which disabled `https_only` and let credentials leave over +/// clear-text HTTP when the caller controlled the base URL. Parse the URL and +/// inspect the *actual* host instead, so only true loopback endpoints qualify: +/// +/// - the scheme must be `http` — an `https` URL never needs the guard relaxed; +/// - the URL must carry no userinfo, rejecting the `user@host` smuggle; +/// - the host must be exactly `localhost`, `127.0.0.1`, or `::1`. +pub fn is_loopback_http_url(base_url: &str) -> bool { + let url = match url::Url::parse(base_url) { + Ok(u) => u, + Err(_) => return false, + }; + // Any userinfo means the host after `@` is the real destination — reject so + // `http://127.0.0.1@evil.com` cannot masquerade as loopback. + if !url.username().is_empty() || url.password().is_some() { + return false; + } + // A URL with no host at all (e.g. a `data:` / `mailto:` URL) is not loopback. + let host = match url.host() { + Some(h) => h, + None => return false, + }; + // Relax the guard only for clear-text http whose host is *exactly* a + // loopback identity. Compare the parsed host, not the raw string, so a + // lookalike domain like `localhost.evil.com` cannot match; `Ipv4Addr` / + // `Ipv6Addr::LOCALHOST` are exactly `127.0.0.1` / `::1`, not the wider + // loopback ranges. + url.scheme() == "http" + && match host { + Host::Domain(host) => host.eq_ignore_ascii_case("localhost"), + Host::Ipv4(addr) => addr == Ipv4Addr::LOCALHOST, + Host::Ipv6(addr) => addr == Ipv6Addr::LOCALHOST, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn accepts_genuine_loopback() { + assert!(is_loopback_http_url("http://localhost:4000")); + assert!(is_loopback_http_url("http://localhost")); // no port + assert!(is_loopback_http_url("http://127.0.0.1:4000")); + assert!(is_loopback_http_url("http://[::1]:4000")); + assert!(is_loopback_http_url("http://LOCALHOST:4000")); // host is case-insensitive + } + + #[test] + fn rejects_lookalike_hosts() { + // A host that merely *starts with* the loopback literal but resolves to + // the attacker's domain must NOT be treated as loopback. + assert!(!is_loopback_http_url("http://localhost.evil.com")); + assert!(!is_loopback_http_url("http://127.0.0.1.evil.com")); + // The userinfo trick: the connection's real host is evil.com. + assert!(!is_loopback_http_url("http://127.0.0.1@evil.com")); + assert!(!is_loopback_http_url("http://localhost@evil.com")); + // Plain non-loopback host. + assert!(!is_loopback_http_url("http://evil.com")); + } + + #[test] + fn rejects_https_userinfo_and_garbage() { + // https never needs the guard relaxed (stays https_only). + assert!(!is_loopback_http_url("https://localhost:4000")); + // Even a genuine loopback host is rejected when it carries userinfo. + assert!(!is_loopback_http_url("http://user:pass@127.0.0.1:4000")); + // A URL with no host component (data:/mailto:) is not loopback. + assert!(!is_loopback_http_url("data:text/plain,hello")); + assert!(!is_loopback_http_url("mailto:dev@example.com")); + // Non-URL input is not loopback. + assert!(!is_loopback_http_url("not a url")); + } +} diff --git a/src/runner/anthropic_api.rs b/src/runner/anthropic_api.rs index e53a22c4..4cdec786 100644 --- a/src/runner/anthropic_api.rs +++ b/src/runner/anthropic_api.rs @@ -81,9 +81,7 @@ impl AnthropicApiRunner { base_url: String, max_tokens: u32, ) -> Result { - let is_localhost = base_url.starts_with("http://localhost") - || base_url.starts_with("http://127.0.0.1") - || base_url.starts_with("http://[::1]"); + let is_localhost = crate::net::is_loopback_http_url(&base_url); let client = reqwest::Client::builder() .timeout(timeout) .https_only(!is_localhost) diff --git a/src/runner/openai_api.rs b/src/runner/openai_api.rs index bc9d2706..61f4959d 100644 --- a/src/runner/openai_api.rs +++ b/src/runner/openai_api.rs @@ -149,9 +149,7 @@ impl OpenAiApiRunner { /// Rebuilds the inner `reqwest::Client` with `https_only` disabled when the /// URL is a loopback address so that mock servers work in tests. pub(crate) fn with_base_url(mut self, base_url: String) -> Self { - let is_localhost = base_url.starts_with("http://localhost") - || base_url.starts_with("http://127.0.0.1") - || base_url.starts_with("http://[::1]"); + let is_localhost = crate::net::is_loopback_http_url(&base_url); if is_localhost { self.client = reqwest::Client::builder() .timeout(self.timeout)