test(icaptcha-client): cover the insecure-flag resolver seam

resolve_solver_url passes icaptcha_insecure_enabled() into
resolve_solver_url_with, and that argument had no coverage in the
fail-open direction. Hardcoding it true restores the #227 bug (plain-http
loopback trusted as the solver origin unconditionally in release) while
leaving this crate at 23 passed and gl at 11 passed. Only the fail-closed
direction was caught, and only cross-crate via gl's mockito tests.

Add a guard driving the real-env path under EnvGuard. A loopback operator
URL is load-bearing: it is the only input whose verdict changes with the
flag, so it is the only input that can detect the seam being hardcoded.
One arm advertises a URL on the operator's own origin, because otherwise
nothing drives a non-None advertised argument through the env-reading
wrapper. All five seam mutations go RED: hardcoded true fails the unset
arm, hardcoded false fails the =1 arm, and forwarding None in place of
advertised, dropping operator, or swapping the two each fail as well.

Also convert the six resolver tests that called the env-reading
resolve_solver_url to pass the flag explicitly. EnvGuard writes
GITLAWB_ICAPTCHA_INSECURE under a mutex those tests never took, so
set_var ran concurrent with getenv across parallel test threads. None of
the six uses a loopback host, so their verdicts never depended on the
flag and no assertion could flake; the read was unsound rather than
wrong. Passing false preserves their meaning and leaves one env-touching
test in the binary, which also shrinks the edition-2024 set_var surface.

Test-only. No production lines change.

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<String, std::env::VarError>) -> 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<String> {
 /// 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<OsString>,
     }
 
-    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", "1");
-            InsecureEnv { _lock: lock, prev }
+            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::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,170 @@ 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");
+        }
+    }
+
+    /// Cover the seam in `resolve_solver_url`, which is the only place the
+    /// parsed flag reaches the trust decision it governs.
+    ///
+    /// A loopback operator URL is load-bearing here: it is the only input whose
+    /// verdict changes with the flag, so it is the only input that can detect
+    /// the seam being hardcoded. Every other resolver test deliberately passes
+    /// the flag explicitly (`resolve_solver_url_with(.., false)`) and therefore
+    /// cannot see this. Without this test, replacing the seam's argument with
+    /// `true` — restoring the #227 bug, loopback HTTP trusted unconditionally
+    /// in release — leaves both this crate's suite and `gl`'s green.
+    #[test]
+    fn resolve_solver_url_honors_the_real_env_flag() {
+        let op = "http://127.0.0.1:9000";
+
+        {
+            // Flag on: the loopback operator origin is trusted and keeps the key.
+            let _g = EnvGuard::with_value("1");
+            let (url, key_trusted) = resolve_solver_url(None, Some(op));
+            assert_eq!(url, op, "=1 must honor the loopback operator origin");
+            assert!(key_trusted, "=1 must keep the key with the operator origin");
+        }
+        {
+            // Flag absent: plaintext loopback is not a valid key destination.
+            let _g = EnvGuard::unset();
+            let (url, key_trusted) = resolve_solver_url(None, Some(op));
+            assert_eq!(url, DEFAULT_URL, "unset must reject the loopback operator");
+            assert!(!key_trusted, "unset must withhold the key");
+        }
+        {
+            // The #227 footgun: presence is not enough, the value must be truthy.
+            let _g = EnvGuard::with_value("0");
+            let (url, key_trusted) = resolve_solver_url(None, Some(op));
+            assert_eq!(url, DEFAULT_URL, "=0 must reject the loopback operator");
+            assert!(!key_trusted, "=0 must withhold the key");
+        }
+        {
+            // The advertised argument must reach the resolver, not just the
+            // operator one. This is the only assertion driving a non-None
+            // advertised URL through the env-reading wrapper, so without it a
+            // wrapper that forwards `None` in its place stays green everywhere.
+            let _g = EnvGuard::with_value("1");
+            let advertised = "http://127.0.0.1:9000/v2";
+            let (url, key_trusted) = resolve_solver_url(Some(advertised), Some(op));
+            assert_eq!(
+                url, advertised,
+                "an advertised URL on the operator's own origin must be honored verbatim"
+            );
+            assert!(key_trusted, "the operator origin still owns the key");
+        }
+    }
+
+    /// #227: `=0` / `=false` / empty / unset must NOT enable loopback HTTP;
+    /// only explicit truthy `1` / `true` may. Uses pure helpers, so it covers
+    /// the parsing rules without touching process-global environment; the
+    /// env-reading path itself is covered by the two tests above, which hold
+    /// `ICAPTCHA_ENV_LOCK` for the duration.
+    #[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]
@@ -480,14 +719,15 @@ mod tests {
     #[test]
     fn rejects_non_https_advertised_url() {
         // A plaintext node-advertised URL is ignored; we fall back to default.
-        let (url, key_trusted) = resolve_solver_url(Some("http://evil.example/x"), None);
+        let (url, key_trusted) =
+            resolve_solver_url_with(Some("http://evil.example/x"), None, false);
         assert_eq!(url, DEFAULT_URL);
         assert!(!key_trusted);
     }
 
     #[test]
     fn rejects_https_url_with_non_allowlisted_host() {
-        let (url, key_trusted) = resolve_solver_url(Some("https://evil.example"), None);
+        let (url, key_trusted) = resolve_solver_url_with(Some("https://evil.example"), None, false);
         assert_eq!(url, DEFAULT_URL, "attacker host must not be honored");
         assert!(!key_trusted);
     }
@@ -496,7 +736,8 @@ mod tests {
     fn honors_advertised_default_host_but_withholds_key_without_operator() {
         // Default public host is allowlisted for talking, but with no operator
         // origin configured the API key has no trusted destination.
-        let (url, key_trusted) = resolve_solver_url(Some("https://icaptcha.gitlawb.com/v2"), None);
+        let (url, key_trusted) =
+            resolve_solver_url_with(Some("https://icaptcha.gitlawb.com/v2"), None, false);
         assert_eq!(url, "https://icaptcha.gitlawb.com/v2");
         assert!(!key_trusted);
     }
@@ -506,13 +747,14 @@ mod tests {
         let op = "https://icap.mynode.example";
         // Node advertises the operator's own origin → talk there, key allowed.
         let (url, key_trusted) =
-            resolve_solver_url(Some("https://icap.mynode.example/v1"), Some(op));
+            resolve_solver_url_with(Some("https://icap.mynode.example/v1"), Some(op), false);
         assert_eq!(url, "https://icap.mynode.example/v1");
         assert!(key_trusted);
 
         // Node advertises an attacker origin while an operator is configured →
         // ignore the advert, fall back to the operator origin, key stays with it.
-        let (url, key_trusted) = resolve_solver_url(Some("https://evil.example"), Some(op));
+        let (url, key_trusted) =
+            resolve_solver_url_with(Some("https://evil.example"), Some(op), false);
         assert_eq!(url, op);
         assert!(key_trusted);
     }
@@ -520,9 +762,12 @@ mod tests {
     #[test]
     fn no_advert_uses_operator_or_default() {
         let op = "https://icap.mynode.example";
-        assert_eq!(resolve_solver_url(None, Some(op)), (op.to_string(), true));
         assert_eq!(
-            resolve_solver_url(None, None),
+            resolve_solver_url_with(None, Some(op), false),
+            (op.to_string(), true)
+        );
+        assert_eq!(
+            resolve_solver_url_with(None, None, false),
             (DEFAULT_URL.to_string(), false)
         );
     }
@@ -530,20 +775,20 @@ mod tests {
     #[test]
     fn non_https_operator_is_not_trusted() {
         // A misconfigured plaintext operator URL is not a valid key destination.
-        let (url, key_trusted) = resolve_solver_url(None, Some("http://icap.mynode.example"));
+        let (url, key_trusted) =
+            resolve_solver_url_with(None, Some("http://icap.mynode.example"), false);
         assert_eq!(url, DEFAULT_URL);
         assert!(!key_trusted);
     }
 
     #[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"