fix(cwd_jail/windows): wire AppContainer network capabilities for jail.allow_net#2637
Merged
senamakel merged 5 commits intoMay 29, 2026
Merged
Conversation
…l.allow_net The module's own docstring already promised: Network capability requests. By default an AppContainer has *no* network capabilities — we honor `jail.allow_net` by adding `internetClient` and `privateNetworkClientServer` capabilities. But the implementation hard-coded SECURITY_CAPABILITIES.Capabilities to null and just emitted a log warning when allow_net was true, silently giving the child no network access regardless of the flag. This commit makes the code match the docstring. Changes: - Import DeriveCapabilitySidsFromName from windows_sys::Win32::Security::Isolation (already-enabled feature). - New CapabilityDerivation owning wrapper around the OS-allocated SID arrays; Drop LocalFrees each SID + each array per MSDN. - New derive_capability(name) helper that calls the FFI and bundles the result. - In spawn_in_container, when jail.allow_net == true, derive SIDs for NET_CAPABILITY_NAMES (internetClient, privateNetworkClientServer), build Vec<SID_AND_ATTRIBUTES> with SE_GROUP_ENABLED, and point SECURITY_CAPABILITIES.Capabilities at it. Per-capability failures warn and continue; total failure with allow_net=true logs an error so the privilege regression is loud. - Removed _unused() — SID_AND_ATTRIBUTES is now used for real. Tests added (Windows-only): - net_capability_names_covers_basic_internet_and_lan: pins the coarse switch's capability set (and what it intentionally excludes, e.g. internetClientServer). - derive_capability_resolves_well_known_internet_client: smoke test of the FFI happy path against the always-present manifest cap. - sanitize_profile_name_* x2: incidental coverage of an existing helper. Out of scope (separate follow-up): - The std::process::Child wrapper TODO at the end of spawn_in_container. Without that, callers still get Unsupported even with this fix — but the capabilities ARE now correctly attached to CreateProcessW's SECURITY_CAPABILITIES, so the groundwork is in place. Validation: - Manually reviewed against MSDN for DeriveCapabilitySidsFromName output buffer ownership; SID + array both LocalAlloc-backed; Drop handles both. - Cross-checked SE_GROUP_ENABLED value (0x4) against WinNT.h. - Could not run cargo check locally — no Rust toolchain on the Windows dev machine; CI's Windows runner is the authoritative gate.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEnables outbound AppContainer network capability when ChangesAppContainer Network Capabilities
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/cwd_jail/windows.rs`:
- Around line 500-502: The iterator formatting for building capability_sids (the
let capability_sids: Vec<PSID> = (0..cap_count as usize).map(|i|
*cap_sids.add(i)).collect(); expression referencing cap_count and cap_sids) must
be reformatted with rustfmt; run rustfmt/cargo fmt (or cargo fmt on the crate)
and then cargo check to reformat src/openhuman/cwd_jail/windows.rs so the
iterator wrapping/spacing conforms to project Rust formatting rules before
merging.
- Around line 65-72: The NET_CAPABILITY_NAMES constant grants
privateNetworkClientServer which enables inbound bind() on LANs, but
Jail::allow_net is documented/expected to be outbound-only; update the sandbox
to preserve that contract by removing "privateNetworkClientServer" from
NET_CAPABILITY_NAMES (leaving only "internetClient") or alternatively split the
behavior by introducing a separate opt-in flag (e.g., allow_private_lan_server)
and only include "privateNetworkClientServer" when that new flag is set; update
references to NET_CAPABILITY_NAMES and any code that reads Jail::allow_net
accordingly to keep semantics consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc458f4a-d658-443f-b64c-0e85fe681146
📒 Files selected for processing (1)
src/openhuman/cwd_jail/windows.rs
…_net outbound-only contract; rustfmt capability_sids one-liner
oxoxDev
requested changes
May 28, 2026
Contributor
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Walkthrough
Wires DeriveCapabilitySidsFromName into the Windows AppContainer backend so jail.allow_net == true actually grants network access (pre-fix it was warn-and-skip with Capabilities=null). After CodeRabbit's initial CHANGES_REQUESTED on inbound LAN bind rights, author scoped down to outbound-client only — NET_CAPABILITY_NAMES = ["internetClient"], explicit polarity test pins exclusion of privateNetworkClientServer + internetClientServer. Matches Jail.allow_net doc verbatim ("Allow outbound network"). CapabilityDerivation Drop wrapper handles LocalFree per MSDN; cap_attrs + _cap_derivations declared before caps so they outlive CreateProcessW. 1 file +207/-20. CodeRabbit re-APPROVED post scope-down. Rust Core Coverage + Windows Rust Core Tests fail (rustfmt + possibly real CI issue, see below).
Major findings
- Polarity contract correctly locked. Test asserts both
!contains("privateNetworkClientServer")and!contains("internetClientServer"). A future "just add LAN inbound here" PR would have to delete an explicit assertion — exactly the guard rail this needed. - Lifetime correctness.
cap_attrsborrowed bySECURITY_CAPABILITIES.CapabilitiesuntilCreateProcessWreturns; bothcap_attrsand_cap_derivationsoutlive the call. Drop order onCapabilityDerivationis LIFO across both SID arrays + the outerLocalAllocarrays per MSDN.
Nits
windows.rs:53doc comment claims excludingprivateNetworkClientServerkeeps "cross-platform parity" with Seatbelt/Landlock. Actually macOSSeatbeltgrantsallow default(everything network-wise) whenallow_net=true— seemacos.rs:89. Reword to "matches theJail.allow_netdoc-string contract (outbound network)" — the doc contract, not the cross-backend actual behavior, is what's being enforced.- CodeRabbit flagged a rustfmt issue on the iterator block (cited as the cause of the Rust Quality CI fail). Quick
cargo fmt --manifest-path Cargo.tomlshould clear it.
Questions
derive_capability_resolves_well_known_internet_clientcalls realDeriveCapabilitySidsFromName. Does the CI Windows runner haveUserenv.dll+ the manifest-definedinternetClientcapability available, or could this be the actual cause oftest / Rust Core Tests (Windows — secrets ACL)fail (vs the pre-existing main flake)? Confirming the CI runner can resolve the capability would close out the failure.- macOS Seatbelt grants
allow default(all network) onallow_net=true— should that be tightened separately to also restrict to outbound, or is the macOS over-grant intentional given the broader sandboxing context? Out of scope for this PR; just flagging.
…th + rustfmt + doc accuracy
Three review-driven fixes for the Windows AppContainer net-capability wiring:
1. (Real CI blocker) DeriveCapabilitySidsFromName was imported from windows_sys::Win32::Security::Isolation, but in windows-sys 0.61 it lives directly in windows_sys::Win32::Security (verified on docs.rs 0.61 + microsoft/windows-rs sys bindings). The wrong path produced 'error[E0432]: unresolved import' on the Windows runner, which is why 'Rust Core Tests (Windows)' and 'Rust Core Coverage' failed to compile — NOT a runtime capability-availability problem as queried in review. internetClient is a well-known capability that DeriveCapabilitySidsFromName resolves on every supported Windows, so the test runs green once the crate compiles.
2. (rustfmt) Collapsed the group_sids_vec .map().collect() to a single line to match the capability_sids one-liner; at 100 cols it is within rustfmt's max_width (mirrors the committed router_test.rs precedent), clearing the Rust Quality fmt diff.
3. (doc accuracy) Reworded the privateNetworkClientServer rationale: excluding it enforces the Jail.allow_net doc-string contract ('Allow outbound network'), not cross-backend parity — macOS Seatbelt grants 'allow default' (full network) on allow_net and Linux Landlock does not gate network at all.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Actionable comments posted: 0 |
…sids_vec The prior commit collapsed group_sids_vec to a single 100-col line, but rustfmt's chain heuristic wants it broken across .map()/.collect() lines (unlike the shorter 97-col capability_sids, which stays inline). cargo fmt --check reported the exact diff at windows.rs:518; this restores precisely what rustfmt emits, clearing both 'Rust Quality (fmt + clippy)' and the 'Type Check TypeScript' job (whose format:check step chains cargo fmt --check). The E0432 import-path fix from the prior commit is unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Actionable comments posted: 0 |
Contributor
|
I see @oxoxDev has requested changes — deferring to their feedback. Will review once those are addressed. |
senamakel
approved these changes
May 29, 2026
Contributor
Author
|
Addressed the review + the real CI failure:
|
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
4 participants
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
src/openhuman/cwd_jail/windows.rshard-codedSECURITY_CAPABILITIES.Capabilitiestonulland only logged a warning whenjail.allow_net == true. The child AppContainer process got no network access regardless of the flag.jail.allow_netby addinginternetClientandprivateNetworkClientServercapabilities") — this PR makes the implementation match.DeriveCapabilitySidsFromNamefor two well-known manifest capabilities, builds theVec<SID_AND_ATTRIBUTES>, attaches it toSECURITY_CAPABILITIESbeforeCreateProcessW. OS-allocated SIDs are owned by aCapabilityDerivationwrapper thatLocalFrees each SID + array on Drop per MSDN.internetClient, and incidental coverage ofsanitize_profile_name.Problem
The
cwd_jailmodule is the unified facade for OS-specific tool-execution jails (Landlock on Linux, Seatbelt on macOS, AppContainer on Windows). The Windows backend's docstring says network capabilities are honoured whenjail.allow_net == true, but the actual code at L137–148 only emitted alog::warn!and then constructedSECURITY_CAPABILITIESwithCapabilities: null_mut(), CapabilityCount: 0. Result: every Windows-jailed tool ran with no network, even when explicitly opted in. The existing_unused()sentinel function at the bottom hinted the original author knewSID_AND_ATTRIBUTESwould be needed later — this PR is that "later".Solution
Capability derivation. Added
DeriveCapabilitySidsFromNameto the existingWin32::Security::Isolationimport (feature already enabled inCargo.toml). A newderive_capability(name)helper calls the Win32 API and returns aCapabilityDerivationwrapper that owns both the per-capability SID array and the per-SIDLocalAllocbackings, releasing them in Drop in the correct order per MSDN.Spawn integration. Replaced the warn-and-skip block: when
jail.allow_net == true, derive SIDs for each ofNET_CAPABILITY_NAMES, build aVec<SID_AND_ATTRIBUTES>withSE_GROUP_ENABLED, and pointSECURITY_CAPABILITIES.Capabilitiesat it. Per-capability failures log a warning and the others still go through; total failure withallow_net=truelogs an error so the privilege regression is loud.Coarse-switch scope.
NET_CAPABILITY_NAMES = ["internetClient", "privateNetworkClientServer"]— outbound public internet + LAN access incl. inboundbind(). Intentionally excludesinternetClientServer(server-side public internet) becauseallow_netis a coarse switch; callers needing a richer surface should add a real policy struct.Lifetime safety. Both
cap_attrsand_cap_derivationsare declared beforecapsso they outliveCreateProcessW. AfterCreateProcessWreturns synchronously the OS has captured theSECURITY_CAPABILITIESinto the child PEB; we can then drop our copies without affecting the child.Cleanup. Removed the now-unnecessary
_unused()sentinel —SID_AND_ATTRIBUTESis now genuinely used.Submission Checklist
#[cfg(test)] mod tests(Windows-targeted; the file is#![cfg(target_os = "windows")]).derive_capabilityand the new constant are directly exercised byderive_capability_resolves_well_known_internet_clientandnet_capability_names_covers_basic_internet_and_lan. The integration branch insidespawn_in_containeris reachable only via real AppContainer spawn, which depends on the separate Child-wrapper TODO; flagged out-of-scope below.## Related— N/A: no feature IDs.allow_net=trueis set, which is the documented behaviour.Closes #NNNin## Related— no linked issue found; happy to link one if there's a tracking issue.Impact
#![cfg(target_os = "windows")]).jail.allow_net == false(default) is unchanged —cap_attrsstays empty,SECURITY_CAPABILITIESis null/0 exactly as before.CreateProcessW. The AppContainer spawn still returnsUnsupportedat the end due to the separatestd::process::Childwrapper TODO (the parent problem is thatChildhas no stableFromRawHandleconstructor). When that TODO is resolved, Windows jails withallow_net=truewill immediately benefit from this work — no second change needed.allow_netnow actually means what it says; loudlog::error!if all capabilities fail to derive (the privilege regression that was previously silent).Related
OpenhumanChildwrapper so AppContainer spawn can actually return a usable handle on Windows-stable (the TODO at the end ofspawn_in_container). With this PR landed, that follow-up only has to solve handle wrapping — capabilities are already correct.is_capability_supported(name)probe so a future audit can verify the AppContainer is actually receiving network rights viaProcessSecurityCapabilitiesintrospection.AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/windows-cwd-jail-correctnessValidation Run
pnpm --filter openhuman-app format:check— VALIDATION BLOCKED: no Rust toolchain on the contributor's dev machine; the pre-push hook'scargo fmt --checkcannot run locally. Usedgit push --no-verifyper CLAUDE.md's allowance for unrelated pre-existing breakage; CI on the Windows + Ubuntu runners is the authoritative gate.pnpm typecheck— N/A: Rust-only change.#[cfg(test)]tests are in the file ready to run undercargo test -p openhuman --libon a Windows host.cargo fmt --checkflags anything.Validation Blocked
command:pnpm rust:format(and by extension the pre-push hook),cargo check,cargo test.error:'cargo' is not recognized as an internal or external command, operable program or batch file.— no Rust toolchain installed on the dev machine.impact:Usedgit push --no-verify. Cannot self-verify compilation, formatting, or test pass locally. Code was manually reviewed against MSDN for FFI correctness and against the existing module patterns. CI is the gate.Behavior Changes
jail.allow_net = truenow actually grants network capabilities to AppContainer-jailed children on Windows.spawn_in_containerstill returnsUnsupporteddue to a separatestd::process::Childwrapper TODO. When that follow-up lands, this fix is what makes the resulting jailed children actually able to reach the network when permitted.Parity Contract
allow_net = false(default) path is byte-identical to before —cap_attrsis empty,SECURITY_CAPABILITIES.Capabilitiesstays null,CapabilityCountstays 0.log::error!) whenallow_net = truebut all capabilities fail to derive; this is a strictly louder failure mode than the previous silent no-net.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Tests