fix(security): retry required ACL harden after soft timeout

[Wibias]

Summary

Fixes #766

Pivots away from OPENCODEX_ALLOW_UNVERIFIED_ADMIN_TOKEN_ACL. Soft required:false ACL timeouts during loadConfig no longer poison later required:true management-token hardens (namespaced timeout memo). When management still cannot initialize, /api/* 503 responses include the reason and hint to set OPENCODEX_ADMIN_AUTH_TOKEN.

Security

Keeps fail-closed ACL defaults; no unverified-ACL opt-in. Environment admin token remains the explicit escape that skips file-backed hardening.

Test plan

• CI green
• bun test tests/server-management-auth.test.ts tests/windows-secret-acl.test.ts

Summary by CodeRabbit

• Bug Fixes
  • Improved Windows service status reporting and cleanup, including detection of stale processes and incomplete task removal.
  • Prevented unsupported Microsoft-account logins during native Windows service installation.
  • Ensured service shutdowns verify that background processes do not restart unexpectedly.
  • Fixed security-hardening retries so required protection is not skipped after an earlier timeout.
• Usability
  • Added clearer guidance when management authentication or token-file security checks fail.
  • Windows interactive installation now explains that an elevated Command Prompt or PowerShell window is required.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change separates required and optional ACL timeout state, improves management-token diagnostics, and hardens Windows service installation, scheduler inspection, stop, uninstall, and proxy cleanup with account, PID, task, and liveness checks.

Changes

Management authentication hardening

Layer / File(s)	Summary
Required and optional ACL retry behavior src/lib/windows-secret-acl.ts, tests/windows-secret-acl.test.ts	ACL timeout memoization now distinguishes required from optional hardening. Tests verify that required hardening retries after an optional timeout.
Management-token failure diagnostics src/server/management-auth.ts, tests/server-management-auth.test.ts	ACL failures and management API unavailability responses now include initialization details and OPENCODEX_ADMIN_AUTH_TOKEN guidance. Tests cover timeout handling, required retries, and environment-token authorization.

Windows service lifecycle hardening

Layer / File(s)	Summary
Windows scheduler and proxy status inspection src/service.ts	Scheduler status now combines task presence with live-proxy state through asynchronous inspection helpers.
Windows installation account and console checks src/lib/winsw.ts, src/service.ts	Interactive WinSW installation requires a TTY. Native installation rejects Microsoft-account logins.
Scheduler stop and proxy cleanup validation src/service.ts	Stop and uninstall operations tolerate benign failures, verify task removal, validate PID identity, clean orphaned proxies, and suppress native Codex restoration when a proxy remains live.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ServiceStop
  participant Scheduler
  participant ProxyProcess
  participant NativeCodex
  ServiceStop->>Scheduler: Stop installed scheduler service
  ServiceStop->>ProxyProcess: Validate PID identity and poll liveness
  ProxyProcess-->>ServiceStop: Return stopped or still-live state
  alt Proxy is stopped
    ServiceStop->>NativeCodex: Restore native Codex
  else Proxy remains live
    ServiceStop-->>ServiceStop: Set failure exit code
  end

Loading

Possibly related PRs

• lidge-jun/opencodex#800: Overlaps in service-stop and proxy-liveness handling.
• lidge-jun/opencodex#642: Relates to identity-verified proxy liveness used by service diagnostics.
• lidge-jun/opencodex#697: Overlaps in management-token ACL hardening and authentication handling.

Suggested reviewers: ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR fixes ACL timeout contamination but does not address the issue's startup-health, settings-latency, logs-filter, or remaining management-API 503 problems.	Either narrow the linked issue to ACL handling or implement the remaining #766 dashboard fixes, including startup-health recovery, nonblocking settings discovery, and logs filtering.
Out of Scope Changes check	⚠️ Warning	src/service.ts and src/lib/winsw.ts add scheduler, proxy-liveness, account, and interactive-install changes not required by #766.	Move the unrelated service-management and WinSW changes to a separate pull request, or link issues that define those requirements.
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the primary security fix: retrying required ACL hardening after a soft timeout.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e7f61e7c71

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Wibias]

[shipping-github] Merge-ready

[Wibias]

[shipping-github] Security sanity (redacted) — quick pass on Windows admin-token ACL opt-in.

• Scope: When NTFS ACL hardening cannot be verified on Windows, file-backed admin token may load only under an explicit opt-in path; response surfaces \�clUnverified\ so management can fail closed in the GUI.
• Default posture: Unverified ACL without opt-in should still reject / warn (fail-closed for dashboard API).
• Tests: Win32 ACL opt-in test sets \USERNAME\ on CI; no token material in logs.
• Residual risk: Opt-in on shared/multi-user hosts increases exposure if token file permissions are weak — operator-config tradeoff, document clearly at enable time.

Recommend maintainer security review before merge (auth boundary).

[Wibias]

Verdict: gated

• Usefulness: Fixes Dashboard shows no data when ACL hardening cannot be verified (management API fails closed), plus a permanent false startup-protection warning and a 10s first paint #766 — default fail-closed ACL; explicit OPENCODEX_ALLOW_UNVERIFIED_ADMIN_TOKEN_ACL opt-in; baked into Windows service envs.
• Bugs: directory-timeout aggregation / per-server ACL state are valid follow-ups, not merge blockers for the stated fix.
• Security: Maintainer security review required (weakens NTFS ACL guarantees on timeout). Prefer a short docs note for the bypass env before or with merge.
• Owner/maintainer reviews: security sign-off needed.
• Bots: P1 service propagation fixed.
• Base / CI: PR tests green; windows suite noise.
• Gate: security review (+ docs preferred).

[lidge-jun]

Reviewed while sweeping open security-boundary work; not landing this one, and the reason is the design rather than the code.

A concrete bug first. assertSafeDirectory can soft-continue at src/server/management-auth.ts:53-60, but that result is discarded — only the file-hardening path sets aclUnverified. So /api/settings can report aclUnverified: false while the directory boundary was never verified. The flag that exists to make this visible can therefore under-report exactly the case it was added for. That is fixable by propagating the directory status, but it does not resolve the larger question.

The larger question. OPENCODEX_ALLOW_UNVERIFIED_ADMIN_TOKEN_ACL opens a fail-closed boundary. The threat model is narrow but real: it takes an operator to enable, and a normal unprivileged process cannot alter a privileged service's baked environment. But once enabled on a multi-user host where the ACL genuinely is weak, another local user can read the token file — or replace it before restart — and obtain full management access. The opt-in is doing what it says; the question is whether #766 needs it.

It may not. #766's user-visible complaint is a dashboard that shows nothing plus a permanent false startup warning. The underlying cause is that ACL hardening times out (src/lib/windows-secret-acl.ts:403-412 returns {ok:false} even for required:true), and a timeout is not the same as a hostile ACL. Fixing the deadline/spawn behaviour around windows-secret-acl.ts:88-103 and :375-415 would address the reported symptom without adding a way to run unverified. There is also an existing escape that does not weaken the boundary: an operator-supplied OPENCODEX_ADMIN_AUTH_TOKEN skips file creation entirely (management-auth.ts:122-126), which pairs well with a dashboard error that names that remedy instead of failing silently.

If it does land anyway, the negative test that matters: simulate hardenSecretDir() timing out while token-file hardening succeeds, and assert initialization stays unavailable and /api/settings returns 503. Without it, a later change can quietly admit an unverified directory.

Not closing — this is the maintainer's call under MAINTAINERS.md, and the diagnosis here is solid work regardless of which direction is chosen. @lidge-jun @Ingwannu for the security review.

[Wibias]

[shipping-github] Addressed owner feedback — propagate directory ACL soft-continue into aclUnverified; add dir-timeout + file-ok negative tests (503 without opt-in, flag with opt-in). Design preference (timeout fix vs opt-in) still open for maintainer call.

[Wibias]

[shipping-github] Addressed owner feedback — pivoted off OPENCODEX_ALLOW_UNVERIFIED_ADMIN_TOKEN_ACL. Timeout memo is now namespaced so loadConfig soft-fails do not poison required management hardens; 503/reason names OPENCODEX_ADMIN_AUTH_TOKEN as the escape.

[Wibias]

Ready to merge @lidge-jun @Ingwannu - React Doctor does not seem to be related to this