Repository navigation
fix(windows): resolve ACL principal natively without cold PowerShell - #7656
Conversation
|
Caution Review failedThe pull request is closed. Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
|
Integration evidence for exact head 25a4a5d (dev merge on top of the reviewed bcd6522, made to remove an overlap in structure/ops/test-sandbox-cleanup.md) (lane L3, run on the Windows host mini; C4 Windows identity/ACL change, Security summary in the description):
Static merge simulation on current dev 4e2940e (this Mac): dev=4e2940e5134dcbb138b47bbed5f65d5ce9ddd414 base=4e2940e5134dcbb138b47bbed5f65d5ce9ddd414 pr_files= 10 overlap=[]; merge clean; install exit=0; gui install exit=0; tsc exit=0; tsc doctor exit=0; structure:check exit=0; privacy:scan exit=0; layout+ratchet guards exit=0 52 pass 0 fail ; Merged by the triage manager as maintainer integration (MAINTAINERS.md, dev only) under the owner's static-judgment rule while GitHub Actions is degraded; dev push CI will be checked when Actions recovers. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Summary
Verification
bun run test tests/windows/windows-user-principal.test.ts tests/windows/windows-user-principal-nonascii.test.ts tests/codex-integration/codex-windows-current-sid.test.ts --parallel=1, usingOCX_TEST_RUNNER_BUNpinned to Bun 1.4.3: exit 0, 30 pass / 0 fail.EACLIDENTITY, 16 pass / 2 fail), then passed with the native implementation. Focused boundaries/layout/worker guards: exit 0, 79 pass / 0 fail across 7 files. Full Windows domain plus the affected fixture/auth/routing suites: exit 0, 766 pass / 0 fail across 32 files. The new native suite was rerun after changing its Microsoft-account fixture to the scanner-approvedexample.comdomain: exit 0, 18 pass / 0 fail.bun run typecheck,bun run privacy:scan,bun run structure:check: exit 0. Tests use the repository-pinned Bun 1.4.3; the bundled runtime is 1.4.4.bun run test:changedfailed at the unchanged 900s limit: 2,187/2,599 files selected, 23,905 pass events / 44 fail events observed before termination, exit 124 (incomplete suite). It is not passing evidence. The spill-child failure it exposed is repaired below; other failures remain. Focused Desktop/link/routing diagnostics returned 21 pass / 24 fail. Two representative Desktop/link failures reproduced unchanged on pristinedev(ceef4889e) in a detached control checkout: 0 pass / 2 fail, exit 1, including the samedesktop_lifecycle_busy_or_unsaferesponse and 5s timeout. The remaining failures are not all classified. No timeouts, assertions, skips or runner policy were weakened.e1338bb75b476726e7f5c9406eef8c87bec083e9: updated focused Windows/fixture/response-state/layout/worker/ratchet set passed 949/949 tests across 37 files; typecheck, privacy and structure exited 0. Fresh independent gpt-6.1-sol/high source/security review round 2 reviewed all 10 changed files and the complete interdiff: PASS, zero blockers. It did not run tests or clear runtime gates.bun run teston that head also hit the unchanged 900s cap: exit 124, 26,741 pass events / 44 fail events observed, incomplete suite. The repaired spill case passes in this run. A zero-byte coordinator case that failed in the broad changed run passes in fresh isolated current-head and pristine-base controls (1/0 each); that does not classify every broad failure. Neither broad invocation produced a passing Check receipt. This PR stays draft; no suite or timeout policy was relaxed.ci.yml lane=all: run 40056591526,headSha=e1338bb75b476726e7f5c9406eef8c87bec083e9, queued with no conclusion at the latest inspection. Initial dispatch covers superseded4e34eb9fand is not current-head passing evidence. Full GUI/desktop packaging, live service installation and real domain/Microsoft-account host parity are NOT RUN; account-name formats have synthetic boundary coverage.Decisions
src/lib/windows-native-principal.tshelper, the minimum source-scope extension permitted by the lane brief, to reusereadWindowsCurrentSidwithout changing its SID-only consumers. Resolve the account from the live token SID before formatting it; this avoids a SID string round trip and leaves allocation cleanup with the existing reader.tests/helpers/responses-state-never-settling-acl-child.ts: native on/off/on probes showed that faster identity resolution reaches its intentionally stalled icacls runner, which remained unowned during blocking teardown. Keep the selected runner held through queue settlement, assert that it was reached, then release and drain before removal. The existing parent assertions and deadlines stay unchanged. Regression red-to-green: focused case 0/1 before, 1/0 after; entire original response-state file 145/0. This does not claim to explain [Bug][CI]: Windows fixture teardown exhausts the remove retry budget (EBUSY/EPERM) in two existing tests #7515's production empty-DACL interleaving.Security summary
The effective process token remains the authority for the ACL grant and cached account name. Native DLLs load only from the trusted OS system directory. API/translation/worker failures refuse; only unavailable native support selects the existing trusted helper, using the remaining budget. Token, allocated SID and DLL cleanup finish before worker completion. Negative tests cover native failures, bounded buffers, Unicode/name shape, explicit fallback, late-result refusal and ACL-timeout memo isolation. There are no dependency, workflow, credential-format or ACL-policy changes. The C4 plan and current-head implementation passed independent sol source/security review. This source verdict does not clear the failed broad checks or incomplete CI.
Checklist