diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index b9a811e9..a138de70 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -88,20 +88,78 @@ fn sanitize_excerpt(s: &str) -> String { out } -/// Whether `u` parses as a trusted URL. +/// Fixed warning when `GITLAWB_ICAPTCHA_INSECURE` is present but not an +/// explicit truthy value. Does not include the raw env value (arbitrary +/// deployment input — secrets / terminal controls). +fn warn_icaptcha_insecure_non_truthy() { + tracing::warn!( + "GITLAWB_ICAPTCHA_INSECURE is set but not truthy (expected 1 or true); \ + loopback HTTP trust relaxation remains disabled" + ); +} + +/// Parse a possibly-present `GITLAWB_ICAPTCHA_INSECURE` string. /// -/// Production trusts only `https`. When `GITLAWB_ICAPTCHA_INSECURE` is set (a -/// runtime escape hatch for integration tests) the function also trusts -/// `http://127.0.0.1` and `http://localhost` so the full iCaptcha retry path -/// can be exercised against a local mockito server. -fn is_https(u: &str) -> bool { +/// Only an explicit truthy value (`1` / `true`, case-insensitive, after trim) +/// enables the loopback-HTTP trust relaxation. Presence alone is not enough: +/// `=0`, `=false`, or an empty value leave it off. A non-empty non-truthy +/// value emits [`warn_icaptcha_insecure_non_truthy`]. +fn icaptcha_insecure_from_str(raw: Option<&str>) -> bool { + let Some(v) = raw else { + return false; + }; + let t = v.trim(); + if t.eq_ignore_ascii_case("1") || t.eq_ignore_ascii_case("true") { + true + } else { + if !t.is_empty() { + warn_icaptcha_insecure_non_truthy(); + } + false + } +} + +/// Interpret `std::env::var("GITLAWB_ICAPTCHA_INSECURE")` without reading the +/// environment again — used by production and by unit tests. +/// +/// `NotPresent` → disabled. `NotUnicode` → disabled + the same fixed warning +/// as a present non-truthy value (fail closed; do not treat as unset). +fn icaptcha_insecure_from_var(result: Result) -> bool { + match result { + Ok(v) => icaptcha_insecure_from_str(Some(&v)), + Err(std::env::VarError::NotUnicode(_)) => { + warn_icaptcha_insecure_non_truthy(); + false + } + Err(std::env::VarError::NotPresent) => false, + } +} + +/// Whether `GITLAWB_ICAPTCHA_INSECURE` explicitly enables the loopback-HTTP +/// trust relaxation used by integration tests. +/// +/// Only an explicit truthy value (`1` / `true`, case-insensitive, after trim) +/// enables it. Presence alone is not enough: `=0`, `=false`, or an empty +/// value leave the relaxation off. A non-empty non-truthy value (including a +/// present non-Unicode value) emits a `tracing::warn!` (visible when the +/// `icaptcha_client` target is at WARN+, e.g. `RUST_LOG=icaptcha_client=warn`). +fn icaptcha_insecure_enabled() -> bool { + icaptcha_insecure_from_var(std::env::var("GITLAWB_ICAPTCHA_INSECURE")) +} + +/// Whether `u` parses as a trusted URL, given a pre-parsed insecure flag. +/// +/// Production trusts only `https`. When `insecure` is true the function also +/// trusts `http://127.0.0.1` and `http://localhost` so the full iCaptcha retry +/// path can be exercised against a local mockito server. +fn is_https_with(u: &str, insecure: bool) -> bool { let Ok(parsed) = reqwest::Url::parse(u) else { return false; }; if parsed.scheme() == "https" { return true; } - if std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some() + if insecure && parsed.scheme() == "http" && matches!(parsed.host_str(), Some("127.0.0.1" | "localhost")) { @@ -139,7 +197,15 @@ fn origin_key(u: &str) -> Option { /// explicitly-configured host (never a node-discovered one), so a key is never /// exfiltrated to an origin the operator did not choose. fn resolve_solver_url(advertised: Option<&str>, operator: Option<&str>) -> (String, bool) { - let operator = operator.filter(|u| is_https(u)); + resolve_solver_url_with(advertised, operator, icaptcha_insecure_enabled()) +} + +fn resolve_solver_url_with( + advertised: Option<&str>, + operator: Option<&str>, + insecure: bool, +) -> (String, bool) { + let operator = operator.filter(|u| is_https_with(u, insecure)); let operator_origin = operator.as_ref().and_then(|u| origin_key(u)); let default_origin = origin_key(DEFAULT_URL); @@ -157,7 +223,7 @@ fn resolve_solver_url(advertised: Option<&str>, operator: Option<&str>) -> (Stri }; let chosen = match advertised { - Some(a) if is_https(a) && allowed(&origin_key(a)) => a.to_string(), + Some(a) if is_https_with(a, insecure) && allowed(&origin_key(a)) => a.to_string(), Some(a) => { tracing::warn!( advertised = %a, @@ -383,27 +449,36 @@ mod tests { use std::ffi::OsString; use std::sync::{Mutex, MutexGuard}; - /// Serializes tests that touch the process-global - /// `GITLAWB_ICAPTCHA_INSECURE` env var so they never race. + /// Serializes the real-env reader test so it never races with other + /// process-global env mutations (and recovers from a poisoned lock). static ICAPTCHA_ENV_LOCK: Mutex<()> = Mutex::new(()); - /// Set `GITLAWB_ICAPTCHA_INSECURE` for the test lifetime, restoring any - /// prior value on drop. - struct InsecureEnv { + struct EnvGuard { _lock: MutexGuard<'static, ()>, prev: Option, } - impl InsecureEnv { - fn new() -> Self { - let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); + impl EnvGuard { + fn lock() -> MutexGuard<'static, ()> { + ICAPTCHA_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()) + } + + fn with_value(val: &str) -> Self { + let lock = Self::lock(); + let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); + std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", val); + EnvGuard { _lock: lock, prev } + } + + fn unset() -> Self { + let lock = Self::lock(); let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); - std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); - InsecureEnv { _lock: lock, prev } + std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); + EnvGuard { _lock: lock, prev } } } - impl Drop for InsecureEnv { + impl Drop for EnvGuard { fn drop(&mut self) { match self.prev.take() { Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", v), @@ -412,6 +487,118 @@ mod tests { } } + /// #227 / #246: cover the production env read in `icaptcha_insecure_enabled`. + /// Pure-helper tests alone leave mutations of this function green. + #[test] + fn icaptcha_insecure_enabled_reads_real_env() { + { + let _g = EnvGuard::unset(); + assert!(!icaptcha_insecure_enabled(), "unset must disable"); + } + { + let _g = EnvGuard::with_value("0"); + assert!(!icaptcha_insecure_enabled(), "=0 must disable"); + } + { + let _g = EnvGuard::with_value("false"); + assert!(!icaptcha_insecure_enabled(), "=false must disable"); + } + { + let _g = EnvGuard::with_value(""); + assert!(!icaptcha_insecure_enabled(), "empty must disable"); + } + { + let _g = EnvGuard::with_value("1"); + assert!(icaptcha_insecure_enabled(), "=1 must enable"); + } + { + let _g = EnvGuard::with_value("true"); + assert!(icaptcha_insecure_enabled(), "=true must enable"); + } + { + let _g = EnvGuard::with_value("TRUE"); + assert!(icaptcha_insecure_enabled(), "=TRUE must enable"); + } + { + let _g = EnvGuard::with_value(" 1 "); + assert!(icaptcha_insecure_enabled(), "\" 1 \" (trimmed) must enable"); + } + } + + /// #227: `=0` / `=false` / empty / unset must NOT enable loopback HTTP; + /// only explicit truthy `1` / `true` may. Uses pure helpers so tests do + /// not mutate process-global environment (avoids races with parallel + /// resolver tests that also call `resolve_solver_url`). + #[test] + fn insecure_env_only_truthy_enables_loopback_http() { + assert!( + !is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("0"))), + "=0 must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_from_str(Some("0"))); + + assert!( + !is_https_with( + "http://127.0.0.1", + icaptcha_insecure_from_str(Some("false")) + ), + "=false must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_from_str(Some("false"))); + + assert!( + !is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some(""))), + "empty value must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_from_str(Some(""))); + + assert!( + !is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(None)), + "unset must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_from_str(None)); + assert!(!icaptcha_insecure_from_var(Err( + std::env::VarError::NotPresent + ))); + + assert!( + is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("1"))), + "=1 must enable loopback HTTP" + ); + assert!(is_https_with( + "http://localhost", + icaptcha_insecure_from_str(Some("1")) + )); + assert!(icaptcha_insecure_from_str(Some("1"))); + + assert!( + is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("true"))), + "=true must enable loopback HTTP" + ); + assert!(icaptcha_insecure_from_str(Some("true"))); + + assert!( + is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some("TRUE"))), + "=TRUE (case-insensitive) must enable loopback HTTP" + ); + + // Trim is load-bearing: whitespace around a truthy token must still enable. + assert!( + is_https_with("http://127.0.0.1", icaptcha_insecure_from_str(Some(" 1 "))), + "\" 1 \" (trimmed) must enable loopback HTTP" + ); + assert!(icaptcha_insecure_from_str(Some(" 1 "))); + + // Present non-Unicode: fail closed + same fixed warning path as non-truthy. + assert!(!icaptcha_insecure_from_var(Err( + std::env::VarError::NotUnicode(OsString::from("x")) + ))); + + // https remains trusted regardless of the insecure flag. + assert!(is_https_with("https://icaptcha.gitlawb.com", false)); + assert!(is_https_with("https://icaptcha.gitlawb.com", true)); + } + // ── P1: hostile error bodies must not reach the terminal raw ────────── #[test] @@ -537,13 +724,12 @@ mod tests { #[test] fn rejects_insecure_advertised_url_on_different_port() { - // With GITLAWB_ICAPTCHA_INSECURE=1, two loopback URLs on different + // With insecure mode enabled, two loopback URLs on different // ports must be treated as distinct origins so a hostile node cannot // redirect the bearer key to a different listener on localhost. - let _env = InsecureEnv::new(); - let op = "http://localhost:3000"; - let (url, key_trusted) = resolve_solver_url(Some("http://localhost:9000"), Some(op)); + let (url, key_trusted) = + resolve_solver_url_with(Some("http://localhost:9000"), Some(op), true); assert_eq!( url, op, "must fall back to operator origin, not use different port"