fix(service): stage elevated Task Scheduler XML instead of inlining it (#4692) - #4759
Conversation
#4692) When "ocx service repair" re-registered the task through the elevated fallback, the spawn failed before UAC ever appeared: WindowsElevationError: ENAMETOOLONG: name too long, uv_spawn at startPowerShellCommand (src/lib/windows-elevation.ts:560) at runWindowsElevatedScheduledTaskRegistration (.../windows-elevation.ts:704) runWindowsElevatedScheduledTaskRegistration embedded the new task XML and the expected-existing snapshot as base64(utf16le) inside an inner PowerShell script, which was then base64(utf16le)-encoded again into -EncodedCommand. Two base64 layers over UTF-16 cost roughly 14.2 command-line characters per XML character, and a replacement carries two payloads, so a ~2 KB definition put the outer command past the Windows limit. On a host where Task Scheduler exports the trigger scope as an account name the re-register path runs on every repair, so repair could never exit 0. Both payloads are now staged to files and the command carries two paths and two 64-character digests, so its length no longer depends on the size of the XML at all. A file an administrator process will read is itself a privilege-escalation surface, so three properties hold together and none is sufficient alone: - Access. The staging directory is created fresh by mkdtemp and ACL-hardened through the existing hardenSecretDir/hardenSecretPath before anything is written into it, so the payload is private from the moment it exists. - No redirection. Each artifact is inspected with lstat and rejected unless it is what it claims to be. Exclusive "wx" creation inside a directory that did not exist a moment ago is the atomic step; the explicit check keeps that guarantee from resting on a reading of O_EXCL semantics. - Tamper evidence. The digest covers the exact bytes written, and the elevated script reads the file once, hashes what it read, and refuses before decoding. An ACL cannot cover this: a process running as the same user has the same SID and can rewrite the file, so the digest is what makes a swap during the UAC prompt fail closed instead of registering a different definition. Cleanup runs on every exit -- success, UAC cancellation, a synchronous spawn failure, a failed digest check, and a partial staging failure -- and a cleanup error is aggregated with the registration error rather than replacing it. The original "immutable bytes, never a caller-writable pathname" goal is kept by different means rather than abandoned, and the replacement precondition is untouched: the elevated process still re-queries the live registration and compares it to the verified predecessor before passing -Force. Payloads are UTF-16LE with no BOM and are decoded straight into Register-ScheduledTask, so what is hashed is exactly what is registered, with no trimming step the two sides could disagree about. Closes #4692
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 77 / 80이 PR은 Windows에서 더 아픈 점은 #4691과 겹칩니다. Task Scheduler가 트리거 범위를 계정 이름으로 내보내는 호스트에서는 보안 모델도 같이 바뀝니다. 예전 목표는 “불변 바이트, 호출자가 쓰는 pathname은 안 넘김”이었고, 지금은 그 목표를 다른 수단으로 지킵니다. (1) 디렉터리를 먼저 코드 경로는 현재 라인 ~670+ (windows-elevation.ts 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac87703657
ℹ️ 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".
| const hardenDir = deps.hardenDir ?? ((path: string) => { hardenSecretDir(path, { required: true }); }); | ||
| const writePayload = deps.writePayload ?? ((path: string, bytes: Buffer) => { | ||
| writeFileSync(path, bytes, { flag: "wx", mode: 0o600 }); | ||
| }); | ||
| const hardenPath = deps.hardenPath ?? ((path: string) => { hardenSecretPath(path, { required: true }); }); |
There was a problem hiding this comment.
When OpenCodex runs under a standard Windows account and the RunAs credential prompt is completed with a separate administrator account, hardenSecretDir and hardenSecretPath grant access only to the original user's SID and remove inherited grants. The elevated PowerShell process therefore cannot traverse or read these staged files, so every access-denied install or repair fallback fails at ReadAllBytes even though the previous inline transport worked. Stage the payload with an ACL readable by the elevated administrator context, or use a transport that does not depend on the unelevated user's private ACL while retaining the digest check.
Useful? React with 👍 / 👎.
| writePayload(path, bytes); | ||
| files.push(path); |
There was a problem hiding this comment.
If writeFileSync creates or partially writes the file and then throws—for example because the volume fills or an I/O error occurs—the path is never added to files. The catch path consequently attempts to remove only the directory, gets ENOTEMPTY, and leaves the staged payload behind, contradicting the cleanup-on-every-exit contract. Arrange for a file created by a failed write to be tracked and removed without risking deletion of a pre-existing path.
Useful? React with 👍 / 👎.
Staging the elevated Task Scheduler XML introduces exactly one new failure of its own: hardenSecretPath grants the staging account and strips inheritance, so a split-token elevation of the same user reads the file while an elevation answered with a DIFFERENT administrator's credentials does not. The inline form had no such dependency. The elevated process runs hidden, so nothing it writes survives and only the exit code crosses back. That made the failure an unexplained non-zero status -- the same undiagnosable shape as the ENAMETOOLONG this change set removes. The read failure now has its own protocol code, and the parent turns it into a message that names both the cause and the way out: approve the prompt as the signed-in user, or run again from a session already elevated as that user. The code sits outside OCX_ELEVATED_PROTOCOL_CODES, which is the create-and-run transaction's alphabet, and cannot collide with UAC cancellation. Whether to widen the ACL to SYSTEM and Administrators is left as a separate security decision rather than bundled here, because it changes a security-sensitive module.
…ce suite (#4692) The file-size ratchet failed: tests/service/service.test.ts has a committed cap of 4106 lines and the new staging cover pushed it to 4245. The ratchet only ever lowers baselines, so growing past a cap is the thing it exists to refuse, not something to re-baseline around. The cover moves to tests/windows/windows-elevation-spawn.test.ts, which is the better home anyway: its subject is the elevated registration payload, which is exactly what these tests exercise. That file has no cap and stays well under the 2000-line threshold, and the service suite returns to its baseline unchanged, so no new test file and no test-layout registration are needed. Also replaces a logical-assignment shorthand in the staging cleanup with the explicit form the surrounding code already uses. No behaviour change; folded in here rather than spending a separate CI cycle on it.
Brings in the Windows desktop-restart fix f79c147 through the chain, so this lane's Windows evidence measures this lane. The previous dispatch at 6a2b148 had windows 2/6 fail with nine assertions, every one of them in tests/clients/desktop-app-restart-posix.test.ts and none touching anything this lane changes. The same nine failures, at the same line numbers, are on the dev-only dispatch 34795291889 from 2026-09-14, which is what identifies the defect as pre-existing rather than introduced here. The other eleven shards were green: test 1/4 through 4/4 and windows 1, 3, 4, 5 and 6 of 6. No [skip ci] here: this is the lane tip, and its run is the gate for all three layers.
|
Landing the Windows service and update recovery lane, top layer first. Elevated Task Scheduler re-registration no longer passes the XML on the command line, so the payload size stops deciding whether the spawn exceeds the limit. The staging directory is ACL-hardened before anything is written to it, and the elevated script verifies the bytes it read by digest rather than reopening a path. Evidence at the verified tip 8a1b010 (tree
Chained-child stacks merge top-down, so this lands in the parent branch and cascades to Maintainer integration decision under MAINTAINERS.md / AGENTS.md: a maintainer with maintain or admin access may integrate into |
…registration-length fix(service): stage elevated Task Scheduler XML instead of inlining it (lidge-jun#4692)
1 participant
Summary
When
ocx service repairre-registered the Task Scheduler task through the elevated fallback, the spawn failed before UAC ever appeared:runWindowsElevatedScheduledTaskRegistrationembedded the new task XML and the expected-existing snapshot as base64(utf16le) inside an inner PowerShell script, which was then base64(utf16le)-encoded again into-EncodedCommand. Two base64 layers over UTF-16 cost roughly 14.2 command-line characters per XML character, and a replacement carries two payloads, so a ~2 KB definition put the outer command past the Windows limit. On a host where Task Scheduler exports the trigger scope as an account name (#4691)identityUpgradeNeededstays true, so the re-register path runs on every repair and repair could never exit 0.Both payloads are now staged to files, and the command carries two paths and two 64-character digests. Its length no longer depends on the size of the XML at all.
A file an administrator process will read is itself a privilege-escalation surface, so three properties hold together and none of them is sufficient alone:
mkdtempand ACL-hardened through the existinghardenSecretDir/hardenSecretPathbefore anything is written into it, so the payload is private from the moment it exists.lstatand rejected unless it is what it claims to be. Exclusivewxcreation inside a directory that did not exist a moment ago is the atomic step — there is no replace path to race — and the explicit check keeps that guarantee from resting on a reading ofO_EXCLsemantics.Cleanup runs on every exit — success, UAC cancellation, a synchronous spawn failure, a failed digest check, and a partial staging failure — and a cleanup error is aggregated with the registration error rather than replacing it.
The original "immutable bytes, never a caller-writable pathname" goal is kept by different means rather than abandoned, and the replacement precondition is untouched: the elevated process still re-queries the live registration and compares it to the verified predecessor before passing
-Force. All three existing safety checks remain — capture-and-validate before elevation, re-check the predecessor immediately before UAC, and re-query and compare after consent.Payloads are UTF-16LE with no BOM and are decoded straight into
Register-ScheduledTask, so what is hashed is exactly what is registered, with no trimming step the two sides could disagree about.This is the tip of a three-layer stack (#4746 -> #4749 -> this), so it targets
codex/win2-schtasks-gbk. Retarget todevonce the parents land.Closes #4692
Verification
No local test suite, single test file, typecheck, build or install was run — the repository owner prohibits it for this lane. Local verification is explicitly NOT RUN. The Windows job in this PR's hosted CI run is the only platform evidence for all three layers in the stack; the two parent layers carry
[skip ci]by design, so this run gates the whole lane.Static verification performed:
repairService->reregisterWindowsSchedulerTask->registerFreshWindowsSchedulerTask-> structuredcreate/access-denied->runWindowsElevatedScheduledTaskRegistration->startPowerShellCommand, and confirmed the access-denied classification and the UAC-cancellation mapping are correct and unchanged; only the transport was wrong.inner = C + B(N) + r*B(M)andouter = 4*ceil(2*inner/3), giving about14.22 * Nfor a replacement with equal-sized payloads, so a 2 KB definition contributes roughly 29,000 characters against the 32,767 limit. That is the arithmetic the size-independence test now pins.deps.elevateseam keeps its string signature so existing capture-before-elevation tests still exercise the same invariant.ReadAllBytes, since hashing a path and reopening it would reintroduce the swap window.Regression tests added or updated (they run in CI, not locally):
tests/windows/windows-elevation-spawn.test.tsasserts the elevated script carries the staged paths and digests, performsReadAllBytes->ComputeHash-> digest comparison before decoding, contains neither XML's base64 nor anyFromBase64String, and that the outer command length is identical across payloads and stays under a conservative 8 KB bound for both the create and the replace shapes. It also pins that a replacement without a captured predecessor still throws.tests/service/service.test.tspins the staging contract: the directory is hardened before either payload is written, the digest equals a SHA-256 computed independently over the bytes actually on disk, the bytes are UTF-16LE with no BOM, cleanup is idempotent and removes everything, a redirected path is refused without hardening it, and a failure on the second payload still removes the first.Not provable without a real Windows host, and not claimed: the exact quoting libuv produces before
CreateProcessWand the precise length at which this Bun version reportsENAMETOOLONG; PowerShell 5.1 behaviour forSHA256/Unicode.GetStringin this launcher chain; split-token UAC access to the current-user-hardened file on every account type; NTFS ACL and reparse behaviour on a real volume; and antivirus interference with cleanup timing.One behavioural limitation worth calling out for review:
hardenSecretPathgrants the current user's SID and strips inheritance, so the staged files are readable by a split-token elevation of the same user (the normal case) but not by an elevation performed with a different administrator's credentials. The previous inline form had no such dependency. Widening the ACL to SYSTEM/Administrators would be a deliberate change to a security-sensitive module and is deliberately not bundled here.Checklist
stageElevatedSchedulerRegistrationandrunWindowsElevatedScheduledTaskRegistration.-Forceprecondition is unchanged. No credential, token, OAuth, workflow or release-automation surface is touched, and nothing new is logged.Prior-art check
No existing pull request, open or closed, implements this fix. Searched the repository's pull requests by issue number and by implementation signature, and inspected the adjacent antecedents (#3040, #4313, #2918, #3067) — each addresses a different problem and no code is carried from any of them. No
Co-authored-bytrailer is therefore owed. Recording the check here so the question does not have to be reopened.Follow-up
The ACL limitation described above is tracked as #4779: whether the staged payloads should also grant SYSTEM and Administrators is a change to a security-sensitive module and is deliberately a separate decision. This PR makes the failure diagnosable rather than silent — the elevated read failure has its own protocol exit code and the parent reports the cause and the remedy instead of a bare number, so the limitation cannot present as an unexplained "access denied".
CI evidence (lane tip)
Hosted CI at the exact tip head
8a1b01011dbf68cd5eef07c568281590981fcaea:workflow_dispatchlane=all — success: run 35054231781. All 26 jobs succeeded, with no non-success conclusion.test 1/4–4/4andwindows 1/6–6/6are allcompleted success.platform-windowsis gated ongithub.event_name == 'workflow_dispatch', so it never runs on apull_requestevent. Since all three layers of this lane are Windows-only, a PR-event run alone would carry no platform evidence for them.enforce-target,PR hygieneandPR Labelerare all green at the same head.An earlier
pull_requestrun at the previous head6a2b148f5awas also green (run 35051066482); the dispatch run above is a strict superset of its job set.One earlier dispatch surfaced a
windows 2/6failure whose nine assertions were all intests/clients/desktop-app-restart-posix.test.ts. That defect is not from this lane: the identical file and the identical nine line numbers appear on the dev-only dispatch 34795291889 from 2026-09-14. It is fixed ondevbyf79c147309, which landed after this lane's original base, sodevwas merged up the chain to absorb it. Verified by blob: this tree carriedsrc/codex/desktop-app/windows.tsat863a20057abefore andc73e067e5bafter.