feat(windows-sandbox): implement production-identity readiness probe - #3161
feat(windows-sandbox): implement production-identity readiness probe#3161liugddx wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughSummaryThis PR replaces Windows launcher-file availability checks with a production-identity readiness probe. The launcher adds The runtime extends the existing launcher as the source of truth. It does not create a parallel availability path. Startup warms a memoized cache keyed by launcher path. Synchronous This is the smallest coherent implementation of the production-boundary check. Lifecycle serialization, cleanup, settlement, timeout handling, and cache states are necessary to prevent stale or concurrent readiness profiles and repeated process creation. No code or tests can be removed from the supplied changes without weakening failure handling, lifecycle safety, memoization, or regression coverage. Complexity delta
The added complexity is necessary for the fail-closed security boundary. Total maintenance complexity stays justified. Validation
Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe launcher adds a production-identity readiness probe. Runtime availability now depends on its cached result. A PowerShell smoke test validates repeatability and argument handling. The workflow and RFC documents record the probe and preview status. ChangesWindows sandbox readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR makes readiness checks authoritative and cached, but the predictable readiness coordination object still does not validate the security policy of an existing object, which could allow unintended interference with readiness results. The change is mergeable with explicit security-owner follow-up. Sequence Diagram(s)sequenceDiagram
participant Runtime as Sandbox manager
participant Launcher as Windows launcher
participant Profile as Readiness AppContainer profile
participant Job as Kill-on-close Job
participant Child as cmd.exe probe child
Runtime->>Launcher: run --readiness-probe
Launcher->>Profile: create deterministic readiness profile
Launcher->>Job: create kill-on-close Job
Launcher->>Child: launch constrained cmd.exe
Launcher->>Child: verify exact AppContainer SID and Job membership
Launcher->>Child: wait for timeout and successful exit
Launcher-->>Runtime: return readiness result
Runtime->>Runtime: cache result and report availability
``
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 5</summary>
<details>
<summary>✅ Passed checks (5 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| Description check | ✅ Passed | The description clearly explains the problem, implementation, deferred work, AI contribution, and verification results, but it omits the template checklist. |
| Title check | ✅ Passed | The title clearly and concisely identifies the main change: implementing a production-identity readiness probe for the Windows sandbox. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Ai Use Disclosure | ✅ Passed | The PR selects substantive generative tooling and names Claude Fable 5 and its scope; all five PR commits contain one standalone `Generated-by: Claude Fable 5` trailer. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/sandbox/default-sandbox-manager.ts (1)
47-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winShare the Windows readiness cache across availability checks.
Each
createWindowsReadinessProbecall creates a separate cache. Backend checks from different managers can therefore runspawnSyncrepeatedly.isBuiltinFilesystemWorkerSandboxAvailablestill reportstruewhen only the launcher exists. Cache readiness byclientPathat module scope and use it in both paths. Add tests for repeated backend checks and failed readiness exits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 966c663d-9334-41d4-a58c-1f1761d6cc5f
📒 Files selected for processing (7)
.github/workflows/windows-sandbox-w0.ymldocs/architecture/windows-sandbox-rfc-v1.mddocs/architecture/windows-sandbox-rfc-v1.zh-CN.mdexperiments/windows-sandbox/launcher/src/main.rsexperiments/windows-sandbox/launcher/src/windows_launcher.rsexperiments/windows-sandbox/readiness-probe-smoke.ps1packages/runtime/src/sandbox/default-sandbox-manager.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
There was a problem hiding this comment.
Pull request overview
This PR strengthens Windows sandbox “availability” by replacing a file-presence check with a real production-identity readiness probe, ensuring the runtime only reports the Windows AppContainer backend as available when the host can actually stand up and enforce the boundary. It also adds CI smoke coverage for the probe and updates the RFC (EN + zh-CN) to align documented guarantees with the shipped preview slice.
Changes:
- Runtime: Windows backend
isAvailablenow runs a memoized--readiness-probe(spawn + timeout) instead ofexistsSync. - Launcher: implements
--readiness-probethat creates the AppContainer identity + kill-on-close Job and verifies confinement via a throwaway child. - CI/docs: adds a Windows readiness-probe smoke test and updates the RFC preview-status sections to reflect the implemented guarantees and deferred gates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/sandbox/default-sandbox-manager.ts | Switches Windows backend availability to a memoized --readiness-probe spawn with timeout. |
| experiments/windows-sandbox/readiness-probe-smoke.ps1 | Adds a smoke test to validate readiness probe success, repeatability, and argument rejection. |
| experiments/windows-sandbox/launcher/src/windows_launcher.rs | Implements the production-identity readiness probe and child confinement verification logic. |
| experiments/windows-sandbox/launcher/src/main.rs | Adds a --readiness-probe CLI entrypoint with argument rejection. |
| docs/architecture/windows-sandbox-rfc-v1.md | Updates RFC status and adds preview implementation status clarifying enforced vs deferred gates. |
| docs/architecture/windows-sandbox-rfc-v1.zh-CN.md | Same as EN RFC update, in zh-CN. |
| .github/workflows/windows-sandbox-w0.yml | Wires the new readiness-probe smoke test into the Windows sandbox W0 CI lane. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Review-fix round pushed as b794b7b. Summary:
Local gate: cargo fmt --check clean, cargo build --locked clean, readiness-probe-smoke.ps1 green under pwsh 7 (exit 0 / repeatable / argument-rejecting), @maka/core + @maka/runtime build clean, default-sandbox-manager tests 4/4, biome clean. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/runtime/src/sandbox/default-sandbox-manager.ts:66
probeWindowsReadiness()correctly fail-closes on--readiness-probe, but the runtime-host still gates filesystem-worker enablement viaisBuiltinFilesystemWorkerSandboxAvailable()(see packages/runtime-host/src/server/execution-composition.ts:307), which forwin32currently only checksbuiltinWindowsClientPath(...) !== undefined(file presence). On hosts where the launcher exists but AppContainer is disabled/policy-blocked, this can incorrectly treat the sandbox as "available" and defer failure until first launch, undermining the goal of replacing file-presence availability.
Consider wiring isBuiltinFilesystemWorkerSandboxAvailable() (and/or builtinWindowsClientPath) to the same readiness probe (ideally reusing the module-scope cache) so all availability/enablement paths agree and fail closed consistently.
function probeWindowsReadiness(clientPath: string): boolean {
const cached = windowsReadinessCache.get(clientPath);
if (cached !== undefined) return cached;
let available = false;
if (existsSync(clientPath)) {
try {
const result = spawnSync(clientPath, ['--readiness-probe'], {
timeout: WINDOWS_READINESS_PROBE_TIMEOUT_MS,
windowsHide: true,
stdio: 'ignore',
});
available = result.error === undefined && result.status === 0;
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Request changes on the current head b794b7be8.
I agree with replacing file presence with a real production-identity probe, but the current ownership and failure lifecycle are not merge-ready yet:
-
isBuiltinFilesystemWorkerSandboxAvailable()still treats the Windows launcher file as sufficient, and Runtime Host uses that helper to construct/publish the filesystem worker. The real readiness result is deferred until the firstSandboxManager.transform()call. This keeps two availability authorities: a host can advertise the managed worker even though the probe will reject it on first use. Please run readiness once at Runtime Host composition and make worker registration and the backend consume the same result. -
That deferred check is a synchronous
spawnSyncwith a 15-second timeout. On the first filesystem operation it can block the Runtime Host event loop before the operation's abort/timeout handling is active. Readiness should be an async startup/composition gate, not work performed inside the synchronousisAvailable()contract. -
The native probe does not reuse the production settlement path. After
ResumeThread, SID/Job verification failures and child timeout return without explicitly terminating and draining the child. Closing the probe Job is not sufficient in the exact failure case where the child is found outside that Job. Route every post-resume outcome through the existing terminate/drain settlement primitive before closing handles and deleting the profile. -
The Node timeout can terminate the launcher after it has registered a nonce-named AppContainer profile, bypassing
Drop. The profile has no ledger/reconciliation entry, andDeleteAppContainerProfilefailures are ignored, so repeated failed probes can leave unrecoverable registrations. Cleanup needs a deterministic, retryable lifecycle rather than relying on process destruction.
The Windows smoke is useful and proves the happy path on the runner, but the existing default-manager test only calls selectInitial() with a text file named .exe; it never exercises the new probe or Runtime Host composition. Please add coverage for failed readiness, one shared probe result, no worker publication on failure, non-blocking startup, and timeout cleanup.
There was a problem hiding this comment.
English
Review update
Request changes. I agree with and defer to hqhq1025’s review for the runtime/native lifecycle findings:
- file presence remains a second availability authority and the worker is published before readiness is known;
- the deferred synchronous probe can block the Runtime Host event loop;
- post-resume failures do not consistently terminate and drain the child through the production settlement path;
- timeout or process termination can leave nonce-named AppContainer profiles without reconciliation.
I have removed the overlapping availability, test-coverage, and native-lifecycle discussion from this review. The findings below are additional merge gates not covered there.
Important — align the RFC’s unavailable diagnostics with shipped behavior
The RFC still states that readiness failures return a stable typed unavailable reason and expose setup version/failure stage:
The implementation runs the probe with stdio: 'ignore', retains only a boolean, and ultimately reports generic backend_not_available. Section 6.5 says unimplemented guarantees are explicitly deferred, but structured unavailable reasons and diagnostics are absent from that deferral list.
Please either mark these guarantees as later gates in both RFC versions or implement and propagate a structured probe result. Documenting them as deferred is the smaller fix for this PR.
Merge-policy gate — complete the required AI attribution
CONTRIBUTING.md requires Generated-by: trailers for commits containing material AI-authored content. Both commits currently contain only Co-Authored-By: Claude Fable 5.
Please also complete the PR template’s AI-use selection and tool scope, and ensure the final squash commit retains the required trailer.
Validation
All currently reported GitHub checks pass, including windows_sandbox_w0_protocol, typecheck, runtime tests, packaging, Windows baseline, and recovery. I also verified cargo fmt --check, Biome on the changed TypeScript, and git diff --check.
The native probe was not rerun locally on Windows; the real Windows CI lane is the evidence for that behavior.
简体中文
Review 更新
Request changes。 对于 runtime/native lifecycle 问题，我认同并以 hqhq1025 的 review 为主：
- 文件存在性仍然是第二个 availability authority，worker 在 readiness 尚未确认时就会被发布；
- 延迟执行的同步 probe 可能阻塞 Runtime Host event loop；
ResumeThread之后的失败没有统一经过生产 settlement path 来终止并 drain child；- timeout 或进程终止可能留下没有 reconciliation 的 nonce-named AppContainer profile。
我已从本 review 中删除重复的 availability、测试覆盖和 native lifecycle 讨论。下面只保留对方 review 尚未覆盖的额外合并门禁。
Important — RFC 的 unavailable diagnostics 与实际行为不一致
RFC 仍然声明 readiness 失败会返回 stable typed unavailable reason，并暴露 setup version/failure stage：
实际实现使用 stdio: 'ignore' 运行 probe，只保留 boolean，最终统一报告 backend_not_available。§6.5 声称尚未实现的保证均已显式列为 later gates，但 structured unavailable reason 和 diagnostics 没有出现在 deferral 列表中。
请在双语 RFC 中把这些保证标记为 later gates，或者实现并传播结构化 probe 结果。对本 PR 而言，明确标记 deferred 是更小的修复。
合并政策门禁 — 补齐 AI attribution
CONTRIBUTING.md 要求包含 AI 实质创作内容的 commit 使用 Generated-by: trailer。当前两个 commit 都只有 Co-Authored-By: Claude Fable 5。
还需要按 PR 模板选择 AI use、说明工具与使用范围，并确保最终 squash commit 保留要求的 trailer。
验证
当前 GitHub checks 全部通过，包括 windows_sandbox_w0_protocol、typecheck、runtime tests、package、Windows baseline 与 recovery。本地还验证了 cargo fmt --check、变更 TypeScript 的 Biome check，以及 git diff --check。
本机没有重新运行真实 Windows native probe；该行为以 Windows CI lane 为验证证据。
The Windows AppContainer backend advertised availability from file existence alone (`existsSync(clientPath)`), which never proves the host can actually stand up the sandbox identity. Replace that with a real readiness probe (RFC §6.4) and align the RFC to the shipped behavior. - launcher: add `--readiness-probe`, which creates the real AppContainer identity/token and a kill-on-close Job and launches a throwaway confined child, failing closed if the host cannot create or enforce the boundary. - runtime: back the Windows backend's `isAvailable` with a memoized spawnSync of `--readiness-probe`, so `auto`/`require` fail closed on hosts where the OS cannot create the boundary rather than trusting the packaged binary's presence. - CI: add readiness-probe-smoke.ps1 to the W0 lane (exit 0, repeatable, argument-rejecting). - docs: align RFC §6.4/§6.5 (EN + zh-CN) — mark the readiness probe as implemented and enforced, and keep private desktop, full per-profile policy at readiness, and launcher signature/version as later gates. Advances apache#2142 Phase 4 (align the RFC's guarantees with the shipped slice). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…entity spoofing Address adversarial review on the production-identity readiness probe: - Make the throwaway AppContainer profile name unique per invocation with a monotonic nonce (SystemTime nanos + PID) so a recycled PID can never collide with a still-registered profile and deadlock the probe (finding A). - Prove enforcement, not mere presence: verify the confined child runs under the SPECIFIC requested Job (IsProcessInJob against our handle) and carries the EXACT requested AppContainer SID (TokenAppContainerSid + EqualSid), failing closed otherwise. "some job / some AppContainer" is not evidence the boundary we asked for is real (finding B). - Cache Windows readiness at module scope keyed by client path so backends sharing a launcher share one probe result; distinct paths stay independent (finding D). - Sync RFC (EN + zh) Updated: date to 2026-08-17 (finding C). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
…eadiness probe deterministically Address maintainer review on the readiness-probe PR: - Availability: make `isBuiltinFilesystemWorkerSandboxAvailable(win32)` consume the memoized readiness probe instead of treating launcher-file presence as a second authority. Runtime Host composition calls this at startup, warming the readiness cache so the backend's later synchronous `isAvailable()` on the transform hot path hits the cache rather than spawning on the event loop. Export `probeWindowsReadiness` with an injectable spawn seam and cover it with unit tests (clean/non-zero/spawn-error/external-timeout/memoized/missing). - Settlement: route the readiness child's post-resume outcomes (verification failure, timeout, clean exit) through explicit TerminateProcess plus the existing `terminate_and_drain_job` primitive, so a child found outside the Job is settled rather than relying on Job close as the only backstop. - Cleanup: give the readiness AppContainer profile one fixed, self-reconciling name that best-effort-deletes any leftover before create, so a profile leaked by an externally-killed probe is reclaimed by the next probe. Replaces the PID+nonce scheme. - Docs: mark the structured unavailable-reason / diagnostics surface as a deferred later gate in both RFC language variants (§6.4/§6.5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
b794b7b to
4506446
Compare
|
Thanks both — pushed
Test coverage. Added a
简体中文感谢两位 review,已推
测试覆盖。 新增
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/runtime/src/sandbox/default-sandbox-manager.ts (1)
28-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
spawnSyncblocks the caller for up to 15 seconds.
isBuiltinFilesystemWorkerSandboxAvailableruns during startup composition, andprobeWindowsReadinessis synchronous. On a pathological host the event loop stalls for the fullWINDOWS_READINESS_PROBE_TIMEOUT_MSwindow before the first cached answer exists.The launcher already bounds itself at 10 seconds and settles its own child, so the outer 15-second budget only covers spawn overhead. Consider warming the cache from an async path at startup and keeping the sync call as a cache read, or reduce the outer budget so the worst-case stall is smaller.
Disposition: follow-up. The sync
SandboxBackend.isAvailablecontract forces the blocking call here, so this is not a defect in this PR.Note on the static analysis hint for the
node:child_processimport: it does not apply. The argument list is a literal array, no shell is used, andclientPathcomes fromresourcesPath, not from user input.Source: Linters/SAST tools
experiments/windows-sandbox/launcher/src/windows_launcher_tests.rs (1)
102-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOne assertion in this test cannot fail.
appcontainer_profile_nameis a pure SHA-256 derivation, so Line 109 restates that a hash of a constant input is constant. It protects no observable behavior.Line 112 is the assertion worth keeping. It guards the real invariant: the readiness identity must never collide with a production request identity.
Disposition: optional.
♻️ Proposed trim
#[test] - fn readiness_profile_name_is_fixed_and_self_reconciling() { - // The readiness profile uses one stable name so a profile leaked by an - // externally-killed probe is reclaimed by the next probe rather than - // accumulating under a unique per-invocation name. - let first = appcontainer_profile_name(READINESS_PROFILE_REQUEST_ID); - let second = appcontainer_profile_name(READINESS_PROFILE_REQUEST_ID); - assert_eq!(first, second); - // It must stay distinct from any production request identity so the two - // lifecycles never share a registration. - assert_ne!(first, appcontainer_profile_name("request-one")); - } + fn readiness_profile_identity_never_collides_with_a_request_identity() { + // The readiness and production profile lifecycles must never share a + // registration, so their derived names must differ. + let readiness = appcontainer_profile_name(READINESS_PROFILE_REQUEST_ID); + assert_ne!(readiness, appcontainer_profile_name("request-one")); + }As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3bf7e7c-4dea-4abf-b83f-5df9fa67d4a2
📒 Files selected for processing (6)
docs/architecture/windows-sandbox-rfc-v1.mddocs/architecture/windows-sandbox-rfc-v1.zh-CN.mdexperiments/windows-sandbox/launcher/src/windows_launcher.rsexperiments/windows-sandbox/launcher/src/windows_launcher_tests.rspackages/runtime/src/__tests__/default-sandbox-manager.test.tspackages/runtime/src/sandbox/default-sandbox-manager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/architecture/windows-sandbox-rfc-v1.zh-CN.md
- docs/architecture/windows-sandbox-rfc-v1.md
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, leaving the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) open — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. Enforce the desktop-scoped half under an explicit no-Win32k contract: each AppContainer launch (production create + readiness probe) now stands up a per-launch alternate desktop on the existing window station, with a protected DACL that grants only the launching user, Local System, and the child's AppContainer SID the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate) and never DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal. STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - This keeps the worker off the interactive Default desktop, so it cannot enumerate/post messages to the user's interactive windows or install desktop hooks against them. - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work (no-Win32k contract). A verified Low mandatory- integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own desktop name and refuses to run on Default; appcontainer-smoke asserts the private-desktop name prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up review on current head 4506446f2.
The previous availability-authority split is fixed, the hot operation path now reads the warmed cache, AI attribution/RFC deferrals are corrected, and the ordinary post-resume path now attempts explicit child/Job settlement. However, the fixed readiness-profile design introduces new lifecycle blockers:
-
The readiness and production profile namespaces still overlap.
READINESS_PROFILE_REQUEST_IDis the valid production request IDreadiness-probe, and both paths call the sameappcontainer_profile_name(request_id).LaunchRequest::validateaccepts that exact value. A production launch using it therefore gets the same profile/SID thatcreate_readiness()best-effort deletes and recreates. This invalidates the claim that the readiness identity can never inherit production filesystem ACEs. Please give readiness a profile namespace that production requests cannot enter, and enforce that separation rather than testing against one sample ID (request-one). -
The fixed profile is not serialized across processes.
windowsReadinessCacheis process-local, whilecreate_readiness()performs delete → create andDroplater deletes the same global profile without a named mutex/lease. Two Maka processes, app instances, side-by-side installs, or direct probe invocations can therefore delete or drop each other's active profile, race creation, and produce a transient failure that is then cached for the process lifetime. Please add cross-process ownership/serialization and a concurrent real-Windows test. -
Settlement failure is still discarded when verification also fails. In
match (verify, settled),(Err(error), _)ignoressettled = Err(...);readiness_probe()then closes the Job and unconditionally drops/deletes the fixed profile. That is the exact state where the child/Job was not proven drained. Preserve the unsettled state and do not delete/reuse the profile until cleanup is proven, matching the production path'sLaunchFailure::Unsettledownership rule. -
Every negative readiness result is cached permanently. Timeout, antivirus/file contention, spawn failure, and the profile race above are transient, but one occurrence disables the Windows sandbox until Runtime Host restart. Positive caching is sound; negative results need a bounded retry/TTL/backoff or an explicit async startup state machine whose backend hot path remains cache-only.
The added TypeScript tests cover boolean mapping and same-process memoization, but they do not exercise external launcher termination/profile reclaim, two-process contention, namespace collision, or combined verify+settlement failure. The W0 smoke only runs two successful probes sequentially, so it also does not cover those paths.
Current verification: full repository build, focused runtime/runtime-host tests, Biome, cargo fmt --check, diff check, and merge check passed; all current GitHub checks are green. These lifecycle cases remain unverified and block approval.
Address four readiness-probe lifecycle blockers on the availability path: 1. Reserve the `readiness-probe` request_id in protocol and reject it in LaunchRequest::validate; derive the probe profile under a disjoint `maka.readiness.` namespace so no production launch can resolve to the profile the probe deletes and recreates. 2. Serialize the whole delete->create->probe->settle->drop window across processes with the DACL-hardened named mutex the ACL ledger uses (LedgerLock), scoped by SID under Global\Maka.WindowsSandbox.ReadinessProfile.v1, failing closed on timeout. 3. Preserve the AppContainer registration on drop when the Job could not be proven empty (keep_on_drop + preserve()), mirroring the production Unsettled contract, instead of deleting a possibly-live identity. 4. Cache negative Windows readiness for a bounded TTL (60s) rather than the process lifetime, and make the synchronous isAvailable hot path strictly cache-only (readCachedWindowsReadiness) so it never spawns on the event loop; positive results stay cached permanently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/architecture/windows-sandbox-rfc-v1.md (1)
197-224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winfix-now — Do not attribute readiness behavior to PR
#2961.Lines 199-203 identify PR
#2961as the shipped preview slice. Lines 216-224 then list the readiness probe and readiness lifecycle as enforced in that slice. The supplied PR objective states that this PR adds those features.Keep the
#2961baseline separate. Attribute readiness to this change after it merges, or mark it as pending until then.Disposition: fix-now.
As per path instructions, report concrete risks and keep the documented source of truth coherent.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 888f801c-35cf-44cb-980e-629c9bdda5ac
📒 Files selected for processing (8)
docs/architecture/windows-sandbox-rfc-v1.mddocs/architecture/windows-sandbox-rfc-v1.zh-CN.mdexperiments/windows-sandbox/launcher/src/acl_ledger.rsexperiments/windows-sandbox/launcher/src/protocol.rsexperiments/windows-sandbox/launcher/src/windows_launcher.rsexperiments/windows-sandbox/launcher/src/windows_launcher_tests.rspackages/runtime/src/__tests__/default-sandbox-manager.test.tspackages/runtime/src/sandbox/default-sandbox-manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture/windows-sandbox-rfc-v1.zh-CN.md
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, leaving the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) open — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. Enforce the desktop-scoped half under an explicit no-Win32k contract: each AppContainer launch (production create + readiness probe) now stands up a per-launch alternate desktop on the existing window station, with a protected DACL that grants only the launching user, Local System, and the child's AppContainer SID the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate) and never DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal. STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - This keeps the worker off the interactive Default desktop, so it cannot enumerate/post messages to the user's interactive windows or install desktop hooks against them. - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work (no-Win32k contract). A verified Low mandatory- integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own desktop name and refuses to run on Default; appcontainer-smoke asserts the private-desktop name prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
Thanks — all four lifecycle blockers are addressed in 1. Namespace overlap → dedicated, enforced readiness namespace.
2. Cross-process serialization.
3. Unsettled state preserved.
4. Bounded negative caching + strictly cache-only hot path.
Validation on this head: 中文四条 lifecycle blocker 均已在
本 head 验证:cargo fmt/build/test(45 通过,含 4 个新 readiness 测试)、W0 readiness+appcontainer 冒烟、本地全量 PS 冒烟回归、runtime 的 node --test + biome。#3174 已 rebase 到本 head(内容不变,已复验)。 |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up review on current head c8b6527d2.
The namespace collision, cross-process serialization, and combined verification/settlement handling from my previous review are substantially improved. Two lifecycle blockers remain:
-
The 60-second negative TTL does not produce recovery in a running Runtime Host.
probeWindowsReadiness()is warmed only whilecreateExecutionRuntimeHostComposition()is built. If that initial probe fails, the filesystem worker and its launch-spec provider are not constructed. Later backend checks only callreadCachedWindowsReadiness(), which never spawns, and there is no timer, retry owner, or dynamic worker publication path. Expiry therefore changes the cached read from “negative” to “absent”, but nothing re-probes until the Runtime Host/composition is restarted. Please either add a real async readiness state machine that retries and publishes the worker, or document and test restart-required recovery rather than claiming TTL recovery. -
preserve()does not make an unsettled readiness identity safe to reuse. On settlement failure, the profile is preserved only untilreadiness_probe()returns; the named-mutex lease is then released. The next probe acquires the lease andcreate_readiness()unconditionally callsDeleteAppContainerProfilefor that same fixed identity while the previous child may still be alive outside the drained Job. The delete result is ignored, and creation proceeds. Please quarantine an unsettled identity durably until cleanup is proven, or use unique probe identities plus an orphan/reconciliation ledger. Add a real Windows test covering an unsettled child followed by another probe.
CI is fully green, and local build/focused tests/Biome/Rust formatting and cross-target checks pass. Those checks do not exercise either failure path above, so this revision is not ready to merge yet.
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
Follow-up review on c8b6527d2
I completed a full review of the current PR head, including the runtime availability authority, Runtime Host composition lifecycle, native AppContainer/Job/profile lifecycle, protocol namespace, CI smoke coverage, and both RFC variants.
No additional merge blocker needs a duplicate comment. The production recovery gap is already covered precisely by hqhq1025’s current-head review: the negative entry expires, but the Runtime Host has no retry owner and the filesystem worker was permanently omitted from the immutable composition. I independently confirmed that finding and consider it blocking.
The unsettled-profile reclaim concern in the same review is technically valid, but I would classify it as a follow-up under the current threshold: it requires native settlement itself to fail, the child is a fixed no-roots readiness command, and the production namespace is structurally disjoint. The implementation and RFC should still stop claiming that the mutex serializes reclaim against a potentially surviving descendant.
I added two non-blocking follow-ups:
- pass
/dtocmd.exeso machine/userCommand Processor\AutoRunconfiguration cannot contaminate readiness; - remove the now-redundant reserved production
requestId, because themaka.sandbox.*/maka.readiness.*namespace split already proves non-collision.
I did not duplicate CodeRabbit’s existing RFC-attribution or cross-user mutex-squatting comments.
Correctness verdict: not ready until the production recovery behavior matches the TTL claim, or the design explicitly adopts restart-required recovery.
Design verdict: the recovery authority must live either in a retrying/dynamically published Runtime Host state or in a deliberately one-shot startup decision. The current composition mixes the latter ownership model with TTL state from the former.
Validation on exact head c8b6527d22603c07aa9be4487ed8cc230bcb5132: 15/15 focused runtime tests passed, Biome passed, cargo fmt --check passed, Windows GNU cross-target cargo check --locked passed, git diff --check passed, and all current GitHub checks are green. Native Windows behavior was not rerun locally; the Windows 2025 CI lane remains the execution evidence.
简体中文
c8b6527d2 后续 review
我已完成当前 PR head 的全量复审，覆盖 runtime availability authority、Runtime Host composition 生命周期、原生 AppContainer/Job/profile 生命周期、协议命名空间、CI smoke 覆盖和双语 RFC。
没有需要重复发布的新 blocker。生产恢复缺口已经由 hqhq1025 针对当前 head 的 review 精确覆盖：负缓存虽然会过期，但 Runtime Host 没有 retry owner，filesystem worker 也已从不可变 composition 中永久省略。我已独立确认该问题，并认为它阻塞合并。
同一 review 中 unsettled profile 的 reclaim 问题在技术上成立，但按当前门槛我会将其归为 follow-up：它需要原生 settlement 本身失败，child 是固定且无 roots 的 readiness 命令，生产命名空间也已结构性隔离。不过，实现和 RFC 仍不应声称 mutex 能把 reclaim 与一个可能存活的 descendant 串行起来。
我新增了两个非阻塞 follow-up：
- 为
cmd.exe添加/d，避免机器或用户的Command Processor\AutoRun配置污染 readiness； - 删除现在已经冗余的生产
requestId保留值，因为maka.sandbox.*/maka.readiness.*命名空间分离已经证明不会碰撞。
我没有重复 CodeRabbit 已有的 RFC 归属和跨用户 mutex squatting 评论。
Correctness 结论： 在生产恢复行为与 TTL 声明一致，或者设计明确改为“恢复需要重启”之前，不宜合并。
Design 结论： 恢复 authority 应当明确归属于可重试、可动态发布的 Runtime Host state，或者明确采用一次性启动决策。当前 composition 使用后者的 ownership，却又维护了属于前者的 TTL state。
精确 head c8b6527d22603c07aa9be4487ed8cc230bcb5132 的验证结果：focused runtime tests 15/15 通过，Biome 通过，cargo fmt --check 通过，Windows GNU cross-target cargo check --locked 通过，git diff --check 通过，当前 GitHub checks 全绿。本地没有重新运行原生 Windows 行为；该部分仍以 Windows 2025 CI lane 为执行证据。
| { | ||
| return Err("requestId must use 1-128 safe ASCII characters".to_owned()); | ||
| } | ||
| if self.request_id == RESERVED_READINESS_REQUEST_ID { |
There was a problem hiding this comment.
English
Follow-up — remove the redundant reserved request ID
The new structural namespace split already makes the identities disjoint: every production request derives maka.sandbox.*, while readiness derives maka.readiness.*. Even the literal requestId == "readiness-probe" cannot resolve to the readiness profile.
This validation branch therefore adds no remaining collision protection, but it introduces a forbidden protocol value, an extra test/documentation contract, and coupling between production request validation and an internal probe seed. Keep the readiness seed private and rely on the existing namespace-disjointness test; the reservation can disappear.
简体中文
Follow-up — 删除冗余的保留 request ID
新的结构性命名空间已经保证 identity 不相交：所有生产请求都派生为 maka.sandbox.*，readiness 则派生为 maka.readiness.*。即使生产请求的字面值是 requestId == "readiness-probe"，也不可能解析到 readiness profile。
因此，这个 validation 分支已经不再提供额外的碰撞保护，却新增了一个协议禁用值、一组测试/文档契约，以及生产请求校验对内部 probe seed 的耦合。可以把 readiness seed 保持为私有常量，依靠现有的 namespace-disjointness 测试，并删除这层 reservation。
There was a problem hiding this comment.
Keeping this one deliberately, as defense in depth rather than redundancy — reasons: (1) the namespace split is the structural guarantee, but it lives in windows_launcher.rs; the validate() rejection is the only guard at the protocol boundary itself, and it keeps the two files independently safe against drift (a future edit that unifies the prefixes would otherwise silently re-open the collision); (2) without the rejection, a production request literally named readiness-probe would work while aliasing the probe's identity in logs, ledger names, and diagnostics — rejecting it with a typed error is clearer than letting the ambiguity exist; (3) the cost is four lines plus one test. If you feel strongly this should go, I'd rather drop it in a follow-up than churn this stack again — but my recommendation is to keep it.
中文:有意保留作为纵深防御:命名空间分离是结构性保证但在另一文件,validate 拒绝是协议边界上唯一的守卫,防止未来前缀统一类改动静默重开碰撞;且不拒绝的话生产请求可合法叫 readiness-probe,在日志/账本命名上与探针身份混淆。成本仅四行+一测。若坚持删除建议放后续 PR,不再折腾本 stack。
The readiness probe launched its throwaway child as `"<cmd>" /c exit 0` without `/d`, so cmd.exe ran the machine-wide `Software\Microsoft\Command Processor\AutoRun` value before the payload. Because AutoRun is machine-constant, a host that ships an AutoRun which exits non-zero would make *every* probe report failure, and one that ships a blocking AutoRun would hang the child until the Job drain times out -- either way the Windows sandbox fails closed on that machine on every startup, independent of whether the OS can actually stand up the boundary. The probe would then be measuring the host's shell customization instead of the sandbox. Route the command line through a pure `readiness_probe_command_line` helper that emits `"<cmd>" /d /c exit 0`, matching the `/d` the production launch path already passes, and add a unit test asserting `/d` precedes `/c`. Also correct the RFC (EN + zh): the readiness probe and its serialized profile lifecycle are added by this PR (apache#3161), not the merged apache#2961 preview slice -- tag those two bullets accordingly and note the AutoRun-disabled command line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
Thanks @Astro-Han — acted on this at head P2-2 ( P2-1 (transient startup failure → restart-scoped recovery) — unchanged, explicitly deferred. As you noted, this is documented and tested as the RFC §6.5 "Active running-host readiness recovery" gate: recovery = new composition build or restart. If the team wants in-process recovery instead, that's the async readiness state machine + dynamic worker publication follow-up — out of scope for this PR. P3 RFC attribution — fixed. §6.5 no longer credits the readiness probe to merged #2961: the two readiness bullets are now tagged P3 tautological assertions — acknowledged, not churned. Agreed the two equal-hash assertions in P3 positive-cache-forever / composition-time One meta note: rather than only patching the quoted lines, we also swept both PRs for any remaining spots where documented claims exceed what the code enforces — that sweep is what surfaced the residual overclaimed comment fixed on #3174. 中文P2-2（缺 P2-1（瞬时启动失败→重启级恢复）— 不改，显式延后。 如你所述已按 RFC §6.5 "Active running-host readiness recovery" 门禁文档化+测试化：恢复 = 新 composition 构建或重启；进程内恢复属异步状态机+动态 worker 发布的后续工作。 P3 RFC 归属 — 已修。 §6.5 两条 readiness 条目标注 P3 同义反复断言 — 认同，不为此重推。 双哈希相等确实无证明力；该测试有效的是前缀+长度断言，真契约在 disjoint + validate 两测试。后续 readiness 变更时顺手收掉。 P3 正结果永久缓存 / composition 期 spawnSync — 已文档化的权衡，不改。 另：本轮没有只改被引用的行，而是对两个 PR 全量清扫了"声明超出代码强制"的残留——#3174 上又扫出并修掉了一处过度声明注释。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The readiness proof now matches the boundary it protects: the launcher verifies the AppContainer identity and Job assignment on the actual host, Runtime warms that result before admitting the filesystem worker, and the backend hot path consumes the cached authority instead of running a second probe. Failure and concurrency paths remain fail-closed, with focused evidence for SID mismatch, Job membership, timeouts, and profile cleanup.
I found no new reproducible P0-P2 issue on the latest head. The remaining hardening ideas are non-blocking and overlap existing review discussion; the live checks are green.
Review performed with Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified the conclusion against the latest head and current main.
中文评论
当前 readiness proof 与受保护边界一致：launcher 在真实 host 上验证 AppContainer identity 与 Job assignment，Runtime 在 filesystem worker admission 前预热结果，backend 热路径只消费这一缓存权威，不再运行第二套探测。SID mismatch、Job membership、timeout 和 profile cleanup 的失败与并发路径均保持 fail-closed，并有针对性证据。
最新 head 上未发现新的可复现 P0-P2 问题。其余强化建议均为非阻塞，且已与现有 review 讨论重合；实时检查全部通过。
本次审查使用了 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已依据最新 head 和当前 main 复核结论。
The named-lock comment claimed a squatted Global\ name "fails closed at acquisition", but that only held for a squatter with a restrictive DACL: CreateMutexW ignores the supplied security descriptor when the name already exists, so a local user pre-creating the predictable mutex name with a *permissive* DACL would hand the launcher an attacker-owned arbitration object -- readiness and ACL-ledger operations would then block on a mutex the squatter can hold forever (availability DoS), while the code read as if the DACL had rejected them. Close the gap where it is checkable: when CreateMutexW reports ERROR_ALREADY_EXISTS (also the normal same-user contention path), read the existing object's owner via GetSecurityInfo before any wait and require it to be the current user or SYSTEM. Ownership is the one property a permissive squatter cannot forge -- re-owning an object to another SID requires SeTakeOwnership/SeRestore, which standard users do not hold. Any other owner fails closed with an explicit squatted-mutex error instead of blocking. The 10-way concurrency smoke (same-user contention) passes unchanged; the cross-user negative path cannot be exercised by an unprivileged CI runner and is enforced by the owner check itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
Head is now 中文head 更新为 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The new cross-user mutex-squatting defense is the right problem, but the current identity rule rejects the legitimate owner produced by the elevated Windows runner and fails the required W0 check.
The first-principles fix is not to widen trust to the whole Administrators group. The creator and validator should share one explicit owner identity: set the mutex security descriptor owner to the intended SID, then require that exact SID on reuse. This preserves fail-closed cross-user protection without breaking same-user elevated acquisition.
Reviewed with Codex using two independent review passes and DeepSeek V4 Flash as an external adversarial pass; I verified the owner path and live required-check failure against this exact head.
中文
防止跨用户 mutex squatting 的问题定义正确，但当前 identity 规则会拒绝 elevated Windows runner 产生的合法 owner，并导致 required W0 失败。
第一性原理下，不应直接扩大为信任整个 Administrators group。创建方与校验方应共享一个显式 owner identity：安全描述符设置预期 SID，复用时只接受该 SID。这样既保持跨用户 fail-closed，也不破坏同用户 elevated acquisition。
本次由 Codex 两轮独立审查，并使用 DeepSeek V4 Flash 做外部对抗审查；我核对了当前 head 和 required check。
| return Err(error); | ||
| } | ||
| if already_exists { | ||
| if let Err(error) = validate_existing_lock_owner(handle, user_sid) { |
There was a problem hiding this comment.
P1 — The new owner rule rejects legitimate elevated acquisitions. Required Windows CI reports the existing mutex owner as S-1-5-32-544 (Administrators), not TokenUser or SYSTEM, so the same workflow fails before waiting on its lock. Set an explicit expected owner SID when creating the mutex and validate that exact identity on reuse (or consistently use the token's actual owner); do not broadly accept arbitrary Administrators owners. Add repeated elevated acquisition coverage and rerun W0.
…ting locks
The squatted-lock owner check rejected locks owned by
BUILTIN\Administrators (S-1-5-32-544), which broke on elevated hosts:
an elevated administrator token stamps the Administrators group -- not
the user SID -- as the default owner on objects it creates, so a mutex
this very code created earlier on the elevated CI runner failed the
{user, SYSTEM} check and the W0 protocol lane went red
(unsettled_launch_preserves_grants_and_quarantines_the_ledger).
Accept the process token's default-owner SID (TokenOwner) as a third
legitimate owner. This stays inside the threat model: only an elevated
administrator can create objects owned by Administrators, and RFC
S1/S5 explicitly does not defend against administrators. Standard-user
squatters are still rejected -- they cannot forge either the user SID
or the Administrators owner. Adds a unit test pinning the helper to
{user SID, S-1-5-32-544} so both elevation states stay covered.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
CI red on 中文:CI 红因新 owner 校验对提权宿主过严——提权管理员 token 创建对象的默认 owner 是 |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Follow-up review on current head 0df509bc3.
No remaining merge-blocking finding.
Problem and mechanism
The PR fixes a real availability-authority bug: launcher-file presence did not prove that the host could create and enforce the AppContainer identity, exact SID, specific Job assignment, and child lifecycle. The implementation now probes those host invariants once at composition time, shares the result with the cache-only backend hot path, and fails closed instead of advertising a worker whose boundary cannot be created. That problem definition and mechanism follow first principles and avoid a second availability authority.
The previous blockers are resolved or honestly scoped: running-host recovery is explicitly deferred rather than claimed; ineffective profile preservation was removed; readiness and production identities are structurally disjoint; lifecycle mutation is serialized; /d makes the shell probe hermetic; and the existing-lock owner check closes the permissive mutex-squatting path within the stated non-administrator threat model.
Optimality and deletion
For this preview scope, the current architecture is sufficient and a dynamic readiness state machine or durable orphan ledger would be disproportionate unless in-process recovery or unsettled-identity quarantine becomes a product requirement. The reserved readiness-probe request ID and its test are technically redundant after the namespace split, and the deterministic-hash equality assertions can be removed, but both are non-blocking maintenance nits. No production code must be deleted before merge.
Verification
All live GitHub checks are green, including Windows W0, recovery, package, installed Windows CLI, runtime-host, E2E, and test. On this exact head I also ran cargo fmt --check, Windows MSVC cross-target cargo check --locked, the full repository build, focused runtime readiness/Windows sandbox tests (22 passed), Biome on the changed TypeScript, diff checks, and merge-conflict checks.
Residual risk
A transient negative still requires a new composition or Runtime Host restart; unsettled readiness identities are not durably quarantined; cross-user owner validation is not exercised by unprivileged CI; and mixed elevated/non-elevated same-user instances can fail closed on an Administrators-owned pre-existing mutex. These are availability/hardening gaps, not false-positive sandbox admission, and are documented or outside the current threat model.
Merge verdict: ready to merge.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up review on current head 0df509bc3208430a125ebabbbcbcea9cced2e954. The token default-owner adjustment resolves the elevated-runner regression without weakening the cross-user mutex-squatting boundary: the accepted owners remain the current user, SYSTEM, or an owner already inside the stated elevated-administrator threat model. The readiness probe still verifies the actual AppContainer SID and Job membership, the Runtime composition warms one shared availability authority before admission, and all failure paths remain fail-closed.
All current CI is green, including the Windows W0 evidence lane. I found no reproducible P0-P2 issue on this head.
AI-assisted review disclosure: Codex verified the final diff, launcher identity and mutex lifecycle, Runtime cache/admission ordering, documented threat model, focused tests, and live CI. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs; speculative hardening ideas were treated as non-blocking.
中文复核
已在最新 head 0df509bc3208430a125ebabbbcbcea9cced2e954 复核。token default owner 的调整修复了 elevated Windows runner 回归，同时没有放宽跨用户 mutex 抢注边界：允许的 owner 仍限定为当前用户、SYSTEM，或已经处于文档化管理员威胁模型内的 owner。readiness probe 仍验证真实 AppContainer SID 与 Job membership，Runtime 在 admission 前预热并复用唯一可用性权威，失败路径保持 fail-closed。当前全部 CI（包括 Windows W0 evidence）为绿色，未发现可复现的 P0-P2。
本次为 AI 辅助审查：Codex 核验最终 diff、launcher identity/mutex 生命周期、Runtime cache/admission 顺序、威胁模型、聚焦测试与实时 CI；另使用两次独立 reviewer 及一次 OpenCode Go DeepSeek V4 Flash（high）对抗审查，推测性 hardening 未作为阻塞项。
|
/agentic_review |
Code Review by Qodo
1. Cold managers report unavailable
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| function createWindowsReadinessProbe(clientPath: string): () => boolean { | ||
| return () => readCachedWindowsReadiness(clientPath); | ||
| } | ||
|
|
||
| function builtinWindowsBackend( | ||
| platform: SandboxPlatform, | ||
| resourcesPath: string | undefined, |
There was a problem hiding this comment.
2. Cold managers report unavailable 🐞 Bug ≡ Correctness
A Windows backend created by createBuiltinSandboxManager('win32', resourcesPath) uses a cache-only
isAvailable callback without warming the cache, so direct consumers—including the existing factory
test—receive false or backend_not_available even when the packaged launcher exists. Only the
Runtime Host filesystem-worker composition path invokes
isBuiltinFilesystemWorkerSandboxAvailable() to warm the cache, making backend availability depend
on an undocumented, unrelated API call order rather than actual readiness.
Agent Prompt
## Issue description
The Windows backend created by `createBuiltinSandboxManager` reads only the readiness cache, but factory construction does not populate that cache. A directly created manager therefore reports the packaged backend unavailable until the unrelated filesystem-worker availability helper happens to run the spawning probe.
## Issue Context
Consolidate readiness initialization in the existing built-in manager construction seam rather than adding another public warm-up contract or readiness state. When constructing a Windows built-in manager, reuse the existing `probeWindowsReadiness` function—or equivalent single construction-time initialization—before exposing the cache-only backend; keep hot-path availability reads synchronous and non-spawning, eliminate the external caller-order precondition, and make the existing cold-cache factory test deterministic.
## Fix Focus Areas
- packages/runtime/src/sandbox/default-sandbox-manager.ts[142-170]
- packages/runtime/src/sandbox/default-sandbox-manager.ts[173-195]
- packages/runtime/src/sandbox/sandbox-manager.ts[128-135]
- packages/runtime/src/sandbox/windows-sandbox.ts[93-123]
- packages/runtime/src/__tests__/default-sandbox-manager.test.ts[64-79]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The owner check accepted {user SID, SYSTEM, this token's default owner},
but the default owner differs between elevation states: a mutex created
by an *elevated* instance is owned by BUILTIN\Administrators, while a
concurrent *non-elevated* instance of the same user resolves its own
token owner to the user SID -- so it rejected the elevated instance's
legitimate lock and readiness/ACL-ledger acquisition failed closed
whenever elevated and unelevated Maka processes overlapped.
Make the lock's identity elevation-independent at the source: the lock
security descriptor now pins an explicit owner (O:<user SID> -- always
an assignable owner for the user's own token, elevated or not) ahead of
the protected DACL, so every lock this code creates carries the same
owner in every elevation state. Validation of a pre-existing lock
accepts exactly {user SID, SYSTEM, BUILTIN\Administrators} -- the last
for locks created by builds that predate the pinning during side-by-side
overlap, and safe because only an elevated administrator (outside the
RFC S1/S5 threat model) can create Administrators-owned objects. The
per-token default-owner query is deleted. Adds a lock_sddl unit test
asserting the pinned owner precedes the protected DACL.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generated-by: Claude Fable 5
Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on apache#3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on apache#3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5
|
Thanks — evaluated both findings; one is a real bug and is fixed at head Finding 1 (mixed elevation rejects locks) — real, fixed with the stable-owner option. Confirmed: the accepted-owner set included "this token's default owner", which differs between elevation states — an elevated instance's lock is owned by Finding 2 (cold managers report unavailable) — deliberate, per prior review consensus on this PR. The hot-path 中文发现 1(混合提权互拒锁)— 真 bug,已修(选稳定 owner 方案)。 属实:白名单里的"本 token 默认 owner"随提权态变化——提权实例的锁 owner 是 发现 2(冷启动 manager 报不可用)— 有意设计,系本 PR 前几轮评审共识。 热路径 |
|
@hqhq1025 @Astro-Han Both PRs are now green (20/20) with your approvals in place, and every review thread is addressed. Could you merge when convenient? Suggested order: this PR (#3161) first, then I will immediately rebase #3174 onto the new 中文:两 PR 均 20/20 绿且已获两位批准,所有 review threads 已处理。烦请按顺序合并:先本 PR(#3161),合入后我会立刻把 #3174 rebase 到新 main 使其 stacked commits 消失即可跟进合并。(我持 write 权限尝试过合并,但 base 分支策略将合并限制给维护者且未开 auto-merge——这是合理配置。)squash commit 请保留 |
…3174) * feat(windows-sandbox): implement production-identity readiness probe The Windows AppContainer backend advertised availability from file existence alone (`existsSync(clientPath)`), which never proves the host can actually stand up the sandbox identity. Replace that with a real readiness probe (RFC §6.4) and align the RFC to the shipped behavior. - launcher: add `--readiness-probe`, which creates the real AppContainer identity/token and a kill-on-close Job and launches a throwaway confined child, failing closed if the host cannot create or enforce the boundary. - runtime: back the Windows backend's `isAvailable` with a memoized spawnSync of `--readiness-probe`, so `auto`/`require` fail closed on hosts where the OS cannot create the boundary rather than trusting the packaged binary's presence. - CI: add readiness-probe-smoke.ps1 to the W0 lane (exit 0, repeatable, argument-rejecting). - docs: align RFC §6.4/§6.5 (EN + zh-CN) — mark the readiness probe as implemented and enforced, and keep private desktop, full per-profile policy at readiness, and launcher signature/version as later gates. Advances #2142 Phase 4 (align the RFC's guarantees with the shipped slice). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): harden readiness probe against PID reuse and identity spoofing Address adversarial review on the production-identity readiness probe: - Make the throwaway AppContainer profile name unique per invocation with a monotonic nonce (SystemTime nanos + PID) so a recycled PID can never collide with a still-registered profile and deadlock the probe (finding A). - Prove enforcement, not mere presence: verify the confined child runs under the SPECIFIC requested Job (IsProcessInJob against our handle) and carries the EXACT requested AppContainer SID (TokenAppContainerSid + EqualSid), failing closed otherwise. "some job / some AppContainer" is not evidence the boundary we asked for is real (finding B). - Cache Windows readiness at module scope keyed by client path so backends sharing a launcher share one probe result; distinct paths stay independent (finding D). - Sync RFC (EN + zh) Updated: date to 2026-08-17 (finding C). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): consolidate availability authority and settle readiness probe deterministically Address maintainer review on the readiness-probe PR: - Availability: make `isBuiltinFilesystemWorkerSandboxAvailable(win32)` consume the memoized readiness probe instead of treating launcher-file presence as a second authority. Runtime Host composition calls this at startup, warming the readiness cache so the backend's later synchronous `isAvailable()` on the transform hot path hits the cache rather than spawning on the event loop. Export `probeWindowsReadiness` with an injectable spawn seam and cover it with unit tests (clean/non-zero/spawn-error/external-timeout/memoized/missing). - Settlement: route the readiness child's post-resume outcomes (verification failure, timeout, clean exit) through explicit TerminateProcess plus the existing `terminate_and_drain_job` primitive, so a child found outside the Job is settled rather than relying on Job close as the only backstop. - Cleanup: give the readiness AppContainer profile one fixed, self-reconciling name that best-effort-deletes any leftover before create, so a profile leaked by an externally-killed probe is reclaimed by the next probe. Replaces the PID+nonce scheme. - Docs: mark the structured unavailable-reason / diagnostics surface as a deferred later gate in both RFC language variants (§6.4/§6.5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): serialize and bound the readiness probe lifecycle Address four readiness-probe lifecycle blockers on the availability path: 1. Reserve the `readiness-probe` request_id in protocol and reject it in LaunchRequest::validate; derive the probe profile under a disjoint `maka.readiness.` namespace so no production launch can resolve to the profile the probe deletes and recreates. 2. Serialize the whole delete->create->probe->settle->drop window across processes with the DACL-hardened named mutex the ACL ledger uses (LedgerLock), scoped by SID under Global\Maka.WindowsSandbox.ReadinessProfile.v1, failing closed on timeout. 3. Preserve the AppContainer registration on drop when the Job could not be proven empty (keep_on_drop + preserve()), mirroring the production Unsettled contract, instead of deleting a possibly-live identity. 4. Cache negative Windows readiness for a bounded TTL (60s) rather than the process lifetime, and make the synchronous isAvailable hot path strictly cache-only (readCachedWindowsReadiness) so it never spawns on the event loop; positive results stay cached permanently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): scope readiness recovery and quarantine claims to what the code enforces hqhq1025's round-3 review is factually correct on both points; this converges the claims rather than building the heavier machinery, matching the preview's fail-closed scope. Blocker 1 (TTL recovery overclaim): the filesystem worker is published once when a composition is built, so a negative readiness result is not recovered within a running host — the 60s negative TTL only bounds how long a stale negative poisons the module cache so the *next* composition build re-probes. De-claim the RFC (EN+zh §6.4/§6.5) and default-sandbox-manager.ts comments from 're-probes on the next composition rather than disabling until restart' to honest new-composition/restart-scoped recovery, and mark an active running-host readiness retry with dynamic worker publication as a deferred gate. Add a test asserting the hot path never self-recovers a negative; only an explicit re-probe (a new composition build) can. Blocker 2 (ineffective preserve/keep_on_drop): create_readiness deletes the fixed identity unconditionally next cycle, so preserve() only deferred deletion by one probe and never durably quarantined. Remove preserve()/keep_on_drop and the unsettled out-param; the settlement-failure surfacing in the match stays, so an unsettled probe still fails closed (reports unavailable). Document that cleanup relies on the kill-on-close Job's tree termination and a zero-filesystem-root identity, and mark durable quarantine (or unique probe identities plus a reconciliation ledger) as a deferred gate (RFC §6.5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): disable cmd AutoRun in the readiness probe The readiness probe launched its throwaway child as `"<cmd>" /c exit 0` without `/d`, so cmd.exe ran the machine-wide `Software\Microsoft\Command Processor\AutoRun` value before the payload. Because AutoRun is machine-constant, a host that ships an AutoRun which exits non-zero would make *every* probe report failure, and one that ships a blocking AutoRun would hang the child until the Job drain times out -- either way the Windows sandbox fails closed on that machine on every startup, independent of whether the OS can actually stand up the boundary. The probe would then be measuring the host's shell customization instead of the sandbox. Route the command line through a pure `readiness_probe_command_line` helper that emits `"<cmd>" /d /c exit 0`, matching the `/d` the production launch path already passes, and add a unit test asserting `/d` precedes `/c`. Also correct the RFC (EN + zh): the readiness probe and its serialized profile lifecycle are added by this PR (#3161), not the merged #2961 preview slice -- tag those two bullets accordingly and note the AutoRun-disabled command line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): reject a squatted pre-existing lock by owner The named-lock comment claimed a squatted Global\ name "fails closed at acquisition", but that only held for a squatter with a restrictive DACL: CreateMutexW ignores the supplied security descriptor when the name already exists, so a local user pre-creating the predictable mutex name with a *permissive* DACL would hand the launcher an attacker-owned arbitration object -- readiness and ACL-ledger operations would then block on a mutex the squatter can hold forever (availability DoS), while the code read as if the DACL had rejected them. Close the gap where it is checkable: when CreateMutexW reports ERROR_ALREADY_EXISTS (also the normal same-user contention path), read the existing object's owner via GetSecurityInfo before any wait and require it to be the current user or SYSTEM. Ownership is the one property a permissive squatter cannot forge -- re-owning an object to another SID requires SeTakeOwnership/SeRestore, which standard users do not hold. Any other owner fails closed with an explicit squatted-mutex error instead of blocking. The 10-way concurrency smoke (same-user contention) passes unchanged; the cross-user negative path cannot be exercised by an unprivileged CI runner and is enforced by the owner check itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): accept the token default-owner SID for pre-existing locks The squatted-lock owner check rejected locks owned by BUILTIN\Administrators (S-1-5-32-544), which broke on elevated hosts: an elevated administrator token stamps the Administrators group -- not the user SID -- as the default owner on objects it creates, so a mutex this very code created earlier on the elevated CI runner failed the {user, SYSTEM} check and the W0 protocol lane went red (unsettled_launch_preserves_grants_and_quarantines_the_ledger). Accept the process token's default-owner SID (TokenOwner) as a third legitimate owner. This stays inside the threat model: only an elevated administrator can create objects owned by Administrators, and RFC S1/S5 explicitly does not defend against administrators. Standard-user squatters are still rejected -- they cannot forge either the user SID or the Administrators owner. Adds a unit test pinning the helper to {user SID, S-1-5-32-544} so both elevation states stay covered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): pin an elevation-independent owner on named locks The owner check accepted {user SID, SYSTEM, this token's default owner}, but the default owner differs between elevation states: a mutex created by an *elevated* instance is owned by BUILTIN\Administrators, while a concurrent *non-elevated* instance of the same user resolves its own token owner to the user SID -- so it rejected the elevated instance's legitimate lock and readiness/ACL-ledger acquisition failed closed whenever elevated and unelevated Maka processes overlapped. Make the lock's identity elevation-independent at the source: the lock security descriptor now pins an explicit owner (O:<user SID> -- always an assignable owner for the user's own token, elevated or not) ahead of the protected DACL, so every lock this code creates carries the same owner in every elevation state. Validation of a pre-existing lock accepts exactly {user SID, SYSTEM, BUILTIN\Administrators} -- the last for locks created by builds that predate the pinning during side-by-side overlap, and safe because only an elevated administrator (outside the RFC S1/S5 threat model) can create Administrators-owned objects. The per-token default-owner query is deleted. Adds a lock_sddl unit test asserting the pinned owner precedes the protected DACL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * feat(windows-sandbox): place sandboxed children on a private desktop Non-interactive AppContainer workers previously inherited the creator's interactive Winsta0\Default desktop, exposing the same-session GUI attack surface (window-message shatter into the user's interactive windows, desktop hooks) — AppContainer confines files/network/token but not the desktop scope. Per RFC v1 §6.3 the private-desktop promise was designed but deferred. This change enforces initial-desktop *placement* plus DACL protection, not escape-proof confinement. Each AppContainer launch (production create + readiness probe) stands up a per-launch alternate desktop on the existing window station, with a protected DACL: a leading deny ACE strips DESKTOP_SWITCHDESKTOP / HOOKCONTROL / journal record+playback (0x138) from the launching-user SID — which the AppContainer child's token still carries as an effective SID, so without the deny the owner's full-control allow would name the child as a grantee of those rights (maintainer review, P2-1) — then allow ACEs grant the launching user and Local System control for cleanup and the child's AppContainer SID only the minimal DESKTOP_* rights (create window/menu, read/write objects, enumerate). STARTUPINFOEXW.lpDesktop points the child at it. If the desktop cannot be created or the DACL cannot be granted (CreateProcessW then fails ACCESS_DENIED), the launch fails closed — auto/require never fall back to the host desktop. Scope, stated honestly (per maintainer review on #3174): - lpDesktop selects only the child's *initial* desktop. This places the worker off the interactive Default desktop and DACL-protects the private one, but it does NOT structurally confine: absent a no-Win32k mitigation, a dedicated window station, or a token boundary, in-process code can still OpenDesktopW("Default") + SetThreadDesktop to re-attach. Enforcing a no-Win32k mitigation is a deferred gate (§6.5). - It does NOT isolate the clipboard: the clipboard belongs to the window station, which both desktops still share. Clipboard isolation with a dedicated window station is a later hardening gate (§6.5). - The create-window/write DACL rights are granted but not relied upon; the worker does no GUI work. A verified Low mandatory-integrity label proving those rights are usable at AppContainer's Low IL is deferred (§6.5). The boundary probe self-attests its own initial desktop name (desktopPrivatePlacement) and requires the launcher-owned maka-sandbox-desktop. prefix — not merely "not Default", so landing on any other pre-existing desktop also fails the placement check; appcontainer-smoke asserts the same prefix and rejects Default. Digest is unchanged: the desktop is a launch-time detail and does not enter the manifest. Stacked on #3161 (readiness probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): make the private desktop usable, bounded, and attested Follow-ups from inline review on #3174, each closing a spot where the desktop's documented properties exceeded (or under-stated) what the code enforced: - Pin a Low no-write-up mandatory label (S:(ML;;NW;;;LW)) on the private desktop. Without it the desktop inherited the creator's Medium integrity, and because MIC is evaluated before the DACL, the Low-IL AppContainer child's granted create-window/write rights were unusable in exactly the case the DACL claimed to grant them. The rights are now labeled-usable; an in-child window-creation check proving them end to end stays a deferred gate (RFC S6.5) - the worker still does no GUI work, so the shipped guarantee remains placement. - Bound the per-launch desktop heap: CreateDesktopExW with a 512 KiB budget instead of CreateDesktopW's default 3,072 KiB interactive allocation, so the supported ten-way concurrency costs ~5 MiB of the documented 48 MiB system desktop heap instead of ~30 MiB. A real-OS test holds ten confined desktops live simultaneously to prove the budget. - Fix an unaligned read: TokenAppContainerSid was read through a Vec<u8> buffer cast to TOKEN_APPCONTAINER_INFORMATION, whose leading pointer field requires pointer alignment - UB in Rust. The buffer is now sized in usize words (same pattern as current_user_sid_string). - Attest the readiness boundary instead of exiting 0: --readiness-probe now emits a machine-readable JSON of the facts it verified (exact-SID match, specific-Job membership, settlement drain, private-desktop placement), and readiness-probe-smoke.ps1 asserts those fields, so removing any verification would turn release evidence red rather than leaving a hollow exit-0 gate green. RFC (EN + zh) updated to match: label and heap bound recorded as enforced, the deferred gate narrowed to no-Win32k plus in-child window-creation coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 * fix(windows-sandbox): unforgeable desktop names and a narrowed normative claim Maintainer follow-up on the private-desktop delta, both points fixing a gap between what was stated and what the object model guarantees: - Desktop-name uniqueness is now fail-closed. CreateDesktopExW *opens* an existing desktop when the name collides -- silently ignoring the supplied DACL and heap budget -- and the old nonce was wall-clock nanoseconds with a unwrap_or(0) fallback, so PID reuse, clock rollback, same-tick calls, or a failed clock could reopen an older desktop while the prefix attestation still passed. The nonce is now 128 bits from the OS CSPRNG (BCryptGenRandom), collision is cryptographically negligible, and RNG failure fails the launch closed instead of collapsing to a constant name. The ten-desktop live test now also asserts ten *distinct* names that each pass the placement attestation, not merely ten returned handles. - The RFC's top-level S6.3 guarantee no longer states an unconditional "cannot": it now reads "workers start on a launcher-created private desktop, never Default", with the enumerate/message/hook resistance explicitly tied to the deferred no-Win32k/window-station gates (EN + zh). The PR title/description are narrowed the same way. Cleanups from the same review: json_string now delegates to serde_json (hand-rolled escaping and its test deleted), and the redundant '"desktop":"Default"' smoke rejections are removed -- the stronger launcher-prefix assertions already exclude Default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Generated-by: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Closing: #3174 was merged first, and since it was stacked on this branch, its squash commit carried every commit of this PR into 中文:#3174 先合并且 stack 在本分支上,其 squash 已把本 PR 全部 commit 以最终形态带入 main——对全部触及路径 diff 验证本分支已无净增量(差异均为 main 已有内容的旧中间版本)。lock_sddl 提权无关 owner、/d、串行 readiness 生命周期、TTL/cache-only 可用性等全部产物均在 main 上。故关闭本 PR,评审记录保留于此。 |
What
Replaces the Windows AppContainer backend's file-existence availability check with a real production-identity readiness probe, and aligns the RFC's §6.4/§6.5 guarantees with the shipped behavior.
Before this change the backend advertised availability from
existsSync(clientPath)alone — that only proves the packaged launcher is present, not that the OS can actually create the AppContainer identity, token, and Job on this host (AppContainer can be disabled by policy, the edition may not support it, etc.). Availability could therefore claim the boundary is enforceable when it is not.How
--readiness-probe): constructs the real AppContainer identity/token and a kill-on-close Job, then launches a throwaway confined child (cmd.exe /c exit 0) with the SECURITY_CAPABILITIES + JOB_LIST attribute list. It verifies the child token is an AppContainer token and is in the Job, waits for a clean exit, and fails closed if the host cannot create or enforce the boundary. Every post-resume outcome (verification failure, timeout, clean exit) is routed through the productionTerminateProcess+terminate_and_drain_jobsettlement primitive. The readiness AppContainer profile uses one fixed, self-reconciling name (best-effort delete before create), so a profile leaked by an externally-killed probe is reclaimed by the next probe.default-sandbox-manager.ts): file presence only discovers the launcher path; the memoized readiness probe is the single availability authority. Runtime Host composition callsisBuiltinFilesystemWorkerSandboxAvailable()at startup, which warms the readiness cache, so worker registration and the backend's later synchronousisAvailable()on the operation hot path consume the same result instead of spawning on the event loop.auto/requirenever fall back to host execution.readiness-probe-smoke.ps1added to thewindows-sandbox-w0lane — asserts exit 0, repeatable, and argument-rejecting on a realwindows-2025runner.Closes a roadmap checkbox
Advances #2142 Phase 4 ③ — "Align the RFC's implemented guarantees with the shipped slice: either implement private-desktop and full production-boundary readiness probes or mark them explicitly as later gates." This PR takes the implement path for the production-boundary readiness probe and keeps private-desktop + full-policy-at-readiness + structured diagnostics explicitly marked as later gates, so the RFC no longer overclaims.
Honesty / fail-closed
The deferral narrows readiness richness (desktop layer, full per-profile policy re-proven at readiness, structured diagnostics) and defense-in-depth, not the enforcement boundary. An unavailable, drifted, or failed backend still fails closed; a restricted managed profile never falls back to host execution.
Review update
Addresses the maintainer reviews from @hqhq1025 and @M4n5ter:
isAvailable()is cache-only;probeWindowsReadinessunit tests (failed readiness, one shared probe result, no worker publication on failure, external timeout, missing launcher);Generated-by: Claude Fable 5trailer added to every commit.Verification
cargo fmt --checkclean;cargo build --locked;cargo test --locked(38 tests) pass on Windows 11 x64;--readiness-probe→ exit 0, stable across runs, rejects arguments.default-sandbox-manager.test.js(11 tests) +windows-sandbox.test.jspass with the consolidatedisAvailable.readiness-probe-smoke.ps1passes under pwsh 7 locally.windows-sandbox-w0CI lane.AI use
Select exactly one:
Tool(s) and scope: Claude Fable 5 (Anthropic), driven by the human contributor of record, authored the launcher
--readiness-probeimplementation and settlement/cleanup lifecycle, the runtimedefault-sandbox-manager.tsavailability consolidation and unit tests, the PowerShell smoke, and the RFC updates, all under human review. Every commit carries aGenerated-by: Claude Fable 5trailer; the final squash commit must retain it.