From 5e5acfb805d14ee179ae32d633439ddce4077015 Mon Sep 17 00:00:00 2001 From: Shiju Date: Fri, 12 Jun 2026 03:11:51 +0530 Subject: [PATCH 1/4] fix(cli): verify forward listener before success Wait for a connectable local forward listener before reporting foreground forwarding success. Fail background forwarding when the forked SSH process cannot be tracked, probe the listener before writing the PID file, and terminate the tracked SSH process if the listener never opens. Document that forwarded URLs are printed only after listener health is proven. Signed-off-by: Shiju --- crates/openshell-cli/src/ssh.rs | 238 +++++++++++++++--- .../sandbox_create_lifecycle_integration.rs | 119 ++++++++- docs/sandboxes/manage-sandboxes.mdx | 2 + 3 files changed, 326 insertions(+), 33 deletions(-) diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index f5986a1d8..bdfadb5df 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, + ForwardSpec, build_proxy_command, find_ssh_forward_pid, format_gateway_url, + resolve_ssh_gateway, shell_escape, validate_ssh_session_response, write_forward_pid, }; use openshell_core::proto::{ CreateSshSessionRequest, GetSandboxRequest, SshRelayTarget, TcpForwardFrame, TcpForwardInit, @@ -25,10 +25,19 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::Duration; use tokio::io::{AsyncReadExt, AsyncWriteExt}; -use tokio::process::Command as TokioCommand; +use tokio::net::TcpStream; +use tokio::process::{Child, Command as TokioCommand}; use tokio_stream::wrappers::ReceiverStream; -const FOREGROUND_FORWARD_STARTUP_GRACE_PERIOD: Duration = Duration::from_secs(2); +/// Time budget for a forward to become healthy after `ssh` starts: covers both +/// backgrounded-PID discovery and listener readiness, in foreground and +/// background. +const FORWARD_STARTUP_GRACE_PERIOD: Duration = Duration::from_secs(2); +/// Delay between listener/PID probes within the grace period. +const FORWARD_LISTENER_PROBE_INTERVAL: Duration = Duration::from_millis(50); +/// Per-attempt connect timeout, so one hung probe cannot consume the whole +/// grace period. +const FORWARD_LISTENER_CONNECT_TIMEOUT: Duration = Duration::from_millis(200); #[derive(Clone, Copy, Debug)] pub enum Editor { @@ -320,7 +329,7 @@ pub async fn sandbox_connect_editor( pub async fn sandbox_forward( server: &str, name: &str, - spec: &openshell_core::forward::ForwardSpec, + spec: &ForwardSpec, background: bool, tls: &TlsOptions, ) -> Result<()> { @@ -349,44 +358,167 @@ pub async fn sandbox_forward( let port = spec.port; - let status = if background { - command.status().await.into_diagnostic()? - } else { - let mut child = command.spawn().into_diagnostic()?; - if let Ok(status) = - tokio::time::timeout(FOREGROUND_FORWARD_STARTUP_GRACE_PERIOD, child.wait()).await + if background { + let status = command.status().await.into_diagnostic()?; + if !status.success() { + return Err(miette::miette!("ssh exited with status {status}")); + } + + // Background forwards must be both reachable and tracked. Without a PID + // file, `openshell forward stop/list` cannot manage the tunnel, so this + // path fails closed instead of printing a URL for an orphaned process. + let pid = wait_for_ssh_forward_pid(&session.sandbox_id, port) + .await + .ok_or_else(|| { + miette::miette!( + "could not discover backgrounded SSH process for sandbox {name} port {port}; \ + refusing to report an untracked forward" + ) + })?; + + if let Err(err) = wait_for_forward_listener(spec, FORWARD_STARTUP_GRACE_PERIOD) + .await + .wrap_err("ssh process started but local forward listener was not reachable") { - status.into_diagnostic()? - } else { - eprintln!("{}", foreground_forward_started_message(name, spec)); - child.wait().await.into_diagnostic()? + terminate_forward_pid(pid); + return Err(err); } + + write_forward_pid(name, port, pid, &session.sandbox_id, &spec.bind_addr)?; + return Ok(()); + } + + let status = { + let mut child = command.spawn().into_diagnostic()?; + if let Err(err) = wait_for_foreground_forward_start(&mut child, spec).await { + let _ = child.kill().await; + return Err(err); + } + eprintln!("{}", foreground_forward_started_message(name, spec)); + child.wait().await.into_diagnostic()? }; if !status.success() { return Err(miette::miette!("ssh exited with status {status}")); } - 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(), - ); + Ok(()) +} + +/// Wait for the local listener, racing the probe against the `ssh` child +/// exiting. An early exit (e.g. `ExitOnForwardFailure=yes` tearing down the +/// session) means forwarding never came up, so it errors instead of waiting +/// out the grace period. +async fn wait_for_foreground_forward_start(child: &mut Child, spec: &ForwardSpec) -> Result<()> { + let listener = wait_for_forward_listener(spec, FORWARD_STARTUP_GRACE_PERIOD); + tokio::pin!(listener); + tokio::select! { + result = &mut listener => result, + status = child.wait() => { + let status = status.into_diagnostic()?; + if status.success() { + Err(miette::miette!( + "ssh exited before local forward listener opened on {}:{}", + forward_probe_host(spec), + spec.port, + )) + } else { + Err(miette::miette!( + "ssh exited with status {status} before local forward listener opened on {}:{}", + forward_probe_host(spec), + spec.port, + )) + } } } +} - Ok(()) +/// Poll for the backgrounded (`ssh -f`) forward's PID. `-f` forks after auth, +/// so the PID is unknown when `command.status()` returns and must be discovered +/// afterward. Returns `None` if it never appears within the grace period. +async fn wait_for_ssh_forward_pid(sandbox_id: &str, port: u16) -> Option { + let deadline = tokio::time::Instant::now() + FORWARD_STARTUP_GRACE_PERIOD; + loop { + if let Some(pid) = find_ssh_forward_pid(sandbox_id, port) { + return Some(pid); + } + if tokio::time::Instant::now() >= deadline { + return None; + } + tokio::time::sleep(FORWARD_LISTENER_PROBE_INTERVAL).await; + } } -fn foreground_forward_started_message( - name: &str, - spec: &openshell_core::forward::ForwardSpec, -) -> String { +/// Poll the local endpoint until a connect succeeds or `wait_for` elapses. The +/// last probe error is folded into the timeout diagnostic, so a failure reports +/// why the listener never opened, not just that it timed out. +async fn wait_for_forward_listener(spec: &ForwardSpec, wait_for: Duration) -> Result<()> { + let deadline = tokio::time::Instant::now() + wait_for; + loop { + let probe_error = match probe_forward_listener(spec).await { + Ok(()) => return Ok(()), + Err(err) => err, + }; + + if tokio::time::Instant::now() >= deadline { + return Err(miette::miette!( + "local forward listener did not open on {}:{} within {}ms: last probe failed with {probe_error}", + forward_probe_host(spec), + spec.port, + wait_for.as_millis(), + )); + } + + tokio::time::sleep(FORWARD_LISTENER_PROBE_INTERVAL).await; + } +} + +/// One bounded TCP connect to the forward endpoint. Returns a `String` error +/// rather than a `miette` diagnostic to stay cheap in the poll loop. The +/// connection only proves reachability and is dropped at once; SSH forwards +/// this throwaway connect to the sandbox-side target. +async fn probe_forward_listener(spec: &ForwardSpec) -> std::result::Result<(), String> { + match tokio::time::timeout( + FORWARD_LISTENER_CONNECT_TIMEOUT, + TcpStream::connect((forward_probe_host(spec), spec.port)), + ) + .await + { + Ok(Ok(stream)) => { + drop(stream); + Ok(()) + } + Ok(Err(err)) => Err(err.to_string()), + Err(_) => Err(format!( + "connect timed out after {}ms", + FORWARD_LISTENER_CONNECT_TIMEOUT.as_millis() + )), + } +} + +/// Resolve the bind address to a connectable host. Wildcard binds (`0.0.0.0`, +/// `::`, empty) are "any-address" listeners, not valid connect targets, so they +/// map to the matching loopback. Specific addresses are probed as-is. +fn forward_probe_host(spec: &ForwardSpec) -> &str { + match spec.bind_addr.as_str() { + "" | "0.0.0.0" => "127.0.0.1", + "::" => "::1", + host => host, + } +} + +/// Best-effort kill of a forward whose listener never came up. Failures are +/// ignored: the process may already be exiting, and the caller surfaces the +/// original listener error regardless. +fn terminate_forward_pid(pid: u32) { + let _ = Command::new("kill") + .arg(pid.to_string()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); +} + +fn foreground_forward_started_message(name: &str, spec: &ForwardSpec) -> String { format!( "{} Forwarding port {} to sandbox {name}\n Access at: {}\n Press Ctrl+C to stop\n {}", "✓".green().bold(), @@ -1590,7 +1722,7 @@ mod tests { #[test] fn foreground_forward_started_message_includes_port_and_stop_hint() { - let spec = openshell_core::forward::ForwardSpec::new(8080); + let spec = ForwardSpec::new(8080); let message = foreground_forward_started_message("demo", &spec); assert!(message.contains("Forwarding port 8080 to sandbox demo")); assert!(message.contains("Access at: http://127.0.0.1:8080/")); @@ -1603,12 +1735,54 @@ mod tests { #[test] fn foreground_forward_started_message_custom_bind_addr() { - let spec = openshell_core::forward::ForwardSpec::parse("0.0.0.0:3000").unwrap(); + let spec = ForwardSpec::parse("0.0.0.0:3000").unwrap(); let message = foreground_forward_started_message("demo", &spec); assert!(message.contains("Forwarding port 3000 to sandbox demo")); assert!(message.contains("Access at: http://localhost:3000/")); } + #[test] + fn forward_probe_host_uses_connectable_loopback_for_wildcard_binds() { + let ipv4 = ForwardSpec::parse("0.0.0.0:3000").unwrap(); + let ipv6 = ForwardSpec::parse(":::3000").unwrap(); + let loopback = ForwardSpec::new(3000); + + assert_eq!(forward_probe_host(&ipv4), "127.0.0.1"); + assert_eq!(forward_probe_host(&ipv6), "::1"); + assert_eq!(forward_probe_host(&loopback), "127.0.0.1"); + } + + #[tokio::test] + async fn wait_for_forward_listener_accepts_ready_listener() { + let listener = tokio::net::TcpListener::bind(("127.0.0.1", 0)) + .await + .unwrap(); + let port = listener.local_addr().unwrap().port(); + let accept = tokio::spawn(async move { + let _ = listener.accept().await; + }); + let spec = ForwardSpec::new(port); + + wait_for_forward_listener(&spec, Duration::from_secs(1)) + .await + .unwrap(); + accept.await.unwrap(); + } + + #[tokio::test] + async fn wait_for_forward_listener_rejects_missing_listener() { + let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).unwrap(); + let port = listener.local_addr().unwrap().port(); + drop(listener); + let spec = ForwardSpec::new(port); + + let err = wait_for_forward_listener(&spec, Duration::from_millis(20)) + .await + .unwrap_err(); + let text = format!("{err:?}"); + assert!(text.contains("local forward listener did not open")); + } + #[test] fn split_sandbox_path_separates_parent_and_basename() { assert_eq!( diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 7061614cb..2b61e9b52 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -715,6 +715,118 @@ fn install_fake_ssh(dir: &TempDir) -> std::path::PathBuf { ssh_path } +fn install_fake_forwarding_ssh(dir: &TempDir) -> std::path::PathBuf { + let ssh_path = dir.path().join("ssh"); + let pid_path = dir.path().join("fake-forward.pid"); + fs::write( + &ssh_path, + r#"#!/bin/sh +set -eu + +forward="" +sandbox_id="" +previous="" + +for arg in "$@"; do + if [ "$previous" = "-L" ]; then + forward="$arg" + previous="" + continue + fi + + if [ "$previous" = "-o" ]; then + case "$arg" in + ProxyCommand=*) + sandbox_id="$(printf '%s\n' "$arg" | sed -n 's/.*--sandbox-id \([^ ]*\).*/\1/p')" + ;; + esac + previous="" + continue + fi + + case "$arg" in + -L|-o) + previous="$arg" + ;; + esac +done + +if [ -z "$forward" ]; then + exit 0 +fi + +first="${forward%%:*}" +rest="${forward#*:}" +case "$first" in + ''|*[!0-9]*) + port="${rest%%:*}" + ;; + *) + port="$first" + ;; +esac + +if [ -z "$port" ] || [ -z "$sandbox_id" ]; then + exit 1 +fi + +nohup python3 -c ' +import signal +import socket +import sys + +port = int(sys.argv[1]) +sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) +sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) +sock.bind(("127.0.0.1", port)) +sock.listen(16) + +def stop(_signum, _frame): + sock.close() + raise SystemExit(0) + +signal.signal(signal.SIGTERM, stop) +signal.signal(signal.SIGINT, stop) +signal.signal(signal.SIGHUP, signal.SIG_IGN) + +while True: + conn, _addr = sock.accept() + conn.close() +' "$port" ssh ssh-proxy --sandbox-id "$sandbox_id" -L "$forward" >/dev/null 2>&1 & +echo $! > '@PID_PATH@' + +exit 0 +"# + .replace("@PID_PATH@", &pid_path.display().to_string()), + ) + .unwrap(); + let mut perms = fs::metadata(&ssh_path).unwrap().permissions(); + perms.set_mode(0o755); + fs::set_permissions(&ssh_path, perms).unwrap(); + + let pgrep_path = dir.path().join("pgrep"); + fs::write( + &pgrep_path, + format!( + r"#!/bin/sh +if [ -s '{}' ]; then + cat '{}' + exit 0 +fi +exit 1 +", + pid_path.display(), + pid_path.display() + ), + ) + .unwrap(); + let mut perms = fs::metadata(&pgrep_path).unwrap().permissions(); + perms.set_mode(0o755); + fs::set_permissions(&pgrep_path, perms).unwrap(); + + ssh_path +} + fn test_env(fake_ssh_dir: &TempDir, xdg_dir: &TempDir) -> EnvVarGuard { test_env_with(fake_ssh_dir, xdg_dir, &[]) } @@ -1302,7 +1414,7 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { 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); + install_fake_forwarding_ssh(&fake_ssh_dir); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let forward_port = listener.local_addr().unwrap().port(); drop(listener); @@ -1334,6 +1446,11 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { .expect("sandbox create with forward should succeed"); assert!(deleted_names(&server).await.is_empty()); + let record = openshell_core::forward::read_forward_pid("persistent-forward", forward_port) + .expect("fake forward should be tracked"); + let _ = std::process::Command::new("kill") + .arg(record.pid.to_string()) + .status(); } #[tokio::test] diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index 44357cb3f..efb2a6b5a 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/docs/sandboxes/manage-sandboxes.mdx @@ -314,6 +314,8 @@ openshell forward start 8000 my-sandbox openshell forward start 8000 my-sandbox -d # run in background ``` +OpenShell prints the local URL only after the forward listener is reachable. Background forwards must be tracked locally so `openshell forward list` and `openshell forward stop` can manage them. + List and stop active forwards: ```shell From 5a677c8c0323758176158faa4027d40d95e64698 Mon Sep 17 00:00:00 2001 From: Shiju Date: Fri, 12 Jun 2026 11:32:50 +0530 Subject: [PATCH 2/4] fix(cli): harden background forward cleanup Signed-off-by: Shiju --- crates/openshell-cli/src/ssh.rs | 24 ++- .../sandbox_create_lifecycle_integration.rs | 175 +++++++++++++++--- 2 files changed, 170 insertions(+), 29 deletions(-) diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index bdfadb5df..c70794347 100644 --- a/crates/openshell-cli/src/ssh.rs +++ b/crates/openshell-cli/src/ssh.rs @@ -7,6 +7,8 @@ use crate::tls::{TlsOptions, grpc_client}; use miette::{IntoDiagnostic, Result, WrapErr}; #[cfg(unix)] use nix::sys::signal::{SaFlags, SigAction, SigHandler, SigSet, Signal, sigaction}; +#[cfg(unix)] +use nix::unistd::Pid; use openshell_core::ObjectId; use openshell_core::forward::{ ForwardSpec, build_proxy_command, find_ssh_forward_pid, format_gateway_url, @@ -507,17 +509,25 @@ fn forward_probe_host(spec: &ForwardSpec) -> &str { } } -/// Best-effort kill of a forward whose listener never came up. Failures are -/// ignored: the process may already be exiting, and the caller surfaces the +/// Best-effort termination of a forward whose listener never came up. Failures +/// are ignored: the process may already be exiting, and the caller surfaces the /// original listener error regardless. +#[cfg(unix)] fn terminate_forward_pid(pid: u32) { - let _ = Command::new("kill") - .arg(pid.to_string()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status(); + let Ok(raw_pid) = i32::try_from(pid) else { + return; + }; + if raw_pid <= 0 { + return; + } + + let _ = nix::sys::signal::kill(Pid::from_raw(raw_pid), Signal::SIGTERM); } +/// Non-Unix builds cannot manage OpenSSH process IDs with Unix signals. +#[cfg(not(unix))] +fn terminate_forward_pid(_pid: u32) {} + fn foreground_forward_started_message(name: &str, spec: &ForwardSpec) -> String { format!( "{} Forwarding port {} to sandbox {name}\n Access at: {}\n Press Ctrl+C to stop\n {}", diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 2b61e9b52..8223ff463 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -706,20 +706,45 @@ async fn run_server() -> TestServer { } } -fn install_fake_ssh(dir: &TempDir) -> std::path::PathBuf { - let ssh_path = dir.path().join("ssh"); - fs::write(&ssh_path, "#!/bin/sh\nexit 0\n").unwrap(); - let mut perms = fs::metadata(&ssh_path).unwrap().permissions(); +fn install_executable_script( + dir: &TempDir, + name: &str, + contents: impl AsRef<[u8]>, +) -> std::path::PathBuf { + let path = dir.path().join(name); + fs::write(&path, contents).unwrap(); + let mut perms = fs::metadata(&path).unwrap().permissions(); perms.set_mode(0o755); - fs::set_permissions(&ssh_path, perms).unwrap(); - ssh_path + fs::set_permissions(&path, perms).unwrap(); + path +} + +fn install_fake_ssh(dir: &TempDir) -> std::path::PathBuf { + install_executable_script(dir, "ssh", "#!/bin/sh\nexit 0\n") +} + +fn install_fake_pgrep_no_match(dir: &TempDir) -> std::path::PathBuf { + install_executable_script(dir, "pgrep", "#!/bin/sh\nexit 1\n") +} + +async fn wait_for_file(path: &std::path::Path, timeout: Duration) -> bool { + let deadline = Instant::now() + timeout; + loop { + if path.exists() { + return true; + } + if Instant::now() >= deadline { + return false; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } } fn install_fake_forwarding_ssh(dir: &TempDir) -> std::path::PathBuf { - let ssh_path = dir.path().join("ssh"); let pid_path = dir.path().join("fake-forward.pid"); - fs::write( - &ssh_path, + let ssh_path = install_executable_script( + dir, + "ssh", r#"#!/bin/sh set -eu @@ -798,15 +823,11 @@ echo $! > '@PID_PATH@' exit 0 "# .replace("@PID_PATH@", &pid_path.display().to_string()), - ) - .unwrap(); - let mut perms = fs::metadata(&ssh_path).unwrap().permissions(); - perms.set_mode(0o755); - fs::set_permissions(&ssh_path, perms).unwrap(); + ); - let pgrep_path = dir.path().join("pgrep"); - fs::write( - &pgrep_path, + install_executable_script( + dir, + "pgrep", format!( r"#!/bin/sh if [ -s '{}' ]; then @@ -818,15 +839,66 @@ exit 1 pid_path.display(), pid_path.display() ), - ) - .unwrap(); - let mut perms = fs::metadata(&pgrep_path).unwrap().permissions(); - perms.set_mode(0o755); - fs::set_permissions(&pgrep_path, perms).unwrap(); + ); ssh_path } +fn install_fake_unreachable_forwarding_ssh(dir: &TempDir) -> std::path::PathBuf { + let pid_path = dir.path().join("fake-forward.pid"); + let terminated_path = dir.path().join("fake-forward.terminated"); + install_executable_script( + dir, + "ssh", + r#"#!/bin/sh +set -eu + +nohup python3 -c ' +import pathlib +import signal +import sys +import time + +terminated_path = pathlib.Path(sys.argv[1]) + +def stop(_signum, _frame): + terminated_path.write_text("terminated") + raise SystemExit(0) + +signal.signal(signal.SIGTERM, stop) +signal.signal(signal.SIGINT, stop) +signal.signal(signal.SIGHUP, signal.SIG_IGN) + +while True: + time.sleep(1) +' '@TERMINATED_PATH@' >/dev/null 2>&1 & +echo $! > '@PID_PATH@' + +exit 0 +"# + .replace("@TERMINATED_PATH@", &terminated_path.display().to_string()) + .replace("@PID_PATH@", &pid_path.display().to_string()), + ); + + install_executable_script( + dir, + "pgrep", + format!( + r"#!/bin/sh +if [ -s '{}' ]; then + cat '{}' + exit 0 +fi +exit 1 +", + pid_path.display(), + pid_path.display() + ), + ); + + terminated_path +} + fn test_env(fake_ssh_dir: &TempDir, xdg_dir: &TempDir) -> EnvVarGuard { test_env_with(fake_ssh_dir, xdg_dir, &[]) } @@ -1453,6 +1525,65 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { .status(); } +#[tokio::test] +async fn sandbox_forward_background_fails_when_pid_is_not_discoverable() { + 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); + install_fake_pgrep_no_match(&fake_ssh_dir); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let forward_port = listener.local_addr().unwrap().port(); + drop(listener); + + let spec = openshell_core::forward::ForwardSpec::new(forward_port); + let err = run::sandbox_forward(&server.endpoint, "untracked-forward", &spec, true, &tls) + .await + .expect_err("background forward should fail without a discoverable SSH PID"); + let msg = format!("{err}"); + assert!( + msg.contains("could not discover backgrounded SSH process"), + "error should explain the missing tracked SSH process, got: {msg}", + ); + assert!( + openshell_core::forward::read_forward_pid("untracked-forward", forward_port).is_none(), + "untracked background forwards must not write a PID file", + ); +} + +#[tokio::test] +async fn sandbox_forward_background_terminates_discovered_pid_when_listener_never_opens() { + 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); + let terminated_path = install_fake_unreachable_forwarding_ssh(&fake_ssh_dir); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let forward_port = listener.local_addr().unwrap().port(); + drop(listener); + + let spec = openshell_core::forward::ForwardSpec::new(forward_port); + let err = run::sandbox_forward(&server.endpoint, "unreachable-forward", &spec, true, &tls) + .await + .expect_err("background forward should fail when the listener never opens"); + let msg = format!("{err}"); + assert!( + msg.contains("ssh process started but local forward listener was not reachable"), + "error should preserve listener startup context, got: {msg}", + ); + assert!( + openshell_core::forward::read_forward_pid("unreachable-forward", forward_port).is_none(), + "unreachable background forwards must not write a PID file", + ); + assert!( + wait_for_file(&terminated_path, Duration::from_secs(2)).await, + "discovered background SSH process should be terminated after listener failure", + ); +} + #[tokio::test] async fn sandbox_create_sends_environment_variables() { let server = run_server().await; From 859e02df50c09039f31e587119ee7b6c683b5895 Mon Sep 17 00:00:00 2001 From: Shiju Date: Fri, 12 Jun 2026 13:04:10 +0530 Subject: [PATCH 3/4] fix(cli): validate background forward cleanup pid Signed-off-by: Shiju --- crates/openshell-cli/src/ssh.rs | 76 +++++++-- .../sandbox_create_lifecycle_integration.rs | 150 +++++++++++++++--- 2 files changed, 194 insertions(+), 32 deletions(-) diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index c70794347..02ab914a5 100644 --- a/crates/openshell-cli/src/ssh.rs +++ b/crates/openshell-cli/src/ssh.rs @@ -10,6 +10,8 @@ use nix::sys::signal::{SaFlags, SigAction, SigHandler, SigSet, Signal, sigaction #[cfg(unix)] use nix::unistd::Pid; use openshell_core::ObjectId; +#[cfg(unix)] +use openshell_core::forward::pid_matches_forward; use openshell_core::forward::{ ForwardSpec, build_proxy_command, find_ssh_forward_pid, format_gateway_url, resolve_ssh_gateway, shell_escape, validate_ssh_session_response, write_forward_pid, @@ -31,11 +33,15 @@ use tokio::net::TcpStream; use tokio::process::{Child, Command as TokioCommand}; use tokio_stream::wrappers::ReceiverStream; -/// Time budget for a forward to become healthy after `ssh` starts: covers both -/// backgrounded-PID discovery and listener readiness, in foreground and -/// background. -const FORWARD_STARTUP_GRACE_PERIOD: Duration = Duration::from_secs(2); -/// Delay between listener/PID probes within the grace period. +/// Time budget for finding the OpenSSH background process after `ssh -f` +/// returns. PID discovery is separate from listener readiness so missing +/// process tracking still fails quickly. +const FORWARD_PID_DISCOVERY_TIMEOUT: Duration = Duration::from_secs(2); +/// Time budget for the local listener to become reachable after `ssh` starts. +/// This is a user-visible readiness deadline for both foreground and background +/// forwards, not a soft cleanup grace period. +const FORWARD_LISTENER_READINESS_TIMEOUT: Duration = Duration::from_secs(10); +/// Delay between listener/PID probes within the configured timeout. const FORWARD_LISTENER_PROBE_INTERVAL: Duration = Duration::from_millis(50); /// Per-attempt connect timeout, so one hung probe cannot consume the whole /// grace period. @@ -378,11 +384,11 @@ pub async fn sandbox_forward( ) })?; - if let Err(err) = wait_for_forward_listener(spec, FORWARD_STARTUP_GRACE_PERIOD) + if let Err(err) = wait_for_forward_listener(spec, FORWARD_LISTENER_READINESS_TIMEOUT) .await .wrap_err("ssh process started but local forward listener was not reachable") { - terminate_forward_pid(pid); + terminate_forward_pid(pid, port, &session.sandbox_id); return Err(err); } @@ -412,7 +418,7 @@ pub async fn sandbox_forward( /// session) means forwarding never came up, so it errors instead of waiting /// out the grace period. async fn wait_for_foreground_forward_start(child: &mut Child, spec: &ForwardSpec) -> Result<()> { - let listener = wait_for_forward_listener(spec, FORWARD_STARTUP_GRACE_PERIOD); + let listener = wait_for_forward_listener(spec, FORWARD_LISTENER_READINESS_TIMEOUT); tokio::pin!(listener); tokio::select! { result = &mut listener => result, @@ -439,7 +445,7 @@ async fn wait_for_foreground_forward_start(child: &mut Child, spec: &ForwardSpec /// so the PID is unknown when `command.status()` returns and must be discovered /// afterward. Returns `None` if it never appears within the grace period. async fn wait_for_ssh_forward_pid(sandbox_id: &str, port: u16) -> Option { - let deadline = tokio::time::Instant::now() + FORWARD_STARTUP_GRACE_PERIOD; + let deadline = tokio::time::Instant::now() + FORWARD_PID_DISCOVERY_TIMEOUT; loop { if let Some(pid) = find_ssh_forward_pid(sandbox_id, port) { return Some(pid); @@ -513,20 +519,26 @@ fn forward_probe_host(spec: &ForwardSpec) -> &str { /// are ignored: the process may already be exiting, and the caller surfaces the /// original listener error regardless. #[cfg(unix)] -fn terminate_forward_pid(pid: u32) { +fn terminate_forward_pid(pid: u32, port: u16, sandbox_id: &str) { let Ok(raw_pid) = i32::try_from(pid) else { return; }; if raw_pid <= 0 { return; } + if !pid_matches_forward(pid, port, Some(sandbox_id)) { + // The PID came from a process-table scan, not a file we own. Re-check + // immediately before signaling so a stale or spoofed match is left + // untouched instead of terminating an unrelated process. + return; + } let _ = nix::sys::signal::kill(Pid::from_raw(raw_pid), Signal::SIGTERM); } /// Non-Unix builds cannot manage OpenSSH process IDs with Unix signals. #[cfg(not(unix))] -fn terminate_forward_pid(_pid: u32) {} +fn terminate_forward_pid(_pid: u32, _port: u16, _sandbox_id: &str) {} fn foreground_forward_started_message(name: &str, spec: &ForwardSpec) -> String { format!( @@ -1793,6 +1805,48 @@ mod tests { assert!(text.contains("local forward listener did not open")); } + #[cfg(unix)] + #[test] + fn terminate_forward_pid_skips_process_that_no_longer_matches_forward() { + let dir = tempfile::tempdir().unwrap(); + let terminated_path = dir.path().join("terminated"); + let mut child = Command::new("python3") + .arg("-c") + .arg( + r#" +import pathlib +import signal +import sys +import time + +terminated_path = pathlib.Path(sys.argv[1]) + +def stop(_signum, _frame): + terminated_path.write_text("terminated") + raise SystemExit(0) + +signal.signal(signal.SIGTERM, stop) + +while True: + time.sleep(1) +"#, + ) + .arg(&terminated_path) + .spawn() + .unwrap(); + std::thread::sleep(Duration::from_millis(100)); + + terminate_forward_pid(child.id(), 43210, "id-spoofed-forward"); + std::thread::sleep(Duration::from_millis(200)); + + assert!( + !terminated_path.exists(), + "mismatched process should not receive SIGTERM" + ); + let _ = child.kill(); + let _ = child.wait(); + } + #[test] fn split_sandbox_path_separates_parent_and_basename() { assert_eq!( diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 8223ff463..ae23fa6aa 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -727,10 +727,27 @@ fn install_fake_pgrep_no_match(dir: &TempDir) -> std::path::PathBuf { install_executable_script(dir, "pgrep", "#!/bin/sh\nexit 1\n") } -async fn wait_for_file(path: &std::path::Path, timeout: Duration) -> bool { +async fn wait_for_process_exit(pid: u32, timeout: Duration) -> bool { let deadline = Instant::now() + timeout; loop { - if path.exists() { + let output = std::process::Command::new("ps") + .arg("-o") + .arg("stat=") + .arg("-p") + .arg(pid.to_string()) + .stderr(std::process::Stdio::null()) + .output(); + let alive = output.is_ok_and(|output| { + if !output.status.success() { + return false; + } + let stat = String::from_utf8_lossy(&output.stdout); + // Linux can leave the orphaned fake forward as a short-lived zombie + // until the container's init process reaps it. A zombie has already + // exited, so it satisfies this cleanup assertion. + !stat.trim_start().starts_with('Z') + }); + if !alive { return true; } if Instant::now() >= deadline { @@ -844,39 +861,87 @@ exit 1 ssh_path } -fn install_fake_unreachable_forwarding_ssh(dir: &TempDir) -> std::path::PathBuf { +struct FakeUnreachableForward { + log_path: std::path::PathBuf, + pid_path: std::path::PathBuf, +} + +fn install_fake_unreachable_forwarding_ssh(dir: &TempDir) -> FakeUnreachableForward { + let log_path = dir.path().join("fake-forward.log"); let pid_path = dir.path().join("fake-forward.pid"); - let terminated_path = dir.path().join("fake-forward.terminated"); + let ready_path = dir.path().join("fake-forward.ready"); install_executable_script( dir, "ssh", r#"#!/bin/sh set -eu -nohup python3 -c ' +forward="" +sandbox_id="" +previous="" + +for arg in "$@"; do + if [ "$previous" = "-L" ]; then + forward="$arg" + previous="" + continue + fi + + if [ "$previous" = "-o" ]; then + case "$arg" in + ProxyCommand=*) + sandbox_id="$(printf '%s\n' "$arg" | sed -n 's/.*--sandbox-id \([^ ]*\).*/\1/p')" + ;; + esac + previous="" + continue + fi + + case "$arg" in + -L|-o) + previous="$arg" + ;; + esac +done + +if [ -z "$forward" ] || [ -z "$sandbox_id" ]; then + exit 1 +fi + +trap '' HUP +python3 -c ' import pathlib import signal import sys import time -terminated_path = pathlib.Path(sys.argv[1]) - -def stop(_signum, _frame): - terminated_path.write_text("terminated") - raise SystemExit(0) +ready_path = pathlib.Path(sys.argv[1]) -signal.signal(signal.SIGTERM, stop) -signal.signal(signal.SIGINT, stop) signal.signal(signal.SIGHUP, signal.SIG_IGN) +ready_path.write_text("ready") + while True: time.sleep(1) -' '@TERMINATED_PATH@' >/dev/null 2>&1 & -echo $! > '@PID_PATH@' +' '@READY_PATH@' ssh ssh-proxy --sandbox-id "$sandbox_id" -L "$forward" >'@LOG_PATH@' 2>&1 & +pid="$!" +i=0 +while [ "$i" -lt 100 ]; do + if [ -e '@READY_PATH@' ]; then + break + fi + i=$((i + 1)) + sleep 0.05 +done +if [ ! -e '@READY_PATH@' ]; then + exit 1 +fi +echo "$pid" > '@PID_PATH@' exit 0 "# - .replace("@TERMINATED_PATH@", &terminated_path.display().to_string()) + .replace("@LOG_PATH@", &log_path.display().to_string()) + .replace("@READY_PATH@", &ready_path.display().to_string()) .replace("@PID_PATH@", &pid_path.display().to_string()), ); @@ -896,7 +961,7 @@ exit 1 ), ); - terminated_path + FakeUnreachableForward { log_path, pid_path } } fn test_env(fake_ssh_dir: &TempDir, xdg_dir: &TempDir) -> EnvVarGuard { @@ -1553,6 +1618,29 @@ async fn sandbox_forward_background_fails_when_pid_is_not_discoverable() { ); } +#[tokio::test] +async fn sandbox_forward_foreground_fails_when_ssh_exits_before_listener_opens() { + 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 = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let forward_port = listener.local_addr().unwrap().port(); + drop(listener); + + let spec = openshell_core::forward::ForwardSpec::new(forward_port); + let err = run::sandbox_forward(&server.endpoint, "foreground-forward", &spec, false, &tls) + .await + .expect_err("foreground forward should fail when ssh exits before listener readiness"); + let msg = format!("{err}"); + assert!( + msg.contains("ssh exited before local forward listener opened"), + "error should explain that ssh exited before listener readiness, got: {msg}", + ); +} + #[tokio::test] async fn sandbox_forward_background_terminates_discovered_pid_when_listener_never_opens() { let server = run_server().await; @@ -1560,7 +1648,7 @@ async fn sandbox_forward_background_terminates_discovered_pid_when_listener_neve let xdg_dir = tempfile::tempdir().unwrap(); let _env = test_env(&fake_ssh_dir, &xdg_dir); let tls = test_tls(&server); - let terminated_path = install_fake_unreachable_forwarding_ssh(&fake_ssh_dir); + let fake_forward = install_fake_unreachable_forwarding_ssh(&fake_ssh_dir); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let forward_port = listener.local_addr().unwrap().port(); drop(listener); @@ -1578,10 +1666,30 @@ async fn sandbox_forward_background_terminates_discovered_pid_when_listener_neve openshell_core::forward::read_forward_pid("unreachable-forward", forward_port).is_none(), "unreachable background forwards must not write a PID file", ); - assert!( - wait_for_file(&terminated_path, Duration::from_secs(2)).await, - "discovered background SSH process should be terminated after listener failure", - ); + let pid = fs::read_to_string(&fake_forward.pid_path) + .expect("fake forward should record a PID") + .trim() + .parse::() + .expect("fake forward PID should be numeric"); + if !wait_for_process_exit(pid, Duration::from_secs(2)).await { + let log = fs::read_to_string(&fake_forward.log_path).unwrap_or_default(); + let command = std::process::Command::new("ps") + .arg("-ww") + .arg("-o") + .arg("command=") + .arg("-p") + .arg(pid.to_string()) + .output() + .ok() + .map(|output| String::from_utf8_lossy(&output.stdout).to_string()) + .unwrap_or_default(); + panic!( + "discovered background SSH process should exit after listener failure cleanup; pid={}, command={}, log={}", + pid, + command.trim(), + log.trim(), + ); + } } #[tokio::test] From e3e1d5523c5d539072ac565312122a6a037e72f0 Mon Sep 17 00:00:00 2001 From: Shiju Date: Mon, 22 Jun 2026 13:34:34 +0530 Subject: [PATCH 4/4] fix(cli): harden forward PID matching and teardown Require background forward cleanup to prove a candidate PID is the OpenShell-generated SSH forwarding process before reporting it active or signaling it. Match the exact -L forward argument and sandbox ID instead of by substring, so a request for port 80 cannot collide with an existing 8080 forward and terminate the wrong process. Reject legacy PID records as non-authoritative and route port-only lookup through validated live forward state. Recover the exact process argv from /proc//cmdline on Linux so a ProxyCommand whose executable path contains whitespace is parsed correctly; other platforms keep the best-effort ps command-line parse. After listener readiness succeeds, terminate the revalidated background ssh process if the PID-file write fails, instead of returning early and leaving a reachable but untracked forward that forward stop/list cannot manage. Cover adversarial matcher cases (port and sandbox-id collisions, command spoofing, remote-command token confusion, whitespace executable paths) and the PID-file write-failure teardown path with focused tests. Signed-off-by: Shiju --- crates/openshell-cli/src/main.rs | 6 +- crates/openshell-cli/src/ssh.rs | 239 ++++--- .../sandbox_create_lifecycle_integration.rs | 275 +++++--- crates/openshell-core/src/forward.rs | 662 ++++++++++++++++-- 4 files changed, 905 insertions(+), 277 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 9b80f1914..a21431e81 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -2089,7 +2089,7 @@ async fn main() -> Result<()> { } else { let name_width = forwards .iter() - .map(|f| f.sandbox.len()) + .map(|f| f.sandbox_name.len()) .max() .unwrap_or(7) .max(7); @@ -2109,14 +2109,14 @@ async fn main() -> Result<()> { bw = bind_width, ); for f in &forwards { - let status = if f.alive { + let status = if f.validated_alive { "running".green().to_string() } else { "dead".red().to_string() }; println!( "{: Result<()> { +async fn wait_for_forward_start(child: &mut Child, spec: &ForwardSpec) -> Result<()> { let listener = wait_for_forward_listener(spec, FORWARD_LISTENER_READINESS_TIMEOUT); tokio::pin!(listener); tokio::select! { @@ -441,22 +431,6 @@ async fn wait_for_foreground_forward_start(child: &mut Child, spec: &ForwardSpec } } -/// Poll for the backgrounded (`ssh -f`) forward's PID. `-f` forks after auth, -/// so the PID is unknown when `command.status()` returns and must be discovered -/// afterward. Returns `None` if it never appears within the grace period. -async fn wait_for_ssh_forward_pid(sandbox_id: &str, port: u16) -> Option { - let deadline = tokio::time::Instant::now() + FORWARD_PID_DISCOVERY_TIMEOUT; - loop { - if let Some(pid) = find_ssh_forward_pid(sandbox_id, port) { - return Some(pid); - } - if tokio::time::Instant::now() >= deadline { - return None; - } - tokio::time::sleep(FORWARD_LISTENER_PROBE_INTERVAL).await; - } -} - /// Poll the local endpoint until a connect succeeds or `wait_for` elapses. The /// last probe error is folded into the timeout diagnostic, so a failure reports /// why the listener never opened, not just that it timed out. @@ -515,30 +489,27 @@ fn forward_probe_host(spec: &ForwardSpec) -> &str { } } -/// Best-effort termination of a forward whose listener never came up. Failures -/// are ignored: the process may already be exiting, and the caller surfaces the -/// original listener error regardless. -#[cfg(unix)] -fn terminate_forward_pid(pid: u32, port: u16, sandbox_id: &str) { - let Ok(raw_pid) = i32::try_from(pid) else { - return; - }; - if raw_pid <= 0 { - return; - } - if !pid_matches_forward(pid, port, Some(sandbox_id)) { - // The PID came from a process-table scan, not a file we own. Re-check - // immediately before signaling so a stale or spoofed match is left - // untouched instead of terminating an unrelated process. - return; - } - - let _ = nix::sys::signal::kill(Pid::from_raw(raw_pid), Signal::SIGTERM); +/// Best-effort cleanup for the SSH child this process spawned. +fn terminate_owned_forward_child(child: &mut Child) { + let _ = child.start_kill(); } -/// Non-Unix builds cannot manage OpenSSH process IDs with Unix signals. -#[cfg(not(unix))] -fn terminate_forward_pid(_pid: u32, _port: u16, _sandbox_id: &str) {} +/// Track a verified background forward, cleaning it up if PID-file persistence fails. +fn track_background_forward_or_cleanup( + name: &str, + port: u16, + pid: u32, + sandbox_id: &str, + bind_addr: &str, + cleanup: impl FnOnce(), +) -> Result<()> { + if let Err(err) = write_forward_pid(name, port, pid, sandbox_id, bind_addr) { + cleanup(); + return Err(err) + .wrap_err("local forward listener was reachable but tracking the SSH process failed"); + } + Ok(()) +} fn foreground_forward_started_message(name: &str, spec: &ForwardSpec) -> String { format!( @@ -1805,46 +1776,84 @@ mod tests { assert!(text.contains("local forward listener did not open")); } - #[cfg(unix)] #[test] - fn terminate_forward_pid_skips_process_that_no_longer_matches_forward() { - let dir = tempfile::tempdir().unwrap(); - let terminated_path = dir.path().join("terminated"); - let mut child = Command::new("python3") - .arg("-c") - .arg( - r#" -import pathlib -import signal -import sys -import time - -terminated_path = pathlib.Path(sys.argv[1]) - -def stop(_signum, _frame): - terminated_path.write_text("terminated") - raise SystemExit(0) - -signal.signal(signal.SIGTERM, stop) - -while True: - time.sleep(1) -"#, - ) - .arg(&terminated_path) - .spawn() - .unwrap(); - std::thread::sleep(Duration::from_millis(100)); + #[allow(unsafe_code)] // Test-only: env vars require unsafe in Rust 2024. + fn track_background_forward_or_cleanup_runs_cleanup_when_pidfile_write_fails() { + let _guard = TEST_ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let tmp = tempfile::tempdir().unwrap(); + // Make forward PID-file writes fail with ENOTDIR after listener readiness. + let blocking_file = tmp.path().join("not-a-dir"); + fs::write(&blocking_file, b"x").unwrap(); + let old_xdg = std::env::var("XDG_CONFIG_HOME").ok(); + unsafe { + std::env::set_var("XDG_CONFIG_HOME", &blocking_file); + } + + let mut cleaned_up = false; + let result = + track_background_forward_or_cleanup("demo", 8080, 4242, "sbx-1", "127.0.0.1", || { + cleaned_up = true; + }); + + unsafe { + match old_xdg { + Some(val) => std::env::set_var("XDG_CONFIG_HOME", val), + None => std::env::remove_var("XDG_CONFIG_HOME"), + } + } + + assert!( + result.is_err(), + "PID-file write failure must surface as an error" + ); + assert!( + cleaned_up, + "the owned SSH child must be cleaned up when tracking fails so no \ + reachable-but-untracked forward is left running" + ); + } - terminate_forward_pid(child.id(), 43210, "id-spoofed-forward"); - std::thread::sleep(Duration::from_millis(200)); + #[test] + #[allow(unsafe_code)] // Test-only: env vars require unsafe in Rust 2024. + fn track_background_forward_or_cleanup_tracks_pid_without_cleanup_on_success() { + let _guard = TEST_ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let tmp = tempfile::tempdir().unwrap(); + let old_xdg = std::env::var("XDG_CONFIG_HOME").ok(); + unsafe { + std::env::set_var("XDG_CONFIG_HOME", tmp.path()); + } + + let mut cleaned_up = false; + let result = + track_background_forward_or_cleanup("demo", 8080, 4242, "sbx-1", "127.0.0.1", || { + cleaned_up = true; + }); + let pid_file_exists = + openshell_core::forward::forward_pid_path("demo", 8080).is_ok_and(|path| path.exists()); + + unsafe { + match old_xdg { + Some(val) => std::env::set_var("XDG_CONFIG_HOME", val), + None => std::env::remove_var("XDG_CONFIG_HOME"), + } + } assert!( - !terminated_path.exists(), - "mismatched process should not receive SIGTERM" + result.is_ok(), + "a writable PID directory must track successfully" + ); + assert!( + pid_file_exists, + "successful tracking must persist a PID file" + ); + assert!( + !cleaned_up, + "successful tracking must not terminate the forward process" ); - let _ = child.kill(); - let _ = child.wait(); } #[test] diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index ae23fa6aa..ef89cdf1e 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -727,6 +727,130 @@ fn install_fake_pgrep_no_match(dir: &TempDir) -> std::path::PathBuf { install_executable_script(dir, "pgrep", "#!/bin/sh\nexit 1\n") } +fn install_fake_forward_process_helper(dir: &TempDir) -> std::path::PathBuf { + // Linux validation reads exact `/proc` argv, so the fake child must look + // like `ssh`, not Python or shell with appended tokens. + let source_path = dir.path().join("fake-forward-process.rs"); + let binary_path = dir.path().join("fake-forward-process"); + fs::write( + &source_path, + r#" +use std::net::TcpListener; +use std::thread; +use std::time::Duration; + +fn main() { + match std::env::var("OPENSHELL_FAKE_FORWARD_MODE").as_deref() { + Ok("listen") => run_listener(), + Ok("sleep") => loop { + thread::sleep(Duration::from_secs(60)); + }, + _ => std::process::exit(2), + } +} + +fn run_listener() { + let port = forward_port().expect("fake forward must receive an SSH -L argument"); + let listener = TcpListener::bind(("127.0.0.1", port)).expect("fake forward must bind"); + for stream in listener.incoming() { + let _ = stream; + } +} + +fn forward_port() -> Option { + let args = std::env::args().skip(1).collect::>(); + let mut index = 0; + while index < args.len() { + let arg = &args[index]; + if arg == "-L" { + return args.get(index + 1).and_then(|value| local_port(value)); + } + if let Some(value) = arg.strip_prefix("-L").filter(|value| !value.is_empty()) { + return local_port(value); + } + index += 1; + } + None +} + +fn local_port(forward: &str) -> Option { + let (first, rest) = forward.split_once(':')?; + if first.bytes().all(|byte| byte.is_ascii_digit()) { + return first.parse().ok(); + } + rest.split_once(':')?.0.parse().ok() +} +"#, + ) + .unwrap(); + let status = std::process::Command::new("rustc") + .arg("--edition=2021") + .arg(&source_path) + .arg("-o") + .arg(&binary_path) + .status() + .unwrap(); + assert!(status.success(), "failed to compile fake forward process"); + binary_path +} + +fn install_fake_ps_for_pid_revalidation( + dir: &TempDir, + pid_path: &std::path::Path, + command_path: &std::path::Path, +) { + install_executable_script( + dir, + "ps", + format!( + r#"#!/bin/sh +set -eu + +command_mode=0 +requested_pid="" +previous="" + +for arg in "$@"; do + if [ "$previous" = "-o" ]; then + if [ "$arg" = "command=" ]; then + command_mode=1 + fi + previous="" + continue + fi + + if [ "$previous" = "-p" ]; then + requested_pid="$arg" + previous="" + continue + fi + + case "$arg" in + -o|-p) + previous="$arg" + ;; + esac +done + +expected_pid="" +if [ -s '{pid_path}' ]; then + expected_pid="$(cat '{pid_path}')" +fi + +if [ "$command_mode" = "1" ] && [ -n "$expected_pid" ] && [ "$requested_pid" = "$expected_pid" ] && [ -s '{command_path}' ]; then + cat '{command_path}' + printf '\n' + exit 0 +fi + +exec /bin/ps "$@" +"#, + pid_path = pid_path.display(), + command_path = command_path.display(), + ), + ); +} + async fn wait_for_process_exit(pid: u32, timeout: Duration) -> bool { let deadline = Instant::now() + timeout; loop { @@ -759,6 +883,8 @@ async fn wait_for_process_exit(pid: u32, timeout: Duration) -> bool { fn install_fake_forwarding_ssh(dir: &TempDir) -> std::path::PathBuf { let pid_path = dir.path().join("fake-forward.pid"); + let command_path = dir.path().join("fake-forward.command"); + let helper_path = install_fake_forward_process_helper(dir); let ssh_path = install_executable_script( dir, "ssh", @@ -767,12 +893,15 @@ set -eu forward="" sandbox_id="" +saw_no_command=0 +last_arg="" previous="" for arg in "$@"; do if [ "$previous" = "-L" ]; then forward="$arg" previous="" + last_arg="$arg" continue fi @@ -783,20 +912,29 @@ for arg in "$@"; do ;; esac previous="" + last_arg="$arg" continue fi case "$arg" in + -N) + saw_no_command=1 + ;; -L|-o) previous="$arg" ;; esac + last_arg="$arg" done if [ -z "$forward" ]; then exit 0 fi +if [ "$saw_no_command" != "1" ] || [ "$last_arg" != "sandbox" ]; then + exit 1 +fi + first="${forward%%:*}" rest="${forward#*:}" case "$first" in @@ -812,51 +950,17 @@ if [ -z "$port" ] || [ -z "$sandbox_id" ]; then exit 1 fi -nohup python3 -c ' -import signal -import socket -import sys - -port = int(sys.argv[1]) -sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) -sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) -sock.bind(("127.0.0.1", port)) -sock.listen(16) - -def stop(_signum, _frame): - sock.close() - raise SystemExit(0) - -signal.signal(signal.SIGTERM, stop) -signal.signal(signal.SIGINT, stop) -signal.signal(signal.SIGHUP, signal.SIG_IGN) - -while True: - conn, _addr = sock.accept() - conn.close() -' "$port" ssh ssh-proxy --sandbox-id "$sandbox_id" -L "$forward" >/dev/null 2>&1 & -echo $! > '@PID_PATH@' - -exit 0 +helper='@HELPER_PATH@' +echo "$$" > '@PID_PATH@' +printf '%s\n' "ssh -N -o ProxyCommand=/tmp/openshell ssh-proxy --gateway https://127.0.0.1:9443 --sandbox-id $sandbox_id --token test-token --gateway-name test-gateway -o ExitOnForwardFailure=yes -L $forward sandbox" > '@COMMAND_PATH@' +exec env OPENSHELL_FAKE_FORWARD_MODE=listen /bin/bash -c 'exec -a ssh "$0" "$@"' "$helper" -N -o "ProxyCommand=/tmp/openshell ssh-proxy --gateway https://127.0.0.1:9443 --sandbox-id $sandbox_id --token test-token --gateway-name test-gateway" -o ExitOnForwardFailure=yes -L "$forward" sandbox "# - .replace("@PID_PATH@", &pid_path.display().to_string()), + .replace("@PID_PATH@", &pid_path.display().to_string()) + .replace("@COMMAND_PATH@", &command_path.display().to_string()) + .replace("@HELPER_PATH@", &helper_path.display().to_string()), ); - install_executable_script( - dir, - "pgrep", - format!( - r"#!/bin/sh -if [ -s '{}' ]; then - cat '{}' - exit 0 -fi -exit 1 -", - pid_path.display(), - pid_path.display() - ), - ); + install_fake_ps_for_pid_revalidation(dir, &pid_path, &command_path); ssh_path } @@ -869,7 +973,7 @@ struct FakeUnreachableForward { fn install_fake_unreachable_forwarding_ssh(dir: &TempDir) -> FakeUnreachableForward { let log_path = dir.path().join("fake-forward.log"); let pid_path = dir.path().join("fake-forward.pid"); - let ready_path = dir.path().join("fake-forward.ready"); + let helper_path = install_fake_forward_process_helper(dir); install_executable_script( dir, "ssh", @@ -878,12 +982,15 @@ set -eu forward="" sandbox_id="" +saw_no_command=0 +last_arg="" previous="" for arg in "$@"; do if [ "$previous" = "-L" ]; then forward="$arg" previous="" + last_arg="$arg" continue fi @@ -894,71 +1001,36 @@ for arg in "$@"; do ;; esac previous="" + last_arg="$arg" continue fi case "$arg" in + -N) + saw_no_command=1 + ;; -L|-o) previous="$arg" ;; esac + last_arg="$arg" done if [ -z "$forward" ] || [ -z "$sandbox_id" ]; then exit 1 fi -trap '' HUP -python3 -c ' -import pathlib -import signal -import sys -import time - -ready_path = pathlib.Path(sys.argv[1]) - -signal.signal(signal.SIGHUP, signal.SIG_IGN) - -ready_path.write_text("ready") - -while True: - time.sleep(1) -' '@READY_PATH@' ssh ssh-proxy --sandbox-id "$sandbox_id" -L "$forward" >'@LOG_PATH@' 2>&1 & -pid="$!" -i=0 -while [ "$i" -lt 100 ]; do - if [ -e '@READY_PATH@' ]; then - break - fi - i=$((i + 1)) - sleep 0.05 -done -if [ ! -e '@READY_PATH@' ]; then +if [ "$saw_no_command" != "1" ] || [ "$last_arg" != "sandbox" ]; then exit 1 fi -echo "$pid" > '@PID_PATH@' -exit 0 +helper='@HELPER_PATH@' +echo "$$" > '@PID_PATH@' +exec env OPENSHELL_FAKE_FORWARD_MODE=sleep /bin/bash -c 'exec -a ssh "$0" "$@"' "$helper" -N -o "ProxyCommand=/tmp/openshell ssh-proxy --gateway https://127.0.0.1:9443 --sandbox-id $sandbox_id --token test-token --gateway-name test-gateway" -o ExitOnForwardFailure=yes -L "$forward" sandbox >'@LOG_PATH@' 2>&1 "# .replace("@LOG_PATH@", &log_path.display().to_string()) - .replace("@READY_PATH@", &ready_path.display().to_string()) - .replace("@PID_PATH@", &pid_path.display().to_string()), - ); - - install_executable_script( - dir, - "pgrep", - format!( - r"#!/bin/sh -if [ -s '{}' ]; then - cat '{}' - exit 0 -fi -exit 1 -", - pid_path.display(), - pid_path.display() - ), + .replace("@PID_PATH@", &pid_path.display().to_string()) + .replace("@HELPER_PATH@", &helper_path.display().to_string()), ); FakeUnreachableForward { log_path, pid_path } @@ -1591,30 +1663,33 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { } #[tokio::test] -async fn sandbox_forward_background_fails_when_pid_is_not_discoverable() { +async fn sandbox_forward_background_tracks_owned_child_when_pid_discovery_fails() { 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); + install_fake_forwarding_ssh(&fake_ssh_dir); install_fake_pgrep_no_match(&fake_ssh_dir); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let forward_port = listener.local_addr().unwrap().port(); drop(listener); let spec = openshell_core::forward::ForwardSpec::new(forward_port); - let err = run::sandbox_forward(&server.endpoint, "untracked-forward", &spec, true, &tls) + run::sandbox_forward(&server.endpoint, "owned-forward", &spec, true, &tls) .await - .expect_err("background forward should fail without a discoverable SSH PID"); - let msg = format!("{err}"); + .expect("background forward should track the owned SSH child without PID discovery"); + let record = openshell_core::forward::read_forward_pid("owned-forward", forward_port) + .expect("owned background forward should write a PID file"); + assert!( - msg.contains("could not discover backgrounded SSH process"), - "error should explain the missing tracked SSH process, got: {msg}", + openshell_core::forward::stop_forward("owned-forward", forward_port) + .expect("tracked fake forward should stop"), + "tracked fake forward should be recognized as alive and stopped", ); assert!( - openshell_core::forward::read_forward_pid("untracked-forward", forward_port).is_none(), - "untracked background forwards must not write a PID file", + wait_for_process_exit(record.pid, Duration::from_secs(2)).await, + "tracked fake forward process should exit after stop" ); } @@ -1642,7 +1717,7 @@ async fn sandbox_forward_foreground_fails_when_ssh_exits_before_listener_opens() } #[tokio::test] -async fn sandbox_forward_background_terminates_discovered_pid_when_listener_never_opens() { +async fn sandbox_forward_background_terminates_owned_child_when_listener_never_opens() { let server = run_server().await; let fake_ssh_dir = tempfile::tempdir().unwrap(); let xdg_dir = tempfile::tempdir().unwrap(); @@ -1684,7 +1759,7 @@ async fn sandbox_forward_background_terminates_discovered_pid_when_listener_neve .map(|output| String::from_utf8_lossy(&output.stdout).to_string()) .unwrap_or_default(); panic!( - "discovered background SSH process should exit after listener failure cleanup; pid={}, command={}, log={}", + "owned background SSH child should exit after listener failure cleanup; pid={}, command={}, log={}", pid, command.trim(), log.trim(), diff --git a/crates/openshell-core/src/forward.rs b/crates/openshell-core/src/forward.rs index 6c3da2128..70ab74edd 100644 --- a/crates/openshell-core/src/forward.rs +++ b/crates/openshell-core/src/forward.rs @@ -49,22 +49,19 @@ pub fn write_forward_pid( /// SSH process. Falls back to `pgrep` since SSH `-f` forks a new process /// whose PID we cannot capture directly. pub fn find_ssh_forward_pid(sandbox_id: &str, port: u16) -> Option { - // Match the ProxyCommand argument which contains the sandbox ID, plus - // the -L port forwarding spec. The ProxyCommand (with --sandbox-id) - // appears before -L in the SSH command line. - let pattern = format!("ssh.*sandbox-id.*{sandbox_id}.*-L.*{port}:127.0.0.1:{port}"); - let output = Command::new("pgrep") - .arg("-f") - .arg(&pattern) - .output() - .ok()?; + // Use pgrep only as a broad process source. The command line still needs a + // second exact check before the PID can be tracked or signaled, otherwise a + // requested port such as 80 can substring-match an existing 8080 forward. + let pattern = "ssh-proxy"; + let output = Command::new("pgrep").arg("-f").arg(pattern).output().ok()?; let stdout = String::from_utf8_lossy(&output.stdout); - // pgrep may return multiple PIDs (e.g., parent + child). Take the last - // one, which is typically the backgrounded SSH process. + // pgrep may return multiple PIDs; scan from newest to oldest and return + // the first one that still passes the exact command-line validation. stdout .lines() .rev() - .find_map(|l| l.trim().parse::().ok()) + .filter_map(|l| l.trim().parse::().ok()) + .find(|pid| pid_matches_openshell_ssh_forward(*pid, port, Some(sandbox_id))) } /// Record read from a forward PID file. @@ -104,51 +101,305 @@ pub fn pid_is_alive(pid: u32) -> bool { .is_ok_and(|s| s.success()) } -/// Validate that a PID belongs to an SSH forward process matching the expected -/// port and optional sandbox ID. -pub fn pid_matches_forward(pid: u32, port: u16, sandbox_id: Option<&str>) -> bool { - let output = match Command::new("ps") +/// Validate that a PID belongs to the expected `OpenShell` SSH forward. +pub fn pid_matches_openshell_ssh_forward(pid: u32, port: u16, sandbox_id: Option<&str>) -> bool { + let Some(argv) = process_forward_match_tokens(pid) else { + return false; + }; + let tokens: Vec<&str> = argv.iter().map(String::as_str).collect(); + args_match_ssh_forward(&tokens, port, sandbox_id) +} + +/// Read a process command line as matcher tokens. +/// +/// Linux exposes exact argv from `/proc//cmdline`; embedded +/// `ProxyCommand=` values are split so the matcher sees a flat token sequence. +#[cfg(target_os = "linux")] +fn process_forward_match_tokens(pid: u32) -> Option> { + let raw = std::fs::read(format!("/proc/{pid}/cmdline")).ok()?; + if raw.is_empty() { + return None; + } + let argv: Vec = raw + .split(|byte| *byte == 0) + .filter(|segment| !segment.is_empty()) + .flat_map(|segment| expand_proxy_command_arg(&String::from_utf8_lossy(segment))) + .collect(); + (!argv.is_empty()).then_some(argv) +} + +/// Non-Linux fallback. `ps` flattens argv, so paths with spaces fail closed. +#[cfg(not(target_os = "linux"))] +fn process_forward_match_tokens(pid: u32) -> Option> { + let output = Command::new("ps") .arg("-ww") .arg("-o") .arg("command=") .arg("-p") .arg(pid.to_string()) .output() - { - Ok(output) if output.status.success() => output, - _ => return false, + .ok()?; + if !output.status.success() { + return None; + } + let cmd = String::from_utf8_lossy(&output.stdout); + let argv: Vec = cmd.split_whitespace().map(str::to_string).collect(); + (!argv.is_empty()).then_some(argv) +} + +/// Expand a `ProxyCommand=` element into matcher tokens. +#[cfg(any(target_os = "linux", test))] +fn expand_proxy_command_arg(arg: &str) -> Vec { + let Some(value) = arg.strip_prefix("ProxyCommand=") else { + return vec![arg.to_string()]; }; + let mut words = split_proxy_command_words(value); + if words.is_empty() { + return vec![arg.to_string()]; + } + words[0] = format!("ProxyCommand={}", words[0]); + words +} - let cmd = String::from_utf8_lossy(&output.stdout); - let forward_spec = format!("{port}:127.0.0.1:{port}"); - if !cmd.contains("ssh") || !cmd.contains("ssh-proxy") || !cmd.contains(&forward_spec) { +/// Split shell words well enough to round-trip values emitted by [`shell_escape`]. +#[cfg(any(target_os = "linux", test))] +fn split_proxy_command_words(input: &str) -> Vec { + let mut words = Vec::new(); + let mut current: Option = None; + let mut chars = input.chars(); + while let Some(c) = chars.next() { + match c { + c if c.is_whitespace() => { + if let Some(word) = current.take() { + words.push(word); + } + } + '\'' => { + let buf = current.get_or_insert_with(String::new); + for q in chars.by_ref() { + if q == '\'' { + break; + } + buf.push(q); + } + } + '"' => { + let buf = current.get_or_insert_with(String::new); + for q in chars.by_ref() { + if q == '"' { + break; + } + buf.push(q); + } + } + other => current.get_or_insert_with(String::new).push(other), + } + } + if let Some(word) = current.take() { + words.push(word); + } + words +} + +#[derive(Debug, Clone, Copy)] +struct ProxyCommandMatch { + outer_ssh_args_start: usize, + sandbox_id_requirement_met: bool, + prefix_has_no_command: bool, +} + +/// Match an `OpenShell` SSH forward by proxy ownership and outer SSH args. +fn args_match_ssh_forward(args: &[&str], port: u16, sandbox_id: Option<&str>) -> bool { + if args.first().and_then(|arg| arg.rsplit('/').next()) != Some("ssh") { return false; } + let Some(proxy) = find_proxy_command_match(args, sandbox_id) else { + return false; + }; + if sandbox_id.is_some() && !proxy.sandbox_id_requirement_met { + return false; + } + outer_ssh_forward_matches( + &args[proxy.outer_ssh_args_start..], + port, + proxy.prefix_has_no_command, + ) +} - sandbox_id.is_none_or(|id| cmd.contains(id)) +/// Test-only wrapper for flat command lines. +#[cfg(test)] +fn command_matches_ssh_forward(command: &str, port: u16, sandbox_id: Option<&str>) -> bool { + let args = command.split_whitespace().collect::>(); + args_match_ssh_forward(&args, port, sandbox_id) } -/// Find the sandbox name that owns a forward on the given port. -/// -/// Scans all PID files in the forwards directory for a file matching -/// `*-.pid`. Ports are unique across sandboxes so at most one -/// match is expected. -pub fn find_forward_by_port(port: u16) -> Result> { - let dir = forward_pid_dir()?; - let Ok(entries) = std::fs::read_dir(&dir) else { - return Ok(None); +fn find_proxy_command_match(args: &[&str], sandbox_id: Option<&str>) -> Option { + for (index, arg) in args.iter().enumerate().skip(1) { + let Some(prefix_has_no_command) = parse_ssh_prefix_before_proxy(args, index) else { + continue; + }; + if !is_ssh_proxy_arg(arg) || !proxy_command_option_present(args, index) { + continue; + } + + let mut current = index + 1; + let mut sandbox_id_requirement_met = sandbox_id.is_none(); + while current < args.len() { + let arg = args[current]; + if is_outer_ssh_option_start(arg) || arg == "sandbox" { + return Some(ProxyCommandMatch { + outer_ssh_args_start: current, + sandbox_id_requirement_met, + prefix_has_no_command, + }); + } + + if let Some(value) = arg.strip_prefix("--sandbox-id=") { + sandbox_id_requirement_met |= sandbox_id == Some(value); + current += 1; + continue; + } + if arg == "--sandbox-id" { + let value = args.get(current + 1)?; + sandbox_id_requirement_met |= sandbox_id == Some(*value); + current += 2; + continue; + } + if proxy_option_takes_value(arg) { + current += 2; + continue; + } + if proxy_option_has_inline_value(arg) { + current += 1; + continue; + } + + return None; + } + } + None +} + +fn is_ssh_proxy_arg(arg: &str) -> bool { + arg == "ssh-proxy" || arg.rsplit('/').next() == Some("ssh-proxy") +} + +fn proxy_command_option_present(args: &[&str], proxy_index: usize) -> bool { + args.iter() + .take(proxy_index + 1) + .any(|arg| arg.starts_with("ProxyCommand=")) +} + +fn proxy_option_takes_value(arg: &str) -> bool { + matches!(arg, "--gateway" | "--token" | "--gateway-name") +} + +fn proxy_option_has_inline_value(arg: &str) -> bool { + ["--gateway=", "--token=", "--gateway-name="] + .iter() + .any(|prefix| arg.starts_with(prefix)) +} + +fn is_outer_ssh_option_start(arg: &str) -> bool { + matches!(arg, "-N" | "-f" | "-o" | "-L" | "-T" | "-tt") + || arg.starts_with("-L") + || arg.starts_with("-o") + || arg.starts_with("-v") +} + +fn parse_ssh_prefix_before_proxy(args: &[&str], proxy_index: usize) -> Option { + let mut current = 1; + let mut saw_no_command = false; + while current < proxy_index { + let arg = args[current]; + if arg == "-o" { + current += 2; + continue; + } + if arg == "-N" { + saw_no_command = true; + current += 1; + continue; + } + if arg.starts_with("-o") || matches!(arg, "-f" | "-T" | "-tt") { + current += 1; + continue; + } + if arg.starts_with("-v") { + current += 1; + continue; + } + return None; + } + Some(saw_no_command) +} + +fn outer_ssh_forward_matches(args: &[&str], port: u16, prefix_has_no_command: bool) -> bool { + let Some(forward_args) = args.strip_suffix(&["sandbox"]) else { + return false; }; - let suffix = format!("-{port}.pid"); - for entry in entries.flatten() { - let file_name = entry.file_name(); - let file_name = file_name.to_string_lossy(); - if let Some(name) = file_name.strip_suffix(&suffix) - && !name.is_empty() - { - return Ok(Some(name.to_string())); + let mut saw_no_command = prefix_has_no_command; + let mut saw_forward = false; + let mut current = 0; + + while current < forward_args.len() { + let arg = forward_args[current]; + match arg { + "-N" => { + saw_no_command = true; + current += 1; + } + "-f" | "-T" | "-tt" => { + current += 1; + } + "-o" => { + if current + 1 >= forward_args.len() { + return false; + } + current += 2; + } + "-L" => { + let Some(candidate) = forward_args.get(current + 1) else { + return false; + }; + saw_forward |= ssh_forward_arg_matches_openshell_loopback_port(candidate, port); + current += 2; + } + _ if arg.starts_with("-L") => { + let Some(candidate) = arg.strip_prefix("-L").filter(|value| !value.is_empty()) + else { + return false; + }; + saw_forward |= ssh_forward_arg_matches_openshell_loopback_port(candidate, port); + current += 1; + } + _ if arg.starts_with("-o") || arg.starts_with("-v") => { + current += 1; + } + _ => return false, } } - Ok(None) + + saw_no_command && saw_forward +} + +fn expected_sandbox_id_from_record(record: &ForwardPidRecord) -> Option<&str> { + // Legacy one-field PID files are cleanup records, not signal authority. + record.sandbox_id.as_deref().filter(|id| !id.is_empty()) +} + +fn ssh_forward_arg_matches_openshell_loopback_port(arg: &str, port: u16) -> bool { + let unbound = format!("{port}:127.0.0.1:{port}"); + let bind_prefixed_suffix = format!(":{unbound}"); + arg == unbound || arg.ends_with(&bind_prefixed_suffix) +} + +/// Find the live, validated forward owner for a local port. +pub fn find_forward_by_port(port: u16) -> Result> { + Ok(list_forwards()? + .into_iter() + .find(|forward| forward.port == port && forward.validated_alive) + .map(|forward| forward.sandbox_name)) } /// Stop a background port forward. @@ -158,9 +409,14 @@ pub fn stop_forward(name: &str, port: u16) -> Result { return Ok(false); }; let pid = record.pid; + let Some(sandbox_id) = expected_sandbox_id_from_record(&record) else { + // Legacy PID records do not prove process ownership. + let _ = std::fs::remove_file(&pid_path); + return Ok(false); + }; if pid_is_alive(pid) { - if !pid_matches_forward(pid, port, record.sandbox_id.as_deref()) { + if !pid_matches_openshell_ssh_forward(pid, port, Some(sandbox_id)) { let _ = std::fs::remove_file(&pid_path); return Ok(false); } @@ -206,10 +462,14 @@ pub fn stop_forwards_for_sandbox(name: &str) -> Result> { /// Information about a tracked forward. pub struct ForwardInfo { - pub sandbox: String, + /// User-facing sandbox name from the PID file path. + pub sandbox_name: String, + /// Local port bound by the SSH forward. pub port: u16, + /// PID recorded for the background SSH process. pub pid: u32, - pub alive: bool, + /// PID is alive and validates as this `OpenShell` forward. + pub validated_alive: bool, /// Bind address (defaults to `127.0.0.1` for old PID files). pub bind_addr: String, } @@ -234,11 +494,17 @@ pub fn list_forwards() -> Result> { && let Ok(port) = stem[dash_pos + 1..].parse::() && let Some(record) = read_forward_pid(&stem[..dash_pos], port) { + // Revalidate ownership so PID reuse does not look like a live forward. + let validated_alive = + expected_sandbox_id_from_record(&record).is_some_and(|sandbox_id| { + pid_is_alive(record.pid) + && pid_matches_openshell_ssh_forward(record.pid, port, Some(sandbox_id)) + }); forwards.push(ForwardInfo { - sandbox: stem[..dash_pos].to_string(), + sandbox_name: stem[..dash_pos].to_string(), port, pid: record.pid, - alive: pid_is_alive(record.pid), + validated_alive, bind_addr: record .bind_addr .unwrap_or_else(|| ForwardSpec::DEFAULT_BIND_ADDR.to_string()), @@ -246,7 +512,11 @@ pub fn list_forwards() -> Result> { } } - forwards.sort_by(|a, b| a.sandbox.cmp(&b.sandbox).then(a.port.cmp(&b.port))); + forwards.sort_by(|a, b| { + a.sandbox_name + .cmp(&b.sandbox_name) + .then(a.port.cmp(&b.port)) + }); Ok(forwards) } @@ -372,13 +642,15 @@ pub fn check_port_available(spec: &ForwardSpec) -> Result<()> { // Port is occupied. Check if it belongs to a tracked openshell forward. if let Ok(forwards) = list_forwards() - && let Some(fwd) = forwards.iter().find(|f| f.port == port && f.alive) + && let Some(fwd) = forwards + .iter() + .find(|f| f.port == port && f.validated_alive) { return Err(miette::miette!( "Port {port} is already forwarded to sandbox '{}'.\n\ Stop it with: openshell forward stop {port} {}", - fwd.sandbox, - fwd.sandbox, + fwd.sandbox_name, + fwd.sandbox_name, )); } @@ -646,7 +918,7 @@ fn validate_field( pub fn build_sandbox_notes(sandbox_name: &str, forwards: &[ForwardInfo]) -> String { let ports: Vec = forwards .iter() - .filter(|f| f.sandbox == sandbox_name && f.alive) + .filter(|f| f.sandbox_name == sandbox_name && f.validated_alive) .map(|f| f.port.to_string()) .collect(); if ports.is_empty() { @@ -663,6 +935,37 @@ pub fn build_sandbox_notes(sandbox_name: &str, forwards: &[ForwardInfo]) -> Stri #[cfg(test)] mod tests { use super::*; + use std::sync::Mutex; + + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + struct EnvVarGuard { + key: &'static str, + previous: Option, + } + + impl EnvVarGuard { + #[allow(unsafe_code)] // Tests serialize process-wide environment changes with ENV_LOCK. + fn set_path(key: &'static str, value: &std::path::Path) -> Self { + let previous = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, previous } + } + } + + #[allow(unsafe_code)] // Tests serialize process-wide environment changes with ENV_LOCK. + impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { + match &self.previous { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } + } #[test] fn resolve_ssh_gateway_keeps_non_loopback() { @@ -930,24 +1233,24 @@ mod tests { fn build_sandbox_notes_with_forwards() { let forwards = vec![ ForwardInfo { - sandbox: "mybox".to_string(), + sandbox_name: "mybox".to_string(), port: 8080, pid: 123, - alive: true, + validated_alive: true, bind_addr: "127.0.0.1".to_string(), }, ForwardInfo { - sandbox: "mybox".to_string(), + sandbox_name: "mybox".to_string(), port: 3000, pid: 456, - alive: true, + validated_alive: true, bind_addr: "127.0.0.1".to_string(), }, ForwardInfo { - sandbox: "other".to_string(), + sandbox_name: "other".to_string(), port: 9090, pid: 789, - alive: true, + validated_alive: true, bind_addr: "0.0.0.0".to_string(), }, ]; @@ -959,10 +1262,10 @@ mod tests { #[test] fn build_sandbox_notes_dead_forwards_excluded() { let forwards = vec![ForwardInfo { - sandbox: "mybox".to_string(), + sandbox_name: "mybox".to_string(), port: 8080, pid: 123, - alive: false, + validated_alive: false, bind_addr: "127.0.0.1".to_string(), }]; assert_eq!(build_sandbox_notes("mybox", &forwards), ""); @@ -1110,6 +1413,247 @@ mod tests { assert_eq!(spec.ssh_forward_arg(), "127.0.0.1:8080:127.0.0.1:8080"); } + #[test] + fn ssh_forward_command_matches_exact_l_argument() { + let command = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L 80:127.0.0.1:80 sandbox"; + let compact = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L80:127.0.0.1:80 sandbox"; + + assert!(command_matches_ssh_forward(command, 80, Some("sbx-1"))); + assert!(command_matches_ssh_forward(compact, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_matches_bind_prefixed_l_argument() { + let command = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L 127.0.0.1:80:127.0.0.1:80 sandbox"; + let compact = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L[::1]:80:127.0.0.1:80 sandbox"; + + assert!(command_matches_ssh_forward(command, 80, Some("sbx-1"))); + assert!(command_matches_ssh_forward(compact, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_rejects_substring_port_collision() { + let command = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L 127.0.0.1:8080:127.0.0.1:8080 sandbox"; + + assert!(!command_matches_ssh_forward(command, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_requires_matching_sandbox_id() { + let command = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-2 -N -L 80:127.0.0.1:80 sandbox"; + let equals = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id=sbx-1 -N -L 80:127.0.0.1:80 sandbox"; + + assert!(!command_matches_ssh_forward(command, 80, Some("sbx-1"))); + assert!(command_matches_ssh_forward(equals, 80, Some("sbx-1"))); + assert!(command_matches_ssh_forward(command, 80, None)); + } + + #[test] + fn ssh_forward_command_rejects_sandbox_id_prefix_collision() { + let split = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-10 -N -L 80:127.0.0.1:80 sandbox"; + let equals = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id=sbx-10 -N -L 80:127.0.0.1:80 sandbox"; + + assert!(!command_matches_ssh_forward(split, 80, Some("sbx-1"))); + assert!(!command_matches_ssh_forward(equals, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_rejects_host_port_ambiguity() { + let wrong_remote_port = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L 80:127.0.0.1:8080 sandbox"; + let wrong_local_port = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L 127.0.0.1:8080:127.0.0.1:80 sandbox"; + let wrong_remote_host = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N -L 80:localhost:80 sandbox"; + + assert!(!command_matches_ssh_forward( + wrong_remote_port, + 80, + Some("sbx-1") + )); + assert!(!command_matches_ssh_forward( + wrong_local_port, + 80, + Some("sbx-1") + )); + assert!(!command_matches_ssh_forward( + wrong_remote_host, + 80, + Some("sbx-1") + )); + } + + #[test] + fn ssh_forward_command_matches_path_basenames_and_bind_variants() { + let command = "/usr/bin/ssh -o ProxyCommand=/usr/local/bin/ssh-proxy --sandbox-id=sbx-1 -N -L localhost:80:127.0.0.1:80 sandbox"; + + assert!(command_matches_ssh_forward(command, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_matches_generated_forward_shape() { + let command = "/usr/bin/ssh -N -o ProxyCommand=/path/openshell ssh-proxy --gateway https://127.0.0.1:9443 --sandbox-id sbx-1 --token tok_123 --gateway-name local -o ExitOnForwardFailure=yes -L 127.0.0.1:80:127.0.0.1:80 -f sandbox"; + + assert!(command_matches_ssh_forward(command, 80, Some("sbx-1"))); + } + + #[test] + fn split_proxy_command_words_round_trips_shell_escape() { + assert_eq!(split_proxy_command_words("a b c"), vec!["a", "b", "c"]); + // A quoted executable path with whitespace stays a single word. + assert_eq!( + split_proxy_command_words("'/Application Support/openshell' ssh-proxy"), + vec!["/Application Support/openshell", "ssh-proxy"] + ); + // The `'\"'\"'` idiom shell_escape emits for an embedded single quote. + assert_eq!(split_proxy_command_words("'it'\"'\"'s'"), vec!["it's"]); + // An explicitly empty argument survives as an empty word. + assert_eq!(split_proxy_command_words("''"), vec![String::new()]); + } + + #[test] + fn expand_proxy_command_arg_splits_value_and_keeps_prefix() { + let exe = "/Application Support/openshell"; + let arg = format!( + "ProxyCommand={} ssh-proxy --sandbox-id sbx-1", + shell_escape(exe) + ); + assert_eq!( + expand_proxy_command_arg(&arg), + vec![ + format!("ProxyCommand={exe}"), + "ssh-proxy".to_string(), + "--sandbox-id".to_string(), + "sbx-1".to_string(), + ] + ); + // Non-proxy arguments pass through untouched. + assert_eq!(expand_proxy_command_arg("-N"), vec!["-N".to_string()]); + } + + #[test] + fn args_match_ssh_forward_handles_expanded_proxy_command_with_whitespace_in_exe_path() { + // Exact argv as recovered from /proc//cmdline: the ProxyCommand + // value is one element whose executable path contains spaces. The flat + // `ps` parse cannot represent this, but the exact-argv path expands the + // ProxyCommand element and matches correctly. + let exe = "/Application Support/openshell"; + let proxy_arg = format!( + "ProxyCommand={} ssh-proxy --gateway https://127.0.0.1:9443 --sandbox-id sbx-1 --token tok_123 --gateway-name local", + shell_escape(exe) + ); + // Mirror process_forward_match_tokens: the ProxyCommand element is expanded. + let mut argv = vec![ + "/usr/bin/ssh".to_string(), + "-N".to_string(), + "-o".to_string(), + ]; + argv.extend(expand_proxy_command_arg(&proxy_arg)); + argv.extend([ + "-o".to_string(), + "ExitOnForwardFailure=yes".to_string(), + "-L".to_string(), + "127.0.0.1:80:127.0.0.1:80".to_string(), + "-f".to_string(), + "sandbox".to_string(), + ]); + let tokens: Vec<&str> = argv.iter().map(String::as_str).collect(); + + assert!(args_match_ssh_forward(&tokens, 80, Some("sbx-1"))); + // Port and sandbox-id discrimination still holds on the exact path. + assert!(!args_match_ssh_forward(&tokens, 8080, Some("sbx-1"))); + assert!(!args_match_ssh_forward(&tokens, 80, Some("sbx-2"))); + } + + #[test] + fn ssh_forward_command_rejects_proxy_name_collisions() { + let wrong_ssh = "notssh ssh-proxy --sandbox-id sbx-1 -N -L 80:127.0.0.1:80 sandbox"; + let wrong_proxy = "ssh -o ProxyCommand=/usr/local/bin/not-ssh-proxy --sandbox-id=sbx-1 -N -L 80:127.0.0.1:80 sandbox"; + + assert!(!command_matches_ssh_forward(wrong_ssh, 80, Some("sbx-1"))); + assert!(!command_matches_ssh_forward(wrong_proxy, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_rejects_non_ssh_process_with_matching_tokens() { + let command = "python3 /tmp/ssh ssh-proxy --sandbox-id sbx-1 -N -L 80:127.0.0.1:80 sandbox"; + + assert!(!command_matches_ssh_forward(command, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_rejects_bare_ssh_proxy_destination() { + let command = "ssh ssh-proxy --sandbox-id sbx-1 -N -L80:127.0.0.1:80 sandbox"; + + assert!(!command_matches_ssh_forward(command, 80, Some("sbx-1"))); + } + + #[test] + fn ssh_forward_command_rejects_remote_command_l_argument() { + let remote_arg = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 -N sandbox -L 80:127.0.0.1:80"; + let missing_no_command = "ssh ssh-proxy --sandbox-id sbx-1 -L 80:127.0.0.1:80 sandbox"; + let remote_command_lookalike = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-1 real-host echo -N -L 80:127.0.0.1:80 sandbox"; + let sandbox_id_in_remote_command = "ssh -o ProxyCommand=openshell ssh-proxy --sandbox-id sbx-2 real-host --sandbox-id sbx-1 -N -L 80:127.0.0.1:80 sandbox"; + + assert!(!command_matches_ssh_forward(remote_arg, 80, Some("sbx-1"))); + assert!(!command_matches_ssh_forward( + missing_no_command, + 80, + Some("sbx-1") + )); + assert!(!command_matches_ssh_forward( + remote_command_lookalike, + 80, + Some("sbx-1") + )); + assert!(!command_matches_ssh_forward( + sandbox_id_in_remote_command, + 80, + Some("sbx-1") + )); + } + + #[test] + fn stop_forward_removes_legacy_pid_file_without_signaling() { + let _lock = ENV_LOCK.lock().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let _xdg_config = EnvVarGuard::set_path("XDG_CONFIG_HOME", config_dir.path()); + + let pid_path = forward_pid_path("sbx-1", 80).unwrap(); + std::fs::create_dir_all(pid_path.parent().unwrap()).unwrap(); + std::fs::write(&pid_path, "12345").unwrap(); + + assert!(!stop_forward("sbx-1", 80).unwrap()); + assert!(!pid_path.exists()); + } + + #[test] + fn list_forwards_marks_legacy_pid_records_not_alive() { + let _lock = ENV_LOCK.lock().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let _xdg_config = EnvVarGuard::set_path("XDG_CONFIG_HOME", config_dir.path()); + + let pid_path = forward_pid_path("sbx-1", 80).unwrap(); + std::fs::create_dir_all(pid_path.parent().unwrap()).unwrap(); + std::fs::write(&pid_path, "12345").unwrap(); + + let forwards = list_forwards().unwrap(); + assert_eq!(forwards.len(), 1); + assert_eq!(forwards[0].sandbox_name, "sbx-1"); + assert_eq!(forwards[0].port, 80); + assert!(!forwards[0].validated_alive); + } + + #[test] + fn find_forward_by_port_ignores_legacy_pid_records() { + let _lock = ENV_LOCK.lock().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let _xdg_config = EnvVarGuard::set_path("XDG_CONFIG_HOME", config_dir.path()); + + let pid_path = forward_pid_path("old", 80).unwrap(); + std::fs::create_dir_all(pid_path.parent().unwrap()).unwrap(); + std::fs::write(&pid_path, std::process::id().to_string()).unwrap(); + + assert_eq!(find_forward_by_port(80).unwrap(), None); + } + #[test] fn forward_spec_access_url() { let spec = ForwardSpec::parse("8080").unwrap();