fix(cwd): reject a relative process.cwd on schema 0.9 and stop WSLc silently dropping it - #1147
Elliot (theelliotm) wants to merge 16 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Tilde and whitespace handling still allow some relative cwd values to bypass the new validation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds schema-aware process.cwd validation and prevents WSLc from silently discarding unsupported paths.
Changes:
- Adds backend- and lifecycle-specific absolute-path validation.
- Rejects untranslatable WSLc one-shot paths.
- Regenerates schema/SDK artifacts and updates WSLc documentation.
File summaries
| File | Description |
|---|---|
docs/wsl/wsl-container-getting-started.md |
Documents WSLc cwd behavior. |
schemas/dev/mxc-config.schema.0.9.0-alpha.json |
Updates exact-schema cwd documentation. |
schemas/dev/mxc-config.schema.0.9.0-dev.json |
Updates rolling-schema cwd documentation. |
sdk/node/src/generated/v0_9_0_alpha/wire.ts |
Regenerates exact TypeScript types. |
sdk/node/src/generated/wire.ts |
Regenerates rolling TypeScript types. |
src/backends/hyperlight/common/src/lib.rs |
Updates a test fixture to POSIX syntax. |
src/backends/wslc/common/src/wsl_container_runner.rs |
Rejects untranslatable one-shot cwd values. |
src/core/mxc_config_contract/src/dev/stable.rs |
Documents the exact 0.9 cwd contract. |
src/core/wxc_common/src/models.rs |
Introduces backend-specific path styles. |
src/core/wxc_common/src/validator.rs |
Enforces the schema 0.9 absolute-path rule. |
src/core/wxc_common/src/wire.rs |
Documents rolling-wire cwd behavior. |
Review details
- Files reviewed: 10/12 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Path-validation bypasses, incorrect error classification, late WSLc validation, and a temporary workflow remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/core/wxc_common/src/validator.rs:204
- Validation checks the trimmed value but every non-WSLc backend receives the original string. For example,
" /tmp"passes the Unix check as/tmp, then Bubblewrap receives the relative path" /tmp"; similarly" C:\\work"passes the Windows check before the original is sent to the OS. This preserves the host-relative behavior the change is intended to prevent. Only treat the actual empty-string sentinel as omitted and validate the exact supplied value.
let cwd = request.working_directory.trim();
if cwd.is_empty() || !schema_requires_absolute_cwd(&request.schema_version) {
return Ok(());
src/core/wxc_common/src/models.rs:173
- This classifies
~as absolute for every Unix-style backend, although only Seatbelt expands it. Bubblewrap passes the literal value to--chdir, and LXC passes it as a quotedcdargument, so~/workspaceremains relative there and can bypass the new 0.9 rule. Restrict home-anchored forms to Seatbelt (for example with a distinct style/capability), while generic Unix backends accept only/....
fn is_unix_absolute(path: &str) -> bool {
path.starts_with('/') || path == "~" || path.starts_with("~/")
src/core/wxc_common/src/wire.rs:176
- The generated description introduces the 0.9 contract and links users to
docs/schema.md, but that document's Working Directory section still says only that values are passed through and does not mention the version-gated absolute-path requirement. Update the canonical schema documentation (and the public SDK cwd docs) so consumers discover this breaking validation rule outside generated artifacts.
/// From schema 0.9 on, a supplied value must be absolute for the target the
/// path reaches — `C:\workspace` or a UNC path for the Windows backends,
/// `/workspace` (or a `~` path on Seatbelt) for the Unix ones. WSL
/// Container reads it as a Windows host path one-shot and as an
/// in-container path on a state-aware `exec`. A relative path is rejected
- Files reviewed: 10/12 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
User-controlled diagnostics need escaping, existing documentation is contradictory, and the temporary branch-specific workflow should not be merged.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/core/wxc_common/src/validator.rs:220
cwdis user-controlled and is interpolated withDisplay, so newline, ANSI, bidi, and other invisible format characters reach the diagnostic sink unchanged. This violates the crate's explicit “no raw control/format bytes in diagnostics” guarantee inconfig_deserialize.rs:309-313and permits forged/misleading multi-line errors. Escape the value with the shared helper before formatting it.
src/core/wxc_common/src/wire.rs:178
- The generated description now documents the 0.9 rule, but the user-facing docs still contradict it:
docs/schema.md:243-245says every supplied value is passed to the backend verbatim, anddocs/seatbelt/seatbelt-backend.md:387-390says relative cwd values are resolved against MXC's directory. Update those sections with the 0.9 version gate and Seatbelt's~exception so users do not follow behavior that this validator now rejects.
/// From schema 0.9 on, a supplied value must be absolute for the target the
/// path reaches — `C:\workspace` or a UNC path for the Windows backends,
/// `/workspace` (or a `~` path on Seatbelt) for the Unix ones. WSL
/// Container reads it as a Windows host path one-shot and as an
/// in-container path on a state-aware `exec`. A relative path is rejected
/// because it would resolve against the launching process's working
/// directory.
.github/workflows/Validation.Infrastructure.Tests.yml:6
- This new workflow is hard-coded to a contributor branch and is unrelated to the cwd fix. Once that branch is deleted or this merges, it can never run for
mainor pull requests, leaving a dead permanent workflow; meanwhile the existing scheduled validation workflow already builds all three platforms and invokes the nightly matrix. Remove this temporary validation workflow before merging.
push:
branches:
- user/emichlin/remove-absolute-cwd-0.9
- Files reviewed: 10/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Validation can use a different path style than the backend ultimately selected, leaving host-relative execution possible.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/core/wxc_common/src/models.rs:150
- Treating
~as intrinsically absolute is unsafe when Seatbelt cannot expand it.seatbelt_runner.rs:745-751swallowsexpand_tildeerrors (notably whenHOMEis absent) and returns the literal~;Command::current_dirthen interprets it relative to the launcher, so a host~/directory can still determine the sandbox cwd. Make failed/non-absolute home expansion reject the request rather than falling back to the original relative string.
.github/workflows/Validation.Infrastructure.Tests.yml:6
- This new workflow is unrelated to the cwd fix and is permanently scoped to a contributor branch. Once merged, pushes and pull requests against the repository will never trigger it, leaving an inert duplicate of the existing build/validation workflow in the default branch. Remove this temporary branch-only workflow from the PR.
on:
push:
branches:
- user/emichlin/remove-absolute-cwd-0.9
- Files reviewed: 10/12 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The temporary contributor-branch workflow must be removed, and the cwd rejection diagnostic should not incorrectly claim all targets resolve against the host.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/core/wxc_common/src/validator.rs:218
- This diagnostic is inaccurate for targets such as Windows Sandbox and WSLc state-aware exec: their cwd is resolved inside the guest/container, not against the host process. The surrounding contract already uses “launching process”; use that wording here so callers are not given a false resolution location.
- Files reviewed: 9/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
WSLc still silently truncates cwd values containing an embedded NUL, and contract documentation overstates UNC support.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
src/backends/wslc/common/src/wsl_container_runner.rs:717
- This preflight still accepts a cwd such as
C:\safe\u{0}ignored:windows_path_to_container_pathpreserves the interior NUL, andstart_containerthen passes it as aPCSTR, so the SDK sees only/mnt/c/safe. The state-aware marshalling path already rejects this case incontainer_steps.rs:62-76; reject an interior NUL here as well so one-shot cannot silently run in a different directory.
src/core/mxc_config_contract/src/dev/stable.rs:42 - The exact-contract description likewise implies that WSLc one-shot accepts UNC paths even though its new validator rejects them. Keep this source description aligned with the rolling contract so the regenerated 0.9 schema and TypeScript types accurately document the drive-rooted-only WSLc form.
src/core/wxc_common/src/wire.rs:176 - This description advertises UNC paths for “the Windows backends” and then identifies WSLc one-shot as consuming a Windows host path, but the new WSLc validation explicitly rejects every UNC path. Qualify the generated schema/JSDoc so consumers know that WSLc one-shot accepts only drive-rooted host paths.
- Files reviewed: 9/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
86e3c83 to
f62dc09
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The validation, backend handling, tests, generated artifacts, and documentation consistently implement the stated behavior.
Review details
- Files reviewed: 20/23 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The new repository-wide backend convention must be documented in Copilot instructions, and unrelated LXC documentation churn should be removed.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
docs/lxc-support/lxc-backend.md:1
- This file has no content change; the diff only rewrites every line (apparently line-ending normalization). Please restore its original formatting so this focused fix does not obscure history and future reviews with unrelated whole-file churn.
# LXC Container Backend
- Files reviewed: 21/23 changed files
- Comments generated: 1
- Review effort level: Balanced
| /// Path shape an explicit `process.cwd` must have for this backend on | ||
| /// `scope`. | ||
| /// | ||
| /// Linux falls an unsupported request back to LXC and macOS overrides every | ||
| /// request to Seatbelt. | ||
| pub fn working_directory_style(&self, scope: WorkingDirectoryScope) -> WorkingDirectoryStyle { |
There was a problem hiding this comment.
🟡 Changes recommended
Public Rust, C#, and Node SDK documentation remains incomplete for the newly enforced constraint.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/core/mxc_engine/src/policy.rs:659
- This public Rust SDK behavior change is not reflected in
src/core/mxc-sdk/README.md: its request-building section still only says callers may adjust the working directory. Repository guidance requires Rust SDK API changes to update both the crate docs and README; please document the schema gate and backend-specific absolute forms there so README users do not discover the new rejection only at runtime.
/// Override the working directory the sandboxed child starts in. Left unset,
/// it defaults to the policy's resolution. Must be absolute for the target
/// backend from schema `0.9.0-alpha` on.
sdk/dotnet/Microsoft.Mxc.Sdk/SandboxRequest.cs:42
- The C# README's
SandboxRequestand state-aware exec sections (sdk/dotnet/README.md:192-220and816-836) still presentWorkingDirectorywithout this new constraint. Repository guidance requires the C# README to accompany SDK API behavior changes; please state the 0.9 requirement and clarify that WSLc exec expects a Unix in-container path while Windows-backed requests expect a Windows path.
/// <summary>
/// An optional initial working directory. Must be absolute from schema
/// <c>0.9.0-alpha</c> on.
/// </summary>
- Files reviewed: 21/23 changed files
- Comments generated: 1
- Review effort level: Balanced
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Summary
Reviewed the current PR head 481495b against base 6e12d6d; the merge-base matches the PR base and the locally captured diff matches gh pr diff apart from line-ending size differences.
The target-aware path predicate itself came back clean: the one-shot, streaming, dry-run, and state-aware routes reach the new validation; diagnostic input is escaped; Linux fallback, macOS override, Windows path forms, and the WSLc one-shot/exec split are consistent. I found no security, performance, or cross-platform-parity defects.
Findings outside the diff
Medium (test coverage) - State-aware dispatcher wiring is not regression-tested. src/core/wxc_common/src/state_aware_dispatch.rs:90,123 — newly_exposed_by_change: this file is byte-identical base versus head, but the PR adds a new invariant inside validate_exec_common and relies on these dispatcher calls to enforce it. The added tests call the validator directly; there is no sibling to the existing empty-command-line dispatcher test that passes a relative cwd through streaming or relayed exec and proves rejection occurs before backend validation/execution.
Fix: Add dispatcher-boundary tests for both exec entry points using the call-counting backend, asserting policy_validation and zero backend validation/exec calls.
Medium (proportionality/claim) - The #902 closure and test-coverage claims conflict with the registered regression. PR description — claim_mismatch: the description says “Resolves #902” and “No existing test or example config used a relative cwd,” but tests/regression_tests/test_cases/Invoke-Issue902-RelativeWorkingDirectory.ps1 and its TestCases.psd1 registration are byte-identical base versus head. That test uses 0.8.0-alpha and passes only when relative cwd is rejected, while this PR intentionally leaves 0.8 unchanged. This is non-blocking as a claim mismatch, but the acceptance artifact and description need to agree.
Fix: Re-version the regression to 0.9.0-alpha if that is the intended acceptance contract, or explicitly document that 0.6-0.8 retain the old behavior and correct the PR description.
Verified pre-existing - not attributed to this PR
None posted as findings. The unchanged files above are retained only because the PR newly depends on their dispatcher/error behavior or makes a claim about them; they are not charged as standalone pre-existing defects.
There was a problem hiding this comment.
🟡 Changes recommended
The envelope mapping misclassifies missing backend prerequisites as caller policy errors.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 24/26 changed files
- Comments generated: 1
- Review effort level: Balanced
FailurePhase::Rejected covered both a policy refusal and a missing host prerequisite, so the one-shot error envelope reported a disabled Containers-DisposableClientVM feature as policy_validation -- an invalid caller policy. No policy change makes such a host serve the request. Retag OneShotError::SandboxUnavailable as BackendUnavailable, which the state-aware surface already returns for the same condition, and map that phase to backend_unavailable in the envelope so both surfaces agree. Narrow the Rejected contract to caller-fixable refusals only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new Node integration test can fail before launching MXC, so it does not reliably verify the intended validation.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
sdk/node/tests/integration/common.test.ts:124
- This value is also forwarded as the executor PTY's
cwdbyspawnSandboxAsync(sandbox.ts:817-827), so the test usually fails to spawnwxc-execbecauserelative-subdirdoes not exist and never exercises MXC validation. Use an existing relative directory such as.so the executor starts while the config still carries a relativeprocess.cwd.
- Files reviewed: 25/27 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This PR documents where schema-aware working-directory validation should run on the exact-contract stack through #1186. Details * Compare the current #1147 checks with the #1184-#1186 normalization flow. * Recommend typed compatibility validation before `ExecutionRequest` creation. * Preserve SDK mutation defenses, error classification, and WSLC translation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bc45e3c7-c6f1-4ed8-9d08-fb1e18622401 Generated-with: gpt-5.6-sol
📖 Description
A
process.cwdthe target cannot resolve fails silently today, in two ways:cwdresolves against the host process's working directory. Nothing required it to be absolute, so"cwd": "work"lands whereverwxc-execwas started from — the result depends on the caller's shell, not on the policy.cwdit could not translate, falling through anif let Some(..)with noelseand launching in the container's default directory.The WSLc fix is not version-gated. Silently ignoring a requested
cwdis wrong on every schema version, so an untranslatable path is now rejected on all of them. Only the relative-path rule below is gated on0.9.0-alpha.Absoluteness belongs to whatever consumes the path, so
ContainmentBackenddeclares aWorkingDirectoryStyle(Windows/Unix) perWorkingDirectoryScope(OneShot/Exec):Enforcement sits in
validate_commonandvalidate_exec_common, the choke points every backend passes through. Unlike a check inconfig_parser, they also catch acwdinjected after parsing viaSandboxRequest::set_working_directory— the repro in #873.Notes:
🔗 References
Resolves #902 in schema version 0.9+ — both folded defects, #872 (WSLc drop) and #873 (host-relative resolution), are fixed here.
🔍 Validation
cargo test -p wxc_common --lib validator— 23 passed, covering every backend, each style, cross-style rejection, the WSLc per-phase split, both sides of the version gate, an omittedcwd, and the state-awarepolicy_validationmapping.wxc_common,mxc_engine,wslc_common,nanvix_runner— no new failures; pre-existing sandbox-related failures confirmed identical against a stashed baseline.cargo clippy --workspace --all-targets -- -D warningsandcargo fmt --all -- --check— clean.validate-configs.js(349 configs) pass.The WSLc rejection is not unit-testable; it sits in the
unsafeSDK-backed launch path.Docs: the 0.9 rule lives in the
cwddoc comments that generate the schema descriptions (wire.rs,mxc_config_contract/src/dev/stable.rs), with all four artifacts regenerated. The WSLc behavior change is documented indocs/wsl/wsl-container-getting-started.md.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see docs/pull-requests.md)📋 Issue Type
Microsoft Reviewers: Open in CodeFlow