From 3462faa2e1e73e2b1122812654cd491c24e1ad11 Mon Sep 17 00:00:00 2001 From: fullsend-code <289857995+devtest-coder[bot]@users.noreply.github.com> Date: Wed, 17 Jun 2026 20:54:24 +0000 Subject: [PATCH] fix(#4): fail closed when background forward has no listener sandbox_forward() previously returned Ok(()) in background mode even when find_ssh_forward_pid() could not discover the forked SSH process and no local TCP listener was reachable on the forwarded port. This left users with a reported forward that could not be stopped by OpenShell and might not be serving traffic. Changes: - Return an error when PID discovery fails in background mode instead of printing a warning and succeeding. - Add probe_listener() in openshell-core::forward that attempts a TCP connect with retries to confirm the local listener is reachable before writing the PID file. - If the listener probe fails but a PID was found, kill the orphaned SSH process and return an error with a descriptive message. - Add kill_process() helper for best-effort PID cleanup. - Add integration test reproducing the issue with the fake SSH binary. - Update existing sandbox_create_keeps_sandbox_with_forwarding test to expect the forward to fail closed (sandbox is still kept). Note: Rust toolchain was not available in the sandbox environment. Unit and integration tests could not be run locally. Manual verification of the test suite is required. Closes #4 Signed-off-by: fullsend-code <289857995+devtest-coder[bot]@users.noreply.github.com> --- crates/openshell-cli/src/ssh.rs | 34 ++++++++++++----- .../sandbox_create_lifecycle_integration.rs | 35 +++++++++++++++++- crates/openshell-core/src/forward.rs | 37 ++++++++++++++++++- 3 files changed, 93 insertions(+), 13 deletions(-) diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index f5986a1..e4706c2 100644 --- a/crates/openshell-cli/src/ssh.rs +++ b/crates/openshell-cli/src/ssh.rs @@ -9,8 +9,8 @@ use miette::{IntoDiagnostic, Result, WrapErr}; use nix::sys::signal::{SaFlags, SigAction, SigHandler, SigSet, Signal, sigaction}; use openshell_core::ObjectId; use openshell_core::forward::{ - build_proxy_command, find_ssh_forward_pid, format_gateway_url, resolve_ssh_gateway, - shell_escape, validate_ssh_session_response, write_forward_pid, + build_proxy_command, find_ssh_forward_pid, format_gateway_url, kill_process, probe_listener, + resolve_ssh_gateway, shell_escape, validate_ssh_session_response, write_forward_pid, }; use openshell_core::proto::{ CreateSshSessionRequest, GetSandboxRequest, SshRelayTarget, TcpForwardFrame, TcpForwardInit, @@ -369,15 +369,29 @@ pub async fn sandbox_forward( if background { // SSH has forked — find its PID and record it. - if let Some(pid) = find_ssh_forward_pid(&session.sandbox_id, port) { - write_forward_pid(name, port, pid, &session.sandbox_id, &spec.bind_addr)?; - } else { - eprintln!( - "{} Could not discover backgrounded SSH process; \ - forward may be running but is not tracked", - "!".yellow(), - ); + let pid = find_ssh_forward_pid(&session.sandbox_id, port); + + if pid.is_none() { + return Err(miette::miette!( + "Could not discover backgrounded SSH process for port {port}; \ + the forward is not tracked and may not be running" + )); } + + let pid = pid.unwrap(); + + // Probe the local listener to confirm the tunnel is actually serving. + if !probe_listener(&spec.bind_addr, port) { + // Listener never came up — kill the orphaned SSH process and fail. + kill_process(pid); + return Err(miette::miette!( + "SSH process (pid {pid}) started but the local listener on \ + {}:{port} is not reachable; killed the background process", + spec.bind_addr, + )); + } + + write_forward_pid(name, port, pid, &session.sandbox_id, &spec.bind_addr)?; } Ok(()) diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 7061614..c41d4aa 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -1307,7 +1307,11 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { let forward_port = listener.local_addr().unwrap().port(); drop(listener); - run::sandbox_create( + // The fake SSH binary exits immediately without creating a background + // process or local listener, so the forward correctly fails closed. + // The sandbox is still kept (not deleted) because --forward implies + // persistence regardless of the forward outcome. + let _err = run::sandbox_create( &server.endpoint, Some("persistent-forward"), None, @@ -1331,7 +1335,7 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { &tls, ) .await - .expect("sandbox create with forward should succeed"); + .expect_err("sandbox create with forward should fail when listener is unreachable"); assert!(deleted_names(&server).await.is_empty()); } @@ -1430,3 +1434,30 @@ async fn sandbox_create_env_rejects_invalid_key_name() { "error should mention invalid key, got: {msg}" ); } + +#[tokio::test] +async fn sandbox_forward_background_fails_when_ssh_process_untracked_and_listener_missing() { + let server = run_server().await; + let fake_ssh_dir = tempfile::tempdir().unwrap(); + let xdg_dir = tempfile::tempdir().unwrap(); + let _env = test_env(&fake_ssh_dir, &xdg_dir); + let tls = test_tls(&server); + install_fake_ssh(&fake_ssh_dir); + + let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).unwrap(); + let port = listener.local_addr().unwrap().port(); + drop(listener); + let spec = openshell_core::forward::ForwardSpec::new(port); + + let err = run::sandbox_forward(&server.endpoint, "forward-repro", &spec, true, &tls) + .await + .expect_err( + "background forward should fail when ssh exits successfully but no background process or local listener exists", + ); + + let text = format!("{err:?}"); + assert!( + text.contains("Could not discover") || text.contains("listener"), + "unexpected error: {text}", + ); +} diff --git a/crates/openshell-core/src/forward.rs b/crates/openshell-core/src/forward.rs index 6c3da21..1cb93c5 100644 --- a/crates/openshell-core/src/forward.rs +++ b/crates/openshell-core/src/forward.rs @@ -8,9 +8,10 @@ use crate::paths::{create_dir_restricted, xdg_config_dir}; use miette::{IntoDiagnostic, Result, WrapErr}; -use std::net::TcpListener; +use std::net::{TcpListener, TcpStream}; use std::path::PathBuf; use std::process::Command; +use std::time::Duration; // --------------------------------------------------------------------------- // Forward PID file management @@ -425,6 +426,40 @@ fn lsof_listeners(port: u16) -> Option { } } +// --------------------------------------------------------------------------- +// Listener probe +// --------------------------------------------------------------------------- + +/// Maximum number of TCP connect attempts when probing a local listener. +const LISTENER_PROBE_MAX_ATTEMPTS: u32 = 10; + +/// Delay between TCP connect probe attempts. +const LISTENER_PROBE_INTERVAL: Duration = Duration::from_millis(100); + +/// Probe the local listener on `bind_addr:port` by attempting a TCP connect. +/// +/// Retries up to [`LISTENER_PROBE_MAX_ATTEMPTS`] times with a short delay +/// between attempts to allow the SSH tunnel time to set up the listener. +/// Returns `true` if the listener is reachable, `false` otherwise. +pub fn probe_listener(bind_addr: &str, port: u16) -> bool { + for _ in 0..LISTENER_PROBE_MAX_ATTEMPTS { + if TcpStream::connect((bind_addr, port)).is_ok() { + return true; + } + std::thread::sleep(LISTENER_PROBE_INTERVAL); + } + false +} + +/// Kill a process by PID. Best-effort; errors are silently ignored. +pub fn kill_process(pid: u32) { + let _ = Command::new("kill") + .arg(pid.to_string()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status(); +} + // --------------------------------------------------------------------------- // SSH utility functions (shared between CLI and TUI) // ---------------------------------------------------------------------------