Repository navigation
chore: preserve sandbox startup on private test desktops - #1791
Merged
Merged
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesAdds an owned private-desktop launcher and SDK startup harness that preserve desktop permissions, record diagnostics, and verify cleanup. Example: SDK 1.0.0 starts an AppContainer on a newly created NULL-security test desktop.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: it provides distinct private-desktop validation tooling absent from main, addresses the previous findings, and has no newly identified blocking defect. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherDeveloper proof scripts create task-owned Windows desktops, invoke the existing MxcCommandRunner and published SDK, and produce startup, authorization, observation, and cleanup receipts. flowchart TD
A[Developer proof invocation] --> B[Owned desktop launcher]
B --> C[Desktop security policy]
C --> D[Desktop harness]
D --> E[MxcCommandRunner and SDK]
E --> F[Native startup probe]
F --> G[Proof receipts and cleanup]
Technical reviewBest possible solution: Land the owned desktop admission and validation tooling when the author considers the draft ready, preserving the existing sandbox policy owner. Do we have a high-confidence way to reproduce the issue? The descriptor handling explains the startup failure, and supplied native evidence exercises the repaired admission path; this read-only review did not run an independent baseline reproduction. Is this the best way to solve the issue? The repair belongs in the test-desktop creator: it preserves MxcCommandRunner, rejects existing denies before launch, and keeps token restrictions intact; the observer and recorder support distinct diagnostic and cleanup contracts. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against ed1444b0de69. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (6 earlier review cycles)
Reviewed October 26, 2026, 11:25 PM ET / October 27, 2026, 03:25 UTC (Revision 7). |
Extract the independently reviewed reliability seam without the held native timeout consumer. No change to executor releases, package pins or sandbox enforcement.
Use caller prefixes with internally generated unadvertised desktop names instead of reusable exact names. Retain nonblocking launch ownership until teardown. Restore caller isolation environment existence and values on every path, including explicitly unset variables. Add actual-child contender and environment readback, exact-name binding rejection, and real launch-failure cleanup checks. The four shipped-SDK USER32/PowerShell negative and positive controls and new ownership/environment regressions pass. No native serializer dependency, product policy change, existing desktop mutation or elevation.
UNVALIDATED handoff: initial SDK-port harness build passed with zero warnings/errors. Final waiting-child receipt fixture, runtime qualification, full build, Shared and private Tray suites remain blocked by sustained CPU admission failures. No product SDK pin or policy owner changed.
UNVALIDATED source checkpoint. Set and restore inherited quiet process error mode before native child initialization. Retain native-handle identity and explicit initialization failure status. Add a real DLL_PROCESS_ATTACH failure fixture and native ownership controls. Compilation passed before the final small native-helper adjustment; final rebuild and all runtime proof require resource admission and explicit Home confirmation. No further harness or failure fixture was executed after the hold.
UNVALIDATED accepted-P2 follow-up. Pin the timeout root handle and compare native creation FILETIME before Kill(true), then retain the bounded wait on the original native handle. Preserve root-only cleanup for other failures and successful nonblocking behavior. Add a native event-controlled root/descendant timeout regression with identity-bound exit and desktop cleanup checks. Parser, docs, skills and diff checks passed; new native fixture compilation and all runtime proof remain pending. No child execution before Home confirms the quiet fix.
paulcam206
force-pushed
the
draft/hub-desktop-harness
branch
from
October 21, 2026 03:46
e37970a to
a91649f
Compare
6 of 16 tasks
Contributor
Author
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
status: 📣 needs proof
Contributor
Author
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Contributor
Author
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
Contributor
Author
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
Contributor
Author
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
paulcam206
marked this pull request as ready for review
October 27, 2026 03:28
This was referenced Nov 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Fixes: an intended restricted SDK process can fail USER32 initialization when a newly owned private test desktop lacks the required package rights and integrity label.
User Impact
Developers can validate startup on exclusively task-owned desktops while preserving existing denies and caller-token restrictions. This is a test-harness change, not a new product capability or a promise of desktop isolation.
Why This Change Was Made
Complete only the missing package write/create rights and Low no-write-up label on a newly owned NULL-security desktop. Reject existing write/create denies before child creation. SDK 1.0.0 can inherit the caller's task desktop; inspect its actual assignment instead of treating requested UI policy as an isolation boundary.
The former SDK-isolation documentation claim is corrected in exact published head
15cd728cb8d3833c266e089e9aae3486dab28444. That followup changes prose only; launcher/security owners are unchanged fromfe5bdafe.Evidence
Current-head live authority proof: PASS, using an exact clean
15cd728cb8d3833c266e089e9aae3486dab28444guest checkout, not a source overlay.scripts\Invoke-PrivateDesktop.ps1launcher ran the current harness through published SDK 1.0.0. The native child was an AppContainer at Low integrity; Windows AccessCheck allowed caller-desktop write/create, USER32 initialized, and the command exited 0 without timing out. The observed assigned desktop equaled the newly owned caller desktop. This SDK positive executed once.0x82deny for Restricted Code SIDS-1-5-12. A live desktop readback and Windows AccessCheck on a genuinely restricted token allowed read but denied write objects and create window. The actual launcher then encountered an identical live-created denied DACL and rejected it with its existing write/create-denial error before child creation: no successful-child launch receipt or forbidden-child sentinel.Boundary: the negative token was genuinely restricted, not an AppContainer. The launcher was not run as that token: the token authorization check and actual launcher's fail-closed guard on the matching DACL are distinct, linked observations. This establishes the bounded admission/rejection/teardown contract, not separate-desktop isolation, a full SDK matrix, ETW attribution, or absence of dialogs.
This is an actual rendered guest-console capture (
CopyFromScreen), not a generated terminal image. The normal fork asset commit has parented1444b0; anonymous GET returned HTTP 200 and image bytes matched SHA2560278730E537F84D516DA29431DCCD9906E2AADE8256A664CCFAF55E845D6D33B. GitHub's upstream attachment permission was not bypassed.Copied terminal result:
Redacted executed-outcome receipt:
{ "head": "15cd728cb8d3833c266e089e9aae3486dab28444", "sourceVersion": "1.0.0+15cd728cb8d3833c266e089e9aae3486dab28444", "actualAppContainer": true, "lowIntegrity": true, "user32Initialized": true, "assignedDesktopEqualsCaller": true, "realRestrictedToken": true, "liveDeniedDesktopWriteCreateDenied": true, "identicalDeniedDaclEncounteredByLauncher": true, "launcherRejectedBeforeChildCreation": true, "positiveExactProcessExited": true, "positiveDesktopRemoved": true, "deniedDesktopRemoved": true, "tokenDefaultDaclRestored": true, "ownedWindowStationRemoved": true, "winSta0DefaultDaclAndLabelUnchanged": true, "noDialogVerified": false }80895F5A7096C61EF6776AEF631FB2AD955F013E6E6E3B26A894FEDA59BE6E1B12B0CBB43A7D0AED6E0BF05DA118C1373DCFDD4F4CC5FC7807EE94B7662EA7A63266DD1CA4E58E570DE6D219720896ED4579256C7AC3753F605FF224FF1EA950E61C5226D90BFAA7E05FE28158F9A97FE8257F1437BBBF4B082D667036D252D956B152836698771FBE56D2AB8BE72F2F619031601DD2F00188B95C0FEC2A53DE5898FB8EE76C90759A78772C1253EB9DEBF5BF70F65ADF6BAA883D294FB5261FChange Type
Scope
winnodeRequired proof pools
windows-wsl-mxc: native Windows capacity is needed for the real token/desktop authorization boundary. The bounded current-head authority proof above ran; no claim that this alone completes the full WSL/MXC pool.Validation
15cd728c:.\scripts\validate-docs.ps1PASS, 81 Markdown files and documentation-flow regression;.\scripts\validate-agent-skills.ps1PASS, 12 skills / 21 Markdown files;git diff --checkPASS.dotnet build .\tests\OpenClaw.DesktopHarness\OpenClaw.DesktopHarness.csproj -c Debug -m:1 --disable-build-servers -p:UseSharedCompilation=false --nologo --verbosity quietPASS. The current native probe compiled with MSVC/O1 /MT /W4andadvapi32.lib: PASS.scripts\Invoke-PrivateDesktop.ps1with-NullSecurityNegativeControl -ConsoleProcess -WaitForExit, current harness and hash-bound native probe/runtime: PASS, AppContainer/Low/USER32, exit 0.Invoke-AuthorityProof.ps1 -Repository <owned-exact-checkout> -HarnessExecutable <hash-bound-harness> -NativeRuntimeLibrary <hash-bound-runtime> -NativeProbeExecutable <hash-bound-probe> -OutputDirectory <owned-proof> -ExpectedHead 15cd728cb8d3833c266e089e9aae3486dab28444 -ResumePositive: PASS, existing-deny guard and teardown. It consumed the preserved positive receipt rather than rerunning the positive SDK case. Crabboxrun_35ea07423c5f5d45388984dc85d7e78a, exit 0, nonexpired.fe5bdafefull build PASS, 395.665 seconds; native-free finalization checks 84 PASS. Its High Shared suite recorded 4664 passed / 2 failed / 8 nonexecuted; both failures passed exact focused current/main comparisons (2/2 each), so originals remain unreproduced, not demonstrated baseline failures. Queue sanity 2/2 PASS. These earlier results are not relabeled as fresh15cd728csuite runs.Failed external-fixture setup attempts were retained, not counted as passing security proof. Windows rejected an application-package restricting SID in
CreateRestrictedTokenwith error 87; changing token-default DACL did not inject a desktop deny. The final fixture used the documented NULL-desktop inheritance from a newly owned window station. An unnecessary thread-desktop assignment failed with Windows 170 and was removed from the external fixture. No product-policy assertion, source or token restriction was relaxed to obtain a pass.Real Behavior Proof
cbx_87ef81f79ecc; no provider run URL returned.15cd728cb8d3833c266e089e9aae3486dab28444, clean genuine checkout, informational version and executed binary hashes above.Yes/No/N/A) Yes: anonymous HTTP 200 with matching PNG bytes at a commit-pinned normal fork branch. Redacted local export SHA256FC5754D806077B3C39B950ECA2C2D8513946F2466CFB51703BAB335FF45845FB; it is not represented as a public downloadable artifact.2ae4a900reported 21 unattributed OBJECT_SHOW events with a separate decoder limitation andnoDialogVerified=false; it is not current-head proof.Security Impact
Yes/No) Yes, missing package rights and a Low no-write-up label only on the newly owned test desktop.Yes/No) No.Yes/No) No product network changes.Yes/No) Yes, test-harness prepared execution and validation, not a new Windows node command.Yes/No) No user-data access expansion.Yes, explain the risk and mitigation: preserve existing deny entries, owner/group/audit and caller-token restrictions; reject an existing write/create deny before child creation. Only a new exclusively owned desktop is eligible. The actual Windows authorization and launcher observations above are separate from synthetic descriptor fixtures. WinSta0 readback compares DACL and mandatory label; it does not independently verify every owner/group/audit field.Compatibility and Migration
Yes/No) Yes. Ordinary fidelity runs keep their normal explicit build path when no expected harness hash is supplied.Yes/No) No product configuration changes. Native proof uses an explicitly authorized throwaway guest and task-owned artifacts.Yes/No) No.Review Conversations