From 3346b0f0140c0c0fdb4176b67d187288ef180cbe Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Fri, 24 Jul 2026 03:10:55 +0530 Subject: [PATCH 1/5] fix(icaptcha-client): parse GITLAWB_ICAPTCHA_INSECURE as truthy only Presence checks treated =0/=false/empty as enabled. Only explicit 1/true now enable loopback HTTP trust relaxation (#227). --- crates/icaptcha-client/src/lib.rs | 111 ++++++++++++++++++++++++++++-- 1 file changed, 107 insertions(+), 4 deletions(-) diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index b9a811e9..bac39935 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -88,10 +88,38 @@ fn sanitize_excerpt(s: &str) -> String { out } +/// Whether `GITLAWB_ICAPTCHA_INSECURE` explicitly enables the loopback-HTTP +/// trust relaxation used by integration tests. +/// +/// Only an explicit truthy value (`1` / `true`, case-insensitive) enables it. +/// Presence alone is not enough: `=0`, `=false`, or an empty value leave the +/// relaxation off (and a non-empty non-truthy value is logged), matching +/// operator intent rather than a bare `var_os(...).is_some()` presence check. +fn icaptcha_insecure_enabled() -> bool { + match std::env::var("GITLAWB_ICAPTCHA_INSECURE") { + Ok(v) => { + let t = v.trim(); + if t.eq_ignore_ascii_case("1") || t.eq_ignore_ascii_case("true") { + true + } else { + if !t.is_empty() { + tracing::warn!( + value = %t, + "GITLAWB_ICAPTCHA_INSECURE is set but not truthy (expected 1 or true); \ + loopback HTTP trust relaxation remains disabled" + ); + } + false + } + } + Err(_) => false, + } +} + /// Whether `u` parses as a trusted URL. /// -/// Production trusts only `https`. When `GITLAWB_ICAPTCHA_INSECURE` is set (a -/// runtime escape hatch for integration tests) the function also trusts +/// Production trusts only `https`. When `GITLAWB_ICAPTCHA_INSECURE` is an +/// explicit truthy value (`1` / `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(u: &str) -> bool { @@ -101,7 +129,7 @@ fn is_https(u: &str) -> bool { if parsed.scheme() == "https" { return true; } - if std::env::var_os("GITLAWB_ICAPTCHA_INSECURE").is_some() + if icaptcha_insecure_enabled() && parsed.scheme() == "http" && matches!(parsed.host_str(), Some("127.0.0.1" | "localhost")) { @@ -396,9 +424,20 @@ mod tests { impl InsecureEnv { fn new() -> Self { + Self::with_value("1") + } + + fn with_value(val: &str) -> Self { + let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); + let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); + std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", val); + InsecureEnv { _lock: lock, prev } + } + + fn unset() -> Self { let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); - std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", "1"); + std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); InsecureEnv { _lock: lock, prev } } } @@ -412,6 +451,70 @@ mod tests { } } + /// #227: `=0` / `=false` / empty / unset must NOT enable loopback HTTP; + /// only explicit truthy `1` / `true` may. + #[test] + fn insecure_env_only_truthy_enables_loopback_http() { + { + let _env = InsecureEnv::with_value("0"); + assert!( + !is_https("http://127.0.0.1"), + "=0 must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_enabled()); + } + { + let _env = InsecureEnv::with_value("false"); + assert!( + !is_https("http://127.0.0.1"), + "=false must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_enabled()); + } + { + let _env = InsecureEnv::with_value(""); + assert!( + !is_https("http://127.0.0.1"), + "empty value must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_enabled()); + } + { + let _env = InsecureEnv::unset(); + assert!( + !is_https("http://127.0.0.1"), + "unset must leave loopback HTTP disabled" + ); + assert!(!icaptcha_insecure_enabled()); + } + { + let _env = InsecureEnv::with_value("1"); + assert!( + is_https("http://127.0.0.1"), + "=1 must enable loopback HTTP" + ); + assert!(is_https("http://localhost")); + assert!(icaptcha_insecure_enabled()); + } + { + let _env = InsecureEnv::with_value("true"); + assert!( + is_https("http://127.0.0.1"), + "=true must enable loopback HTTP" + ); + assert!(icaptcha_insecure_enabled()); + } + { + let _env = InsecureEnv::with_value("TRUE"); + assert!( + is_https("http://127.0.0.1"), + "=TRUE (case-insensitive) must enable loopback HTTP" + ); + } + // https remains trusted regardless of the env flag. + assert!(is_https("https://icaptcha.gitlawb.com")); + } + // ── P1: hostile error bodies must not reach the terminal raw ────────── #[test] From 7d218acd082b1e1b455da1b696de99977637733c Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Fri, 24 Jul 2026 19:10:28 +0530 Subject: [PATCH 2/5] fix(icaptcha-client): address #246 review (trim test, fmt, lock) Pin trim with a whitespace truthy case, recover from a poisoned ICAPTCHA_ENV_LOCK, document trim + warn visibility, and cargo fmt. --- crates/icaptcha-client/src/lib.rs | 36 ++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index bac39935..91bf6a3c 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -91,10 +91,12 @@ fn sanitize_excerpt(s: &str) -> String { /// Whether `GITLAWB_ICAPTCHA_INSECURE` explicitly enables the loopback-HTTP /// trust relaxation used by integration tests. /// -/// Only an explicit truthy value (`1` / `true`, case-insensitive) enables it. -/// Presence alone is not enough: `=0`, `=false`, or an empty value leave the -/// relaxation off (and a non-empty non-truthy value is logged), matching -/// operator intent rather than a bare `var_os(...).is_some()` presence check. +/// 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 emits a +/// `tracing::warn!` (visible when the `icaptcha_client` target is at WARN+, +/// e.g. `RUST_LOG=icaptcha_client=warn`), matching operator intent rather +/// than a bare `var_os(...).is_some()` presence check. fn icaptcha_insecure_enabled() -> bool { match std::env::var("GITLAWB_ICAPTCHA_INSECURE") { Ok(v) => { @@ -119,9 +121,9 @@ fn icaptcha_insecure_enabled() -> bool { /// Whether `u` parses as a trusted URL. /// /// Production trusts only `https`. When `GITLAWB_ICAPTCHA_INSECURE` is an -/// explicit truthy value (`1` / `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. +/// explicit truthy value (`1` / `true`, case-insensitive, after trim) 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 { let Ok(parsed) = reqwest::Url::parse(u) else { return false; @@ -428,14 +430,16 @@ mod tests { } fn with_value(val: &str) -> Self { - let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); + // Recover from a poisoned lock so a prior failing assert in this + // suite does not cascade into unrelated tests. + let lock = ICAPTCHA_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", val); InsecureEnv { _lock: lock, prev } } fn unset() -> Self { - let lock = ICAPTCHA_ENV_LOCK.lock().unwrap(); + let lock = ICAPTCHA_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); InsecureEnv { _lock: lock, prev } @@ -489,10 +493,7 @@ mod tests { } { let _env = InsecureEnv::with_value("1"); - assert!( - is_https("http://127.0.0.1"), - "=1 must enable loopback HTTP" - ); + assert!(is_https("http://127.0.0.1"), "=1 must enable loopback HTTP"); assert!(is_https("http://localhost")); assert!(icaptcha_insecure_enabled()); } @@ -511,6 +512,15 @@ mod tests { "=TRUE (case-insensitive) must enable loopback HTTP" ); } + { + // Trim is load-bearing: whitespace around a truthy token must still enable. + let _env = InsecureEnv::with_value(" 1 "); + assert!( + is_https("http://127.0.0.1"), + "\" 1 \" (trimmed) must enable loopback HTTP" + ); + assert!(icaptcha_insecure_enabled()); + } // https remains trusted regardless of the env flag. assert!(is_https("https://icaptcha.gitlawb.com")); } From 8e20002734e485b7b2192affb51750ccaf93ffa0 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Sun, 26 Jul 2026 15:24:57 +0530 Subject: [PATCH 3/5] fix(icaptcha-client): do not log raw ICAPTCHA_INSECURE value The non-truthy warn path previously recorded the full trimmed env value. That is arbitrary deployment input; report only that the setting is invalid (#246 review). --- crates/icaptcha-client/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index 91bf6a3c..26a5677b 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -105,8 +105,9 @@ fn icaptcha_insecure_enabled() -> bool { true } else { if !t.is_empty() { + // Do not log the raw env value: it is arbitrary deployment + // input and may contain secrets or terminal controls (#246). tracing::warn!( - value = %t, "GITLAWB_ICAPTCHA_INSECURE is set but not truthy (expected 1 or true); \ loopback HTTP trust relaxation remains disabled" ); From 16eaa3e068964c2101bb0d83dcd7f8e0cce720fe Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Sun, 26 Jul 2026 15:31:58 +0530 Subject: [PATCH 4/5] fix(icaptcha-client): pure insecure flag helpers for #246 review Warn on present non-Unicode GITLAWB_ICAPTCHA_INSECURE (fail closed). Pass insecure mode into is_https_with / resolve_solver_url_with so tests no longer mutate process-global env (removes InsecureEnv race). --- crates/icaptcha-client/src/lib.rs | 276 +++++++++++++++--------------- 1 file changed, 135 insertions(+), 141 deletions(-) diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index 26a5677b..6cd28cce 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -88,51 +88,77 @@ fn sanitize_excerpt(s: &str) -> String { out } +/// 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. +/// +/// 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 emits a -/// `tracing::warn!` (visible when the `icaptcha_client` target is at WARN+, -/// e.g. `RUST_LOG=icaptcha_client=warn`), matching operator intent rather -/// than a bare `var_os(...).is_some()` presence check. +/// 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 { - match std::env::var("GITLAWB_ICAPTCHA_INSECURE") { - Ok(v) => { - let t = v.trim(); - if t.eq_ignore_ascii_case("1") || t.eq_ignore_ascii_case("true") { - true - } else { - if !t.is_empty() { - // Do not log the raw env value: it is arbitrary deployment - // input and may contain secrets or terminal controls (#246). - tracing::warn!( - "GITLAWB_ICAPTCHA_INSECURE is set but not truthy (expected 1 or true); \ - loopback HTTP trust relaxation remains disabled" - ); - } - false - } - } - Err(_) => false, - } + icaptcha_insecure_from_var(std::env::var("GITLAWB_ICAPTCHA_INSECURE")) } -/// Whether `u` parses as a trusted URL. +/// Whether `u` parses as a trusted URL, given a pre-parsed insecure flag. /// -/// Production trusts only `https`. When `GITLAWB_ICAPTCHA_INSECURE` is an -/// explicit truthy value (`1` / `true`, case-insensitive, after trim) 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 { +/// 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 icaptcha_insecure_enabled() + if insecure && parsed.scheme() == "http" && matches!(parsed.host_str(), Some("127.0.0.1" | "localhost")) { @@ -170,7 +196,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); @@ -188,7 +222,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, @@ -412,118 +446,79 @@ fn interactive_prompt(challenge: &Challenge) -> Option { mod tests { use super::*; 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. - static ICAPTCHA_ENV_LOCK: Mutex<()> = Mutex::new(()); + /// #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"))); - /// Set `GITLAWB_ICAPTCHA_INSECURE` for the test lifetime, restoring any - /// prior value on drop. - struct InsecureEnv { - _lock: MutexGuard<'static, ()>, - prev: Option, - } + 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"))); - impl InsecureEnv { - fn new() -> Self { - Self::with_value("1") - } + 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(""))); - fn with_value(val: &str) -> Self { - // Recover from a poisoned lock so a prior failing assert in this - // suite does not cascade into unrelated tests. - let lock = ICAPTCHA_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); - std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", val); - InsecureEnv { _lock: lock, prev } - } + 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 + ))); - fn unset() -> Self { - let lock = ICAPTCHA_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let prev = std::env::var_os("GITLAWB_ICAPTCHA_INSECURE"); - std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"); - InsecureEnv { _lock: lock, prev } - } - } + 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"))); - impl Drop for InsecureEnv { - fn drop(&mut self) { - match self.prev.take() { - Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", v), - None => std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"), - } - } - } + 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"))); - /// #227: `=0` / `=false` / empty / unset must NOT enable loopback HTTP; - /// only explicit truthy `1` / `true` may. - #[test] - fn insecure_env_only_truthy_enables_loopback_http() { - { - let _env = InsecureEnv::with_value("0"); - assert!( - !is_https("http://127.0.0.1"), - "=0 must leave loopback HTTP disabled" - ); - assert!(!icaptcha_insecure_enabled()); - } - { - let _env = InsecureEnv::with_value("false"); - assert!( - !is_https("http://127.0.0.1"), - "=false must leave loopback HTTP disabled" - ); - assert!(!icaptcha_insecure_enabled()); - } - { - let _env = InsecureEnv::with_value(""); - assert!( - !is_https("http://127.0.0.1"), - "empty value must leave loopback HTTP disabled" - ); - assert!(!icaptcha_insecure_enabled()); - } - { - let _env = InsecureEnv::unset(); - assert!( - !is_https("http://127.0.0.1"), - "unset must leave loopback HTTP disabled" - ); - assert!(!icaptcha_insecure_enabled()); - } - { - let _env = InsecureEnv::with_value("1"); - assert!(is_https("http://127.0.0.1"), "=1 must enable loopback HTTP"); - assert!(is_https("http://localhost")); - assert!(icaptcha_insecure_enabled()); - } - { - let _env = InsecureEnv::with_value("true"); - assert!( - is_https("http://127.0.0.1"), - "=true must enable loopback HTTP" - ); - assert!(icaptcha_insecure_enabled()); - } - { - let _env = InsecureEnv::with_value("TRUE"); - assert!( - is_https("http://127.0.0.1"), - "=TRUE (case-insensitive) must enable loopback HTTP" - ); - } - { - // Trim is load-bearing: whitespace around a truthy token must still enable. - let _env = InsecureEnv::with_value(" 1 "); - assert!( - is_https("http://127.0.0.1"), - "\" 1 \" (trimmed) must enable loopback HTTP" - ); - assert!(icaptcha_insecure_enabled()); - } - // https remains trusted regardless of the env flag. - assert!(is_https("https://icaptcha.gitlawb.com")); + 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 ────────── @@ -651,13 +646,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" From 45f3de13582d519019608684254b0e1311850b1a Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Mon, 27 Jul 2026 19:26:59 +0530 Subject: [PATCH 5/5] fix(icaptcha-client): cover real env read + fix warn spacing Add a serialized test that drives icaptcha_insecure_enabled() against the process env so presence-check / typo / always-true mutations fail. Collapse the mid-sentence space run in the non-truthy warning (#246). --- crates/icaptcha-client/src/lib.rs | 80 ++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index 6cd28cce..a138de70 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -93,7 +93,8 @@ fn sanitize_excerpt(s: &str) -> String { /// 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" + "GITLAWB_ICAPTCHA_INSECURE is set but not truthy (expected 1 or true); \ + loopback HTTP trust relaxation remains disabled" ); } @@ -446,6 +447,83 @@ fn interactive_prompt(challenge: &Challenge) -> Option { mod tests { use super::*; use std::ffi::OsString; + use std::sync::{Mutex, MutexGuard}; + + /// 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(()); + + struct EnvGuard { + _lock: MutexGuard<'static, ()>, + prev: Option, + } + + 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::remove_var("GITLAWB_ICAPTCHA_INSECURE"); + EnvGuard { _lock: lock, prev } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + match self.prev.take() { + Some(v) => std::env::set_var("GITLAWB_ICAPTCHA_INSECURE", v), + None => std::env::remove_var("GITLAWB_ICAPTCHA_INSECURE"), + } + } + } + + /// #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