diff --git a/crates/openshell-driver-mxc/examples/mxc-openclaw-gateway.toml b/crates/openshell-driver-mxc/examples/mxc-openclaw-gateway.toml index b85617a216..b49116fca7 100644 --- a/crates/openshell-driver-mxc/examples/mxc-openclaw-gateway.toml +++ b/crates/openshell-driver-mxc/examples/mxc-openclaw-gateway.toml @@ -15,6 +15,9 @@ # (the AppContainer here can only read paths granted by policy -- see the # script's "Stage artifacts into share_dir" step for why). +[openshell] +version = 2 + [openshell.drivers.mxc] wxc_exec_path = "C:\\mxc-kit\\bin\\wxc-exec.exe" diff --git a/crates/openshell-driver-mxc/examples/mxc-openclaw-isolation.toml b/crates/openshell-driver-mxc/examples/mxc-openclaw-isolation.toml index b81f5b5565..2bab7c5655 100644 --- a/crates/openshell-driver-mxc/examples/mxc-openclaw-isolation.toml +++ b/crates/openshell-driver-mxc/examples/mxc-openclaw-isolation.toml @@ -26,6 +26,9 @@ # openshell-supervisor-relay.exe / the caller's OpenClaw install into # the sandbox's policy-authorized working directory before creating it. +[openshell] +version = 2 + [openshell.drivers.mxc] wxc_exec_path = "C:\\mxc-kit\\bin\\wxc-exec.exe" diff --git a/crates/openshell-driver-mxc/examples/mxc-openclaw-localnet.toml b/crates/openshell-driver-mxc/examples/mxc-openclaw-localnet.toml index f0e4c578d9..6fbaced702 100644 --- a/crates/openshell-driver-mxc/examples/mxc-openclaw-localnet.toml +++ b/crates/openshell-driver-mxc/examples/mxc-openclaw-localnet.toml @@ -6,6 +6,9 @@ # allowLocalNetwork=true lets the AppContainer reach the host's loopback (the gateway # relay) without routing through the egress proxy, which breaks node.js DLL init. +[openshell] +version = 2 + [openshell.drivers.mxc] wxc_exec_path = "C:\\FromSenthil\\mxc-fixes-env-vars\\wxc-exec.exe" diff --git a/crates/openshell-driver-mxc/examples/run-openclaw-forward-test.ps1 b/crates/openshell-driver-mxc/examples/run-openclaw-forward-test.ps1 index 6935b68754..7c3a7c6e80 100644 --- a/crates/openshell-driver-mxc/examples/run-openclaw-forward-test.ps1 +++ b/crates/openshell-driver-mxc/examples/run-openclaw-forward-test.ps1 @@ -56,15 +56,10 @@ param( [Parameter(Mandatory = $true)] [string] $OpenClawInstallDir, # Must be a DIRECT CHILD of a drive root (e.g. C:\openshell-openclaw, not - # C:\work\openshell-openclaw). Node's CommonJS module resolver calls - # fs.realpathSync while resolving the entry script, which lstat()s every - # parent directory up the chain -- including ones OUTSIDE share_dir. The - # AppContainer only grants share_dir itself, so an intermediate parent like - # C:\work fails with EPERM (confirmed empirically: this exact test failed - # with "EPERM: operation not permitted, lstat 'C:\work'" until the share - # dir was moved to the drive root). The drive root itself (C:\) apparently - # doesn't need an explicit grant to lstat successfully, so a one-level path - # sidesteps the problem entirely. + # C:\work\openshell-openclaw). The staged Node invocation below uses + # --preserve-symlinks-main so Node does not realpath the main module before + # our capture script starts; keeping a one-level path also avoids exposing + # or depending on unrelated intermediate directories. [string] $ShareDir = "C:\openshell-openclaw", [int] $TargetPort = 18889, [int] $ForwardLocalPort = 28889, @@ -217,6 +212,7 @@ $fwdProc = $null $fwdLog = Join-Path $resultDir "forward.log" $fwdErrLog = Join-Path $resultDir "forward.err.log" $passed = $false +$failureMessage = "" $healthJson = $null $selfProbeOutcome = "not-recorded" $selfProbeResponseBytes = 0 @@ -426,6 +422,7 @@ try { mxc = @{ command = @( "$shareDirToml/node.exe", + "--preserve-symlinks-main", "$shareDirToml/openclaw-capture.mjs", "gateway", "run", "--dev", "--allow-unconfigured", "--auth", "token", "--bind", "loopback", "--port", "$TargetPort" @@ -456,43 +453,23 @@ try { "--env", "NEMOCLAW_MXC_EGRESS_LOOPBACK_PORT=29999", "--no-tty", "--", "exit" ) + # Windows PowerShell 5.1 wraps native stderr as ErrorRecord objects. Keep + # warnings in the captured diagnostic without letting them terminate the + # command before its real exit code and output are collected. + $createPrevEAP = $ErrorActionPreference + $ErrorActionPreference = "Continue" try { $createOut = & $cli @createArgs 2>&1; $createCode = $LASTEXITCODE } catch { $createOut = $_.Exception.Message; $createCode = 1 } + finally { $ErrorActionPreference = $createPrevEAP } $createBenign = Show-SandboxCreate $createOut $SandboxName if ($createCode -ne 0 -and -not $createBenign) { throw "sandbox create '$SandboxName' failed (exit $createCode): $($createOut | Out-String)" } - # 9. Wait for OpenClaw's gateway to report ready, by tailing the gateway's - # own log for the line it prints on successful startup (forwarded from - # the sandbox's stdout via "wxc-exec stdout:"). Generous timeout: Node - # startup + AppContainer/UAC elevation + plugin warmup can take a while - # on a cold run. - Step "Wait for OpenClaw gateway readiness" - $readyDeadline = (Get-Date).AddSeconds(90) - $openclawReady = $false - while ((Get-Date) -lt $readyDeadline) { - if (Test-Path $gwLog) { - # `.*` (not `\s+`) between "[gateway]" and "ready": OpenClaw wraps its - # log lines in ANSI color codes whenever it inherits enough of the host - # env to detect a color-capable terminal -- which happens with - # mxc-openclaw-localnet.toml (-UseLocalNetwork), since that config - # doesn't set pc_minimal_env and so inherits the full host env, unlike - # mxc-openclaw-gateway.toml's curated minimal set. A strict \s+ match - # missed this entirely and timed out waiting for a line that had - # already printed. Those codes render in this log as LITERAL backslash- - # escaped text (e.g. "...\x1b[36mready..."), not real ESC bytes -- so - # "m" from "36m" directly abuts "ready" with no word boundary, which is - # why a \bready\b tightening (tried once) also failed to match; a bare - # substring check is what actually works here. The resulting collision - # risk with "already" is theoretical -- no such line has been observed - # on this "[gateway]"-tagged forwarded-stdout path in practice. - if (Select-String -Path $gwLog -Pattern '\[gateway\].*ready' -Quiet -ErrorAction SilentlyContinue) { $openclawReady = $true; break } - } - Start-Sleep -Seconds 2 - } - if (-not $openclawReady) { throw "OpenClaw did not report ready within 90s (see gateway.log in the results bundle)" } - Ok "OpenClaw gateway ready" + # `sandbox create` does not return success until the MXC driver receives the + # relay's target_ready event. Trust that lifecycle result directly instead + # of racing a second, text-based poll against gateway.log. + Ok "OpenClaw gateway ready (sandbox target_ready received)" # 10. openshell forward service: opens a fresh, on-demand relay for this # one call and bridges TargetPort (inside the sandbox) to @@ -610,26 +587,55 @@ try { } } catch { - Bad $_.Exception.Message + $failureMessage = $_.Exception.Message + Bad $failureMessage } finally { # Stop the forward before the sandbox so its relay tears down cleanly. if ($fwdProc -and -not $fwdProc.HasExited) { try { Stop-Process -Id $fwdProc.Id -Force -ErrorAction SilentlyContinue } catch {} } + if ($fwdProc) { + try { + if (-not $fwdProc.WaitForExit(5000)) { + throw "forward process did not exit within 5s" + } + } catch { + Info "forward teardown: $($_.Exception.Message)" + if ($passed) { $passed = $false; $failureMessage = $_.Exception.Message } + } + } # Tear down the sandbox while the gateway is still up (delete needs it). if ($cli -and $SandboxName) { - try { & $cli sandbox delete $SandboxName 2>&1 | Out-Null } - catch { Info "sandbox teardown '$SandboxName': $($_.Exception.Message) (continuing)" } + $deleteCode = 1 + $deleteOut = @() + $deletePrevEAP = $ErrorActionPreference + $ErrorActionPreference = "Continue" + try { $deleteOut = & $cli sandbox delete $SandboxName 2>&1; $deleteCode = $LASTEXITCODE } + catch { $deleteOut = $_.Exception.Message } + finally { $ErrorActionPreference = $deletePrevEAP } + if ($deleteCode -ne 0) { + $cleanupFailure = "sandbox teardown '$SandboxName' failed (exit $deleteCode): $($deleteOut | Out-String)" + Info $cleanupFailure + if ($passed) { $passed = $false; $failureMessage = $cleanupFailure } + } } if ($KeepRunning -and $gw -and -not $gw.HasExited) { Info "leaving gateway pid $($gw.Id) running (-KeepRunning); stop with: Stop-Process -Id $($gw.Id) -Force" } elseif ($gw -and -not $gw.HasExited) { Step "Cleanup"; Stop-Process -Id $gw.Id -Force -ErrorAction SilentlyContinue - try { $gw.WaitForExit(5000) | Out-Null } catch {} - Info "stopped gateway pid $($gw.Id)" + try { + if (-not $gw.WaitForExit(5000)) { + throw "gateway process did not exit within 5s" + } + Info "stopped gateway pid $($gw.Id)" + } catch { + $cleanupFailure = "gateway teardown failed: $($_.Exception.Message)" + Info $cleanupFailure + if ($passed) { $passed = $false; $failureMessage = $cleanupFailure } + } } Step "Gateway log (tail)" @@ -663,12 +669,17 @@ finally { Step "RESULT" $verdict = if ($passed) { "PASS" } else { "FAIL" } + if (-not $passed -and [string]::IsNullOrWhiteSpace($failureMessage)) { + $failureMessage = "one or more qualification assertions failed" + } + $failureSummary = ($failureMessage -replace '\s+', ' ').Trim() $summary = @" OpenShell MXC OpenClaw + dynamic forward test ===================================================================== timestamp : $stamp machine : $env:COMPUTERNAME verdict : $verdict +failure : $failureSummary sandbox : $SandboxName backend : $Backend config : $tomlName diff --git a/crates/openshell-driver-mxc/src/control_channel.rs b/crates/openshell-driver-mxc/src/control_channel.rs index 673b82177e..e0240a1d45 100644 --- a/crates/openshell-driver-mxc/src/control_channel.rs +++ b/crates/openshell-driver-mxc/src/control_channel.rs @@ -30,13 +30,13 @@ pub enum ControlChannelError { type PendingMap = Mutex>>; /// Slot for one of the spawner's one-time, unsolicited events -- startup- -/// ready (see `try_route_ready`) and target-ready (see -/// `try_route_target_ready`) each get their own instance of this type. +/// ready (see `try_route_ready`) and target status (see +/// `try_route_target_status`) each get their own instance of this type. /// Not part of `PendingMap`: neither has a correlation id or is a reply to /// anything the driver sent. The payload is `Ok(())` for a normal fire, or /// `Err(reason)` when the event fired but something about it was rejected -/// (currently only the "ready" event's protocol version check uses this; -/// `"target_ready"` always sends `Ok(())`). +/// `"target_ready"` sends `Ok(())`; `"target_failed"` sends its diagnostic +/// as `Err(reason)`. pub type ReadySlot = Mutex>>>; /// Wire protocol version this driver requires from @@ -50,7 +50,7 @@ pub type ReadySlot = Mutex>>>; /// "forward", or the `"target_ready"` event itself) -- an independently /// staged, stale relay binary then fails fast with a clear error instead of /// hanging or misbehaving against fields/events it doesn't understand. -const REQUIRED_SUPERVISOR_RELAY_PROTOCOL_VERSION: u64 = 2; +const REQUIRED_SUPERVISOR_RELAY_PROTOCOL_VERSION: u64 = 3; /// One control channel per sandboxed process. `request()` is safe to call /// concurrently — each call gets its own correlation id and awaits only its @@ -129,20 +129,25 @@ impl ControlChannel { .await } - /// Try to recognize `line` as the spawner's unsolicited target-ready - /// event (`{"event":"target_ready"}`) -- sent once the spawner has - /// actually spawned the target and confirmed its configured port is - /// accepting connections (see `wait_for_port_ready` in - /// `openshell-supervisor-relay`). Distinct from the `"launch"` - /// control-channel *response*, which only confirms the command/env - /// arrived, not that the target is running: driver.rs awaits this event - /// too before publishing the sandbox `Ready=True`, so a caller acting on - /// `Ready` can't race a target that hasn't bound its port yet. Returns - /// `true` if `line` was consumed this way. No version gate here -- the - /// startup "ready" handshake above already rejected an incompatible - /// peer long before this could fire. - pub async fn try_route_target_ready(target_ready: &ReadySlot, line: &str) -> bool { - Self::try_route_named_event(target_ready, line, "target_ready", |_| Ok(())).await + /// Route the spawner's one-time target status event. `target_ready` + /// confirms the configured port is accepting connections; + /// `target_failed` carries the target's actual exit/error diagnostic. + /// Both are distinct from the `launch` response, which only confirms the + /// command and environment arrived. No version gate is needed here: the + /// startup handshake already rejected an incompatible peer. + pub async fn try_route_target_status(target_status: &ReadySlot, line: &str) -> bool { + if Self::try_route_named_event(target_status, line, "target_ready", |_| Ok(())).await { + return true; + } + Self::try_route_named_event(target_status, line, "target_failed", |value| { + let error = value + .get("error") + .and_then(Value::as_str) + .filter(|error| !error.trim().is_empty()) + .unwrap_or("target failed without a diagnostic"); + Err(error.to_string()) + }) + .await } async fn try_route_named_event( @@ -279,7 +284,7 @@ mod tests { let (slot, rx) = armed_ready_slot(); let consumed = - ControlChannel::try_route_ready(&slot, r#"{"event":"ready","protocol_version":2}"#) + ControlChannel::try_route_ready(&slot, r#"{"event":"ready","protocol_version":3}"#) .await; assert!(consumed); @@ -298,13 +303,13 @@ mod tests { consumed, "a recognized ready event is consumed even when rejected" ); - let err = rx.await.unwrap().expect_err("version 2 must be rejected"); + let err = rx.await.unwrap().expect_err("version 1 must be rejected"); assert!( - err.contains('2'), + err.contains('1'), "error should name the offending version: {err}" ); assert!( - err.contains('1'), + err.contains('3'), "error should name the required version: {err}" ); } @@ -334,7 +339,7 @@ mod tests { assert!(!ControlChannel::try_route_ready(&slot, "garbage").await); } - // ── try_route_target_ready ─────────────────────────────────────────── + // ── try_route_target_status ────────────────────────────────────────── #[tokio::test] async fn try_route_target_ready_fires_ok_with_no_version_gate() { @@ -342,9 +347,9 @@ mod tests { // No protocol_version field at all -- unlike "ready", "target_ready" // must not be gated on one (see the doc comment on - // try_route_target_ready). + // try_route_target_status). let consumed = - ControlChannel::try_route_target_ready(&slot, r#"{"event":"target_ready"}"#).await; + ControlChannel::try_route_target_status(&slot, r#"{"event":"target_ready"}"#).await; assert!(consumed); assert_eq!(rx.await.unwrap(), Ok(())); @@ -356,9 +361,9 @@ mod tests { // "ready" and "target_ready" must not be cross-routed into each // other's slot. - let consumed = ControlChannel::try_route_target_ready( + let consumed = ControlChannel::try_route_target_status( &slot, - r#"{"event":"ready","protocol_version":2}"#, + r#"{"event":"ready","protocol_version":3}"#, ) .await; @@ -369,13 +374,15 @@ mod tests { async fn try_route_named_event_is_a_safe_no_op_once_the_slot_is_already_empty() { let (slot, rx) = armed_ready_slot(); - assert!(ControlChannel::try_route_target_ready(&slot, r#"{"event":"target_ready"}"#).await); + assert!( + ControlChannel::try_route_target_status(&slot, r#"{"event":"target_ready"}"#).await + ); // The slot's sender was taken (and used) on the first fire. A // repeat of the same event on the wire is still recognized as a // "target_ready" line (so the caller doesn't mistake it for plain // log text) but must not panic just because the slot is now empty. let consumed_again = - ControlChannel::try_route_target_ready(&slot, r#"{"event":"target_ready"}"#).await; + ControlChannel::try_route_target_status(&slot, r#"{"event":"target_ready"}"#).await; assert!( consumed_again, @@ -388,6 +395,23 @@ mod tests { ); } + #[tokio::test] + async fn try_route_target_failed_preserves_the_diagnostic() { + let (slot, rx) = armed_ready_slot(); + + let consumed = ControlChannel::try_route_target_status( + &slot, + r#"{"event":"target_failed","error":"exit 23; stderr: early crash"}"#, + ) + .await; + + assert!(consumed); + assert_eq!( + rx.await.unwrap(), + Err("exit 23; stderr: early crash".to_string()) + ); + } + // ── fail_all_pending ────────────────────────────────────────────────── #[tokio::test] diff --git a/crates/openshell-driver-mxc/src/driver.rs b/crates/openshell-driver-mxc/src/driver.rs index 513a92d858..3f767210d5 100644 --- a/crates/openshell-driver-mxc/src/driver.rs +++ b/crates/openshell-driver-mxc/src/driver.rs @@ -1801,14 +1801,10 @@ async fn run_lifecycle( } else { (None, None) }; - // Target-ready signal from the spawner (see control_channel.rs's - // try_route_target_ready) -- fired once the target is actually running - // and its configured port is accepting connections, distinct from the - // "launch" response below (which only confirms the command/env - // arrived). Awaited after "launch" succeeds and before publishing - // Ready=True, so Ready can't be reported while the target is still - // unreachable. Always Ok(()) when it fires (no version gate on this - // event -- see try_route_target_ready). + // Target-status signal from the spawner (see control_channel.rs's + // try_route_target_status): Ok once the target port accepts connections, + // or Err with the target's real exit/stderr diagnostic. Distinct from the + // "launch" response below, which only confirms the command/env arrived. let (target_ready_slot, target_ready_rx) = if spawner_wrapping_active { let (tx, rx) = oneshot::channel::>(); (Some(Arc::new(Mutex::new(Some(tx)))), Some(rx)) @@ -1830,7 +1826,9 @@ async fn run_lifecycle( None => false, }; let routed_target_ready = match &target_ready_slot { - Some(slot) => ControlChannel::try_route_target_ready(slot, &line).await, + Some(slot) => { + ControlChannel::try_route_target_status(slot, &line).await + } None => false, }; let routed = routed_ready @@ -1861,7 +1859,7 @@ async fn run_lifecycle( }); } // Drop this scope's Arc clones now that the stdout task holds its own: - // if the spawner exits before ever sending "ready"/"target_ready", the + // if the spawner exits before ever sending "ready"/target status, the // stdout task's clone is the only thing keeping the // Mutex> alive, so its loop ending (EOF) drops the last // reference -- which drops the still-`Some` Sender and makes @@ -2048,11 +2046,8 @@ async fn run_lifecycle( info!(sandbox = %sandbox_name, "control-channel target ready"); None } - // No version gate on this event, so this arm - // never actually fires today -- see - // try_route_target_ready -- but match it - // explicitly rather than unreachable!(), in case - // that ever changes. + // `target_failed` carries the bounded target stderr + // diagnostic supplied by openshell-supervisor-relay. Ok(Ok(Err(target_err))) => Some(target_err), Ok(Err(_)) => { Some("spawner exited before its target became ready".to_string()) diff --git a/crates/openshell-driver-mxc/tests/openclaw_appcontainer_compat.rs b/crates/openshell-driver-mxc/tests/openclaw_appcontainer_compat.rs index 3a3a16af59..609de6ee05 100644 --- a/crates/openshell-driver-mxc/tests/openclaw_appcontainer_compat.rs +++ b/crates/openshell-driver-mxc/tests/openclaw_appcontainer_compat.rs @@ -3,6 +3,9 @@ const CAPTURE: &str = include_str!("../examples/openclaw-capture.mjs"); const RUNNER: &str = include_str!("../examples/run-openclaw-forward-test.ps1"); +const PROCESS_CONTAINER_CONFIG: &str = include_str!("../examples/mxc-openclaw-gateway.toml"); +const ISOLATION_CONFIG: &str = include_str!("../examples/mxc-openclaw-isolation.toml"); +const LOCAL_NETWORK_CONFIG: &str = include_str!("../examples/mxc-openclaw-localnet.toml"); #[cfg(windows)] #[test] @@ -44,6 +47,30 @@ fn capture_preloads_appcontainer_safe_realpath_before_openclaw() { assert!(CAPTURE.contains("syncBuiltinESMExports()")); } +#[test] +fn runner_preserves_the_node_main_symlink_inside_processcontainer() { + let node = RUNNER + .find("$shareDirToml/node.exe") + .expect("runner must launch the staged Node.js binary"); + let preserve_main = RUNNER[node..] + .find("--preserve-symlinks-main") + .map(|offset| node + offset) + .expect("runner must prevent Node's pre-entrypoint realpath of the drive root"); + let capture = RUNNER[node..] + .find("$shareDirToml/openclaw-capture.mjs") + .map(|offset| node + offset) + .expect("runner must launch the OpenClaw capture entry point"); + + assert!( + preserve_main < capture, + "--preserve-symlinks-main must be a Node option before the main module" + ); + assert!( + !RUNNER.contains("Grant-AppContainerWritableDirectory $shareDirNorm"), + "the runtime workaround must not widen the share grant to the drive root" + ); +} + #[test] fn runner_limits_package_group_dacl_grants_to_writable_data_directories() { assert!(RUNNER.contains("*S-1-15-2-1:(OI)(CI)(M)")); @@ -56,3 +83,28 @@ fn runner_limits_package_group_dacl_grants_to_writable_data_directories() { ); assert!(!RUNNER.contains("Grant-AppContainerWritableDirectory $shareDirNorm")); } + +#[test] +fn runner_uses_lifecycle_readiness_and_preserves_failure_diagnostics() { + assert!(RUNNER.contains("sandbox target_ready received")); + assert!( + !RUNNER.contains("Select-String -Path $gwLog -Pattern '\\[gateway\\].*ready'"), + "target readiness must come from the lifecycle event, not log polling" + ); + assert!(RUNNER.contains("$failureMessage = $_.Exception.Message")); + assert!(RUNNER.contains("failure : $failureSummary")); + assert!(RUNNER.contains("$fwdProc.WaitForExit(5000)")); + assert!(RUNNER.contains("$deleteCode = $LASTEXITCODE")); + assert!(RUNNER.contains("$gw.WaitForExit(5000)")); +} + +#[test] +fn openclaw_gateway_configs_declare_the_current_schema() { + for config in [ + PROCESS_CONTAINER_CONFIG, + ISOLATION_CONFIG, + LOCAL_NETWORK_CONFIG, + ] { + assert!(config.contains("[openshell]\nversion = 2")); + } +} diff --git a/crates/openshell-driver-mxc/tests/wxc_exec_real.rs b/crates/openshell-driver-mxc/tests/wxc_exec_real.rs index abd048c73c..dc0f47c77a 100644 --- a/crates/openshell-driver-mxc/tests/wxc_exec_real.rs +++ b/crates/openshell-driver-mxc/tests/wxc_exec_real.rs @@ -1030,6 +1030,80 @@ fn pc_oneshot_out_of_policy_write_denied() { ); } +/// Reading an unrelated root-level path remains denied when only the workload +/// fixture is granted. This guards the `OpenClaw` Node.js workaround against +/// accidentally granting broad access beneath `C:\`. +#[test] +#[ignore = "requires real wxc-exec"] +fn pc_oneshot_unrelated_root_path_read_denied() { + let Some(wxc) = wxc_path() else { + eprintln!("SKIP: wxc-exec not found"); + return; + }; + + if let Err(reason) = probe_processcontainer(&wxc) { + eprintln!("SKIP: processcontainer not live: {reason}"); + return; + } + + let granted_dir = tempfile::tempdir().expect("granted tempdir"); + let denied_root_dir = tempfile::Builder::new() + .prefix("openshell-pc-denied-") + .tempdir_in(r"C:\") + .expect("root-level denied tempdir"); + let denied_file = denied_root_dir.path().join("sentinel.txt"); + std::fs::write(&denied_file, "root-level secret").expect("write denied sentinel"); + let denied_file_str = denied_file.to_string_lossy().into_owned(); + let granted_str = granted_dir.path().to_string_lossy().into_owned(); + let diagnostic = granted_dir.path().join("root-read.txt"); + let diagnostic_str = diagnostic.to_string_lossy().into_owned(); + let config = serde_json::json!({ + "version": "0.6.0-alpha", + "containerId": "pc-root-read-denied", + "containment": "processcontainer", + "process": { + "commandLine": format!("cmd /d /c type \"{denied_file_str}\" 1>\"{diagnostic_str}\" 2>&1"), + "cwd": granted_str, + "timeout": 30_000, + }, + "filesystem": { + "readwritePaths": [granted_str], + }, + "processContainer": { + "leastPrivilege": false, + }, + "ui": { + "disable": false, + "clipboard": "none", + "injection": false, + }, + }); + + let json = serde_json::to_string(&config).unwrap(); + let b64 = base64::engine::general_purpose::STANDARD.encode(json.as_bytes()); + let out = Command::new(&wxc) + .arg("--config-base64") + .arg(&b64) + .output() + .expect("wxc-exec spawn"); + let code = out.status.code().unwrap_or(-1); + let root_read = std::fs::read_to_string(&diagnostic) + .expect("sandboxed cmd must run and write its drive-root diagnostic"); + + assert_ne!( + code, 0, + "unrelated root-level read must remain denied; diagnostic={root_read}" + ); + assert!( + root_read.to_ascii_lowercase().contains("access is denied"), + "failure must specifically be the root-level access denial; diagnostic={root_read}" + ); + assert!( + !root_read.contains("root-level secret"), + "root-level file contents must not be readable; diagnostic={root_read}" + ); +} + // ── Isolation session enforcement tests ────────────────────────────────────── /// Full `isolation_session` round trip: provision → start → exec → stop → diff --git a/crates/openshell-supervisor-relay/src/imp.rs b/crates/openshell-supervisor-relay/src/imp.rs index 8db89f176e..f215981f37 100644 --- a/crates/openshell-supervisor-relay/src/imp.rs +++ b/crates/openshell-supervisor-relay/src/imp.rs @@ -63,7 +63,9 @@ //! Request: `{"id": , "op": "", "data": }` //! Response: `{"id": , "ok": true, "data": }` //! or `{"id": , "ok": false, "error": ""}` -//! Event (unsolicited, no id): `{"event": ""}` +//! Event (unsolicited, no id): `{"event": ""}` (startup also emits +//! either `target_ready` or `target_failed`; the latter includes an +//! `error` string with the target's exit and bounded stderr detail) //! //! Startup handshake: before spawning anything, this process emits //! `{"event":"ready","protocol_version":N}` on stdout (`N` = `PROTOCOL_VERSION` @@ -93,8 +95,8 @@ use base64::Engine; use futures::{SinkExt, StreamExt}; -use std::collections::HashMap; -use std::sync::Arc; +use std::collections::{HashMap, VecDeque}; +use std::sync::{Arc, Mutex as StdMutex}; use std::time::Duration; use tokio::sync::oneshot; use tokio_tungstenite::tungstenite::Message; @@ -108,7 +110,22 @@ use tokio_tungstenite::tungstenite::Message; /// out-of-sync peer can't safely ignore, so an independently staged, stale /// binary on either side fails fast with a clear version-mismatch error /// instead of hanging or misbehaving against a field/event it predates. -const PROTOCOL_VERSION: u64 = 2; +const PROTOCOL_VERSION: u64 = 3; + +const TARGET_STDERR_TAIL_LINES: usize = 20; +const TARGET_STDERR_LINE_CHARS: usize = 1024; + +struct SpawnedTarget { + child: tokio::process::Child, + stderr_tail: Arc>>, + stderr_forwarder: Option>, +} + +enum StartupOutcome { + Ready(anyhow::Result<()>), + Exited(std::io::Result), + Shutdown, +} struct ForwardSession { reader: tokio::sync::Mutex, @@ -134,16 +151,18 @@ pub async fn run() -> anyhow::Result<()> { let (shutdown_tx, shutdown_rx) = oneshot::channel::<()>(); let shutdown_slot = Arc::new(tokio::sync::Mutex::new(Some(shutdown_tx))); let forward_sessions = Arc::new(ForwardSessions::new(HashMap::new())); - // Lets main() ask run_control_channel's task to announce "target_ready" - // on stdout once the target is actually up (see below) -- routed - // through that task rather than a second independent stdout handle - // here, since concurrent writers to tokio's stdout can interleave. - let (target_ready_tx, target_ready_rx) = oneshot::channel::<()>(); + // Lets main() ask run_control_channel's task to announce either + // "target_ready" or "target_failed" on stdout. The acknowledgement makes + // failure delivery deterministic: do not let this process exit until the + // real target diagnostic has been flushed into the pipe to the driver. + let (target_status_tx, target_status_rx) = oneshot::channel::>(); + let (target_status_ack_tx, target_status_ack_rx) = oneshot::channel::<()>(); tokio::spawn(run_control_channel( launch_slot, shutdown_slot, forward_sessions, - target_ready_rx, + target_status_rx, + target_status_ack_tx, )); eprintln!("[openshell-supervisor-relay] waiting for launch request from driver..."); @@ -151,7 +170,16 @@ pub async fn run() -> anyhow::Result<()> { .await .map_err(|_| anyhow::anyhow!("control channel closed before a launch request arrived"))?; - let mut child = spawn_target(command, env)?; + let mut target = match spawn_target(command, env) { + Ok(target) => target, + Err(error) => { + let message = format!("target process failed to start: {error:#}"); + announce_target_status(target_status_tx, target_status_ack_rx, Err(message.clone())) + .await; + eprintln!("[openshell-supervisor-relay] {message}"); + std::process::exit(1); + } + }; eprintln!("[openshell-supervisor-relay] waiting for target on 127.0.0.1:{port} ..."); // Race the (up to ~300s worst case) port-readiness wait against a @@ -164,16 +192,37 @@ pub async fn run() -> anyhow::Result<()> { // (and the target it spawned) alive for up to the full port-readiness // budget after a caller was told shutdown succeeded. let mut shutdown_rx = shutdown_rx; - tokio::select! { - result = wait_for_port_ready(&mut child, port, PORT_READY_PER_TRY_TIMEOUT) => { - result?; + let startup = tokio::select! { + result = wait_for_port_ready(port, PORT_READY_PER_TRY_TIMEOUT) => { + StartupOutcome::Ready(result) } - _ = &mut shutdown_rx => { + status = target.child.wait() => StartupOutcome::Exited(status), + _ = &mut shutdown_rx => StartupOutcome::Shutdown, + }; + match startup { + StartupOutcome::Ready(Ok(())) => {} + StartupOutcome::Ready(Err(error)) => { + let _ = target.child.kill().await; + let status = target.child.wait().await; + let message = target_failure_message(&mut target, port, status, Some(&error)).await; + announce_target_status(target_status_tx, target_status_ack_rx, Err(message.clone())) + .await; + eprintln!("[openshell-supervisor-relay] {message}"); + std::process::exit(1); + } + StartupOutcome::Exited(status) => { + let message = target_failure_message(&mut target, port, status, None).await; + announce_target_status(target_status_tx, target_status_ack_rx, Err(message.clone())) + .await; + eprintln!("[openshell-supervisor-relay] {message}"); + std::process::exit(1); + } + StartupOutcome::Shutdown => { eprintln!( "[openshell-supervisor-relay] shutdown request -- stopping before target became ready" ); - let _ = child.kill().await; - let _ = child.wait().await; + let _ = target.child.kill().await; + let _ = target.child.wait().await; eprintln!("[openshell-supervisor-relay] done"); // Not `return Ok(())`: run_control_channel loops on // stdin.next_line() for this process's entire lifetime and @@ -193,12 +242,12 @@ pub async fn run() -> anyhow::Result<()> { // target is actually reachable) -- driver.rs awaits this before // publishing the sandbox Ready=True. A send failure just means the // control-channel task already exited; nothing to do about that here. - let _ = target_ready_tx.send(()); + announce_target_status(target_status_tx, target_status_ack_rx, Ok(())).await; // No bridge at startup -- relay bridging is entirely on-demand via the // control channel's "forward" op (see module docs). Just run the target // process's lifecycle from here. - run_lifecycle(child, shutdown_rx).await; + run_lifecycle(target, shutdown_rx).await; Ok(()) } @@ -244,7 +293,7 @@ fn byte_preview(data: &[u8]) -> String { /// because our stdout is reserved exclusively for the control-channel /// protocol with the driver. The child's stdin is closed; it isn't part of /// this channel. -fn spawn_target(command: Vec, env: Vec) -> anyhow::Result { +fn spawn_target(command: Vec, env: Vec) -> anyhow::Result { if command.is_empty() { anyhow::bail!("launch command must not be empty"); } @@ -284,26 +333,97 @@ fn spawn_target(command: Vec, env: Vec) -> anyhow::Result>>>, +) { use tokio::io::{AsyncBufReadExt, BufReader}; let mut lines = BufReader::new(reader).lines(); while let Ok(Some(line)) = lines.next_line().await { + if let Some(tail) = &capture_tail { + let mut chars = line.chars(); + let mut captured: String = chars.by_ref().take(TARGET_STDERR_LINE_CHARS).collect(); + if chars.next().is_some() { + captured.push_str("..."); + } + if let Ok(mut tail) = tail.lock() { + if tail.len() == TARGET_STDERR_TAIL_LINES { + tail.pop_front(); + } + tail.push_back(captured); + } + } eprintln!("[{label}] {line}"); } } +async fn announce_target_status( + sender: oneshot::Sender>, + announced: oneshot::Receiver<()>, + status: Result<(), String>, +) { + if sender.send(status).is_err() { + return; + } + if tokio::time::timeout(Duration::from_secs(5), announced) + .await + .is_err() + { + eprintln!("[openshell-supervisor-relay] control channel did not flush target status"); + } +} + +async fn target_failure_message( + target: &mut SpawnedTarget, + port: u16, + status: std::io::Result, + readiness_error: Option<&anyhow::Error>, +) -> String { + if let Some(forwarder) = target.stderr_forwarder.take() { + let _ = tokio::time::timeout(Duration::from_secs(2), forwarder).await; + } + let status = status.map_or_else( + |error| format!("status unavailable: {error}"), + |status| status.to_string(), + ); + let stderr = target + .stderr_tail + .lock() + .map(|tail| tail.iter().cloned().collect::>().join(" | ")) + .unwrap_or_default(); + let readiness = readiness_error + .map(|error| format!("; readiness error: {error:#}")) + .unwrap_or_default(); + let stderr = if stderr.is_empty() { + String::new() + } else { + format!("; stderr: {stderr}") + }; + format!("target process exited before port {port} came up: {status}{readiness}{stderr}") +} + // ── Control channel ─────────────────────────────────────────────────────────── // // See module docs for the protocol and why this is safe to run with no @@ -323,7 +443,8 @@ async fn run_control_channel( launch: Arc, shutdown: Arc, forward_sessions: Arc, - target_ready_rx: oneshot::Receiver<()>, + target_status_rx: oneshot::Receiver>, + target_status_ack: oneshot::Sender<()>, ) { use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; @@ -344,11 +465,11 @@ async fn run_control_channel( } eprintln!("[openshell-supervisor-relay] control channel ready (stdin/stdout)"); - // `None` once fired (or once main()'s sender is dropped without firing, - // e.g. wait_for_port_ready failed) -- the `if` guard below then disables - // that select arm instead of it firing repeatedly on every subsequent - // poll of an already-resolved oneshot. - let mut target_ready_rx = Some(target_ready_rx); + // `None` once fired (or once main()'s sender is dropped without firing) -- + // the `if` guard below then disables that select arm instead of it firing + // repeatedly on every subsequent poll of an already-resolved oneshot. + let mut target_status_rx = Some(target_status_rx); + let mut target_status_ack = Some(target_status_ack); loop { tokio::select! { @@ -395,19 +516,25 @@ async fn run_control_channel( // See module docs' startup handshake -- distinct from the // "launch" response, which only confirms the command/env // arrived. Unsolicited, like "ready" above. - result = async { target_ready_rx.as_mut().unwrap().await }, if target_ready_rx.is_some() => { - target_ready_rx = None; - if result.is_ok() { - let event = serde_json::json!({"event": "target_ready"}).to_string() + "\n"; - if stdout.write_all(event.as_bytes()).await.is_err() || stdout.flush().await.is_err() { - eprintln!("[openshell-supervisor-relay] control channel: failed to announce target_ready"); + result = async { target_status_rx.as_mut().unwrap().await }, if target_status_rx.is_some() => { + target_status_rx = None; + let event = match result { + Ok(Ok(())) => Some(serde_json::json!({"event": "target_ready"})), + Ok(Err(error)) => Some(serde_json::json!({"event": "target_failed", "error": error})), + Err(_) => None, + }; + if let Some(event) = event { + let out = event.to_string() + "\n"; + if stdout.write_all(out.as_bytes()).await.is_err() || stdout.flush().await.is_err() { + eprintln!("[openshell-supervisor-relay] control channel: failed to announce target status"); + } + if let Some(ack) = target_status_ack.take() { + let _ = ack.send(()); } } - // A dropped sender (main() bailed before the target ever - // came up, e.g. wait_for_port_ready's own error) means - // there's nothing to announce -- driver.rs's timeout on the - // corresponding event will surface that as a launch failure - // on its own. + // A dropped sender means main() exited before announcing + // target status. There is nothing to write in that case; + // driver.rs will observe control-channel EOF. } } } @@ -722,28 +849,12 @@ where const PORT_READY_MAX_TRIES: u32 = 5; const PORT_READY_PER_TRY_TIMEOUT: Duration = Duration::from_mins(1); -/// Poll for the target port accepting TCP connections, bailing out early -/// (rather than waiting out all tries) if the child process exits first — a -/// dead child will never open the port, so there's no reason to wait. Makes -/// up to `PORT_READY_MAX_TRIES` tries, each given the full `per_try_timeout` -/// budget, logging the start of every try so a long cold-start wait (Node.js -/// first-run JIT/module resolution, first-touch AV scan of freshly staged -/// binaries, etc.) is visible rather than silent until success or final -/// timeout. -async fn wait_for_port_ready( - child: &mut tokio::process::Child, - port: u16, - per_try_timeout: Duration, -) -> anyhow::Result<()> { - // Bound each individual connect attempt: on at least one observed - // wxc-exec build, a connect() against a not-yet-listening loopback port - // inside the AppContainer never resolved at all (no fast ECONNREFUSED, - // no error) instead of failing quickly like an ordinary closed-port - // connect. Without this, a single early attempt can hang forever and - // this function -- and the whole readiness wait -- never returns even - // after the target's port genuinely opens, since nothing ever retries. - const ATTEMPT_TIMEOUT: Duration = Duration::from_secs(2); - +/// Poll for the target port accepting TCP connections. Target termination is +/// observed independently by `run` through `Child::wait`, so this function +/// never polls the process handle and cannot miss an exit while a connect is +/// stuck. Makes up to `PORT_READY_MAX_TRIES` tries, each given the full +/// `per_try_timeout` budget. +async fn wait_for_port_ready(port: u16, per_try_timeout: Duration) -> anyhow::Result<()> { let overall_start = tokio::time::Instant::now(); for try_num in 1..=PORT_READY_MAX_TRIES { eprintln!( @@ -751,21 +862,24 @@ async fn wait_for_port_ready( ); let try_deadline = tokio::time::Instant::now() + per_try_timeout; loop { - let attempt = tokio::time::timeout( - ATTEMPT_TIMEOUT, - tokio::net::TcpStream::connect(("127.0.0.1", port)), - ) + // A ProcessContainer loopback connect has been observed to block + // inside the OS call long enough that Tokio's timer and every + // later `try_wait()` poll on this task were starved for ~300s. + // Isolate it on the blocking pool and use the socket API's own + // deadline. `run` continues awaiting the child handle in parallel, + // so an early crash is reported even if this worker remains stuck. + let address = std::net::SocketAddr::from(([127, 0, 0, 1], port)); + let attempt = tokio::task::spawn_blocking(move || { + std::net::TcpStream::connect_timeout(&address, Duration::from_secs(2)) + }) .await; - if let Ok(Ok(_)) = attempt { + if matches!(attempt, Ok(Ok(_))) { eprintln!( "[openshell-supervisor-relay] port {port} ready after {:?} (try {try_num}/{PORT_READY_MAX_TRIES})", overall_start.elapsed() ); return Ok(()); } - if let Ok(Some(status)) = child.try_wait() { - anyhow::bail!("target process exited before port {port} came up: {status}"); - } if tokio::time::Instant::now() >= try_deadline { eprintln!( "[openshell-supervisor-relay] port readiness try {try_num}/{PORT_READY_MAX_TRIES} timed out after {per_try_timeout:?} (elapsed {:?} total)", @@ -998,17 +1112,17 @@ async fn run_relay_bridge( /// Any active dynamic relay bridges are tokio tasks in this same process, so /// `std::process::exit` below tears them down too -- no separate stop signal /// needed. -async fn run_lifecycle(mut child: tokio::process::Child, shutdown_rx: oneshot::Receiver<()>) { +async fn run_lifecycle(mut target: SpawnedTarget, shutdown_rx: oneshot::Receiver<()>) { tokio::select! { - status = child.wait() => { + status = target.child.wait() => { let code = status.map_or(1, |s| s.code().unwrap_or(1)); eprintln!("[openshell-supervisor-relay] target exited with code {code}"); std::process::exit(code); } _ = shutdown_rx => { eprintln!("[openshell-supervisor-relay] shutdown request -- stopping"); - let _ = child.kill().await; - let _ = child.wait().await; + let _ = target.child.kill().await; + let _ = target.child.wait().await; eprintln!("[openshell-supervisor-relay] done"); std::process::exit(0); } diff --git a/crates/openshell-supervisor-relay/tests/control_channel_contract.rs b/crates/openshell-supervisor-relay/tests/control_channel_contract.rs index 1f3fb51464..65317e8a55 100644 --- a/crates/openshell-supervisor-relay/tests/control_channel_contract.rs +++ b/crates/openshell-supervisor-relay/tests/control_channel_contract.rs @@ -187,7 +187,7 @@ impl RelayProcess { async fn expect_ready(&mut self) { let v = self.next_json().await; assert_eq!(v["event"], "ready"); - assert_eq!(v["protocol_version"], 2); + assert_eq!(v["protocol_version"], 3); } async fn launch(&mut self, id: u64, command: &[&str]) -> Value { @@ -354,6 +354,45 @@ async fn launch_success_then_target_ready_ordering() { ); } +#[tokio::test(flavor = "multi_thread")] +async fn early_child_crash_is_reported_promptly_with_its_stderr() { + let port = { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + listener.local_addr().unwrap().port() + }; + let mut relay = RelayProcess::spawn(port).await; + relay.expect_ready().await; + + let ack = relay + .launch( + 1, + &[ + "cmd", + "/d", + "/c", + "echo early-child-crash-sentinel 1>&2 & exit /b 23", + ], + ) + .await; + assert_eq!(ack["ok"], true, "launch ack: {ack}"); + + let failed = relay.next_json().await; + assert_eq!(failed["event"], "target_failed", "failure event: {failed}"); + let error = failed["error"] + .as_str() + .expect("target_failed error string"); + assert!(error.contains("23"), "exit status missing from: {error}"); + assert!( + error.contains("early-child-crash-sentinel"), + "target stderr missing from: {error}" + ); + + tokio::time::timeout(TIMEOUT, relay.child.wait()) + .await + .expect("relay must exit promptly after reporting an early target crash") + .expect("wait() failed"); +} + #[tokio::test(flavor = "multi_thread")] async fn shutdown_is_acked_and_the_process_exits() { let port = { @@ -445,11 +484,11 @@ async fn shutdown_during_port_wait_stops_promptly() { let mut relay = RelayProcess::spawn_capturing_stderr(port).await; relay.expect_ready().await; // A real target that never binds `port` -- port-readiness polling never - // succeeds on its own. cmd.exe rather than powershell.exe: smaller, - // simpler, and this test's target just needs to run for a while and - // never bind `port`, not do anything powershell-specific. + // succeeds on its own. Do not use `timeout.exe` here: it exits immediately + // when its stdin is not attached to a console, which is precisely how the + // relay launches targets. let ack = relay - .launch(1, &["cmd", "/c", "timeout /t 300 /nobreak >nul"]) + .launch(1, &["cmd", "/d", "/c", "ping -n 301 127.0.0.1 >nul"]) .await; assert_eq!(ack["ok"], true, "launch ack: {ack}");