fix(appcontainer): default sandbox cwd to a granted path instead of NULL#674
Open
caarlos0 wants to merge 2 commits into
Open
fix(appcontainer): default sandbox cwd to a granted path instead of NULL#674caarlos0 wants to merge 2 commits into
caarlos0 wants to merge 2 commits into
Conversation
When `process.cwd` was empty, the AppContainer and BaseContainer runners passed a NULL current directory to `CreateProcessW`, so the child inherited the host process's cwd. Under a deny-by-default AppContainer token that directory is often unopenable, and the kernel then silently resets the child to the drive root (`C:\`) instead of failing the launch — surfacing as sandboxed shells starting on `C:\` rather than the working directory (notably via the in-process Rust SDK, whose host cwd is the caller's, not a granted path). Mirror the Seatbelt backend's resolver: add `ExecutionRequest::resolved_working_directory()` (explicit `working_directory` wins, else the first `readwrite` path, else the first `readonly` path) and use it in both Windows runners so an unset cwd defaults to a directory the sandbox token can actually open. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a38efe5c-db9c-4ad7-a1c6-537b6cc3df4f Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an AppContainer/BaseContainer Windows behavior where an unset working directory resulted in passing NULL to CreateProcessW, causing the child process to inherit the host cwd and (when inaccessible under a deny-by-default token) silently start at C:\. It adds a shared resolver on ExecutionRequest to choose a policy-granted cwd when the request doesn’t specify one, and uses it in both Windows runners.
Changes:
- Add
ExecutionRequest::resolved_working_directory()to derive an appropriate cwd (explicit wins; else firstreadwritepath; else firstreadonlypath; elseNone). - Use the resolver in both
AppContainerScriptRunnerandBaseContainerRunnerwhen building theCreateProcess*working-directory argument. - Update the schema documentation example to mention the new defaulting behavior.
Show a summary per file
| File | Description |
|---|---|
| src/core/wxc_common/src/models.rs | Adds ExecutionRequest::resolved_working_directory() and unit tests covering precedence and empty-policy behavior. |
| src/backends/appcontainer/common/src/base_container_runner.rs | Uses the resolved working directory when passing the cwd pointer to Experimental_CreateProcessInSandbox. |
| src/backends/appcontainer/common/src/appcontainer_runner.rs | Uses the resolved working directory when passing the cwd pointer to CreateProcessW. |
| docs/schema.md | Updates the config example comment for process.cwd to reflect defaulting behavior when omitted. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
The resolver only picks the first policy path; it does not verify the path exists or is a directory, so drop the "guaranteed to open" wording. Also note that the schema example's defaulting is backend-specific rather than a universal rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a38efe5c-db9c-4ad7-a1c6-537b6cc3df4f Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
bbonaby
reviewed
Jul 23, 2026
Comment on lines
+714
to
+732
| /// Resolve a working directory for the sandboxed child, preferring a | ||
| /// policy-granted path over inheriting the host process's current directory, | ||
| /// for backends that must **not** inherit that cwd. | ||
| /// | ||
| /// An explicit `working_directory` always wins. Otherwise — rather than | ||
| /// leaving it empty and letting the OS inherit the host process's cwd, which | ||
| /// under a deny-by-default sandbox may be inaccessible to the sandboxed | ||
| /// token — we prefer a directory the policy grants: the first `readwrite` | ||
| /// path, else the first `readonly` path. This only picks the path; it does | ||
| /// not verify the path exists or is a directory (the config parser merely | ||
| /// warns on missing paths), so the launch can still fail if it doesn't. | ||
| /// Returns `None` when neither an explicit directory nor any policy path is | ||
| /// set, leaving the caller on the backend's own default. | ||
| /// | ||
| /// This matters most on Windows: passing a `NULL` current directory to | ||
| /// `CreateProcessW` makes the child inherit the parent's cwd, and when the | ||
| /// AppContainer token can't open it the kernel silently resets the child to | ||
| /// the drive root (`C:\`) instead of failing the launch. The macOS Seatbelt | ||
| /// backend already avoids the same trap with an equivalent resolver. |
Collaborator
There was a problem hiding this comment.
note: can we trim this comment down, I think sometime the agent over comments?
Comment on lines
+836
to
+840
| // Working directory. An explicit cwd wins; otherwise fall back to the | ||
| // first granted path so the sandboxed child starts somewhere the | ||
| // AppContainer token can open. A NULL current directory would inherit | ||
| // this process's cwd, and if the AppContainer can't open it the kernel | ||
| // silently resets the child to the drive root (C:\). |
Collaborator
There was a problem hiding this comment.
issue: I feel like instead of commenting in both places we can have some information in resolved_working_directory for it. There might already be. If someone in the future comes a long and wonders what resolved_working_directory is used for they can see the comment on top of that function.
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.
📖 Description
Sandboxed shells were starting on
C:\instead of the intended working directory on the Windows AppContainer + DACL (and BaseContainer) backends.Root cause: when
process.cwdwas empty, both Windows runners passed aNULLcurrent directory toCreateProcessW, so the child inherited the host process's cwd. Under a deny-by-default AppContainer token that directory is often unopenable, and the kernel then silently resets the child to the drive root (C:\) instead of failing the launch. This surfaced most via the in-process Rust SDK, whose host cwd is the caller's process directory rather than a granted path (the Node SDK happened to inherit the executor's cwd, which is usually the granted workspace).Fix: mirror the macOS Seatbelt backend, which already avoids this trap. Add
ExecutionRequest::resolved_working_directory()— explicitworking_directorywins, else the firstreadwritepath, else the firstreadonlypath — and use it in both Windows runners. An unset cwd now defaults to a policy-granted path (firstreadwrite/readonly) instead ofNULL. The resolver only picks the path; it does not verify the directory exists, so an explicit-but-ungranted (or missing) cwd still fails loudly — parity with Seatbelt — rather than silently landing onC:\.Scope note: only the AppContainer/BaseContainer family was affected. Seatbelt already had an equivalent resolver; the micro-VM backends (NanVix/Hyperlight) reject a working directory by design; the Linux/WSL backends fall back to
// the container root.🔗 References
🔍 Validation
resolved_working_directory()(explicit wins, first readwrite, first readonly, none) —cargo test -p wxc_commonpasses (397 tests).cargo checkandcargo clippy -- -D warningsonappcontainer_commonforx86_64-pc-windows-msvc— clean.cargo check -p mxc-sdkandcargo fmt --all -- --check— clean.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type