Repository navigation
[Bubblewrap] Find host tools in standard system directories when PATH omits them - #1578
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
Node’s normalization of symlink-parent PATH entries can make support detection select a different executable from native execution.
1 open issue
What changed in this PR
This PR addresses #1545 by letting Bubblewrap find host tools in standard system directories when the caller’s PATH omits them, without changing the sandboxed workload’s environment.
Changes:
- Adds PATH-first tool discovery and passes checked absolute paths through validation and launch.
- Updates Node support probing and diagnostics to use the same fallback directories.
- Adds restricted-PATH regression coverage, expands Linux CI checks, and documents lookup behavior.
| File | Description |
|---|---|
tests/scripts/run_bwrap_version_gate_test.sh |
Tests fallback lookup and rejection of unusable launchers. |
tests/scripts/run_bwrap_network_proxy_test.sh |
Tests proxy discovery and execution with restricted or unset PATH. |
tests/scripts/run_bwrap_firewall_test.sh |
Tests firewall enforcement without host tools on PATH. |
src/mxc-sdk/src/core/mxc_engine/platform.rs |
Updates version-failure test fixtures. |
src/mxc-sdk/src/backends/bubblewrap/common/proxy_network.rs |
Passes checked tool paths into network supervision. |
src/mxc-sdk/src/backends/bubblewrap/common/mod.rs |
Registers the host-tool lookup module. |
src/mxc-sdk/src/backends/bubblewrap/common/host_tools.rs |
Implements shared lookup and focused tests. |
src/mxc-sdk/src/backends/bubblewrap/common/bwrap_version.rs |
Retains the selected launcher path and improves diagnostics. |
src/mxc-sdk/src/backends/bubblewrap/common/bwrap_runner.rs |
Launches using validated executable paths. |
sdk/node/tests/unit/platform.test.ts |
Adds lookup precedence and failure tests. |
sdk/node/src/v1/platform.ts |
Configures fallback directories and path-aware probe results. |
sdk/node/src/bwrap-probe-worker.ts |
Forwards fallback directories to the probe process. |
sdk/node/src/bwrap-probe-helper.ts |
Resolves and probes an absolute launcher path. |
sdk/node/src/bwrap-probe-anchor.ts |
Passes lookup configuration to the helper. |
docs/development/build-and-test/ci-validation-infrastructure.md |
Documents expanded lookup validation. |
docs/backends/bwrap/bubblewrap-backend.md |
Explains tool search order and failure behavior. |
.github/workflows/Build.Linux.Job.yml |
Adds host-tool and version unit tests. |
Review effort: Balanced
|
|
||
| if (child) { | ||
| child.stdout.on('data', (chunk: Buffer) => { | ||
| async function probe(): Promise<void> { |
There was a problem hiding this comment.
thought: thinking about things, we probably should make sure all the probes specific to each sdk happen in one place e.g through rust. I just realized this was happening through node directly. So probably not for this PR but we probably want to clean that part up.
The thinking would be that the non-rust sdks are thin wrappers that call into the mxc_ffi dll which has access to the rust code/modules internally. Then all sdks share common code.
Gudge (MGudgin)
left a comment
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Summary
Reviewed e3e3e7d against the PR merge base cf7d037: 17 changed files, with the captured diff matching GitHub's PR diff after line-ending normalization. The branch is behind the current main base (ad54c694); the findings below concern the PR's changes, not intervening changes on main.
Two Medium, nonblocking findings are attached inline.
Verified clean: Rust and Node both search PATH before the fixed fallback directories and ignore empty/relative PATH entries (src/mxc-sdk/src/backends/bubblewrap/common/host_tools.rs:10-17,63-73 and sdk/node/src/bwrap-probe-helper.ts:60-69). The Node-specific normalization exception is finding 2.
Findings outside the diff
None. Both comments are anchored to added lines, including the new documentation claim for finding 1.
Verified pre-existing - not attributed as a new PR defect
Generic bwrap probe errors already omitted the selected executable path before this change: the merge-base Rust classify_spawn_failure returns only err.to_string() for non-ENOENT errors, and Node's completed-failure mapping retains status/stderr but no selected path. Finding 1 is explicitly a claim_mismatch against the new diagnostic promise, not a claim that this PR introduced the underlying generic error behavior or a merge condition by itself.
Verification: base/head source comparisons, added-line anchor checks, and a read-only Node POSIX path-normalization check. The Linux symlink scenario and backend/SDK suites were not rerun for filing.
e3e3e7d to
ec5b61f
Compare
Gudge (MGudgin)
left a comment
Gudge (MGudgin)
left a comment
There was a problem hiding this comment.
Both findings from my previous review are addressed at ec5b61fe2.
- Finding 1 (
claim_mismatch): the backend guide and PR description now accurately limit which failures name the selected executable. The pre-existing generic probe diagnostics are not being treated as a new defect. - Finding 2 (
introduced_by_change): Node now appends the executable name without normalizing PATH components. The new symlink-followed-by-..regression exercises the real probe helper; I verified its passing result in the Linux SDK unit-test log for run40729355116at this exact head.
Local verification on Windows: npm run build:test-unit passed, and node --test dist-tests\tests\unit\platform.test.js passed with 89 passed, 0 failed, and 21 skipped. The Linux-only regression was skipped locally as expected; local WSL access was denied by sandbox policy, so Linux execution evidence comes from CI. Backend suites were not rerun locally.
No outstanding findings from my review.
… omits them Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…and narrow the error docs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
bb0580b
ec5b61f to
bb0580b
Compare
📖 Description
Bubblewrap looked up every host program it needs (
bwrap,slirp4netns,unshare,nsenter,ip, theiptablestools,shandsleep) by name on the caller'sPATH. Apps started from a desktop session often have aPATHwithout/usr/sbinand/sbin. On hosts with everything installed, proxy-only and firewall network modes were then reported unavailable or refused with "requires 'iptables' on PATH".MXC now searches the caller's
PATHfirst, then/usr/local/sbin,/usr/local/bin,/usr/sbin,/usr/bin,/sbinand/bin.PATHat launch. The Node SDK'sgetPlatformSupport()uses the same search.iptablesbackend the sandbox cannot drive, the request fails and MXC does not fall back to another copy. The error names that file for the network tools, and for abwrapthat is not executable, is missing its interpreter or loader, or is too old.PATHentries are ignored, so a tool is never taken from the current directory.sleepis now checked before launch, like the other network tools.No API or schema changes. When a tool is missing, the error now lists the directories searched. Tools installed elsewhere, such as
/optor a Nix profile, still need to be onPATH.🔗 References
Resolves #1545. Related: #1494, microsoft/vscode#342498, and the downstream workaround microsoft/vscode#343645.
🔍 Validation
New Rust and Node unit tests cover the search order and its failure cases. The version-gate, proxy and firewall suites now also run with no host tools on
PATH, and the Linux build job now runs thehost_toolsandbwrap_versionunit tests. The new version-gate case (bwrapmissing fromPATH) and the proxy suite's no-tools case both fail on unchangedmain. A Node test checks that aPATHentry with..after a symlink selects the samebwrapas the native lookup; it fails if the entry is normalized withpath.join.Ubuntu 24.04 (WSL), from
src/:cargo fmt --all -- --check: passedcargo clippy -p mxc-sdk -p lxc --all-targets -- -D warnings: passedcargo test -p mxc-sdk --lib -- bubblewrap::common::: 407 passedcargo test -p mxc-sdk --lib -- mxc_engine::platform: 9 passedUbuntu 24.04 (WSL), from the repo root with
PATH=/usr/local/bin:/usr/bin:/bin:tests/scripts/run_bwrap_version_gate_test.sh: passedtests/scripts/run_bwrap_network_proxy_test.sh: passedtests/scripts/run_bwrap_firewall_test.sh: passedLXC_EXEC=<debug lxc-exec> tests/scripts/run_bwrap_resolver_isolation_test.sh: passedWindows:
cargo clippy -p mxc-sdk --all-targets -- -D warnings(fromsrc/): passedcargo test -p mxc-sdk --lib -- bwrap_version host_tools(fromsrc/): 29 passednpm test(fromsdk/node/, Node 24): 437 passed, 23 skippednode scripts/versioning/check-linux-probe-timeouts.js: passedUbuntu, Node 20, from
sdk/node/:node --test dist-tests/tests/unit/platform.test.js: 79 passed, 31 skippedNot run: the Bubblewrap scripts that need
sudoor root, the Node integration tests, and a macOS build.✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See pull request builds.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See pull request builds
for the steps.