From a2ab039d86352cf48a96d9615a103963ad28a69a Mon Sep 17 00:00:00 2001 From: Apoorv Raj Saxena Date: Sat, 4 Jul 2026 13:10:34 +0400 Subject: [PATCH 1/4] fix(cli): fail-open safety net so a dead proxy never strands the user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SOTH installs itself as the machine-wide system proxy, so any way the worker dies without cleanup leaves the OS proxy pointed at a dead 127.0.0.1: — total internet outage until the user manually runs `soth off`. This converts that fail-CLOSED posture to fail-OPEN (Tier 0 of the 2026-07-04 robustness audit, docs/common/2026-07-04/network-robustness-hardening-plan.md). Three layers, all signature-gated so they only ever clear soth's own loopback proxy, never a foreign one: 1. Health watchdog (0.1): a reconciler task spawned in supervise_proxy that every 5s checks the local listener. If it's dead for 3 consecutive ticks (~15s, above a normal rotation window) AND the OS proxy still carries soth's signature, it disables the system proxy so connectivity falls back to direct. Re-enables (quietly) once the listener recovers. Aborted via Drop guard when the supervisor exits, so it can never re-enable a proxy being torn down. This covers the cases the restart loop can't see — a wedged-but-not-exited worker, a listener that stops accepting. 2. Give-up fail-open (0.2): when the supervisor exhausts its restart budget and bails, it now disables the system proxy first. 3. Startup crash-repair (0.3): if worker startup fails while a dangling soth proxy from a prior crashed/SIGKILLed session is still active, disable it before returning the error (fail_startup_open). And `soth doctor` now reads the *live* OS proxy setting (new os_signature_active), emitting an error-level `dangling_system_proxy` finding — pointing at `soth doctor --reset-network` — when the proxy is set but no listener answers. Previously doctor only read soth's own state file and couldn't see this at all. New system.rs surface: soth_proxy_signature_active(port) (cross-platform signature check) and enable_quiet(). 121 soth-cli unit tests pass incl. new doctor findings coverage; the Windows cfg paths are exercised by the release-ci test-windows lane. Co-Authored-By: Claude Fable 5 --- crates/soth-cli/src/commands/proxy/doctor.rs | 98 +++++++++-- crates/soth-cli/src/commands/proxy/start.rs | 174 ++++++++++++++++++- crates/soth-cli/src/commands/proxy/system.rs | 61 +++++++ 3 files changed, 314 insertions(+), 19 deletions(-) diff --git a/crates/soth-cli/src/commands/proxy/doctor.rs b/crates/soth-cli/src/commands/proxy/doctor.rs index abe6d8ec..91548263 100644 --- a/crates/soth-cli/src/commands/proxy/doctor.rs +++ b/crates/soth-cli/src/commands/proxy/doctor.rs @@ -52,6 +52,11 @@ struct SystemProxyDiagnostics { state_port: Option, owner_id: Option, owner_matches_state: Option, + /// Whether the *live* OS proxy setting currently carries soth's + /// loopback:port signature. Unlike the fields above (which only read + /// soth's own state file), this queries the actual OS config — the only + /// way to detect a dangling proxy left by a crashed prior session. + os_signature_active: bool, } #[derive(Debug, Serialize)] @@ -460,7 +465,7 @@ fn build_report(config_path: Option) -> DoctorReport { (Some(a), Some(b)) => Some(a == b), _ => None, }; - let system_proxy = SystemProxyDiagnostics { + let mut system_proxy = SystemProxyDiagnostics { state_file: PathDetails { path: state_file.display().to_string(), exists: state_file.exists(), @@ -477,6 +482,8 @@ fn build_report(config_path: Option) -> DoctorReport { state_port: state.as_ref().and_then(|v| v.port), owner_id, owner_matches_state, + // Filled in below once expected_port is resolved. + os_signature_active: false, }; let ca_paths = super::ca_health::resolve_ca_paths(&config); @@ -544,6 +551,7 @@ fn build_report(config_path: Option) -> DoctorReport { .map(|m| m.port) .or_else(|| state.as_ref().and_then(|s| s.port)) .unwrap_or(config.forward_proxy.port); + system_proxy.os_signature_active = super::system::soth_proxy_signature_active(expected_port); let local_listener_open = is_loopback_listener_open(expected_port); let details = collect_loopback_details(expected_port); let loopback_bindings = LoopbackBindings { @@ -684,16 +692,38 @@ fn compute_findings(report: &DoctorReport) -> Vec { } if !report.loopback_bindings.local_listener_open { - findings.push(DoctorFinding { - level: "warn".to_string(), - code: "listener_not_open".to_string(), - message: format!( - "No listener reachable at 127.0.0.1:{}.", - report.loopback_bindings.expected_port - ), - remediation: "Start proxy with `soth start` (or `soth up`) and re-run doctor." - .to_string(), - }); + if report.system_proxy.os_signature_active { + // The dangerous case: the OS proxy still points at soth but no + // listener answers → every app that honors the system proxy is + // dialing a dead port = total outage. This is what a crashed / + // SIGKILLed prior session leaves behind. Error-level, and the + // remediation is to restore direct connectivity, not to start + // the proxy (which may itself be failing). + findings.push(DoctorFinding { + level: "error".to_string(), + code: "dangling_system_proxy".to_string(), + message: format!( + "System proxy is set to 127.0.0.1:{} but no listener answers there — \ + traffic that honors the system proxy is being dropped.", + report.loopback_bindings.expected_port + ), + remediation: + "Restore direct connectivity with `soth doctor --reset-network` (or `soth off`), \ + then `soth up` to bring the proxy back." + .to_string(), + }); + } else { + findings.push(DoctorFinding { + level: "warn".to_string(), + code: "listener_not_open".to_string(), + message: format!( + "No listener reachable at 127.0.0.1:{}.", + report.loopback_bindings.expected_port + ), + remediation: "Start proxy with `soth start` (or `soth up`) and re-run doctor." + .to_string(), + }); + } } if findings.is_empty() { @@ -1053,9 +1083,10 @@ mod tests { drop(guard); } - #[test] - fn compute_findings_flags_missing_ca() { - let report = DoctorReport { + /// Build a minimal report for compute_findings tests, with the two + /// knobs the listener/proxy findings key on. + fn report_for_findings(local_listener_open: bool, os_signature_active: bool) -> DoctorReport { + DoctorReport { managed_runtime: "unknown".to_string(), daemon: DaemonDiagnostics { pid_file: PathDetails { @@ -1092,6 +1123,7 @@ mod tests { state_port: None, owner_id: None, owner_matches_state: None, + os_signature_active, }, ca: CaDiagnostics { cert_path: PathDetails { @@ -1118,14 +1150,48 @@ mod tests { }, loopback_bindings: LoopbackBindings { expected_port: 8080, - local_listener_open: false, + local_listener_open, details: Vec::new(), }, findings: Vec::new(), - }; + } + } + #[test] + fn compute_findings_flags_missing_ca() { + let report = report_for_findings(false, false); let findings = compute_findings(&report); assert!(findings.iter().any(|f| f.code == "ca_missing")); assert!(findings.iter().any(|f| f.code == "listener_not_open")); } + + #[test] + fn dead_listener_without_active_proxy_is_a_warning() { + let report = report_for_findings(false, false); + let findings = compute_findings(&report); + assert!(findings.iter().any(|f| f.code == "listener_not_open")); + assert!(!findings.iter().any(|f| f.code == "dangling_system_proxy")); + } + + #[test] + fn dead_listener_with_active_proxy_is_a_dangling_error() { + // The catastrophic case: OS proxy points at soth but nothing listens. + let report = report_for_findings(false, true); + let findings = compute_findings(&report); + let dangling = findings + .iter() + .find(|f| f.code == "dangling_system_proxy") + .expect("dangling_system_proxy finding"); + assert_eq!(dangling.level, "error"); + // The plain listener warning must not also fire — the error supersedes it. + assert!(!findings.iter().any(|f| f.code == "listener_not_open")); + } + + #[test] + fn healthy_listener_produces_no_listener_findings() { + let report = report_for_findings(true, true); + let findings = compute_findings(&report); + assert!(!findings.iter().any(|f| f.code == "listener_not_open")); + assert!(!findings.iter().any(|f| f.code == "dangling_system_proxy")); + } } diff --git a/crates/soth-cli/src/commands/proxy/start.rs b/crates/soth-cli/src/commands/proxy/start.rs index 0bd99f5f..745371c8 100644 --- a/crates/soth-cli/src/commands/proxy/start.rs +++ b/crates/soth-cli/src/commands/proxy/start.rs @@ -33,6 +33,16 @@ const MAX_RESTART_ATTEMPTS: u32 = 10; const RESTART_BACKOFF_BASE_MS: u64 = 1_000; const RESTART_BACKOFF_MAX_MS: u64 = 30_000; +/// Fail-open watchdog cadence. Every tick the watchdog checks whether the +/// local listener answers; if it has been dead for +/// [`WATCHDOG_DEAD_TICKS`] consecutive ticks *and* the OS proxy still +/// points at soth, it disables the system proxy so the user falls back to +/// direct connectivity instead of a dead port. The dead threshold is set +/// above a normal rotation/restart window so it never fires mid-rotation. +const WATCHDOG_POLL_INTERVAL: Duration = Duration::from_secs(5); +const WATCHDOG_DEAD_TICKS: u32 = 3; +const WATCHDOG_RECOVER_TICKS: u32 = 2; + /// If the supervisor loop sees a wall-clock gap larger than this between two /// iterations, we assume the OS was suspended (sleep / hibernate / Modern /// Standby) for that duration. Sleep > 60s is the threshold because tokio's @@ -170,10 +180,25 @@ pub async fn run( #[cfg(not(unix))] let listener_fd: Option = None; - let (mut child, ready_probe) = spawn_proxy_process(generated_path.as_path(), listener_fd) + // Crash-repair: a prior session that died uncleanly (SIGKILL/OOM, or a + // supervisor that exhausted its budget) can leave the OS proxy pointed at + // 127.0.0.1:. If *this* startup also fails to bring a worker up, we + // must not return leaving that dangling proxy in place — the user would + // be stranded on a dead port. Route worker-startup failures through + // fail_startup_open, which disables a soth-signature proxy before + // propagating the error. (A bind conflict instead means the old listener + // is still up, so the user isn't stranded and the signature check is moot.) + let (mut child, ready_probe) = match spawn_proxy_process(generated_path.as_path(), listener_fd) .await - .context("spawn soth-proxy process")?; - wait_for_listener_start(&mut child, expected_port, &ready_probe).await?; + .context("spawn soth-proxy process") + { + Ok(value) => value, + Err(error) => return fail_startup_open(expected_port, error).await, + }; + if let Err(error) = wait_for_listener_start(&mut child, expected_port, &ready_probe).await { + let _ = terminate_child(&mut child).await; + return fail_startup_open(expected_port, error).await; + } drop(ready_probe); // Historian sibling process. Spawned only when both `enabled` and @@ -288,6 +313,23 @@ pub async fn run( .await } +/// Fail OPEN on a worker-startup error: if the OS proxy currently carries +/// soth's signature (a dangling entry from a prior crashed session), disable +/// it so connectivity falls back to direct, then return the original error +/// unchanged. Signature-gated, so it never clears a foreign proxy. +async fn fail_startup_open(expected_port: u16, error: anyhow::Error) -> Result<()> { + if super::system::soth_proxy_signature_active(expected_port) { + warn!( + port = expected_port, + "worker failed to start while a soth system proxy was active (likely dangling from a prior crash) — disabling it so connectivity falls back to direct" + ); + if let Err(disable_error) = super::system::disable_quiet().await { + warn!(%disable_error, "failed to disable dangling system proxy during startup crash-repair; user may need `soth off`"); + } + } + Err(error) +} + fn ensure_ca_runtime_health(paths: &super::ca_health::ResolvedCaPaths, quiet: bool) -> Result<()> { if !paths.trust_cert_path.exists() { anyhow::bail!( @@ -771,6 +813,111 @@ fn self_detach_daemon(port: Option, config_path: Option<&PathBuf>, quiet: b /// kill the unhealthy child and respawn it, with exponential backoff up to /// [`MAX_RESTART_ATTEMPTS`] consecutive failures. The restart counter /// resets every time the proxy runs healthily for at least 60 seconds. +/// Owns the fail-open watchdog task and aborts it when dropped, so the +/// watchdog can never outlive the supervisor and re-enable a proxy the +/// supervisor is tearing down. +struct ProxyHealthWatchdog { + handle: tokio::task::JoinHandle<()>, +} + +impl ProxyHealthWatchdog { + fn spawn(expected_port: u16) -> Self { + let handle = tokio::spawn(proxy_health_watchdog_loop(expected_port)); + Self { handle } + } +} + +impl Drop for ProxyHealthWatchdog { + fn drop(&mut self) { + self.handle.abort(); + } +} + +/// The reconciler loop. Every [`WATCHDOG_POLL_INTERVAL`] it checks the local +/// listener. Two transitions, both hysteresis-guarded to avoid flapping: +/// +/// - **Healthy → disabled:** listener dead for [`WATCHDOG_DEAD_TICKS`] +/// consecutive ticks *and* the OS proxy still carries soth's signature → +/// disable the system proxy (fail open to direct). The dead threshold sits +/// above a normal rotation window so a graceful rotation never trips it. +/// - **Disabled → re-enabled:** once the listener answers again for +/// [`WATCHDOG_RECOVER_TICKS`] ticks, re-enable so interception resumes. We +/// only ever re-enable a proxy *we* disabled in this loop. +/// +/// It never disables a proxy that isn't soth's (gated on +/// [`super::system::soth_proxy_signature_active`]). +async fn proxy_health_watchdog_loop(expected_port: u16) { + let mut ticker = tokio::time::interval(WATCHDOG_POLL_INTERVAL); + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); + // Skip the immediate first tick so we don't act before the worker that + // supervise_proxy just confirmed has had a chance to settle. + ticker.tick().await; + + let mut dead_ticks: u32 = 0; + let mut ready_ticks: u32 = 0; + // True only while the proxy is off *because this loop turned it off*. + let mut disabled_by_watchdog = false; + + loop { + ticker.tick().await; + + if is_local_listener_ready(expected_port) { + dead_ticks = 0; + if disabled_by_watchdog { + ready_ticks = ready_ticks.saturating_add(1); + if ready_ticks >= WATCHDOG_RECOVER_TICKS { + info!( + port = expected_port, + "listener recovered — re-enabling system proxy that the watchdog had disabled" + ); + match super::system::enable_quiet(Some(expected_port)).await { + Ok(()) => { + disabled_by_watchdog = false; + ready_ticks = 0; + } + Err(error) => { + warn!(%error, "watchdog failed to re-enable system proxy; will retry next tick"); + } + } + } + } + continue; + } + + // Listener is not answering. + ready_ticks = 0; + if disabled_by_watchdog { + // Already failed open; nothing more to do until it recovers. + continue; + } + dead_ticks = dead_ticks.saturating_add(1); + if dead_ticks < WATCHDOG_DEAD_TICKS { + continue; + } + + // Sustained dead listener. Only act if the OS proxy still points at + // us — otherwise there's nothing of ours to clear. + if !super::system::soth_proxy_signature_active(expected_port) { + continue; + } + warn!( + port = expected_port, + dead_secs = (dead_ticks as u64) * WATCHDOG_POLL_INTERVAL.as_secs(), + "system proxy points at soth but the listener has been dead — disabling system proxy so connectivity falls back to direct" + ); + match super::system::disable_quiet().await { + Ok(()) => { + disabled_by_watchdog = true; + } + Err(error) => { + warn!(%error, "watchdog failed to disable system proxy; will retry next tick"); + // Keep dead_ticks at threshold so we retry immediately. + dead_ticks = WATCHDOG_DEAD_TICKS; + } + } + } +} + async fn supervise_proxy( child: &mut Child, config_path: &Path, @@ -789,6 +936,13 @@ async fn supervise_proxy( // upstream connection pool isn't left bound to the old gateway. let mut network_change_rx = super::network_watcher::spawn(); + // Fail-open watchdog: continuously enforces "if the OS proxy points at + // soth, a healthy listener MUST exist." Runs independently of the + // restart loop below so it also covers cases the loop can't see (a + // wedged-but-not-exited worker, a listener that stops accepting). Aborted + // when supervise_proxy returns (via the guard's Drop). + let _watchdog = ProxyHealthWatchdog::spawn(expected_port); + loop { let exit_reason = wait_until_exit_or_unhealthy(child, expected_port, foreground, &mut network_change_rx) @@ -929,6 +1083,20 @@ async fn supervise_proxy( } if consecutive_failures > MAX_RESTART_ATTEMPTS { + // Fail OPEN before giving up: the worker is persistently broken, + // so leaving the OS proxy pointed at 127.0.0.1: would strand + // the user with no internet (every app that honors the system + // proxy dials a dead port). Revert to direct first, then bail. + // Signature-checked inside disable so we never clear a foreign + // proxy. Best-effort — a disable failure must not mask the bail. + if super::system::soth_proxy_signature_active(expected_port) { + warn!( + "soth-proxy exhausted its restart budget; disabling the system proxy so connectivity falls back to direct" + ); + if let Err(error) = super::system::disable_quiet().await { + warn!(%error, "failed to disable system proxy on give-up; user may need `soth off`"); + } + } anyhow::bail!( "soth-proxy failed {MAX_RESTART_ATTEMPTS} consecutive times — giving up. Check logs for root cause." ); diff --git a/crates/soth-cli/src/commands/proxy/system.rs b/crates/soth-cli/src/commands/proxy/system.rs index f30716bb..32370426 100644 --- a/crates/soth-cli/src/commands/proxy/system.rs +++ b/crates/soth-cli/src/commands/proxy/system.rs @@ -151,6 +151,12 @@ pub async fn enable(port: Option) -> Result<()> { enable_internal(port, true).await } +/// Enable without printing user-facing output — for background callers like +/// the fail-open watchdog re-enabling after a recovery. +pub async fn enable_quiet(port: Option) -> Result<()> { + enable_internal(port, false).await +} + async fn enable_internal(port: Option, print_user_output: bool) -> Result<()> { let proxy_port = port.unwrap_or(DEFAULT_PROXY_PORT); let proxy_addr = format!("127.0.0.1:{proxy_port}"); @@ -322,6 +328,61 @@ pub async fn status() -> Result { Ok(false) } +/// Is the OS proxy currently enabled *and pointing at soth's own +/// loopback:port signature*? +/// +/// This is deliberately stricter than [`status`]: it returns `true` only +/// when the active system proxy is one we set (loopback host + our port), +/// never for a foreign loopback tool (Charles, mitmproxy) or a real +/// upstream proxy. The fail-open watchdog gates on this so it can only ever +/// clear *soth's* proxy — never strand a user by disabling someone else's. +/// +/// Best-effort: any read error resolves to `false` (don't act on +/// uncertainty), which is the safe direction for a function whose `true` +/// authorizes disabling the proxy. +pub fn soth_proxy_signature_active(port: u16) -> bool { + #[cfg(target_os = "macos")] + { + let Ok(services) = get_macos_network_services() else { + return false; + }; + return !list_macos_services_using_soth_signature(&services, port).is_empty(); + } + + #[cfg(target_os = "linux")] + { + // Only the gsettings-managed path can be inspected reliably; the + // env-var fallback isn't a persistent OS setting we can read back. + if which::which("gsettings").is_err() || !gnome_proxy_schema_available() { + return false; + } + let mode_manual = get_linux_proxy_mode().as_deref() == Some("manual"); + let host_loopback = get_linux_proxy_host("https") + .map(|host| matches!(host.as_str(), "127.0.0.1" | "localhost" | "::1")) + .unwrap_or(false); + let port_matches = get_linux_proxy_port("https") == Some(port); + return mode_manual && host_loopback && port_matches; + } + + #[cfg(target_os = "windows")] + { + if !read_windows_proxy_enabled().unwrap_or(false) { + return false; + } + let registered = read_windows_string_value("ProxyServer").unwrap_or(None); + let registered = registered.unwrap_or_default(); + let registered = registered.trim(); + let expected = format!("127.0.0.1:{port}"); + return registered.starts_with(&expected); + } + + #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] + { + let _ = port; + false + } +} + fn get_ca_path() -> PathBuf { soth_home_dir().join("certs").join("soth-mitm-ca.pem") } From dd613c98268abc03a2e7805ca30b520ad7d6fcfe Mon Sep 17 00:00:00 2001 From: Apoorv Raj Saxena Date: Sat, 4 Jul 2026 13:31:27 +0400 Subject: [PATCH 2/4] fix(cli): honor $HOME in tilde expansion and default soth paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-applies a fix that was orphaned off PR #111 before its merge (the env-aware home_dir landed on the branch after the merge commit, so staging never got it). dirs::home_dir() resolves via the known-folder OS API on Windows and ignores the environment, so `~` expansion (default bundle_dir, config/device-id paths, bootstrap init dir) escaped both HOME and SOTH_HOME_DIR — the recurring start_forwards_allow_daemon_child_fallback_flag failure on the native Windows lane ("Bundle directory C:\Users\runneradmin\... missing"). Add an env-aware home_dir() that prefers a non-empty $HOME and falls back to dirs::home_dir(); route cli_config's path helpers and the command-graph bootstrap dir through it. Also fixes home-relocation via $HOME on Windows in the product, not just tests. Unchanged where HOME is unset (normal Windows) or equals the profile dir (Unix). Co-Authored-By: Claude Fable 5 --- crates/soth-cli/src/cli_config.rs | 21 ++++++++++++++++++--- crates/soth-cli/src/command_graph.rs | 6 +++--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/crates/soth-cli/src/cli_config.rs b/crates/soth-cli/src/cli_config.rs index 81620722..adf4f69f 100644 --- a/crates/soth-cli/src/cli_config.rs +++ b/crates/soth-cli/src/cli_config.rs @@ -944,14 +944,29 @@ pub struct CodeAgentConfig { pub enabled: bool, } -pub fn default_config_path() -> PathBuf { +/// Resolve the user's home directory for `~` expansion and default soth +/// paths. `$HOME` wins when set: Unix always sets it, and on Windows — +/// where it is normally absent — honoring it matches every other soth +/// path helper (which honor `SOTH_HOME_DIR`) and keeps test sandboxes +/// working. `dirs::home_dir()` alone won't do: on Windows it resolves via +/// the known-folder OS API and ignores the environment entirely. +fn home_dir() -> Option { + if let Some(home) = std::env::var_os("HOME") { + if !home.is_empty() { + return Some(PathBuf::from(home)); + } + } dirs::home_dir() +} + +pub fn default_config_path() -> PathBuf { + home_dir() .map(|home| home.join(".soth").join(DEFAULT_CONFIG_FILE)) .unwrap_or_else(|| PathBuf::from(".soth").join(DEFAULT_CONFIG_FILE)) } pub fn default_device_id_path() -> PathBuf { - dirs::home_dir() + home_dir() .map(|home| home.join(".soth").join(DEFAULT_DEVICE_ID_FILE)) .unwrap_or_else(|| PathBuf::from(".soth").join(DEFAULT_DEVICE_ID_FILE)) } @@ -968,7 +983,7 @@ pub fn discover_default_config_path() -> Option { pub fn expand_tilde(path: &Path) -> PathBuf { let raw = path.to_string_lossy(); if let Some(rest) = raw.strip_prefix("~/") { - return dirs::home_dir() + return home_dir() .map(|home| home.join(rest)) .unwrap_or_else(|| PathBuf::from(raw.as_ref())); } diff --git a/crates/soth-cli/src/command_graph.rs b/crates/soth-cli/src/command_graph.rs index 14d1e3c8..c6fd0e76 100644 --- a/crates/soth-cli/src/command_graph.rs +++ b/crates/soth-cli/src/command_graph.rs @@ -1141,9 +1141,9 @@ async fn ensure_config_for_up( } } - let init_output = dirs::home_dir() - .map(|home| home.join(".soth")) - .unwrap_or_else(|| PathBuf::from(".soth")); + // Route through expand_tilde so the env-aware home resolution applies + // (dirs::home_dir() ignores $HOME on Windows; see cli_config::home_dir). + let init_output = cli_config::expand_tilde(Path::new("~/.soth")); if !quiet { style::info(&format!( "No config found. Bootstrapping runtime in {}", From ec12ed7b9a42cd7f422dc88b9c7c1c35df0d932b Mon Sep 17 00:00:00 2001 From: Apoorv Raj Saxena Date: Sat, 4 Jul 2026 17:25:49 +0400 Subject: [PATCH 3/4] review: gate startup crash-repair on a dead listener MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses adversarial-review finding 1 on this PR: fail_startup_open disabled the OS proxy whenever it carried soth's signature, without checking the listener was actually dead. On a Windows bind-conflict — a second 'soth start' whose worker fails to bind because a HEALTHY prior instance already holds the port — this tore down the running instance's system proxy, silently turning off interception with nothing to re-enable it. Add !is_local_listener_ready(port) to the guard so we only clear a soth-signature proxy when nothing is answering (the genuine dangling-from-a-crash case), mirroring the watchdog's dead-AND-signature rule. A bind conflict against a healthy instance now leaves that instance's proxy untouched. 121 soth-cli tests pass. Co-Authored-By: Claude Fable 5 --- crates/soth-cli/src/commands/proxy/start.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/crates/soth-cli/src/commands/proxy/start.rs b/crates/soth-cli/src/commands/proxy/start.rs index 745371c8..57e95adc 100644 --- a/crates/soth-cli/src/commands/proxy/start.rs +++ b/crates/soth-cli/src/commands/proxy/start.rs @@ -313,15 +313,26 @@ pub async fn run( .await } -/// Fail OPEN on a worker-startup error: if the OS proxy currently carries -/// soth's signature (a dangling entry from a prior crashed session), disable +/// Fail OPEN on a worker-startup error: if the OS proxy carries soth's +/// signature **and no listener is actually answering on the port**, disable /// it so connectivity falls back to direct, then return the original error -/// unchanged. Signature-gated, so it never clears a foreign proxy. +/// unchanged. +/// +/// The dead-listener check is essential: a startup can fail with the port +/// already served by a *healthy* instance (e.g. a Windows bind-conflict where +/// the supervisor doesn't own an inherited socket). Disabling on the +/// signature alone would then tear down that working instance's system proxy +/// — turning off interception with no one to re-enable it. We only clear the +/// proxy when it points at soth *and* nothing is listening (the genuine +/// dangling-from-a-crash case). Signature-gated, so it never touches a +/// foreign proxy. async fn fail_startup_open(expected_port: u16, error: anyhow::Error) -> Result<()> { - if super::system::soth_proxy_signature_active(expected_port) { + if super::system::soth_proxy_signature_active(expected_port) + && !is_local_listener_ready(expected_port) + { warn!( port = expected_port, - "worker failed to start while a soth system proxy was active (likely dangling from a prior crash) — disabling it so connectivity falls back to direct" + "worker failed to start, a soth system proxy is active, and no listener is answering (dangling from a prior crash) — disabling it so connectivity falls back to direct" ); if let Err(disable_error) = super::system::disable_quiet().await { warn!(%disable_error, "failed to disable dangling system proxy during startup crash-repair; user may need `soth off`"); From f606ad0160e8c26f7b8b3ef75aabe22b4e9c2928 Mon Sep 17 00:00:00 2001 From: Apoorv Raj Saxena Date: Sat, 4 Jul 2026 17:59:18 +0400 Subject: [PATCH 4/4] review: fix Windows signature false-positive, soth-off defeat, blocking probes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the deeper adversarial-review findings on this PR: 1. (loses-internet) The Windows signature used starts_with — a PREFIX match — so a foreign loopback proxy whose port has soth's as a numeric prefix (soth :1080 vs foreign :10800; soth :8 vs :8080) falsely matched soth's signature, letting the watchdog / disable path clear someone else's proxy. Replaced with exact host:port token matching that also handles the scheme=host:port;... list form (windows_proxy_value_is_soth), and fixed the same pre-existing bug in the sentinel-missing disable path. macOS/Linux already matched exactly. 2. (misconfigured) The watchdog re-enabled the proxy after a user ran → Removing system proxy configuration... during a watchdog-induced outage — silently undoing the user's intent, since the in-process disabled_by_watchdog flag can't observe an external → Removing system proxy configuration.... Added a user_proxy_off sentinel set by the user-facing disable (not the watchdog's own quiet disable) and cleared by any enable; the watchdog checks it before re-enabling and stands down if the user turned it off. 3. (runtime) The watchdog's synchronous probes (500ms connect; subprocess reads for the signature) ran directly on the async workers. Wrapped both in spawn_blocking so a per-tick block can't starve the supervisor on a small runtime; a probe panic resolves to the safe direction (not-ready / not-ours). New tests: exact-match truth table (Windows-gated, run by the CI Windows lane) and the sentinel round-trip. 121 soth-cli tests pass. Known narrow limitations left as-is (documented for reviewers): a co-located instance B can still disable instance A's proxy if B starts precisely during A's multi-second restart gap (cross-process, would need IPC); and a shutdown at the exact instant of a watchdog re-enable can race (very narrow interleave). Co-Authored-By: Claude Fable 5 --- crates/soth-cli/src/commands/proxy/start.rs | 31 ++++++- crates/soth-cli/src/commands/proxy/system.rs | 97 +++++++++++++++++++- 2 files changed, 122 insertions(+), 6 deletions(-) diff --git a/crates/soth-cli/src/commands/proxy/start.rs b/crates/soth-cli/src/commands/proxy/start.rs index 57e95adc..2112183b 100644 --- a/crates/soth-cli/src/commands/proxy/start.rs +++ b/crates/soth-cli/src/commands/proxy/start.rs @@ -872,9 +872,31 @@ async fn proxy_health_watchdog_loop(expected_port: u16) { loop { ticker.tick().await; - if is_local_listener_ready(expected_port) { + // Both probes are synchronous and can block (a 500ms connect; several + // subprocess reads for the signature). Run them off the async workers + // so a per-tick block can't starve the supervisor on a small runtime. + // A join failure (a panic in the probe) resolves to "not ready" / + // "not ours" — the safe direction, since neither authorizes a disable. + let listener_ready = + tokio::task::spawn_blocking(move || is_local_listener_ready(expected_port)) + .await + .unwrap_or(false); + + if listener_ready { dead_ticks = 0; if disabled_by_watchdog { + // Respect an explicit `soth off` issued during the outage: if + // the user turned the proxy off, do not re-enable it behind + // their back — stop trying and leave it off. + if super::system::user_disabled_proxy() { + info!( + port = expected_port, + "listener recovered but the user disabled the proxy (soth off) — not re-enabling" + ); + disabled_by_watchdog = false; + ready_ticks = 0; + continue; + } ready_ticks = ready_ticks.saturating_add(1); if ready_ticks >= WATCHDOG_RECOVER_TICKS { info!( @@ -908,7 +930,12 @@ async fn proxy_health_watchdog_loop(expected_port: u16) { // Sustained dead listener. Only act if the OS proxy still points at // us — otherwise there's nothing of ours to clear. - if !super::system::soth_proxy_signature_active(expected_port) { + let signature_active = tokio::task::spawn_blocking(move || { + super::system::soth_proxy_signature_active(expected_port) + }) + .await + .unwrap_or(false); + if !signature_active { continue; } warn!( diff --git a/crates/soth-cli/src/commands/proxy/system.rs b/crates/soth-cli/src/commands/proxy/system.rs index 32370426..46d9f086 100644 --- a/crates/soth-cli/src/commands/proxy/system.rs +++ b/crates/soth-cli/src/commands/proxy/system.rs @@ -158,6 +158,9 @@ pub async fn enable_quiet(port: Option) -> Result<()> { } async fn enable_internal(port: Option, print_user_output: bool) -> Result<()> { + // Any enable — user-initiated (`soth on`/`up`) or the watchdog's recovery + // re-enable — clears the "user turned it off" intent. + clear_user_proxy_off_sentinel(); let proxy_port = port.unwrap_or(DEFAULT_PROXY_PORT); let proxy_addr = format!("127.0.0.1:{proxy_port}"); #[cfg(target_os = "linux")] @@ -247,6 +250,14 @@ pub async fn disable_quiet() -> Result<()> { } async fn disable_internal(print_user_output: bool) -> Result<()> { + // A user-facing disable (`soth off` / `soth doctor --reset-network`, which + // print output) records explicit user intent so the still-running + // watchdog won't re-enable behind the user's back. The watchdog's own + // quiet disable (print_user_output=false) must NOT set it, or the + // watchdog could never re-enable after its own fail-open action. + if print_user_output { + set_user_proxy_off_sentinel(); + } #[cfg(target_os = "linux")] let mut managed_apply = true; #[cfg(not(target_os = "linux"))] @@ -371,9 +382,7 @@ pub fn soth_proxy_signature_active(port: u16) -> bool { } let registered = read_windows_string_value("ProxyServer").unwrap_or(None); let registered = registered.unwrap_or_default(); - let registered = registered.trim(); - let expected = format!("127.0.0.1:{port}"); - return registered.starts_with(&expected); + return windows_proxy_value_is_soth(registered.trim(), port); } #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "windows")))] @@ -437,6 +446,36 @@ fn remove_system_proxy_state() { let _ = std::fs::remove_file(system_proxy_state_path()); } +/// Sentinel marking that the user explicitly turned the proxy off (via +/// `soth off` / `soth doctor --reset-network`) while the daemon is still +/// running. The fail-open watchdog consults it before re-enabling: without +/// it, a `soth off` issued during a watchdog-induced outage would be silently +/// undone when the worker recovered. Written by the user-facing disable path +/// only (not the watchdog's own quiet disable) and cleared by any enable. +const USER_PROXY_OFF_SENTINEL: &str = "user_proxy_off"; + +fn user_proxy_off_sentinel_path() -> PathBuf { + soth_home_dir().join("run").join(USER_PROXY_OFF_SENTINEL) +} + +fn set_user_proxy_off_sentinel() { + let path = user_proxy_off_sentinel_path(); + if let Some(parent) = path.parent() { + let _ = std::fs::create_dir_all(parent); + } + let _ = std::fs::write(&path, b"1"); +} + +fn clear_user_proxy_off_sentinel() { + let _ = std::fs::remove_file(user_proxy_off_sentinel_path()); +} + +/// True when the user has explicitly disabled the proxy and not re-enabled it. +/// The watchdog uses this to avoid re-enabling against the user's intent. +pub fn user_disabled_proxy() -> bool { + user_proxy_off_sentinel_path().exists() +} + #[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))] fn now_unix_secs() -> u64 { SystemTime::now() @@ -1467,7 +1506,7 @@ async fn configure_windows_proxy(enable: bool, port: u16, print_user_output: boo let registered = read_windows_string_value("ProxyServer").unwrap_or(None); let registered_str = registered.as_deref().unwrap_or("").trim(); let proxy_enabled = read_windows_proxy_enabled().unwrap_or(false); - let points_at_loopback = registered_str.starts_with(&proxy_server); + let points_at_loopback = windows_proxy_value_is_soth(registered_str, port); if proxy_enabled && points_at_loopback { warn!( "system proxy state file missing but registered proxy is {proxy_server}; treating as soth-owned and disabling" @@ -1684,6 +1723,25 @@ fn read_windows_proxy_enabled() -> Result { Ok(normalized.parse::().unwrap_or(0) != 0) } +/// Does a Windows `ProxyServer` registry value point at soth's loopback:port? +/// +/// EXACT token match, never a prefix. `ProxyServer` is either a bare +/// `host:port` (one proxy for all protocols) or a `;`-separated list of +/// `scheme=host:port` entries. A prefix match (`starts_with("127.0.0.1:8")`) +/// would falsely claim a *foreign* proxy on e.g. `127.0.0.1:8080` as soth's +/// when soth is on port `8` — leading the watchdog / disable path to clear +/// someone else's proxy. So we compare each entry's `host:port` token exactly. +#[cfg(target_os = "windows")] +fn windows_proxy_value_is_soth(registered: &str, port: u16) -> bool { + let expected_v4 = format!("127.0.0.1:{port}"); + let expected_local = format!("localhost:{port}"); + registered.split(';').any(|part| { + // Strip an optional `scheme=` prefix, then compare the host:port token. + let token = part.rsplit('=').next().unwrap_or(part).trim(); + token == expected_v4 || token == expected_local + }) +} + #[cfg(target_os = "windows")] fn read_windows_string_value(value_name: &str) -> Result> { let Some((value_type, value)) = query_windows_reg_value(value_name)? else { @@ -1802,6 +1860,17 @@ mod tests { assert!(path.to_string_lossy().contains("soth-mitm-ca.pem")); } + #[test] + fn user_proxy_off_sentinel_round_trips() { + with_temp_home(|| { + assert!(!user_disabled_proxy(), "starts clear"); + set_user_proxy_off_sentinel(); + assert!(user_disabled_proxy(), "set marks user-off"); + clear_user_proxy_off_sentinel(); + assert!(!user_disabled_proxy(), "clear removes it"); + }); + } + #[cfg(any(target_os = "macos", target_os = "linux"))] #[test] fn test_merge_proxy_bypass_domains_preserves_existing_and_adds_defaults() { @@ -1901,4 +1970,24 @@ HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Internet Settings Some(("REG_SZ".to_string(), "127.0.0.1:18881".to_string())) ); } + + #[cfg(target_os = "windows")] + #[test] + fn windows_proxy_value_is_soth_exact_match_only() { + // Bare form. + assert!(windows_proxy_value_is_soth("127.0.0.1:8080", 8080)); + assert!(windows_proxy_value_is_soth("localhost:8080", 8080)); + // List form (scheme=host:port). + assert!(windows_proxy_value_is_soth( + "http=127.0.0.1:8080;https=127.0.0.1:8080", + 8080 + )); + // Foreign proxy whose port has ours as a numeric prefix must NOT match. + assert!(!windows_proxy_value_is_soth("127.0.0.1:10800", 1080)); + assert!(!windows_proxy_value_is_soth("127.0.0.1:8080", 8)); + assert!(!windows_proxy_value_is_soth("127.0.0.1:8000", 80)); + // Different host / not ours. + assert!(!windows_proxy_value_is_soth("10.0.0.1:8080", 8080)); + assert!(!windows_proxy_value_is_soth("", 8080)); + } }