From 377185e7c63da72b58297c6e9734ca2f83e84eac Mon Sep 17 00:00:00 2001 From: Gudge Date: Fri, 26 Jun 2026 09:11:53 -0700 Subject: [PATCH] fix(backends): stop swallowing WinRT/decode failures that corrupt sandbox behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes three silent failure-swallowing bugs in the IsolationSession and Windows Sandbox backends where a failed WinRT accessor or a non-UTF-8 byte was coerced to a default, turning a real failure into a false success or lost output. Details * isolation_session/manager.rs: the three `err.IsError().unwrap_or(false)` lifecycle checks (AddUserAsync / AddUserAsync2 / RunProcessWithOptionsAsync) now propagate the WinRT accessor failure via `map_err(lifecycle_err)?` instead of coercing it to `false` and proceeding down the success path. * isolation_session/manager.rs: the `OutputHandle()`/`ErrorHandle()`/ `InputHandle()` getters now propagate failures (op-prefixed for log legibility) instead of `.unwrap_or(0)`, which downstream treats as "no handle" and silently skips the stdio relay. A genuinely returned 0 still means "absent stream" and is preserved. * isolation_session/error.rs: `format_iso_error` reads `Code()` first and propagates its failure honestly rather than fabricating `code = 0`, which skipped the `ERROR_NOT_FOUND` check and downgraded a stale sandbox to a generic backend error. On that degraded path it still folds in the best-effort `Message()` text; `Remediation()` stays best-effort. * windows_sandbox_runner.rs: daemon stdout/stderr are decoded with `String::from_utf8_lossy` instead of `from_utf8(...).unwrap_or_default()`, so a single invalid byte no longer discards the entire stream. Tests * Added `parse_daemon_response_preserves_non_utf8_output` asserting the lossy decode keeps surrounding bytes (+ U+FFFD) instead of emptying the stream. * `cargo test -p isolation_session_common -p windows_sandbox_common` — 107 + 17 pass. * `cargo clippy -p isolation_session_common -p windows_sandbox_common --all-targets -- -D warnings` and `cargo fmt ... -- --check` both clean. Both crates are Windows-only and were verified on a Windows host. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Generated-with: claude-opus-4.8 --- .../isolation_session/common/src/error.rs | 18 ++++++++- .../isolation_session/common/src/manager.rs | 40 ++++++++++++++++--- .../common/src/windows_sandbox_runner.rs | 31 +++++++++++++- 3 files changed, 80 insertions(+), 9 deletions(-) diff --git a/src/backends/isolation_session/common/src/error.rs b/src/backends/isolation_session/common/src/error.rs index 4c241788c..89599f6ed 100644 --- a/src/backends/isolation_session/common/src/error.rs +++ b/src/backends/isolation_session/common/src/error.rs @@ -61,8 +61,24 @@ const ERROR_NOT_FOUND_HRESULT: u32 = 0x80070490; /// Formats an `IsoSessionError` into a typed `IsolationSessionError`. /// Promotes `ERROR_NOT_FOUND` to `Stale`. pub(super) fn format_iso_error(op: &str, err: &IsoSessionError) -> IsolationSessionError { + // Read `Code()` first and propagate its failure honestly: it is the + // classification-critical field (it drives the `Stale` promotion below), + // so fabricating 0 on a getter failure would silently downgrade a stale + // sandbox to a generic lifecycle error. `Message`/`Remediation` are + // cosmetic and stay best-effort. + let code = match err.Code() { + Ok(c) => c.0 as u32, + Err(e) => { + // Code() is gone, but Message() may still carry signal — fold in + // the best-effort text so the failure is diagnosable. + let msg = err.Message().map(|h| h.to_string()).unwrap_or_default(); + return IsolationSessionError::Lifecycle(format!( + "{} failed: {} (could not read HRESULT code: {})", + op, msg, e + )); + } + }; let msg = err.Message().map(|h| h.to_string()).unwrap_or_default(); - let code = err.Code().map(|h| h.0 as u32).unwrap_or(0); let remediation = err.Remediation().map(|h| h.to_string()).unwrap_or_default(); let suffix = if remediation.is_empty() { String::new() diff --git a/src/backends/isolation_session/common/src/manager.rs b/src/backends/isolation_session/common/src/manager.rs index 0d6bbf6c3..586aacd27 100644 --- a/src/backends/isolation_session/common/src/manager.rs +++ b/src/backends/isolation_session/common/src/manager.rs @@ -137,7 +137,9 @@ impl IsolationSessionManager { let err = user_result .Error() .map_err(|e| lifecycle_err(format!("AddUserAsync: get Error failed: {}", e)))?; - let is_error = err.IsError().unwrap_or(false); + let is_error = err + .IsError() + .map_err(|e| lifecycle_err(format!("AddUserAsync: get IsError failed: {}", e)))?; if is_error { return Err(format_iso_error("AddUserAsync", &err)); } @@ -177,7 +179,9 @@ impl IsolationSessionManager { let err = user_result .Error() .map_err(|e| lifecycle_err(format!("AddUserAsync2: get Error failed: {}", e)))?; - let is_error = err.IsError().unwrap_or(false); + let is_error = err + .IsError() + .map_err(|e| lifecycle_err(format!("AddUserAsync2: get IsError failed: {}", e)))?; if is_error { return Err(format_iso_error("AddUserAsync2", &err)); } @@ -303,7 +307,12 @@ impl IsolationSessionManager { e )) })?; - let is_error = err.IsError().unwrap_or(false); + let is_error = err.IsError().map_err(|e| { + lifecycle_err(format!( + "RunProcessWithOptionsAsync: get IsError failed: {}", + e + )) + })?; if is_error { return Err(format_iso_error("RunProcessWithOptionsAsync", &err)); } @@ -333,9 +342,28 @@ impl IsolationSessionManager { // Lifetime: relay-param structs are stack-allocated; we wait on // every spawned thread (INFINITE for stdout/stderr, bounded for // stdin) before this function returns. - let stdout_handle_val = process.OutputHandle().unwrap_or(0); - let stderr_handle_val = process.ErrorHandle().unwrap_or(0); - let stdin_handle_val = process.InputHandle().unwrap_or(0); + // A getter that errors is a backend failure, not an absent stream: + // propagate it rather than coercing to 0, which downstream treats as + // "no handle" and silently skips the corresponding stdio relay. A + // genuinely returned 0 still means absent and is preserved. + let stdout_handle_val = process.OutputHandle().map_err(|e| { + lifecycle_err(format!( + "RunProcessWithOptionsAsync: get OutputHandle failed: {}", + e + )) + })?; + let stderr_handle_val = process.ErrorHandle().map_err(|e| { + lifecycle_err(format!( + "RunProcessWithOptionsAsync: get ErrorHandle failed: {}", + e + )) + })?; + let stdin_handle_val = process.InputHandle().map_err(|e| { + lifecycle_err(format!( + "RunProcessWithOptionsAsync: get InputHandle failed: {}", + e + )) + })?; let wxc_stdout = unsafe { GetStdHandle(STD_OUTPUT_HANDLE) } .map_err(|e| lifecycle_err(format!("GetStdHandle(stdout) failed: {}", e)))?; diff --git a/src/backends/windows_sandbox/common/src/windows_sandbox_runner.rs b/src/backends/windows_sandbox/common/src/windows_sandbox_runner.rs index eb08eb0d4..71a335c0f 100644 --- a/src/backends/windows_sandbox/common/src/windows_sandbox_runner.rs +++ b/src/backends/windows_sandbox/common/src/windows_sandbox_runner.rs @@ -167,8 +167,12 @@ impl WindowsSandboxScriptRunner { if response_line.starts_with("RESULT ") { match DaemonResult::parse(response_line) { Ok(result) => { - let stdout_text = String::from_utf8(result.stdout).unwrap_or_default(); - let stderr_text = String::from_utf8(result.stderr).unwrap_or_default(); + // Lossy decode: the daemon transports raw bytes, so a + // single invalid UTF-8 byte must not discard the entire + // stream (which `String::from_utf8(...).unwrap_or_default()` + // would do). Replace invalid sequences with U+FFFD instead. + let stdout_text = String::from_utf8_lossy(&result.stdout).into_owned(); + let stderr_text = String::from_utf8_lossy(&result.stderr).into_owned(); ScriptResponse { exit_code: result.exit_code, standard_out: stdout_text, @@ -236,4 +240,27 @@ mod tests { daemon_pipe_name_to_port("custom-pipe") ); } + + #[test] + fn parse_daemon_response_preserves_non_utf8_output() { + // A single invalid UTF-8 byte must not discard the whole stream. + let line = DaemonResult { + exit_code: 0, + stdout: vec![b'h', b'i', 0xFF, b'!'], + stderr: vec![0xFE, b'e', b'r', b'r'], + error_message: String::new(), + } + .to_line(); + + let resp = WindowsSandboxScriptRunner::parse_daemon_response(line.trim()); + + assert_eq!(resp.exit_code, 0); + // U+FFFD replacement char is kept; the surrounding bytes survive. + assert!(resp.standard_out.contains("hi")); + assert!(resp.standard_out.contains('!')); + assert!(resp.standard_out.contains('\u{FFFD}')); + assert!(resp.standard_err.contains("err")); + // error_message falls back to stderr text when empty. + assert!(resp.error_message.contains("err")); + } }