Skip to content

fix(service): retry Windows background install with UAC elevation#408

Open
Wibias wants to merge 13 commits into
lidge-jun:devfrom
Wibias:codex/fix-windows-service-install-elevation
Open

fix(service): retry Windows background install with UAC elevation#408
Wibias wants to merge 13 commits into
lidge-jun:devfrom
Wibias:codex/fix-windows-service-install-elevation

Conversation

@Wibias

@Wibias Wibias commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the Windows dashboard background service install failure when schtasks /create returns access denied (Zugriff verweigert).

Changes

  • Add Windows elevation helpers for access-denied detection and UAC relaunch.
  • Improve schtasks error messages with actionable guidance.
  • Retry dashboard install-service through an elevated CLI run when access is denied, then verify the scheduler task exists.

Verification

  • bun test tests/service.test.ts tests/startup-action-control.test.ts tests/windows-elevation.test.ts
  • bun run typecheck

Summary by CodeRabbit

  • New Features
    • Added Windows-only UAC elevation and Task Scheduler elevated create/run with rollback, including protocol exit codes and bounded output handling.
    • Added post-install verification for scheduled task registration (task/XML/asset checks) and richer reconciliation status reporting.
    • Improved CLI install error classification by extracting stable markers and preserving user-facing detail.
  • Bug Fixes
    • Better handling of localized Task Scheduler access-denied and UAC cancellation (including “indeterminate/blocked” install-state behavior).
    • Safer elevated execution outcomes and rollback when verification fails or reconciliation is stale.
  • Tests
    • Expanded Bun coverage for elevation, access-denied detection, scheduler verification/reconciliation, and startup install retry/state transitions.

@github-actions github-actions Bot added the bug Something isn't working label Jul 24, 2026
@coderabbitai

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as resolved.

chatgpt-codex-connector[bot]

This comment was marked as resolved.

@lidge-jun

Copy link
Copy Markdown
Owner

Reviewed — three blocking findings before this can merge:

  1. The UAC retry never triggers for the actual error. windows-elevation.ts:7-9 recognizes "access denied" but windows-elevation.ts:33 rewrites it to "Windows denied access", so startup-action-control.ts:59 tests the rewritten message and returns false. A composition probe confirms retryDetected: false. Use a structured/typed access-denied error rather than matching rendered text.

  2. Attacker-influenceable Bun code runs elevated. startup-action-control.ts:21-25 elevates durableBunPath() + the repo/npm TS entrypoint under RunAs. The Bun executable may come from OPENCODEX_BUN_PATH (validated only by file size) and the CLI source lives in a user-writable tree — replacement before UAC approval yields admin-level arbitrary code execution. Elevation should be narrowed to a trusted, immutable helper with TOCTOU-safe inputs.

  3. Start-Process -ArgumentList loses argument boundaries. windows-elevation.ts:53-56 passes a PowerShell string array; PowerShell joins with spaces and strips quotes, so paths with spaces (e.g. C:\Users\First Last\...) break and a crafted path can inject Bun CLI options. Microsoft recommends one complete argument string with embedded escaped quotes.

Also: execFileSync with no timeout blocks the event loop during UAC, and the test suite only covers detection/formatting helpers — not the actual elevated-retry path.

Per MAINTAINERS.md this is a security-boundary change requiring explicit security review. Happy to re-review after fixes.

@Wibias

Wibias commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Ready

Wibias added 3 commits July 25, 2026 04:03
Narrow UAC retry to the machine-readable create-access-denied marker, verify a conflict-free scheduler install before writing state, and fix elevated spawn exit/cancel/timeout handling.
@Wibias
Wibias force-pushed the codex/fix-windows-service-install-elevation branch from a816f6f to 45f66d8 Compare July 25, 2026 02:24
coderabbitai[bot]

This comment was marked as resolved.

Wibias added 4 commits July 25, 2026 04:36
Preserve create-denial markers across combined CLI streams, fail closed on missing elevated ExitCode, treat unknown WinSW status as unverified without false conflict rollback, and stop swallowing /run timeout failures.
Run elevated schtasks create and run inside a single privileged PowerShell process, treat any /run failure as install failure with rollback, and keep the create-denial marker on a structured CLI failure path.
Remove the privileged write into the unelevated TEMP directory. Carry create/run/rollback outcomes through reserved elevated exit codes inside one UAC process, and reconcile unknown codes against actual task state.
A request-level timeout must not kill the unelevated launcher or clear the install lock while the elevated Task Scheduler transaction may still mutate the same task. Keep observing completion, reconcile late protocol results, and block overlapping installs until ownership settles.
@Wibias

This comment was marked as outdated.

@coderabbitai

This comment was marked as outdated.

coderabbitai[bot]

This comment was marked as resolved.

Separate PowerShell Handle/ExitCode statements so join(") cannot form Handleif, gate UAC-cancel text on non-zero exits, fail closed when reconciliation rejects, and surface leftover-task guidance after signal termination.
@Wibias

This comment was marked as outdated.

@coderabbitai

This comment was marked as outdated.

lidge-jun added a commit that referenced this pull request Jul 25, 2026
독립 리뷰어가 FAIL과 함께 낸 12개 blocker를 전부 반영했다.

- WP1(#433): 카나리 설계를 probe lease로 전면 교체. 조회는 무상태로 두고
  auth 단계에서만 lease를 획득한다. 기존 테스트 codex-routing:312/:806이
  '2xx는 hard cooldown을 보존한다'를 명시적으로 고정하고 있어, lease 소지
  요청의 성공만 cooldown을 해제하도록 했다. transient 경로의 probe 상태
  소실도 함께 막는다.
- WP3(#422): non-stream 경로가 parseResponse 부재로 core.ts:1828에서 400을
  내는 문제를 확인하고 해당 diff를 추가했다.
- WP4(#373): rootPromptMessages()는 serialized를 반환하지 않으므로 반환
  타입 확장 diff를 명시했다. 공유 token-estimate.ts 변경은 kiro와 두 서버
  경로에 영향을 주므로 철회하고 Cursor 국소 helper로 가뒀다.
- WP5(#404): resolveAdapter가 cursor/kiro 등 provider 전용 어댑터를
  포함하므로 override 허용 목록을 openai-chat/openai-responses로 제한했다.
- WP2(#432): 두 판정을 한 parser로 묶어 반쪽 수정이 테스트를 통과하지
  못하게 했다. PR #408 최신 head 대조 절차를 추가했다.
- 문서: 001로 연구 자료를 분리하고(LEXICO-SPLIT-01) 060 WP6 문서를
  작성했다(DIFFLEVEL-ROADMAP-01). 000의 의존성 맵을 실제 공유 지점으로
  정정했다.
@lidge-jun

Copy link
Copy Markdown
Owner

🔒 Under maintainer review — detailed feedback incoming

@lidge-jun (maintainer) has this PR in an active review pass. Please do not merge or close
it
until the detailed review lands; a full comment with specific file:line findings,
failure modes, and suggested fixes is being prepared.

This PR is not part of the current integration batch. It is held back deliberately —
either because it touches a security boundary that MAINTAINERS.md requires an explicit
security review for, because it needs to be split into reviewable units, or because it needs
a rebase onto changes landing in dev right now.

Heads-up on rebasing: dev is receiving a coordinated integration batch today. If this PR
touches the same files, expect to rebase after that batch lands. The upcoming review comment
will name the exact overlapping paths so you only have to do it once.

No action needed from you until then. Thanks for the contribution and for your patience.

Review tracker: devlog/_plan/260725_pr_issue_rework · marker posted by the maintainer review pass

@lidge-jun

Copy link
Copy Markdown
Owner

Thank you for working through the Windows service-install recovery path. I found a security-blocking executable-resolution issue that needs to be fixed before this can merge.

src/lib/windows-elevation.ts:212-213 derives the PowerShell executable passed to UAC RunAs from process.env.SystemRoot. That environment variable is caller-controlled, so a hostile process can point it at an attacker-owned directory containing System32\WindowsPowerShell\v1.0\powershell.exe; this code would then ask Windows to execute that binary with administrator privileges. The same trust rule must cover the schtasks.exe path used around src/lib/windows-elevation.ts:446.

Please resolve the real Windows system directory through a trusted OS mechanism rather than inherited environment state, construct both PowerShell and schtasks.exe from that trusted result, and fail closed if resolution cannot be verified. Add a regression test that launches the resolver with a hostile SystemRoot and proves neither elevated executable comes from it.

Because this code selects binaries for UAC elevation, it is an explicit security boundary under MAINTAINERS.md and requires maintainer security review in addition to the normal CI gates. Thanks again for improving the Windows path; once executable provenance is fixed and tested, the retry behavior can be reviewed on its own merits.


Reviewed as part of a maintainer integration pass. dev moved to ebc62d1f today (7 PRs integrated); Cross-platform CI is green on ubuntu, macOS, and Windows.

Stop deriving UAC PowerShell and schtasks paths from caller-controlled SystemRoot. Resolve the trusted system directory through kernel32 GetSystemDirectoryW, fail closed if verification fails, and add a hostile-SystemRoot regression test.
@Wibias

Wibias commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the executable-provenance block:

  • powershell.exe and schtasks.exe for UAC elevation now resolve from GetSystemDirectoryW (kernel32), not process.env.SystemRoot / WINDIR.
  • Resolution fails closed when the API/load/path verification cannot be trusted.
  • Regression test poisons SystemRoot/WINDIR and asserts neither elevated binary comes from that tree.

Commit: c55db9d — still needs maintainer security review per MAINTAINERS.md.

Linux CI fakes win32 without kernel32/System32. Add a test-only executable override seam so spawn/finalize suites can keep production-shaped paths while production resolution stays GetSystemDirectoryW-only.
@Wibias

Wibias commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Ready for merge. @lidge-jun

@lidge-jun

Copy link
Copy Markdown
Owner

Follow-up review at head 733c6f23. You've addressed everything from the previous round, and the elevation path is materially safer now — the structured access-denied marker survives the process boundary, the elevated target is narrowed from Bun/TypeScript to trusted system PowerShell and schtasks.exe, both resolve through GetSystemDirectoryW, Start-Process receives one Win32-quoted argument string, blocking execFileSync elevation is gone, and the timeout/cancellation/rollback/hostile-SystemRoot cases have tests. CI is green including the Windows and windows-schtasks jobs.

One blocker remains, and it undermines the install lock rather than the elevation itself.

windowsSchedulerTaskInstalled() collapses every query failure into "absent".

try {
  return querySchtasks(["/query", "/tn", taskName]).includes(taskName);
} catch {
  return false;
}

A permission failure and a genuinely missing task are indistinguishable here. That boolean feeds unknown-protocol and late-timeout reconciliation, so isPartialInstallError() can classify the resulting unknown result as releasable — and a task that may in fact be present releases the install lock, permitting another registration attempt against it.

A tri-state probe fixes this. Query the specific task; on failure, fall back to a CSV listing before concluding absence; if both fail, return unknown with the detail:

export type WindowsSchedulerTaskProbe =
  | { status: "present" }
  | { status: "absent" }
  | { status: "unknown"; detail: string };

Keep windowsSchedulerTaskInstalled() as a thin convenience over probe(...).status === "present", and change the security-sensitive reconciliation paths to consume the probe directly. Any unknown must become blocked-partial, never released. clearStartupInstallPartialBlock() should require an observed probe and clear only on absent.

The regression to add: inject a probeTask hook returning { status: "unknown", detail: "Access is denied…" }, drive a late protocol-failed result, and assert the reconciliation resolves to blocked-partial and that install state is never written. That requires adding probeTask to FinalizeHooks; it fails against the current head because a query failure flattens to absence.

Status: held for explicit security review under MAINTAINERS.md — this is a privilege-elevation path, and green CI does not substitute for that review. The probe fix should land first so the review sees the final shape.

Thank you for the iterations on this one; narrowing the elevated target was the right call and it made the remaining issue much easier to see.


Reviewed as part of a maintainer PABCD pass. Adds a finding not present in the earlier pass.

lidge-jun added a commit that referenced this pull request Jul 26, 2026
통합(dev push 완료): #437 803807a, #460 74ddd96, #431 82a47db,
#405 be16c1d, #468 후속 테스트 bcaf029.

close: 통합 4건 + 통합 없이 1건(#459, 설계 충돌). #457은 OPEN 유지.
이슈 close 0건 — dev에서 고쳐졌는데 안 닫힌 이슈가 없었다.

무효화: WP4(#466)와 WP6(#467)은 동료가 직접 머지했고, 우리 분석이
지적한 결함을 동료 커밋이 같은 방향으로 해소했다.

보안 보류 5건에 신규 발견 게시: #429 재분류, #355 OAuth 토큰 목적지,
#424 유료 브리지 기본 ON, #408 설치 락 해제, #403 4분할 요청.

A-gate는 사이클마다 3~5라운드를 돌았다. 반복된 원인은 계획서에
테스트나 코드를 적으면서 실물 소스를 확인하지 않은 것이었다.
Query failures must not collapse to absent and release the install lock while an elevated task may still be present.
coderabbitai[bot]

This comment was marked as resolved.

Wibias added 2 commits July 26, 2026 06:18
Export trusted-path containment and schtasks query test seams, remove dead elevated-result branches, and harden probe/elevation regression coverage across platforms.
Replace the literal backslash prefix check so host-native temp paths on
Linux/macOS CI are accepted as children of the trusted system directory.
@Wibias

Wibias commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Ready for merge. @lidge-jun

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit maintainer review completed at exact head b711ddf4aabd45f4913f2b2a78add4aa751d9645. The elevated path remains restricted to trusted system PowerShell and schtasks.exe resolved through GetSystemDirectoryW, preserves argument boundaries, and does not elevate Bun or repository code. The remaining install-lock blocker from the prior review is fixed with a tri-state scheduler probe: query failures fall back to an exact CSV lookup, unverifiable state remains unknown, late protocol reconciliation becomes blocked-partial, and the lock clears only on proven absence. Verified 121 focused service/elevation/reconciliation tests plus typecheck; all current cross-platform and dedicated Windows service checks are green.

@Ingwannu

Copy link
Copy Markdown
Owner

Maintainer approval is complete at b711ddf4. I also verified the final PR tree is limited to the seven Windows service/elevation source and test files; there is no workflow-file diff. The merge-commit API nevertheless rejects this branch history for the current maintainer OAuth token because it lacks the workflow scope. I am not changing the merge method to bypass that permission boundary. @lidge-jun, please perform the final merge with owner credentials; the exact head, focused tests, typecheck, dedicated Windows jobs, and cross-platform CI are all green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants