Skip to content

fix(cwd): reject a relative process.cwd on schema 0.9 and stop WSLc silently dropping it - #1147

Open
Elliot (theelliotm) wants to merge 16 commits into
mainfrom
user/emichlin/remove-absolute-cwd-0.9
Open

Elliot (theelliotm) wants to merge 16 commits into
mainfrom
user/emichlin/remove-absolute-cwd-0.9

Conversation

@theelliotm

@theelliotm Elliot (theelliotm) commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

📖 Description

A process.cwd the target cannot resolve fails silently today, in two ways:

  • A relative cwd resolves against the host process's working directory. Nothing required it to be absolute, so "cwd": "work" lands wherever wxc-exec was started from — the result depends on the caller's shell, not on the policy.
  • WSLc one-shot dropped a cwd it could not translate, falling through an if let Some(..) with no else and launching in the container's default directory.

The WSLc fix is not version-gated. Silently ignoring a requested cwd is wrong on every schema version, so an untranslatable path is now rejected on all of them. Only the relative-path rule below is gated on 0.9.0-alpha.

Absoluteness belongs to whatever consumes the path, so ContainmentBackend declares a WorkingDirectoryStyle (Windows/Unix) per WorkingDirectoryScope (OneShot/Exec):

Style Backends Accepts
Windows ProcessContainer, Windows Sandbox, IsolationSession, WSLc one-shot drive-rooted and UNC/device paths
Unix Seatbelt, LXC, Bubblewrap, WSLc exec, MicroVM, Hyperlight, VM a leading / only

Enforcement sits in validate_common and validate_exec_common, the choke points every backend passes through. Unlike a check in config_parser, they also catch a cwd injected after parsing via SandboxRequest::set_working_directory — the repro in #873.

Notes:

  • The relative-path rule applies at schema >= 0.9; 0.6-0.8 and absent/malformed versions are unchanged.
  • The exact supplied string is validated, since that is what the backends receive - " /tmp" is relative.
  • Rejections carry FailurePhase::Rejected, so one-shot and state-aware callers both see policy_validation .
  • Avoids Path::is_absolute, which answers for the compiled host. Windows rejects drive-relative paths (C:foo, \foo).
  • No existing test or example config used a relative cwd.

🔗 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 omitted cwd, and the state-aware policy_validation mapping.
  • 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 warnings and cargo fmt --all -- --check — clean.
  • All four codegen/version gates and validate-configs.js (349 configs) pass.

The WSLc rejection is not unit-testable; it sits in the unsafe SDK-backed launch path.

Docs: the 0.9 rule lives in the cwd doc 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 in docs/wsl/wsl-container-getting-started.md.

✅ Checklist

📋 Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

Copilot AI balanced review requested due to automatic review settings September 12, 2026 06:39
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment thread src/core/wxc_common/src/models.rs Outdated
Comment thread src/core/wxc_common/src/validator.rs Outdated
Comment thread src/core/wxc_common/src/wire.rs Outdated
Copilot AI review requested due to automatic review settings September 12, 2026 06:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 quoted cd argument, so ~/workspace remains 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

Comment thread src/backends/wslc/common/src/wsl_container_runner.rs Outdated
Comment thread src/core/wxc_common/src/validator.rs Outdated
Comment thread .github/workflows/Validation.Infrastructure.Tests.yml Outdated
Copilot AI review requested due to automatic review settings September 12, 2026 07:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

  • cwd is user-controlled and is interpolated with Display, 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 in config_deserialize.rs:309-313 and 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-245 says every supplied value is passed to the backend verbatim, and docs/seatbelt/seatbelt-backend.md:387-390 says 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 main or 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

Copilot AI review requested due to automatic review settings September 12, 2026 07:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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-751 swallows expand_tilde errors (notably when HOME is absent) and returns the literal ~; Command::current_dir then 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

Comment thread src/core/wxc_common/src/models.rs
Copilot AI review requested due to automatic review settings September 14, 2026 20:49
@theelliotm
Elliot (theelliotm) marked this pull request as ready for review September 14, 2026 20:52
@theelliotm
Elliot (theelliotm) requested a review from a team September 14, 2026 20:52
@theelliotm
Elliot (theelliotm) requested a review from a team as a code owner September 14, 2026 20:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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

Copilot AI review requested due to automatic review settings September 14, 2026 20:52
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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_path preserves the interior NUL, and start_container then passes it as a PCSTR, so the SDK sees only /mnt/c/safe. The state-aware marshalling path already rejects this case in container_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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Copilot AI review requested due to automatic review settings September 17, 2026 17:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment on lines +87 to +92
/// 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 {
Copilot AI review requested due to automatic review settings September 17, 2026 17:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 SandboxRequest and state-aware exec sections (sdk/dotnet/README.md:192-220 and 816-836) still present WorkingDirectory without 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

Comment thread sdk/node/src/types.ts

@MGudgin Gudge (MGudgin) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,123newly_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 descriptionclaim_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.

Comment thread src/core/wxc_common/src/validator.rs
Comment thread src/backends/wslc/common/src/wsl_container_runner.rs Outdated
Comment thread docs/schema.md
Comment thread sdk/node/README.md Outdated
Comment thread docs/schema.md Outdated
Comment thread src/backends/hyperlight/common/src/lib.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

Comment thread src/core/wxc_common/src/script_runner.rs
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>
Copilot AI review requested due to automatic review settings September 18, 2026 18:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 cwd by spawnSandboxAsync (sandbox.ts:817-827), so the test usually fails to spawn wxc-exec because relative-subdir does not exist and never exercises MXC validation. Use an existing relative directory such as . so the executor starts while the config still carries a relative process.cwd.
  • Files reviewed: 25/27 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Gudge (MGudgin) pushed a commit that referenced this pull request Sep 19, 2026
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
@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Needs-Attention Issue needs attention from Microsoft label Sep 22, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Cross-backend] process.cwd is silently discarded or resolved against host state

5 participants