fix(backends): stop swallowing WinRT/decode failures that corrupt sandbox behavior#570
Open
MGudgin wants to merge 1 commit into
Open
fix(backends): stop swallowing WinRT/decode failures that corrupt sandbox behavior#570MGudgin wants to merge 1 commit into
MGudgin wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens error handling in the Windows Sandbox and IsolationSession backends by removing “false success” paths caused by swallowing WinRT accessor failures and non‑UTF‑8 output bytes, preserving correct failure semantics and daemon output.
Changes:
- Windows Sandbox daemon response parsing now uses lossy UTF‑8 decoding so invalid bytes don’t drop entire stdout/stderr streams.
- IsolationSession lifecycle checks now propagate WinRT
IsError()getter failures instead of coercing them to “no error”. - IsolationSession process stdio handle getters and
IsoSessionError::Code()handling now propagate accessor failures instead of silently defaulting (which previously disabled relays / misclassified stale IDs).
Show a summary per file
| File | Description |
|---|---|
| src/backends/windows_sandbox/common/src/windows_sandbox_runner.rs | Preserve daemon stdout/stderr even when non‑UTF‑8 bytes are present; adds a regression test. |
| src/backends/isolation_session/common/src/manager.rs | Stop coercing WinRT accessor failures (including handle getters) into defaults that mask backend failures. |
| src/backends/isolation_session/common/src/error.rs | Avoid fabricating HRESULT 0; ensure stale-id classification isn’t silently lost on accessor failure. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 4
MGudgin
pushed a commit
that referenced
this pull request
Jun 25, 2026
…) on Code() failure Addresses Copilot review feedback on PR #570. * Prefix the OutputHandle/ErrorHandle/InputHandle getter error strings with the operation name (RunProcessWithOptionsAsync) to match the other error strings in the function. * On a Code() accessor failure in format_iso_error, fold in the best-effort Message() text so the failure stays diagnosable. Remediation() is intentionally left out of this degraded path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Generated-with: claude-opus-4.8
3fccb46 to
d7b45bc
Compare
…dbox behavior 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
d7b45bc to
377185e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three silent failure-swallowing bugs in the IsolationSession and Windows
Sandbox backends. In each case 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—IsError()failures treated as success.The three
err.IsError().unwrap_or(false)lifecycle checks (AddUserAsync/AddUserAsync2/RunProcessWithOptionsAsync) now propagate the WinRTaccessor failure via
map_err(lifecycle_err)?instead of coercing it tofalseand continuing down the success path (which then tried to useAgentUserName()/Process()anyway).isolation_session/manager.rs— process handles silently disabling relays.OutputHandle()/ErrorHandle()/InputHandle()now propagate getterfailures instead of
.unwrap_or(0). Downstream code treats0as "streamabsent", so a getter failure previously skipped the corresponding stdout/
stderr/stdin relay, making a process appear to run while producing no output.
A genuinely returned
0still means "absent" and is preserved.isolation_session/error.rs— fabricatedHRESULT: 0losing stale-idclassification.
format_iso_errornow readsCode()first and propagatesits failure honestly, rather than defaulting to
0, which skipped theERROR_NOT_FOUNDcheck and downgraded a stale sandbox to a genericbackend_error.Message/Remediationremain best-effort (cosmetic).windows_sandbox_runner.rs— daemon output dropped on non-UTF-8. Decodestdout/stderr with
String::from_utf8_lossy(...).into_owned()instead offrom_utf8(...).unwrap_or_default(), so a single invalid byte no longerdiscards the entire stream.
Tests
parse_daemon_response_preserves_non_utf8_output, asserting the lossydecode keeps the surrounding bytes (and a U+FFFD replacement char) instead of
emptying the stream.
cargo test -p isolation_session_common -p windows_sandbox_common— 107 + 17pass.
cargo clippy -p isolation_session_common -p windows_sandbox_common --all-targets -- -D warningsandcargo fmt -p isolation_session_common -p windows_sandbox_common -- --checkboth clean. Both crates are Windows-onlyand were verified on a Windows host.
A follow-up PR (#573) addresses the related non-behavioral idiomatic cleanups
(RAII/ownership wrappers, typed errors over
String,PathBufoverStringfor paths, etc.).