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<()> { @@ -336,57 +344,174 @@ pub async fn sandbox_forward( .arg("-L") .arg(spec.ssh_forward_arg()); + command.arg("sandbox"); + if background { - // SSH -f: fork to background after authentication. - command.arg("-f"); + command + .kill_on_drop(false) + .stdin(Stdio::null()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()); + } else { + command + .stdin(Stdio::inherit()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()); } - command - .arg("sandbox") - .stdin(Stdio::inherit()) - .stdout(Stdio::inherit()) - .stderr(Stdio::inherit()); - let port = spec.port; - let status = if background { - command.status().await.into_diagnostic()? - } else { + if background { let mut child = command.spawn().into_diagnostic()?; - if let Ok(status) = - tokio::time::timeout(FOREGROUND_FORWARD_STARTUP_GRACE_PERIOD, child.wait()).await + let pid = child.id().ok_or_else(|| { + miette::miette!("ssh process did not expose a PID for background tracking") + })?; + + if let Err(err) = wait_for_forward_start(&mut child, spec) + .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_owned_forward_child(&mut child); + return Err(err); + } + + track_background_forward_or_cleanup( + name, + port, + pid, + &session.sandbox_id, + &spec.bind_addr, + || terminate_owned_forward_child(&mut child), + )?; + return Ok(()); + } + + let status = { + let mut child = command.spawn().into_diagnostic()?; + if let Err(err) = wait_for_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_forward_start(child: &mut Child, spec: &ForwardSpec) -> Result<()> { + let listener = wait_for_forward_listener(spec, FORWARD_LISTENER_READINESS_TIMEOUT); + 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 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 cleanup for the SSH child this process spawned. +fn terminate_owned_forward_child(child: &mut Child) { + let _ = child.start_kill(); } -fn foreground_forward_started_message( +/// Track a verified background forward, cleaning it up if PID-file persistence fails. +fn track_background_forward_or_cleanup( name: &str, - spec: &openshell_core::forward::ForwardSpec, -) -> String { + 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!( "{} Forwarding port {} to sandbox {name}\n Access at: {}\n Press Ctrl+C to stop\n {}", "✓".green().bold(), @@ -1590,7 +1715,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 +1728,134 @@ 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] + #[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" + ); + } + + #[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!( + 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" + ); + } + #[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..ef89cdf1e 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -706,15 +706,336 @@ 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(); + 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") +} + +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 { + 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 { + return false; + } + tokio::time::sleep(Duration::from_millis(50)).await; + } +} + +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", + r#"#!/bin/sh +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 + + if [ "$previous" = "-o" ]; then + case "$arg" in + ProxyCommand=*) + sandbox_id="$(printf '%s\n' "$arg" | sed -n 's/.*--sandbox-id \([^ ]*\).*/\1/p')" + ;; + 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 + ''|*[!0-9]*) + port="${rest%%:*}" + ;; + *) + port="$first" + ;; +esac + +if [ -z "$port" ] || [ -z "$sandbox_id" ]; then + exit 1 +fi + +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("@COMMAND_PATH@", &command_path.display().to_string()) + .replace("@HELPER_PATH@", &helper_path.display().to_string()), + ); + + install_fake_ps_for_pid_revalidation(dir, &pid_path, &command_path); + ssh_path } +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 helper_path = install_fake_forward_process_helper(dir); + install_executable_script( + dir, + "ssh", + r#"#!/bin/sh +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 + + if [ "$previous" = "-o" ]; then + case "$arg" in + ProxyCommand=*) + sandbox_id="$(printf '%s\n' "$arg" | sed -n 's/.*--sandbox-id \([^ ]*\).*/\1/p')" + ;; + 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 + +if [ "$saw_no_command" != "1" ] || [ "$last_arg" != "sandbox" ]; then + exit 1 +fi + +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("@PID_PATH@", &pid_path.display().to_string()) + .replace("@HELPER_PATH@", &helper_path.display().to_string()), + ); + + FakeUnreachableForward { log_path, pid_path } +} + fn test_env(fake_ssh_dir: &TempDir, xdg_dir: &TempDir) -> EnvVarGuard { test_env_with(fake_ssh_dir, xdg_dir, &[]) } @@ -1302,7 +1623,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 +1655,116 @@ 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] +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_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); + run::sandbox_forward(&server.endpoint, "owned-forward", &spec, true, &tls) + .await + .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!( + 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!( + wait_for_process_exit(record.pid, Duration::from_secs(2)).await, + "tracked fake forward process should exit after stop" + ); +} + +#[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_owned_child_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 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); + + 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", + ); + 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!( + "owned background SSH child should exit after listener failure cleanup; pid={}, command={}, log={}", + pid, + command.trim(), + log.trim(), + ); + } } #[tokio::test] 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(); 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