From 2de1792944537c95dfe624f11b8da9397c35fd85 Mon Sep 17 00:00:00 2001 From: Ben Wisecup Date: Mon, 6 Jul 2026 10:38:50 -0400 Subject: [PATCH 1/2] fix(auth): match loopback hosts exactly in the HTTPS transport guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_http_client` decided whether to relax the HTTPS-only transport guard with `str::starts_with("http://localhost")` / `127.0.0.1` / `[::1]` on the raw URL. That prefix match is satisfied by attacker-controlled hosts — `http://localhost.evil.com`, `http://127.0.0.1.evil.com`, and the userinfo form `http://127.0.0.1@evil.com` (the connection's real host is `evil.com`) — so a caller who controls the base URL could disable `https_only` and send credentials to a non-loopback host in clear text. Replace the prefix check with a parsed-URL match (`is_loopback_http_url`): require the `http` scheme, reject any userinfo component, and match the host exactly against `localhost` / `127.0.0.1` / `::1`. Genuine loopback dev endpoints still work; every other URL is forced through HTTPS. Add negative tests for the three bypass URLs (the guard now refuses them) and positive tests for real loopback and https endpoints. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-2 On behalf of: @benw5483 Co-Authored-By: Actual Factory Bot --- Cargo.lock | 1 + Cargo.toml | 3 ++ src/auth/oauth.rs | 114 +++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 112 insertions(+), 6 deletions(-) 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/auth/oauth.rs b/src/auth/oauth.rs index 564518a6..8730ac75 100644 --- a/src/auth/oauth.rs +++ b/src/auth/oauth.rs @@ -11,10 +11,12 @@ //! server (the mock at `http://localhost:4000` during dev). It must be //! supplied explicitly — see [`resolve_auth_url`]. +use std::net::{Ipv4Addr, Ipv6Addr}; use std::time::Duration; use chrono::{Duration as ChronoDuration, Utc}; use serde::Deserialize; +use url::Host; use crate::auth::loopback::LoopbackServer; use crate::auth::pkce::{self, PkcePair}; @@ -138,14 +140,51 @@ pub fn build_authorize_url( Ok(url.to_string()) } +/// 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. +/// +/// The earlier implementation matched with `str::starts_with` 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. 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`. +fn is_loopback_http_url(base_url: &str) -> bool { + let url = match url::Url::parse(base_url) { + Ok(u) => u, + Err(_) => return false, + }; + // Only clear-text http gets the guard relaxed; https is already safe. + if url.scheme() != "http" { + 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; + } + // Compare the parsed host, not the raw string. `Ipv4Addr::LOCALHOST` / + // `Ipv6Addr::LOCALHOST` are exactly `127.0.0.1` / `::1` (not the wider + // loopback ranges), matching the intended dev endpoints precisely. + match url.host() { + Some(Host::Domain(host)) => host.eq_ignore_ascii_case("localhost"), + Some(Host::Ipv4(addr)) => addr == Ipv4Addr::LOCALHOST, + Some(Host::Ipv6(addr)) => addr == Ipv6Addr::LOCALHOST, + None => false, + } +} + /// 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 [`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 = 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 +193,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 +478,69 @@ mod tests { } } + // --- Transport guard: loopback detection must be an exact host match, not + // a raw-string prefix, so it cannot be bypassed to send credentials in + // clear text to an attacker-controlled host. --- + + #[test] + fn test_is_loopback_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 test_is_loopback_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 test_is_loopback_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")); + // Non-URL input is not loopback. + assert!(!is_loopback_http_url("not a url")); + } + + #[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"); From b26c7b8854bfd5386e83d6d2ac56fcce7c36e31f Mon Sep 17 00:00:00 2001 From: Ben Wisecup Date: Mon, 6 Jul 2026 10:59:20 -0400 Subject: [PATCH 2/2] refactor(net): route all HTTP clients through one hardened loopback guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bypassable loopback check fixed in the auth client was duplicated, with the same `str::starts_with` shape, in every other HTTP-client constructor in the crate: the platform API client (which attaches the login-session bearer to every authenticated call), the model-cache probes, and the OpenAI / Anthropic runner clients. Each could be tricked by `http://localhost.evil.com` or `http://127.0.0.1@evil.com` into disabling `https_only` for a non-loopback host. Extract the hardened check into a single `crate::net::is_loopback_http_url` (parse with `url::Url`, require the `http` scheme, reject userinfo, and match the host exactly against `localhost` / `127.0.0.1` / `::1`) and route every one of these constructors through it, so there is one guard and no divergent copies. Behavior is preserved for genuine loopback dev endpoints; every other URL is forced through HTTPS. The predicate's unit tests move to the new module alongside it. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-2 On behalf of: @benw5483 Co-Authored-By: Actual Factory Bot --- src/api/client.rs | 8 ++-- src/auth/oauth.rs | 90 ++++--------------------------------- src/lib.rs | 1 + src/model_cache.rs | 6 +-- src/net.rs | 89 ++++++++++++++++++++++++++++++++++++ src/runner/anthropic_api.rs | 4 +- src/runner/openai_api.rs | 4 +- 7 files changed, 107 insertions(+), 95 deletions(-) create mode 100644 src/net.rs 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 8730ac75..e9eabf9a 100644 --- a/src/auth/oauth.rs +++ b/src/auth/oauth.rs @@ -11,12 +11,10 @@ //! server (the mock at `http://localhost:4000` during dev). It must be //! supplied explicitly — see [`resolve_auth_url`]. -use std::net::{Ipv4Addr, Ipv6Addr}; use std::time::Duration; use chrono::{Duration as ChronoDuration, Utc}; use serde::Deserialize; -use url::Host; use crate::auth::loopback::LoopbackServer; use crate::auth::pkce::{self, PkcePair}; @@ -140,50 +138,11 @@ pub fn build_authorize_url( Ok(url.to_string()) } -/// 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. -/// -/// The earlier implementation matched with `str::starts_with` 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. 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`. -fn is_loopback_http_url(base_url: &str) -> bool { - let url = match url::Url::parse(base_url) { - Ok(u) => u, - Err(_) => return false, - }; - // Only clear-text http gets the guard relaxed; https is already safe. - if url.scheme() != "http" { - 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; - } - // Compare the parsed host, not the raw string. `Ipv4Addr::LOCALHOST` / - // `Ipv6Addr::LOCALHOST` are exactly `127.0.0.1` / `::1` (not the wider - // loopback ranges), matching the intended dev endpoints precisely. - match url.host() { - Some(Host::Domain(host)) => host.eq_ignore_ascii_case("localhost"), - Some(Host::Ipv4(addr)) => addr == Ipv4Addr::LOCALHOST, - Some(Host::Ipv6(addr)) => addr == Ipv6Addr::LOCALHOST, - None => false, - } -} - /// 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 — see [`is_loopback_http_url`]). +/// for the local mock — see [`crate::net::is_loopback_http_url`]). fn build_http_client(base_url: &str) -> Result { - let is_loopback = is_loopback_http_url(base_url); + 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). \ @@ -478,41 +437,9 @@ mod tests { } } - // --- Transport guard: loopback detection must be an exact host match, not - // a raw-string prefix, so it cannot be bypassed to send credentials in - // clear text to an attacker-controlled host. --- - - #[test] - fn test_is_loopback_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 test_is_loopback_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 test_is_loopback_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")); - // Non-URL input is not loopback. - assert!(!is_loopback_http_url("not a url")); - } + // --- 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() { @@ -1023,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)