[WRONG BRANCH] fix(service): decode schtasks output with the Windows text decoder (#4691) - #4749
Conversation
…4691) [skip ci] On a zh-CN host (ACP/OEMCP 936) with a CJK account name, "ocx service repair" and the dashboard repair/install buttons failed against a registration OpenCodex had created itself: Service repair failed: Task Scheduler registration is not a recognized legacy OpenCodex definition; it was preserved for manual review. Redirected "schtasks /query /xml" output follows the console output code page of the spawning process tree, not the XML document encoding. In any 936 context -- including the no-console background service on a zh-CN host -- the bytes are GBK. decodeSchtasksOutput probed UTF-16 and then fell back to a plain UTF-8 decode, so the CJK account name inside <SessionStateChangeTrigger><UserId> became U+FFFD. The correctly resolved expected identity [SID, MACHINE\<name>] then never matched the trigger scope, windowsTaskRegistrationHealthy returned false, and repair aborted at its recognition gate. The same mojibake rolled back fresh installs at post-create verification. The fix is entirely in byte decoding, before any XML is parsed. decodeSchtasksOutput now delegates to decodeWindowsTextBytes, the decoder this project already built for exactly this class (UTF-16, then strict UTF-8, then the locale's legacy code page). It already fixed the sibling whoami/PowerShell decode in src/lib/windows-user-principal.ts (#2914, and #722 for CP949); this call site was the last one still ending in a lossy UTF-8 decode. Task ownership is deliberately untouched. windowsTaskTriggerScopeAcceptable still requires an exact identity match, and the tests assert that a different account and the mojibake spelling are both still rejected. Forgiving a replacement character there would let two different non-ASCII accounts collapse to the same value, which is worse than the refusal it replaces. Delegating also fixes a latent UTF-16BE edge: the old local copy allocated buffer.length - 2 bytes for an odd-length payload and left a trailing uninitialized byte. The shared decoder rounds the payload down instead. Closes #4691
|
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은 중국어(zh-CN) Windows에서 고치는 방법은 XML을 손대거나 소유권 비교를 느슨하게 하는 게 아닙니다. 바이트 디코딩만 현재 라인 25~26 (windows-scheduler.ts JSDoc) - 주석 줄 두 곳이 별표 뒤에 공백 없이 이어져 JSDoc 정렬이 깨져 있습니다. 동작에는 영향 없지만 문서 블록만 살짝 흐트러집니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
#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
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.
Propagates the dev merge from the layer below, including the Windows desktop-restart fix f79c147 that the lane's Windows evidence needs. Nothing in this layer changes; cascading keeps this pull request's diff limited to the schtasks decode.
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.
…ion-length fix(service): stage elevated Task Scheduler XML instead of inlining it (#4692)
|
Cascading downward. schtasks output on a zh-CN host is decoded through the shared Windows decoder instead of a UTF-8 fallback that mangled GBK bytes. 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 |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
1 participant
Summary
On a zh-CN host (ACP/OEMCP 936) with a CJK account name,
ocx service repairand the dashboard repair/install buttons failed against a registration OpenCodex had created itself:Redirected
schtasks /query /xmloutput follows the console output code page of the spawning process tree, not the XML document encoding. In any 936 context — including the no-console background service on a zh-CN host — those bytes are GBK.decodeSchtasksOutputprobed UTF-16 and then fell back to a plain UTF-8 decode, so the CJK account name inside<SessionStateChangeTrigger><UserId>became U+FFFD. The correctly resolved expected identity[SID, MACHINE\<name>]then never matched the trigger scope,windowsTaskRegistrationHealthyreturned false, and repair aborted at its recognition gate. The same mojibake rolled back fresh installs at post-create verification.The fix is entirely in byte decoding, before any XML is parsed.
decodeSchtasksOutputnow delegates todecodeWindowsTextBytes(src/lib/windows-text.ts), the decoder this project already built for this class: UTF-16, then strict UTF-8, then the locale's legacy code page. It already fixed the siblingwhoami/PowerShell decode insrc/lib/windows-user-principal.ts(#2914, and #722 for CP949); this call site was the last one still ending in a lossy UTF-8 decode. The decoder selects the code page from the process locale rather than a console handle, which is why it also works in the no-console service context.Task ownership is deliberately untouched.
windowsTaskTriggerScopeAcceptablestill requires an exact identity match, and the added tests assert that a different account and the mojibake spelling are both still rejected. Forgiving a replacement character there would let two different non-ASCII accounts collapse to the same value, which is a worse failure than the refusal it replaces.Delegating also removes a latent UTF-16BE edge: the old local copy allocated
buffer.length - 2bytes for an odd-length payload and left a trailing uninitialized byte, while the shared decoder rounds the payload down.Stacked on #4746, so this PR targets
codex/win1-update-teardown. Retarget todevonce the parent lands.Closes #4691
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. Hosted CI on the lane tip is the only execution evidence, and the Windows job there is the only platform evidence that exists for this change.
Static verification performed:
repairService->statusWindowsXml->schtasks->querySchtasks->runFile(which correctly capturesencoding: "buffer") ->decodeSchtasksOutput, thenresolveWindowsTaskDiagnosticUserId->windowsTaskRegistrationHealthy->windowsTaskHasSessionRecoveryTriggers->windowsTaskTriggerScopeAcceptable->taskXmlDecodedValueEquals. Confirmed the identity comparison is exact and fails closed, and that only the decode is wrong.src/service/windows-ops.ts, reads through the same decoder, so the install rollback path is fixed by the same change.decodeWindowsTextBytesreproduces the existing UTF-16LE/BE and BOM handling, so the delegation is behaviour-preserving for the encodings this function already handled.src/service/windows-ops.ts:279reads the staged UTF-16LE task XML through this same function and is unaffected. Other Windows child-output decoders (readWindowsPrincipalSource,icaclsinwindows-secret-acl.ts,sc.exe/WinSW inwinsw.ts, elevation diagnostics) still use raw UTF-8. Their expected values are ASCII enum-like tokens rather than account names, so they are out of scope here rather than overlooked; folding them in would need separate behaviour review.Regression tests added to
tests/windows/windows-scheduler-install-verification.test.ts(they run in CI, not locally):<UserId>isMACHINE\张三, encoded with literal CP936 bytes (0xD5C5 0xC8FD) for the same reasontests/windows/windows-text-decoding.test.tsuses literal hex: encoding the fixture with the decoder under test would assert nothing. The test asserts the decode is lossless andwindowsTaskRegistrationHealthyaccepts it.zh-CNlocale still decodes as UTF-8, because the strict UTF-8 attempt runs before any code-page guess. Windows service management breaks on non-ASCII (Chinese) usernames / UTF-8 codepage: 'ownership could not be proven' and 'registered as .\���� ... rolled back' #4106 was closed as not-planned because that reporter's console was CP 65001; this pins that the fix leaves that case alone.Not provable without a real zh-CN Windows host, and not claimed: the exact bytes
schtasks.exeemits in every no-console service configuration, whether Bun's Windows runtime exposes the expectedIntllocale inside the installed service, and end-to-end repair/install/rollback against a real Task Scheduler.Checklist
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.