From fb2ce7d20a0225b1698305f51e9c0e5844b8bb32 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:05:58 +0900 Subject: [PATCH 1/5] fix(update): keep a history-preflight refusal from aborting the update (#4718) [skip ci] On Windows, "ocx update" stopped the service, entered the pending shared teardown path, printed "Native restore refused: history_paginated_requires_native_writer", and then aborted with "could not stop the running proxy". The service was down, no listener was left, and the old package was still installed. Installing the same target by hand worked. The refusal itself is correct and stays. The Codex history preflight runs before the config half of the restore, so it returns an envelope whose config, catalog and history artifacts are all "skipped" -- nothing was attempted. restoreSharedClientStateAfterStop classified only two shapes, a later history failure and everything else, so the refusal fell through to "everything else", ocx stop exited 1, and decidePostStopUpdate read 1 as a proxy that would not die. The reported lane is bin/ocx.mjs; the Bun updater shares the same decision module and had the same defect. The obligation really is outstanding here: config and catalog were never restored, so the client still points at the proxy that just stopped. Treating the refusal as the existing history-only case would have discharged the receipt and lost that. So this adds a third outcome rather than widening the second. - CodexNativeRestoreResult.historyPreflightRefusal carries the refusal as a structured reason. The artifact states cannot carry it: an ownership refusal and a desired-state skip produce the same three "skipped" values, and matching the message would put a safety decision on prose. - ocx stop keeps the receipt, says so, and exits 80. Eighty is not 79: seventy-nine means the teardown ran and only history metadata is pending, and a caller reading it discharges the obligation. - Eighty is only emitted when pendingTeardownsAreExactly confirms the obligations left in the home are exactly the ones this run chose to keep. A quarantined receipt or a concurrent stop's claim falls back to exit 1, which is the pre-existing behaviour, so the fallback loses nothing. - decidePostStopUpdate lets 80 past the teardown gate and nothing else. Runtime records, a live proxy and an unreadable probe abort exactly as before, because a history refusal is evidence about history and says nothing about whether the proxy is gone. - Both updater lanes report the deferral as its own outcome instead of reusing the manifest warning, which would imply config and catalog came back. Closes #4718 --- bin/ocx.mjs | 10 ++ src/cli/index.ts | 53 +++++++++- src/codex/inject/restore.ts | 31 +++++- src/config/pending-teardown.ts | 31 ++++++ src/update/index.ts | 10 ++ src/update/stop-contract.d.mts | 1 + src/update/stop-contract.mjs | 19 ++++ src/update/stop-decision.d.mts | 2 +- src/update/stop-decision.mjs | 15 ++- .../codex-inject-integration.test.ts | 9 ++ tests/providers/xai/grok-lifecycle.test.ts | 6 +- tests/service/stop-deferred-teardown.test.ts | 98 ++++++++++++++++++- .../update/update-stop-classification.test.ts | 80 ++++++++++++++- 13 files changed, 349 insertions(+), 16 deletions(-) diff --git a/bin/ocx.mjs b/bin/ocx.mjs index ef3aa80cd86..7b3a54eaa2c 100755 --- a/bin/ocx.mjs +++ b/bin/ocx.mjs @@ -522,6 +522,16 @@ function runPackageManagerSelfUpdate(manager) { " After the update: close the Codex app, run 'ocx doctor', then run 'ocx stop' once to retry.", ); } + if (decision.reason === "history-deferred") { + // The reported #4718 path is this lane. Nothing was restored, so this is a different + // sentence from the manifest warning above: an operator told "history metadata is + // incomplete" would assume config and catalog already came back. + console.warn( + "opencodex: WARNING — the shared teardown was refused by the Codex history preflight and restored nothing.\n" + + " Config, catalog, history and provenance were preserved, and the teardown receipt was kept.\n" + + " The proxy is down, so the update continues; close the Codex app and run 'ocx stop' once afterwards to finish the restore.", + ); + } } // npm keeps the existing stage -> verify -> swap -> rollback flow. pnpm owns a diff --git a/src/cli/index.ts b/src/cli/index.ts index 9d37cb5c210..9df96b3fad5 100755 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -15,7 +15,7 @@ try { } import { currentExternalCodexModelProvider, restoreNativeCodex, restoreNativeCodexAsync, shouldInjectApiAuthHeader } from "../codex/inject"; import { stripGrokConfig } from "../grok/inject"; -import { STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../update/stop-contract.mjs"; +import { STOP_HISTORY_DEFERRED_EXIT_CODE, STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../update/stop-contract.mjs"; import { describeHistoryJobFailure, resolveCodexHistoryJobTarget, @@ -46,6 +46,7 @@ import { isPendingTeardownAbandoned, listPendingTeardowns, pendingTeardownPathFor, + pendingTeardownsAreExactly, quarantinePendingTeardown, } from "../config/pending-teardown"; import { collectStatus, hubStatusLines, remoteHubBannerLine, remoteHubStatusLines, unusedProxyWarningLines } from "./status"; @@ -785,9 +786,16 @@ async function handleRestartStartWhenStopped(): Promise { * * The distinction exists because `ocx update` must proceed for the first and abort for the * second, and it can only see an exit code (#3008). + * + * `historyDeferred` is the third kind (#4718). The Codex history preflight refuses BEFORE + * the config half runs, so nothing was restored at all: config, catalog, history and + * provenance are untouched and the client is still routed at the proxy that just stopped. + * Like `historyOnly` the proxy is genuinely down, so an update may replace package files. + * Unlike `historyOnly` the obligation was not performed, so the receipt must survive. */ -async function restoreSharedClientStateAfterStop(): Promise<{ historyOnly: boolean; other: boolean }> { +async function restoreSharedClientStateAfterStop(): Promise<{ historyOnly: boolean; historyDeferred: boolean; other: boolean }> { let historyOnly = false; + let historyDeferred = false; let other = false; try { const result = await restoreNativeCodexAsync(); @@ -798,7 +806,16 @@ async function restoreSharedClientStateAfterStop(): Promise<{ historyOnly: boole // not — a client reads those, so their failure is a real teardown failure. const artifacts = result.artifacts; const configOrCatalogFailed = artifacts.config.state === "failed" || artifacts.catalog.state === "failed"; - if (!configOrCatalogFailed && artifacts.history.state === "failed") historyOnly = true; + // A preflight refusal reports every artifact as `skipped` because none of them were + // attempted. Reading the states alone cannot tell that apart from an ownership + // refusal, so the structured reason carries it and the states are still required to + // agree — a refusal that somehow reports a failed artifact is not this case. + const preflightRefused = result.historyPreflightRefusal !== undefined + && artifacts.config.state === "skipped" + && artifacts.catalog.state === "skipped" + && artifacts.history.state === "skipped"; + if (preflightRefused) historyDeferred = true; + else if (!configOrCatalogFailed && artifacts.history.state === "failed") historyOnly = true; else other = true; console.error(`⚠️ ${result.message}`); } @@ -816,7 +833,7 @@ async function restoreSharedClientStateAfterStop(): Promise<{ historyOnly: boole other = true; console.error(`⚠️ Grok config restore failed: ${error instanceof Error ? error.message : String(error)}`); } - return { historyOnly, other }; + return { historyOnly, historyDeferred, other }; } async function handleStop() { @@ -860,6 +877,11 @@ async function handleStop() { }; let stopFailed = false; let historyOnlyFailure = false; + /** + * Obligations this run deliberately kept because the Codex history preflight refused + * before restoring anything (#4718). Non-null selects the deferred exit code. + */ + let historyDeferredNonces: string[] | null = null; // Only Task Scheduler respawns after a successful stop (#764), so only it earns the // restart-window wait; launchd, systemd and WinSW are down when they say so. let schedulerCanRespawn = false; @@ -1138,6 +1160,7 @@ async function handleStop() { } const restore = await restoreSharedClientStateAfterStop(); if (restore.other) stopFailed = true; + else if (restore.historyDeferred) historyDeferredNonces = teardownNonce ? [teardownNonce, ...recoveredNonces] : recoveredNonces; else if (restore.historyOnly) historyOnlyFailure = true; // The obligation is discharged whether or not history metadata finalized: config and // catalog are what a client reads, and `restore.other` already fails the stop. @@ -1145,7 +1168,16 @@ async function handleStop() { // Each nonce names its own file, so a clear can only ever remove the obligation it // names — never one a concurrent stop wrote. Both this run's claim and every inherited // receipt it proved discharged are released together. - if (!restore.other) { + // + // A history-preflight refusal is the exception: it restored nothing, so there is + // nothing to discharge. Clearing here would drop a real obligation on the floor and + // leave the client config pointing at a proxy that is gone, with nothing on disk + // saying so — which is the whole failure the receipt exists to prevent (#4718). + if (restore.historyDeferred) { + console.error(" The shared teardown was refused before it changed anything, so it is still owed."); + console.error(" Its receipt is preserved; run 'ocx stop' again once Codex is closed to retry the restore."); + } + if (!restore.other && !restore.historyDeferred) { const discharged = teardownNonce ? [teardownNonce, ...recoveredNonces] : recoveredNonces; for (const nonce of discharged) { // A receipt that survives its discharge re-triggers recovery forever, so a failed @@ -1185,6 +1217,17 @@ async function handleStop() { // still wins: it is the stronger signal. if (stopFailed) process.exitCode = 1; else if (historyOnlyFailure) process.exitCode = STOP_HISTORY_INCOMPLETE_EXIT_CODE; + // The deferred code says "the only obligations left are the ones I just decided to + // keep". It is read across a process boundary by an updater that will replace package + // files on the strength of it, so this run has to be able to prove the claim: if any + // other obligation is sitting in the home — quarantined, or a concurrent stop's — the + // claim is false and the ordinary failure code is the honest answer. That is also the + // behaviour before #4718, so the fallback loses nothing that used to work. + else if (historyDeferredNonces) { + process.exitCode = pendingTeardownsAreExactly(historyDeferredNonces) + ? STOP_HISTORY_DEFERRED_EXIT_CODE + : 1; + } return !stopFailed; } diff --git a/src/codex/inject/restore.ts b/src/codex/inject/restore.ts index 15282ea7716..5e273173ece 100644 --- a/src/codex/inject/restore.ts +++ b/src/codex/inject/restore.ts @@ -89,6 +89,18 @@ export interface CodexNativeRestoreResult { success: boolean; message: string; externalProvider?: string; + /** + * Set when the restore refused at the Codex history preflight (#4718). + * + * The preflight runs before the config half, so a refusal leaves config, catalog, + * history and provenance exactly as they were. That is a different outcome from a + * restore that ran and failed, and callers that decide whether an obligation was + * discharged need to tell them apart. Reading the artifact states alone cannot: a + * refusal reports every artifact as `skipped`, which is also what an ownership refusal + * and a desired-state skip report. Matching the human-readable message instead would + * make a safety decision depend on prose. + */ + historyPreflightRefusal?: string; artifacts: { config: CodexRestoreConfigResult; catalog: CodexRestoreCatalogResult; @@ -216,6 +228,21 @@ function failedConfigRestoreEnvelope(config: CodexRestoreConfigResult): CodexNat return result; } +/** + * The history preflight refused, so nothing was attempted at all (#4718). + * + * The message is unchanged from what this path has always printed; the structured reason + * is added beside it so a caller can act on the refusal without reading the prose. + */ +function historyPreflightRefusalEnvelope(historyError: string): CodexNativeRestoreResult { + const result = skippedRestoreEnvelope( + false, + `Native restore refused: ${historyError}. Config, catalog, history and provenance were preserved.`, + ); + result.historyPreflightRefusal = historyError; + return result; +} + /** The config/profile half of a native restore, reported as one artifact. */ function restoreCodexConfigInline(kind = "sync"): CodexRestoreConfigResult { const preImages = captureCodexPreImages(); @@ -342,7 +369,7 @@ async function restoreNativeCodexAsyncImpl( } const historyError = preflightCodexHistoryInjection(false, false); - if (historyError) return skippedRestoreEnvelope(false, `Native restore refused: ${historyError}. Config, catalog, history and provenance were preserved.`); + if (historyError) return historyPreflightRefusalEnvelope(historyError); const eligibility = codexWriteCoordinationEligibility({ coordinatorPath: () => @@ -490,7 +517,7 @@ export function restoreNativeCodex(options: { skipHistory?: boolean; revalidateD return desiredEnabledRestoreSkip(); } const historyError = preflightCodexHistoryInjection(false, false); - if (historyError) return skippedRestoreEnvelope(false, `Native restore refused: ${historyError}. Config, catalog, history and provenance were preserved.`); + if (historyError) return historyPreflightRefusalEnvelope(historyError); // Captured before the config half: a successful journal restore DELETES the journal, and // restoring the config can drop `model_catalog_json`. Either one would hide the routed // catalog we actually wrote (#1798). diff --git a/src/config/pending-teardown.ts b/src/config/pending-teardown.ts index bfab1cf7575..9b1f5e97a2f 100644 --- a/src/config/pending-teardown.ts +++ b/src/config/pending-teardown.ts @@ -202,6 +202,37 @@ export function pendingTeardownOutstanding(): boolean { } } +/** + * Are the outstanding obligations EXACTLY the ones this stop chose to keep? + * + * `ocx stop` can preserve its own obligations deliberately — the Codex history preflight + * refuses before anything is restored, so the receipt has to survive for a later stop + * (#4718). That is safe for an update to continue past, because the stop knows those + * receipts describe a proxy it just proved down. + * + * Nothing else is. A quarantined receipt is waiting on a human, and a receipt belonging + * to a live owner means another stop is in flight; letting either ride along would turn + * "we deliberately kept ours" into "we ignored everyone's". So membership is the test, + * not a count of ours: an unrecognized obligation of any kind answers false and the + * caller falls back to the ordinary failure code. + * + * Quarantined names are included in the scan on purpose. They do not correspond to any + * nonce this run preserved, so their presence always answers false. + */ +export function pendingTeardownsAreExactly(nonces: readonly string[]): boolean { + const expected = new Set(nonces.map(nonce => `${PREFIX}${nonce}${SUFFIX}`)); + let names: string[]; + try { + names = readdirSync(getConfigDir()); + } catch (error) { + // A home that does not exist holds nothing, which matches only an empty expectation. + // Any other scan failure may be hiding an obligation and must not answer "exactly". + return (error as NodeJS.ErrnoException).code === "ENOENT" && expected.size === 0; + } + const found = names.filter(isAnyTeardownObligationFileName); + return found.length === expected.size && found.every(name => expected.has(name)); +} + /** Paths of quarantined obligations awaiting a human. */ export function listQuarantinedTeardowns(): string[] { try { diff --git a/src/update/index.ts b/src/update/index.ts index dccc63a288a..2197ef0f34b 100644 --- a/src/update/index.ts +++ b/src/update/index.ts @@ -481,6 +481,16 @@ export async function runUpdate(): Promise { " After the update: close the Codex app, run 'ocx doctor', then run 'ocx stop' once to retry.", ); } + if (decision.reason === "history-deferred") { + // Not the same warning: nothing was restored here. Saying "history metadata is + // incomplete" would imply config and catalog came back, and an operator who + // believed that would not know a teardown is still owed. + console.warn( + "⚠️ The shared teardown was refused by the Codex history preflight and restored nothing.\n" + + " Config, catalog, history and provenance were preserved, and the teardown receipt was kept.\n" + + " The proxy is down, so the update continues; close the Codex app and run 'ocx stop' once afterwards to finish the restore.", + ); + } } console.log(`Updating${latest ? ` to v${latest}` : ""}…\n$ ${bin} ${cmdArgs.join(" ")}`); diff --git a/src/update/stop-contract.d.mts b/src/update/stop-contract.d.mts index b077eb21b31..1efb77c42d7 100644 --- a/src/update/stop-contract.d.mts +++ b/src/update/stop-contract.d.mts @@ -1,2 +1,3 @@ /** Declaration for the plain-ESM stop contract shared with `bin/ocx.mjs`. */ export declare const STOP_HISTORY_INCOMPLETE_EXIT_CODE: 79; +export declare const STOP_HISTORY_DEFERRED_EXIT_CODE: 80; diff --git a/src/update/stop-contract.mjs b/src/update/stop-contract.mjs index c72548b7772..b86018ea0d7 100644 --- a/src/update/stop-contract.mjs +++ b/src/update/stop-contract.mjs @@ -13,3 +13,22 @@ * the child's code faithfully enough to propagate the confusion. */ export const STOP_HISTORY_INCOMPLETE_EXIT_CODE = 79; + +/** + * The exit code `ocx stop` uses to say "the proxy is down and the shared teardown was + * refused before it changed anything" (#4718). + * + * This is NOT 79. Seventy-nine means the teardown ran: config and catalog came back to + * their native values and only the Codex history metadata could not be finalized, so the + * receipt is discharged. Eighty means the Codex history preflight refused FIRST, so + * config, catalog, history and provenance are all untouched, the client is still routed + * at the proxy that just stopped, and the receipt stays outstanding for a later stop. + * + * Collapsing the two would be a data-loss bug in the quiet direction: a caller reading 79 + * discharges an obligation that was never performed. + * + * Eighty sits in the same unoccupied window as 79 — above `sysexits.h` (64-78), below + * `128 + signal`, and outside 0, 1, 2, 4, 64 and 130, which are the codes this CLI and its + * dispatcher already emit. + */ +export const STOP_HISTORY_DEFERRED_EXIT_CODE = 80; diff --git a/src/update/stop-decision.d.mts b/src/update/stop-decision.d.mts index f773e786e64..13f1cd93caf 100644 --- a/src/update/stop-decision.d.mts +++ b/src/update/stop-decision.d.mts @@ -6,5 +6,5 @@ export declare function decidePostStopUpdate(input: { teardownOutstanding?: boolean; }): { proceed: boolean; - reason: "stop-failed" | "runtime-state" | "teardown-outstanding" | "proxy-live" | "proxy-unknown" | "history-only" | "ok"; + reason: "stop-failed" | "runtime-state" | "teardown-outstanding" | "proxy-live" | "proxy-unknown" | "history-only" | "history-deferred" | "ok"; }; diff --git a/src/update/stop-decision.mjs b/src/update/stop-decision.mjs index e96c11ae17c..0cc1a71a593 100644 --- a/src/update/stop-decision.mjs +++ b/src/update/stop-decision.mjs @@ -1,4 +1,4 @@ -import { STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "./stop-contract.mjs"; +import { STOP_HISTORY_DEFERRED_EXIT_CODE, STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "./stop-contract.mjs"; /** * May an update replace package files after `ocx stop` returned? @@ -22,13 +22,22 @@ import { STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "./stop-contract.mjs"; * absence, and replacing files under a live server leaves it running a mix of old and * new modules. * - `ok` / `history-only` — proceed; the second also prints the manifest warning. + * - `history-deferred` — proceed; the stop is down but restored nothing, because the + * Codex history preflight refused first (#4718). The receipts it kept are the ONLY + * obligations it left, which the child proved before choosing this status, so + * `teardownOutstanding` seeing them is expected rather than disqualifying. Every other + * gate still applies: runtime records and a live or unreadable endpoint abort exactly + * as they do for a clean stop, because package replacement under a live server is the + * danger this function exists to prevent, and a history refusal says nothing about it. */ export function decidePostStopUpdate({ status, hasRuntimeState, liveness, teardownOutstanding = false }) { const historyOnly = status === STOP_HISTORY_INCOMPLETE_EXIT_CODE; - if (status !== 0 && !historyOnly) return { proceed: false, reason: "stop-failed" }; + const historyDeferred = status === STOP_HISTORY_DEFERRED_EXIT_CODE; + if (status !== 0 && !historyOnly && !historyDeferred) return { proceed: false, reason: "stop-failed" }; if (hasRuntimeState) return { proceed: false, reason: "runtime-state" }; - if (teardownOutstanding) return { proceed: false, reason: "teardown-outstanding" }; + if (teardownOutstanding && !historyDeferred) return { proceed: false, reason: "teardown-outstanding" }; if (liveness === "live") return { proceed: false, reason: "proxy-live" }; if (liveness !== "dead") return { proceed: false, reason: "proxy-unknown" }; + if (historyDeferred) return { proceed: true, reason: "history-deferred" }; return { proceed: true, reason: historyOnly ? "history-only" : "ok" }; } diff --git a/tests/codex-integration/codex-inject-integration.test.ts b/tests/codex-integration/codex-inject-integration.test.ts index 5ffbc54f996..7fbc1e4a7f7 100644 --- a/tests/codex-integration/codex-inject-integration.test.ts +++ b/tests/codex-integration/codex-inject-integration.test.ts @@ -123,6 +123,15 @@ describe("injectCodexConfig integration (Design B)", () => { expect(result.defaultEntries).toBe(1); expect(result.result.success).toBe(false); expect(result.result.message).toContain("history_paginated_requires_native_writer"); + // #4718: the refusal also has to be legible without reading the message. `ocx stop` + // decides whether an obligation was discharged from this envelope, and every artifact + // comes back "skipped" here — the same shape an ownership refusal and a desired-state + // skip produce. Without the structured reason the caller could only match prose, and + // the stop misread this as a generic teardown failure and aborted the update. + expect(result.result.historyPreflightRefusal).toBe("history_paginated_requires_native_writer"); + expect(result.result.artifacts.config.state).toBe("skipped"); + expect(result.result.artifacts.catalog.state).toBe("skipped"); + expect(result.result.artifacts.history.state).toBe("skipped"); expect(result.preserved).toBe(true); }); diff --git a/tests/providers/xai/grok-lifecycle.test.ts b/tests/providers/xai/grok-lifecycle.test.ts index 9602770978c..49d5929a102 100644 --- a/tests/providers/xai/grok-lifecycle.test.ts +++ b/tests/providers/xai/grok-lifecycle.test.ts @@ -315,7 +315,11 @@ describe("Grok fence lifecycle wiring", () => { const updateSource2 = readFileSync(repoPath("src", "update", "index.ts"), "utf8"); expect(updateSource2).toContain("teardownOutstanding: pendingTeardownOutstanding()"); const decisionSource = readFileSync(repoPath("src", "update", "stop-decision.mjs"), "utf8"); - expect(decisionSource).toContain('if (teardownOutstanding) return { proceed: false, reason: "teardown-outstanding" };'); + // The gate has exactly one exemption, and it is the child saying it kept those + // receipts on purpose after the Codex history preflight refused (#4718). Anything + // else — including a stop that merely exited 0 — still aborts the install. + expect(decisionSource).toContain('if (teardownOutstanding && !historyDeferred) return { proceed: false, reason: "teardown-outstanding" };'); + expect(decisionSource).toContain("const historyDeferred = status === STOP_HISTORY_DEFERRED_EXIT_CODE;"); const receiptSource = readFileSync(repoPath("src", "config", "pending-teardown.ts"), "utf8"); expect(receiptSource).toContain('from "./pending-teardown-names.mjs"'); expect(receiptSource).toContain("isPendingTeardownFileName(name)"); diff --git a/tests/service/stop-deferred-teardown.test.ts b/tests/service/stop-deferred-teardown.test.ts index e9d116cfcbf..680a11bf072 100644 --- a/tests/service/stop-deferred-teardown.test.ts +++ b/tests/service/stop-deferred-teardown.test.ts @@ -6,7 +6,7 @@ import { join } from "node:path"; import { stopProxyGracefully } from "../../src/lib/process-control"; import { performStopTeardown } from "../../src/server/stop-teardown"; import type { CodexNativeRestoreResult } from "../../src/codex/inject"; -import { STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../../src/update/stop-contract.mjs"; +import { STOP_HISTORY_DEFERRED_EXIT_CODE, STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../../src/update/stop-contract.mjs"; import { removeTreeWithRetry } from "../helpers/remove-tree"; import { fixturePath, repoPath } from "../helpers/repo-root"; @@ -115,6 +115,65 @@ describe("parent CLI shared teardown completion", () => { expect(outcome.receiptExists).toBe(false); }); + /** + * #4718: a refusal that happens BEFORE anything is restored. + * + * A paginated Codex history store makes the preflight refuse ahead of the config half, + * so every artifact comes back untouched rather than failed. `handleStop` had no branch + * for that shape and fell through to the generic failure, which exited 1 — and the + * updater reads 1 as "the proxy would not stop" and aborts with the service already + * down. The obligation really is still owed, so the receipt has to stay; what was wrong + * was calling it a stop failure. + */ + test("a history-preflight refusal keeps its receipt and reports the deferred code", async () => { + const restore = { + success: false, + message: "Native restore refused: history_paginated_requires_native_writer. Config, catalog, history and provenance were preserved.", + historyPreflightRefusal: "history_paginated_requires_native_writer", + artifacts: { config: { state: "skipped" }, catalog: { state: "skipped" }, history: { state: "skipped" } }, + } as unknown as CodexNativeRestoreResult; + const outcome = await runParentStop({ receipt: true, + response: { success: true, sharedTeardown: "deferred" }, restore }); + // Both halves were attempted; neither was discharged, because neither ran. + expect(outcome.calls).toMatchObject({ killed: 0, native: 1, grok: 1, cleared: 0 }); + expect(outcome.exitCode).toBe(STOP_HISTORY_DEFERRED_EXIT_CODE); + // The receipt is the whole point: the client config still points at a proxy that is + // gone, and only this file says so. Discharging it here loses that permanently. + expect(outcome.receiptExists).toBe(true); + }); + + test("an all-skipped restore without the structured refusal stays an ordinary failure", async () => { + // The artifact states alone cannot carry this decision: an ownership refusal and a + // desired-state skip produce the same three "skipped" values. Treating the shape as + // benign would let an update proceed past a teardown nobody classified. + const restore = { + success: false, + message: "Native restore skipped for an unrelated reason.", + artifacts: { config: { state: "skipped" }, catalog: { state: "skipped" }, history: { state: "skipped" } }, + } as unknown as CodexNativeRestoreResult; + const outcome = await runParentStop({ receipt: true, + response: { success: true, sharedTeardown: "deferred" }, restore }); + expect(outcome.exitCode).toBe(1); + expect(outcome.calls).toMatchObject({ native: 1, grok: 1, cleared: 0 }); + expect(outcome.receiptExists).toBe(true); + }); + + test("a refusal that also failed config is a real teardown failure, not a deferral", async () => { + // The structured reason is not a licence on its own. Config is state a client reads, + // so a run that damaged it must keep failing the stop however it got there. + const restore = { + success: false, + message: "Native restore refused: history_paginated_requires_native_writer.", + historyPreflightRefusal: "history_paginated_requires_native_writer", + artifacts: { config: { state: "failed" }, catalog: { state: "skipped" }, history: { state: "skipped" } }, + } as unknown as CodexNativeRestoreResult; + const outcome = await runParentStop({ receipt: true, + response: { success: true, sharedTeardown: "deferred" }, restore }); + expect(outcome.exitCode).toBe(1); + expect(outcome.calls).toMatchObject({ cleared: 0 }); + expect(outcome.receiptExists).toBe(true); + }); + test("a refused stop keeps the parent from restoring or discharging its receipt", async () => { const outcome = await runParentStop({ receipt: true, status: 409, response: { success: false, message: "Run the stop outside the installed service." }, restore: restoreResult(true) }); @@ -473,6 +532,43 @@ describe("pending teardown receipts", () => { expect(readdirSync(home).some(n => n.endsWith(".unreadable.json"))).toBe(true); }); + /** + * #4718: "the only obligations left are the ones I chose to keep". + * + * `ocx stop` makes that claim across a process boundary, and an updater replaces + * package files on the strength of it. Membership is the test rather than a count: + * anything the stop did not name — a quarantined receipt waiting on a human, a + * concurrent stop's claim — has to answer false, or a deliberate deferral turns into a + * blanket exemption for every obligation in the home. + */ + test("an exact-obligation check accepts only the receipts it was given", async () => { + const mod = await import("../../src/config/pending-teardown"); + // Nothing owed matches nothing expected. + expect(mod.pendingTeardownsAreExactly([])).toBe(true); + + const kept = mod.claimPendingTeardown(ENDPOINT, "exact", 1234); + expect(mod.pendingTeardownsAreExactly([kept.nonce])).toBe(true); + // The same receipt, unnamed, is an obligation nobody classified. + expect(mod.pendingTeardownsAreExactly([])).toBe(false); + // A nonce with no file behind it is not proof of anything either. + expect(mod.pendingTeardownsAreExactly([FOREIGN_NONCE])).toBe(false); + + // A second claim this stop never saw — another stop in flight — disqualifies it. + const other = mod.claimPendingTeardown(ENDPOINT, "exact", 1235); + expect(mod.pendingTeardownsAreExactly([kept.nonce])).toBe(false); + expect(mod.pendingTeardownsAreExactly([kept.nonce, other.nonce])).toBe(true); + expect(mod.clearPendingTeardown(other.nonce)).toBe(true); + + // A quarantined receipt is still outstanding and still counts here, which is the + // whole reason this cannot be built on listPendingTeardowns: that listing skips it. + const filed = mod.claimPendingTeardown(ENDPOINT, "exact", 1236); + writeFileSync(mod.pendingTeardownPathFor(filed.nonce), "{not json"); + expect(mod.quarantinePendingTeardown(filed.nonce)).toBeTruthy(); + expect(mod.listPendingTeardowns().map(read => read.state)).not.toContain("invalid"); + expect(mod.pendingTeardownOutstanding()).toBe(true); + expect(mod.pendingTeardownsAreExactly([kept.nonce])).toBe(false); + }); + test("a directory where a receipt belongs is invalid, not missing", async () => { const mod = await import("../../src/config/pending-teardown"); const claimed = mod.claimPendingTeardown(ENDPOINT, "exact", 1234); diff --git a/tests/update/update-stop-classification.test.ts b/tests/update/update-stop-classification.test.ts index 722bf584fe6..f14334dff5a 100644 --- a/tests/update/update-stop-classification.test.ts +++ b/tests/update/update-stop-classification.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "bun:test"; import { spawn } from "node:child_process"; import { readFileSync } from "node:fs"; import { join } from "node:path"; -import { STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../../src/update/stop-contract.mjs"; +import { STOP_HISTORY_DEFERRED_EXIT_CODE, STOP_HISTORY_INCOMPLETE_EXIT_CODE } from "../../src/update/stop-contract.mjs"; import { probeProxyLiveness } from "../../src/update/proxy-liveness-probe.mjs"; import { decidePostStopUpdate } from "../../src/update/stop-decision.mjs"; import { repoRoot as resolveRepoRoot } from "../helpers/repo-root"; @@ -38,6 +38,17 @@ describe("stop failure classification (#3008)", () => { .map(match => Number(match[1])); expect(cliCodes).not.toContain(STOP_HISTORY_INCOMPLETE_EXIT_CODE); expect(dispatchCodes).not.toContain(STOP_HISTORY_INCOMPLETE_EXIT_CODE); + + // #4718 adds a second code, and it has to be distinct from the first as well as from + // everything else. Reusing 79 would tell a caller "teardown ran, only history metadata + // is outstanding" about a stop that restored nothing, and that caller discharges the + // receipt on the strength of it. + expect(STOP_HISTORY_DEFERRED_EXIT_CODE).toBe(80); + expect(STOP_HISTORY_DEFERRED_EXIT_CODE).not.toBe(STOP_HISTORY_INCOMPLETE_EXIT_CODE); + expect(STOP_HISTORY_DEFERRED_EXIT_CODE).toBeGreaterThan(78); + expect(STOP_HISTORY_DEFERRED_EXIT_CODE).toBeLessThan(128); + expect(cliCodes).not.toContain(STOP_HISTORY_DEFERRED_EXIT_CODE); + expect(dispatchCodes).not.toContain(STOP_HISTORY_DEFERRED_EXIT_CODE); }); test("the shared contract is plain ESM so the Node launcher can import it", () => { @@ -45,6 +56,7 @@ describe("stop failure classification (#3008)", () => { // places is how the two ends drift. const contract = read("src/update/stop-contract.mjs"); expect(contract).toContain("export const STOP_HISTORY_INCOMPLETE_EXIT_CODE"); + expect(contract).toContain("export const STOP_HISTORY_DEFERRED_EXIT_CODE"); expect(read("bin/ocx.mjs")).toContain("stop-contract.mjs"); expect(read("src/update/index.ts")).toContain("stop-contract.mjs"); }); @@ -229,6 +241,63 @@ describe("stop failure classification (#3008)", () => { .toEqual({ proceed: false, reason: "proxy-unknown" }); }); + /** + * #4718: the same abort, from the opposite direction. + * + * A paginated Codex history store makes the shared teardown refuse before it changes + * anything, so `ocx stop` restores nothing and keeps its receipt. Under #3008 that came + * out as exit 1 and a surviving obligation, which reads identically to a proxy that + * refused to die — so the update aborted with the service already stopped and the old + * package still installed, exactly the shape #3008 was opened about. + * + * The receipt genuinely IS outstanding here, so the fix cannot be "ignore the receipt". + * It is the child saying which obligations it deliberately kept, and that claim only + * buys past the teardown gate — never past runtime records or a proxy that might live. + */ + test("a history-deferred stop proceeds past its own receipt and nothing else", () => { + const dead = { hasRuntimeState: false, liveness: "dead" } as const; + + // The reported case: receipt outstanding because the stop chose to keep it. + expect(decidePostStopUpdate({ status: STOP_HISTORY_DEFERRED_EXIT_CODE, teardownOutstanding: true, ...dead })) + .toEqual({ proceed: true, reason: "history-deferred" }); + // And with no receipt at all, which is the same decision for the same reason. + expect(decidePostStopUpdate({ status: STOP_HISTORY_DEFERRED_EXIT_CODE, ...dead })) + .toEqual({ proceed: true, reason: "history-deferred" }); + + // It is a distinct reason, not a second spelling of history-only: the two mean + // different things about whether the obligation was discharged. + expect(decidePostStopUpdate({ status: STOP_HISTORY_INCOMPLETE_EXIT_CODE, teardownOutstanding: true, ...dead })) + .toEqual({ proceed: false, reason: "teardown-outstanding" }); + + // Every other gate still stands. Replacing package files under a server that may be + // live is the danger this function exists to prevent, and a history refusal is + // evidence about history — it says nothing about whether the proxy is gone. + expect(decidePostStopUpdate({ status: STOP_HISTORY_DEFERRED_EXIT_CODE, hasRuntimeState: true, liveness: "dead" })) + .toEqual({ proceed: false, reason: "runtime-state" }); + expect(decidePostStopUpdate({ status: STOP_HISTORY_DEFERRED_EXIT_CODE, hasRuntimeState: false, liveness: "live" })) + .toEqual({ proceed: false, reason: "proxy-live" }); + expect(decidePostStopUpdate({ status: STOP_HISTORY_DEFERRED_EXIT_CODE, hasRuntimeState: false, liveness: "unknown" })) + .toEqual({ proceed: false, reason: "proxy-unknown" }); + + // And no neighbouring status inherits the exemption. + for (const status of [1, 2, 4, 64, 78, 81, 130, null]) { + expect(decidePostStopUpdate({ status, teardownOutstanding: true, ...dead })) + .toEqual({ proceed: false, reason: "stop-failed" }); + } + }); + + test("both updater lanes report the deferred teardown as its own outcome", () => { + // The reported #4718 path is the npm launcher. A lane that proceeded without saying + // the teardown is still owed would leave the operator believing the restore happened. + for (const lane of ["src/update/index.ts", "bin/ocx.mjs"]) { + const source = read(lane); + expect(source).toContain('decision.reason === "history-deferred"'); + // Not folded into the manifest warning: that one says history metadata is + // incomplete, which implies config and catalog already came back. + expect(source).toMatch(/restored nothing/); + } + }); + test("both updater lanes call the shared decision", () => { // The reported path is a dashboard npm update through the plain-Node launcher. Fixing // only the Bun updater would leave that lane broken while every focused test went @@ -246,8 +315,13 @@ describe("stop failure classification (#3008)", () => { // Ordinary failure wins: it is the stronger signal. expect(cli).toMatch(/if \(stopFailed\) process\.exitCode = 1;\s*\n\s*else if \(historyOnlyFailure\) process\.exitCode = STOP_HISTORY_INCOMPLETE_EXIT_CODE;/); // The code is set rather than exited inline so the dispatcher still receives the - // return value and decides what happens next. - expect(cli).toMatch(/process\.exitCode = STOP_HISTORY_INCOMPLETE_EXIT_CODE;\s*\n\s*return !stopFailed;/); + // return value and decides what happens next. The deferred code (#4718) sits between + // them and obeys the same rule, so the function still ends by returning. + expect(cli).toMatch(/process\.exitCode = STOP_HISTORY_INCOMPLETE_EXIT_CODE;[\s\S]*?\n\s*return !stopFailed;\n\}/); + // The deferred code never outranks an ordinary failure, and it is only reachable when + // this run can still prove the obligations left behind are the ones it chose to keep. + expect(cli).toMatch(/else if \(historyDeferredNonces\) \{/); + expect(cli).toMatch(/pendingTeardownsAreExactly\(historyDeferredNonces\)\s*\n?\s*\? STOP_HISTORY_DEFERRED_EXIT_CODE\s*\n?\s*: 1;/); // Config and catalog failures are real teardown failures: a client reads those. expect(cli).toMatch(/artifacts\.config\.state === "failed" \|\| artifacts\.catalog\.state === "failed"/); }); From 52ec0a5e76a751769095589d78f157bbd2dd7d1b Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:13:30 +0900 Subject: [PATCH 2/5] fix(service): decode schtasks output with the Windows text decoder (#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 became U+FFFD. The correctly resolved expected identity [SID, MACHINE\] 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 --- src/service/windows-scheduler.ts | 49 ++++++------ ...ows-scheduler-install-verification.test.ts | 74 +++++++++++++++++++ 2 files changed, 102 insertions(+), 21 deletions(-) diff --git a/src/service/windows-scheduler.ts b/src/service/windows-scheduler.ts index 0f1719aa636..5e460d1857b 100644 --- a/src/service/windows-scheduler.ts +++ b/src/service/windows-scheduler.ts @@ -5,6 +5,7 @@ import { join, resolve } from "node:path"; import { randomUUID } from "node:crypto"; import { ELEVATION_REQUEST_TIMEOUT_MS, OCX_ELEVATED_PROTOCOL_FAILED, raceWithTimeout, resolveTrustedWindowsSchtasksExe, startElevatedSchtasksCreateAndRun, runWindowsElevated, toWindowsSchtasksError, WindowsElevationError, type ElevatedSchedulerOutcome, type ElevatedSchtasksCreateAndRunExecution, type ElevatedSchtasksCreateAndRunResult } from "../lib/windows-elevation"; import { statusWinswRaw } from "../lib/winsw"; +import { decodeWindowsTextBytes, type WindowsTextDecodeOptions } from "../lib/windows-text"; import { isTestHomeGuardArmed } from "../lib/test-home-guard"; import { TASK, windowsServiceScriptPath, windowsLauncherVbsPath, windowsTaskXmlPath, writeServiceInstallState } from "./state"; import { buildWindowsSchtasksCreateArgs, windowsTaskRegistrationOwnedByAttempt, windowsTaskRegistrationHealthy } from "./windows-taskxml"; @@ -16,28 +17,34 @@ import { WINSW_SERVICE_ID } from "../lib/winsw"; * Decode schtasks stdout. `/query /xml` emits UTF-16LE (often with BOM) because the * registered task document is UTF-16; reading that as UTF-8 makes every health check * fail ("registration present but unhealthy") and rolls back a successful elevated create. + * + * Redirected output is NOT always UTF-16. Its encoding follows the console output code + * page of the spawning process tree rather than the XML declaration, so on a zh-CN host + * (ACP/OEMCP 936) the bytes are GBK — including inside a no-console background service. +* Decoding those as UTF-8 turned a CJK account name in + * `` into U+FFFD, the trigger scope then failed to + * match the correctly resolved `[SID, MACHINE\]`, and `ocx service repair` +* aborted at its recognition gate on a registration OpenCodex had itself created. The + * same mojibake rolled back fresh installs at post-create verification (#4691). + * + * The fix is entirely in byte decoding, before any XML is parsed. The trigger scope stays + * an exact identity comparison: 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. + * + * `decodeWindowsTextBytes` is the decoder this project already built for this class + * (UTF-16 -> strict UTF-8 -> the locale's legacy code page), and 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. */ -export function decodeSchtasksOutput(buffer: Buffer): string { - if (buffer.length === 0) return ""; - const bomUtf16Le = buffer.length >= 2 && buffer[0] === 0xff && buffer[1] === 0xfe; - const bomUtf16Be = buffer.length >= 2 && buffer[0] === 0xfe && buffer[1] === 0xff; - const looksUtf16Le = buffer.length >= 4 - && buffer[1] === 0x00 - && buffer[3] === 0x00 - && buffer[0] !== 0x00; - if (bomUtf16Le || looksUtf16Le) { - return buffer.toString("utf16le").replace(/^\uFEFF/, "").trim(); - } - if (bomUtf16Be) { - // Swap pairs then decode as utf16le. - const swapped = Buffer.alloc(buffer.length - 2); - for (let i = 2; i + 1 < buffer.length; i += 2) { - swapped[i - 2] = buffer[i + 1]!; - swapped[i - 1] = buffer[i]!; - } - return swapped.toString("utf16le").trim(); - } - return buffer.toString("utf8").replace(/^\uFEFF/, "").trim(); +export function decodeSchtasksOutput( + buffer: Buffer, + options: WindowsTextDecodeOptions = {}, +): string { + // `options` exists so a test can pin the code page; every production call passes the + // buffer alone and uses the active Intl locale, which is available to a service with no + // console because the selection reads the process locale rather than a console handle. + return decodeWindowsTextBytes(buffer, options); } function runFile(file: string, args: string[]): string { diff --git a/tests/windows/windows-scheduler-install-verification.test.ts b/tests/windows/windows-scheduler-install-verification.test.ts index cc271962116..8b8e5440dee 100644 --- a/tests/windows/windows-scheduler-install-verification.test.ts +++ b/tests/windows/windows-scheduler-install-verification.test.ts @@ -49,6 +49,80 @@ describe("decodeSchtasksOutput", () => { const text = "Folder: \\\nTaskName: opencodex-proxy"; expect(decodeSchtasksOutput(Buffer.from(text, "utf8"))).toBe(text); }); + + /** + * #4691: redirected "schtasks /query /xml" follows the console output code page of the + * spawning process tree, not the XML declaration. On a zh-CN host (ACP/OEMCP 936) those + * bytes are GBK, and the old UTF-8 fallback turned a CJK account name into U+FFFD. The + * trigger scope then stopped matching the correctly resolved [SID, MACHINE\], so + * "ocx service repair" refused a registration OpenCodex had created itself, and fresh + * installs rolled back at post-create verification. + */ + test("decodes GBK schtasks XML so a CJK account name still matches its trigger scope", () => { + const wscript = "C:\\WINDOWS\\System32\\wscript.exe"; + const launcher = "C:\\Users\\x\\.opencodex\\opencodex-service-launcher.vbs"; + // Task Scheduler canonicalizes a SID-scoped trigger back to the account name on + // export, which is why the identity reaching the decoder is non-ASCII at all. + const account = "MACHINE\\张三"; + const xml = buildWindowsTaskXml( + "C:\\Users\\x\\.opencodex\\opencodex-service.cmd", + launcher, + undefined, + account, + ).replace(/.*?<\/Command>/, "" + wscript + ""); + + // Literal CP936 bytes, for the same reason tests/windows/windows-text-decoding.test.ts + // uses literal hex: encoding the fixture with the decoder under test would assert + // nothing. 0xD5C5 0xC8FD is the account name on code page 936, and it is not valid + // UTF-8 — which is why the old fallback was lossy rather than merely wrong. + const cp936 = new Map([["张", [0xd5, 0xc5]], ["三", [0xc8, 0xfd]]]); + const bytes = Buffer.concat([...xml].map(ch => { + const legacy = cp936.get(ch); + if (legacy) return Buffer.from(legacy); + if (ch.codePointAt(0)! > 0x7f) throw new Error("fixture has no CP936 bytes for " + ch); + return Buffer.from(ch, "ascii"); + })); + + const decoded = decodeSchtasksOutput(bytes, { locale: "zh-CN" }); + expect(decoded).toContain("" + account + ""); + expect(decoded).not.toContain("\uFFFD"); + expect(windowsTaskRegistrationHealthy(decoded, wscript, launcher, [TEST_WINDOWS_TASK_SID, account])).toBe(true); + + // The regression itself: the historical decode mangles the name, and the scope check + // then fails — the "not a recognized legacy OpenCodex definition" refusal. + const mojibake = bytes.toString("utf8"); + expect(mojibake).toContain("\uFFFD"); + expect(windowsTaskRegistrationHealthy(mojibake, wscript, launcher, [TEST_WINDOWS_TASK_SID, account])).toBe(false); + + // Decoding correctly does not relax ownership. A different account is still rejected, + // and the mojibake spelling is not accepted as an identity of its own — forgiving it + // would let two different non-ASCII accounts collapse to the same value. + expect(windowsTaskRegistrationHealthy(decoded, wscript, launcher, [TEST_WINDOWS_TASK_SID, "MACHINE\\someone-else"])).toBe(false); + expect(windowsTaskRegistrationHealthy(decoded, wscript, launcher, ["MACHINE\\\uFFFD\uFFFD"])).toBe(false); + }); + + test("a UTF-8 task document is not mistaken for the legacy code page", () => { + // The strict UTF-8 attempt runs before any code-page guess, so a CP 65001 console on + // the same zh-CN host still decodes correctly. #4106 was closed as not-planned because + // that reporter's console was 65001; this pins that the fix leaves that case alone. + const utf8Xml = "MACHINE\\张三"; + expect(decodeSchtasksOutput(Buffer.from(utf8Xml, "utf8"), { locale: "zh-CN" })).toBe(utf8Xml); + }); + + test("delegating the decode leaves the UTF-16 paths intact", () => { + const text = "Folder: \\\nTaskName: opencodex-proxy"; + // UTF-16LE with and without a BOM, and UTF-16BE, all still round-trip: that is what + // "schtasks /query /xml" emits on an ordinary host and the reason this decoder exists. + expect(decodeSchtasksOutput(Buffer.from("\uFEFF" + text, "utf16le"))).toBe(text); + expect(decodeSchtasksOutput(Buffer.from(text, "utf16le"))).toBe(text); + const be = Buffer.from("\uFEFF" + text, "utf16le"); + for (let i = 0; i + 1 < be.length; i += 2) { + const low = be[i]!; + be[i] = be[i + 1]!; + be[i + 1] = low; + } + expect(decodeSchtasksOutput(be)).toBe(text); + }); }); describe("windowsSchedulerCsvIncludesTask", () => { From ac87703657a18087ad7080b47a4528550d52d34f Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:21:56 +0900 Subject: [PATCH 3/5] fix(service): stage elevated Task Scheduler XML instead of inlining it (#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 --- src/lib/windows-elevation.ts | 71 +++++-- src/service.ts | 2 +- src/service/windows-ops.ts | 199 ++++++++++++++++-- tests/service/service.test.ts | 114 ++++++++++ tests/windows/windows-elevation-spawn.test.ts | 62 +++++- 5 files changed, 410 insertions(+), 38 deletions(-) diff --git a/src/lib/windows-elevation.ts b/src/lib/windows-elevation.ts index b2d02b81234..aa728ab1599 100644 --- a/src/lib/windows-elevation.ts +++ b/src/lib/windows-elevation.ts @@ -645,36 +645,79 @@ export function runWindowsElevated(file: string, args: string[]): Promise { - if (replace && !expectedExistingXml?.trim()) { + if (replace && !expectedExisting) { throw new Error("Elevated Task Scheduler replacement requires a captured existing definition."); } - const xmlBase64 = Buffer.from(xml, "utf16le").toString("base64"); - const expectedExistingBase64 = expectedExistingXml === undefined - ? null - : Buffer.from(expectedExistingXml, "utf16le").toString("base64"); const powerShellPath = windowsPowerShell(); const powerShellDirectory = powerShellPath.replace(/[\\/][^\\/]+$/, ""); const scheduledTasksModule = `${powerShellDirectory}\\Modules\\ScheduledTasks\\ScheduledTasks.psd1`; const inner = [ `$taskName = ${psSingleQuote(taskName)}`, - `$xmlBase64 = ${psSingleQuote(xmlBase64)}`, - "$xml = [Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($xmlBase64))", + READ_STAGED_TASK_XML, + `$xml = Read-OcxStagedTaskXml ${psSingleQuote(xml.path)} ${psSingleQuote(xml.sha256)}`, `$module = Microsoft.PowerShell.Core\\Import-Module -Name ${psSingleQuote(scheduledTasksModule)} -PassThru -Force -ErrorAction Stop`, "$registerTask = $module.ExportedCommands['Register-ScheduledTask']", "if ($null -eq $registerTask) { throw 'Trusted ScheduledTasks module does not export Register-ScheduledTask.' }", ...(replace ? [ - `$expectedBase64 = ${psSingleQuote(expectedExistingBase64!)}`, - "$expectedXml = [Text.Encoding]::Unicode.GetString([Convert]::FromBase64String($expectedBase64))", + `$expectedXml = Read-OcxStagedTaskXml ${psSingleQuote(expectedExisting!.path)} ${psSingleQuote(expectedExisting!.sha256)}`, `$schtasks = ${psSingleQuote(resolveTrustedWindowsSchtasksExe())}`, "$currentXml = & $schtasks /query /tn $taskName /xml 2>$null | Out-String", "if ($LASTEXITCODE -ne 0) { throw 'Task Scheduler replacement precondition could not be read.' }", diff --git a/src/service.ts b/src/service.ts index 92cedd7e226..f6a571b5748 100644 --- a/src/service.ts +++ b/src/service.ts @@ -19,7 +19,7 @@ export { decodeSchtasksOutput, setQuerySchtasksForTests, formatWindowsSchedulerS export type { WindowsSchedulerXmlState } from "./service/windows-taskxml"; export { buildWindowsServiceScript, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsLauncherVbs, buildWindowsTaskXml, buildWindowsTaskXmlDocument, windowsTaskRegistrationOwnedByAttempt, windowsTaskRegistrationHealthy, readWindowsSchedulerXmlState } from "./service/windows-taskxml"; export type { WindowsSchedulerRegistrationStageDeps, FreshWindowsSchedulerRegistrationDeps, RemoveNativeWindowsServiceDeps } from "./service/windows-ops"; -export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; +export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, stageElevatedSchedulerRegistration, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; export type { ServiceRepairVerb, RepairServiceDeps } from "./service/repair"; export { repairService } from "./service/repair"; export type { ServiceInstallPreparationDeps, FreshWindowsSchedulerInstallDeps, ServiceStopOutcome, ServiceUninstallOutcome } from "./service/orchestration"; diff --git a/src/service/windows-ops.ts b/src/service/windows-ops.ts index 64e3d48373e..75321fb9fcf 100644 --- a/src/service/windows-ops.ts +++ b/src/service/windows-ops.ts @@ -1,4 +1,5 @@ -import { chmodSync, readFileSync, writeFileSync } from "node:fs"; +import { chmodSync, lstatSync, readFileSync, writeFileSync } from "node:fs"; +import { createHash } from "node:crypto"; import { win32 } from "node:path"; import { winswXmlPath } from "../lib/winsw"; import { hardenSecretPath } from "../lib/windows-secret-acl"; @@ -9,7 +10,7 @@ import { existsSync, mkdirSync, mkdtempSync, rmdirSync, unlinkSync } from "node: import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { getConfigDir } from "../config"; -import { runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError } from "../lib/windows-elevation"; +import { runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError, type StagedWindowsTaskXml } from "../lib/windows-elevation"; import { defaultWinswEntry, installWinswService, statusWinswRaw, uninstallWinswService, WINSW_SERVICE_ID, type WinswStatus } from "../lib/winsw"; import { forgetEphemeralSecretDir, forgetEphemeralSecretPath, hardenSecretDir } from "../lib/windows-secret-acl"; import { recordOwnedConfigPath } from "../lib/config-ownership"; @@ -165,6 +166,170 @@ function cleanupWindowsSchedulerStage( if (cleanupError) throw cleanupError; } +/** A staged payload set for one elevated registration, plus the way to remove it. */ +export interface StagedElevatedSchedulerRegistration { + readonly xml: StagedWindowsTaskXml; + readonly expectedExisting?: StagedWindowsTaskXml; + /** Remove every staged artifact. Idempotent, so a second call after success is a no-op. */ + cleanup(): void; +} + +export interface ElevatedSchedulerStagingDeps { + createStageDir?: () => string; + hardenDir?: (path: string) => void; + writePayload?: (path: string, bytes: Buffer) => void; + hardenPath?: (path: string) => void; + inspect?: (path: string) => { isSymbolicLink(): boolean; isFile(): boolean; isDirectory(): boolean }; + removeStageDir?: (path: string) => void; +} + +/** + * Stage the captured definitions an elevated registration needs, as files rather than + * as command-line payloads (#4692). + * + * A file that an administrator process will read is itself a privilege-escalation + * surface, so three properties have to hold together and none of them is sufficient + * alone: + * + * - **Access.** The directory is created fresh by `mkdtemp`, then ACL-hardened before + * anything is written into it, so another local account cannot read or replace the + * payload while the UAC prompt is open. Hardening the directory first is what makes + * the file private from the moment it exists. + * - **No reparse point.** Each artifact is inspected with `lstat` and rejected unless it + * is what it claims to be. `wx` already refuses to create over an existing name, which + * is the atomic step here — there is no replace path to race, because every path is + * inside a directory that did not exist a moment ago. The explicit check is what keeps + * that guarantee from depending on a reading of `O_EXCL` semantics. + * - **Tamper evidence.** The digest is taken over the exact bytes written, and the + * elevated script recomputes it over the bytes it reads. 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 the only thing that makes such a swap fail closed rather than + * silently register a different task definition. + * + * Payloads are UTF-16LE with no BOM, and the elevated process decodes them straight into + * `Register-ScheduledTask`. What is hashed is therefore exactly what is registered, with + * no trimming step in between that the two sides could disagree about. + */ +export function stageElevatedSchedulerRegistration( + xml: string, + expectedExistingXml?: string, + deps: ElevatedSchedulerStagingDeps = {}, +): StagedElevatedSchedulerRegistration { + const createStageDir = deps.createStageDir + ?? (() => mkdtempSync(join(tmpdir(), WINDOWS_SCHEDULER_STAGE_PREFIX))); + 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 }); }); + const inspect = deps.inspect ?? ((path: string) => lstatSync(path)); + const removeStageDir = deps.removeStageDir ?? ((path: string) => { rmdirSync(path); }); + + const stageDir = createStageDir(); + const files: string[] = []; + const cleanup = (): void => { + let failure: unknown; + for (const file of files.splice(0)) { + try { + unlinkSync(file); + forgetEphemeralSecretPath(file); + } catch (error) { + if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") forgetEphemeralSecretPath(file); + else failure ??= error; + } + } + try { + removeStageDir(stageDir); + forgetEphemeralSecretDir(stageDir); + } catch (error) { + if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") forgetEphemeralSecretDir(stageDir); + else if (failure) throw new AggregateError([failure, error], "Elevated Task Scheduler staging cleanup failed."); + else failure = error; + } + if (failure) throw failure; + }; + + try { + try { chmodSync(stageDir, 0o700); } catch { /* required Windows ACL is authoritative */ } + const dirStats = inspect(stageDir); + if (dirStats.isSymbolicLink() || !dirStats.isDirectory()) { + throw new Error(`Refusing to stage an elevated Task Scheduler payload under a redirected path: ${stageDir}`); + } + hardenDir(stageDir); + const stage = (name: string, value: string): StagedWindowsTaskXml => { + const path = join(stageDir, name); + const bytes = Buffer.from(value, "utf16le"); + writePayload(path, bytes); + files.push(path); + const stats = inspect(path); + if (stats.isSymbolicLink() || !stats.isFile()) { + throw new Error(`Refusing to stage an elevated Task Scheduler payload through a redirected path: ${path}`); + } + hardenPath(path); + return { path, sha256: createHash("sha256").update(bytes).digest("hex") }; + }; + return { + xml: stage("register.xml", xml), + ...(expectedExistingXml === undefined + ? {} + : { expectedExisting: stage("expected.xml", expectedExistingXml) }), + cleanup, + }; + } catch (error) { + try { + cleanup(); + } catch (cleanupError) { + throw new AggregateError( + [error, cleanupError], + "Elevated Task Scheduler staging failed and could not be cleaned up.", + ); + } + throw error; + } +} + +/** + * Stage, elevate, and clean up — on every exit, including UAC cancellation and a + * synchronous spawn failure. + * + * A cleanup failure never replaces the registration failure it followed: an operator + * told only that a temp directory could not be removed would have no idea the task was + * never registered. + */ +async function runStagedElevatedSchedulerRegistration( + taskName: string, + xml: string, + replace: boolean, + expectedExistingXml: string | undefined, + failureLabel: string, +): Promise { + const staged = stageElevatedSchedulerRegistration(xml, expectedExistingXml); + let failure: unknown; + try { + const exitCode = await runWindowsElevatedScheduledTaskRegistration( + taskName, + staged.xml, + replace, + staged.expectedExisting, + ); + if (exitCode !== 0) failure = new Error(`${failureLabel} with exit code ${exitCode}.`); + } catch (error) { + failure = error; + } + try { + staged.cleanup(); + } catch (cleanupError) { + if (failure) { + throw new AggregateError( + [failure, cleanupError], + "Elevated Task Scheduler registration failed and its staging could not be cleaned up.", + ); + } + throw cleanupError; + } + if (failure) throw failure; +} + export function stageWindowsSchedulerRegistrationXml( attemptNonce: string, deps: WindowsSchedulerRegistrationStageDeps = {}, @@ -294,7 +459,8 @@ export async function registerFreshWindowsSchedulerTask( throw error; } // Register from the captured XML string inside the elevated process. Another - // same-user process can mutate its own temp files, but cannot change this command. + // same-user process can mutate its own temp files, so the captured bytes are staged + // privately and the elevated script verifies their digest before registering them. // UAC can remain open for an arbitrary amount of time. Recheck the captured predecessor // before launch; the elevated helper repeats the same check after consent and before Force. assertReplacementPrecondition(); @@ -303,15 +469,13 @@ export async function registerFreshWindowsSchedulerTask( xml: string, replaceCurrent: boolean, previousXml?: string, - ) => { - const exitCode = await runWindowsElevatedScheduledTaskRegistration( - taskName, - xml, - replaceCurrent, - previousXml, - ); - if (exitCode !== 0) throw new Error(`Background service install failed with exit code ${exitCode}.`); - }); + ) => runStagedElevatedSchedulerRegistration( + taskName, + xml, + replaceCurrent, + previousXml, + "Background service install failed", + )); await elevate(TASK, expectedXml, replace, expectedExistingXml); } @@ -501,10 +665,13 @@ export async function restoreWindowsSchedulerTaskIfAbsent(registeredXml: string) ) { throw error; } - const exitCode = await runWindowsElevatedScheduledTaskRegistration(TASK, registeredXml, false); - if (exitCode !== 0) { - throw new Error(`Task Scheduler rollback failed with exit code ${exitCode}.`); - } + await runStagedElevatedSchedulerRegistration( + TASK, + registeredXml, + false, + undefined, + "Task Scheduler rollback failed", + ); } const recoveredXml = statusWindowsXml(); if (!windowsSchedulerRegistrationMatchesSnapshot(recoveredXml, registeredXml)) { diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index 5daaa02ec9c..3b17cd6e93e 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -1,6 +1,7 @@ import { afterAll, afterEach, describe, expect, spyOn, test } from "bun:test"; import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; +import { createHash } from "node:crypto"; import { tmpdir } from "node:os"; import { delimiter, isAbsolute, join, posix, win32 } from "node:path"; import { pathToFileURL } from "node:url"; @@ -2078,6 +2079,119 @@ describe("service lifecycle cleanup ordering", () => { } }); + /** + * #4692: a file an administrator process will read is itself a privilege-escalation + * surface, so access, redirection and tamper-evidence each have to hold. + */ + test("elevated staging hardens before writing, digests the exact bytes, and cleans up", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-")); + const stageDir = join(parent, "private-stage"); + const calls: string[] = []; + try { + const staged = serviceModule.stageElevatedSchedulerRegistration( + "new", + "previous", + { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + calls.push("create-stage-dir"); + return stageDir; + }, + hardenDir: () => { calls.push("harden-dir"); }, + writePayload: (path, bytes) => { + calls.push("write:" + path.slice(stageDir.length + 1)); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: path => { calls.push("harden:" + path.slice(stageDir.length + 1)); }, + }, + ); + + // The directory is private before anything is written into it; hardening after the + // write would leave a window where the payload is readable by another account. + expect(calls).toEqual([ + "create-stage-dir", + "harden-dir", + "write:register.xml", + "harden:register.xml", + "write:expected.xml", + "harden:expected.xml", + ]); + + // The digest covers exactly the bytes on disk, and those bytes are UTF-16LE with no + // BOM: the elevated process decodes them straight into Register-ScheduledTask, so + // what is hashed here is what gets registered, with no trimming step in between. + for (const [payload, value] of [ + [staged.xml, "new"], + [staged.expectedExisting!, "previous"], + ] as const) { + const onDisk = readFileSync(payload.path); + expect(onDisk.equals(Buffer.from(value, "utf16le"))).toBe(true); + expect(onDisk[0]).not.toBe(0xff); + expect(payload.sha256).toBe(createHash("sha256").update(onDisk).digest("hex")); + expect(payload.sha256).toMatch(/^[0-9a-f]{64}$/); + } + expect(staged.xml.sha256).not.toBe(staged.expectedExisting!.sha256); + + staged.cleanup(); + expect(existsSync(stageDir)).toBe(false); + // Idempotent: the success path calls it once, but a failure path may race it. + expect(() => staged.cleanup()).not.toThrow(); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("elevated staging refuses a redirected path and leaves nothing behind", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-reparse-")); + const stageDir = join(parent, "private-stage"); + try { + // A staged payload reached through a reparse point is a payload somebody else chose + // the destination for. Exclusive creation already refuses an existing name, so this + // is the check that keeps the guarantee from resting on a reading of O_EXCL. + expect(() => serviceModule.stageElevatedSchedulerRegistration("", undefined, { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { writeFileSync(path, bytes, { flag: "wx" }); }, + hardenPath: () => { throw new Error("must not harden a redirected payload"); }, + inspect: path => ({ + isSymbolicLink: () => path !== stageDir, + isFile: () => true, + isDirectory: () => path === stageDir, + }), + })).toThrow("redirected path"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("elevated staging cleans up when a payload write fails partway", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-partial-")); + const stageDir = join(parent, "private-stage"); + try { + // The predecessor is the second payload, so this leaves a real file behind unless + // cleanup walks everything it created rather than only the one that failed. + expect(() => serviceModule.stageElevatedSchedulerRegistration("", "", { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { + if (path.endsWith("expected.xml")) throw new Error("synthetic predecessor write failure"); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: () => {}, + })).toThrow("synthetic predecessor write failure"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { const calls: string[] = []; mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/windows/windows-elevation-spawn.test.ts b/tests/windows/windows-elevation-spawn.test.ts index 3eaaec12584..fa0058f51db 100644 --- a/tests/windows/windows-elevation-spawn.test.ts +++ b/tests/windows/windows-elevation-spawn.test.ts @@ -153,7 +153,7 @@ describe("runWindowsElevated spawn contract", () => { await expect(runWindowsElevatedScheduledTaskRegistration( "opencodex-proxy", - "", + { path: "C:\\Temp\\opencodex-service-stage-aaaaaa\\register.xml", sha256: "0".repeat(64) }, )).resolves.toBe(0); const startProcessIndex = commandScript.indexOf("Start-Process"); @@ -174,7 +174,7 @@ describe("runWindowsElevated spawn contract", () => { expect(commandScript).not.toMatch(/-ArgumentList\s+'[^']*';\s+-Verb RunAs/); }); - test("scheduled-task registration embeds immutable XML bytes instead of a file path", async () => { + test("scheduled-task registration passes staged paths and digests, never inline payloads", async () => { let commandScript = ""; setWindowsElevationSpawnForTests((( _cmd: string, @@ -196,7 +196,9 @@ describe("runWindowsElevated spawn contract", () => { }) as never); const xml = "fixed-definition"; - await expect(runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", xml)).resolves.toBe(0); + const stageDir = "C:\\Temp\\opencodex-service-stage-aaaaaa"; + const staged = { path: stageDir + "\\register.xml", sha256: "a".repeat(64) }; + await expect(runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", staged)).resolves.toBe(0); const match = /-EncodedCommand ([A-Za-z0-9+/=]+)/.exec(commandScript); expect(match).not.toBeNull(); const elevatedScript = Buffer.from(match![1]!, "base64").toString("utf16le"); @@ -211,21 +213,67 @@ describe("runWindowsElevated spawn contract", () => { expect(elevatedScript).toContain("& $registerTask -TaskName $taskName -Xml $xml -ErrorAction Stop"); expect(elevatedScript).not.toContain("-Xml $xml -Force"); expect(elevatedScript.match(/\bRegister-ScheduledTask\b/g)).toHaveLength(2); - expect(elevatedScript).toContain(Buffer.from(xml, "utf16le").toString("base64")); + + // #4692: the definition now travels as a path plus a digest. A pathname on its own + // would be a promise about content, so the elevated side has to check it: read the + // bytes once, hash exactly those bytes, and refuse BEFORE decoding them. Hashing and + // then rereading would leave the swap window this check exists to close. + expect(elevatedScript).toContain(staged.path); + expect(elevatedScript).toContain(staged.sha256); + expect(elevatedScript).toContain("[IO.File]::ReadAllBytes($path)"); + expect(elevatedScript).toContain("$sha.ComputeHash($bytes)"); + expect(elevatedScript).toContain("Task Scheduler staged payload failed its integrity check."); + expect(elevatedScript.indexOf("-cne $expectedHash")) + .toBeLessThan(elevatedScript.indexOf("[Text.Encoding]::Unicode.GetString($bytes)")); + // No payload rides the command line any more, in either encoding layer. + expect(elevatedScript).not.toContain(Buffer.from(xml, "utf16le").toString("base64")); + expect(elevatedScript).not.toContain("FromBase64String"); expect(commandScript).not.toContain("/xml"); - expect(commandScript).not.toContain("task.xml"); + + // The regression itself. The old form embedded base64(utf16le) of the XML inside a + // script that was base64(utf16le)-encoded again — about 14.2 command-line characters + // per XML character, twice over for a replacement — so a ~2 KB definition pushed the + // spawn past the Windows command-line limit and failed with ENAMETOOLONG. What is + // pinned here is independence, not one lucky measurement: the same staging shape must + // produce the same command length no matter how large the definition behind it is. + const smallLength = commandScript.length; + const largeStaged = { path: stageDir + "\\register.xml", sha256: "b".repeat(64) }; + await expect(runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", largeStaged)).resolves.toBe(0); + expect(commandScript.length).toBe(smallLength); + expect(commandScript.length).toBeLessThan(8192); const predecessor = "captured-predecessor"; + const stagedPredecessor = { path: stageDir + "\\expected.xml", sha256: "c".repeat(64) }; await expect( - runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", xml, true, predecessor), + runWindowsElevatedScheduledTaskRegistration("opencodex-proxy", staged, true, stagedPredecessor), ).resolves.toBe(0); const replaceMatch = /-EncodedCommand ([A-Za-z0-9+/=]+)/.exec(commandScript); expect(replaceMatch).not.toBeNull(); const replaceScript = Buffer.from(replaceMatch![1]!, "base64").toString("utf16le"); expect(replaceScript).toContain("& $registerTask -TaskName $taskName -Xml $xml -Force"); - expect(replaceScript).toContain(Buffer.from(predecessor, "utf16le").toString("base64")); + expect(replaceScript).toContain(stagedPredecessor.path); + expect(replaceScript).toContain(stagedPredecessor.sha256); + expect(replaceScript).not.toContain(Buffer.from(predecessor, "utf16le").toString("base64")); + // The predecessor is verified the same way before it is used as a precondition: two + // call sites, both digest-checked. The helper is declared as + // "Read-OcxStagedTaskXml([string]$path", so the trailing space matches calls only. + expect(replaceScript.match(/Read-OcxStagedTaskXml /g)).toHaveLength(2); + expect(elevatedScript.match(/Read-OcxStagedTaskXml /g)).toHaveLength(1); expect(replaceScript).toContain("$currentXml = & $schtasks /query /tn $taskName /xml"); expect(replaceScript).toContain("Task Scheduler replacement precondition changed."); + // A replacement used to carry TWO payloads, which is what made this the reported + // failure. It stays bounded now. + expect(commandScript.length).toBeLessThan(8192); + }); + + test("an elevated replacement still refuses without a captured predecessor", () => { + // The post-UAC compare-before-Force is the only thing standing between a repair and + // overwriting a registration somebody else changed while the prompt was open. + expect(() => runWindowsElevatedScheduledTaskRegistration( + "opencodex-proxy", + { path: "C:\\Temp\\opencodex-service-stage-aaaaaa\\register.xml", sha256: "a".repeat(64) }, + true, + )).toThrow("requires a captured existing definition"); }); test("maps exit 1223 to cancelled", async () => { From f88191d5316ef252f6e1df7e1d9a2089e9aa7b65 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 11:37:16 +0900 Subject: [PATCH 4/5] fix(service): report an unreadable staged payload with its cause (#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. --- src/lib/windows-elevation.ts | 21 ++++++++++++- src/service.ts | 2 +- src/service/windows-ops.ts | 31 +++++++++++++++++-- tests/service/service.test.ts | 25 +++++++++++++++ tests/windows/windows-elevation-spawn.test.ts | 10 ++++++ 5 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/lib/windows-elevation.ts b/src/lib/windows-elevation.ts index aa728ab1599..171545276cf 100644 --- a/src/lib/windows-elevation.ts +++ b/src/lib/windows-elevation.ts @@ -250,6 +250,21 @@ export const OCX_ELEVATED_PROTOCOL_FAILED = 13; /** Windows ERROR_CANCELLED — reserved for UAC denial; never emitted by the elevated script. */ export const OCX_ELEVATED_UAC_CANCELLED = 1223; +/** + * The elevated process could not read a staged payload (#4692). + * + * `hardenSecretPath` grants the staging account and strips inheritance, so a split-token + * elevation of the same user reads the file and an elevation answered with a DIFFERENT + * administrator's credentials does not. The elevated side cannot explain that itself: it + * runs hidden, so its stderr goes nowhere and only the exit code survives the boundary. + * Without a code of its own the operator would be told "exit code 1" for a cause that + * names its own remedy — the same undiagnosable failure this change set exists to remove. + * + * Deliberately outside OCX_ELEVATED_PROTOCOL_CODES: that list is the create-and-run + * transaction's alphabet, and this code belongs to the registration path. + */ +export const OCX_ELEVATED_STAGING_UNREADABLE = 14; + export const OCX_ELEVATED_PROTOCOL_CODES = [ OCX_ELEVATED_SUCCESS, OCX_ELEVATED_CREATE_FAILED, @@ -667,7 +682,11 @@ export interface StagedWindowsTaskXml { * exists to close. */ const READ_STAGED_TASK_XML = "function Read-OcxStagedTaskXml([string]$path, [string]$expectedHash) {" - + " $bytes = [IO.File]::ReadAllBytes($path);" + // An unreadable payload is a diagnosable condition, not a generic throw: a hidden + // elevated process has nowhere to print, so the cause has to ride the exit code. + + " try { $bytes = [IO.File]::ReadAllBytes($path) }" + + " catch [System.UnauthorizedAccessException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " }" + + " catch [System.Security.SecurityException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " };" + " $sha = [Security.Cryptography.SHA256]::Create();" + " try { $actual = [BitConverter]::ToString($sha.ComputeHash($bytes)).Replace('-', '').ToLowerInvariant() } finally { $sha.Dispose() };" + " if ($actual -cne $expectedHash) { throw 'Task Scheduler staged payload failed its integrity check.' };" diff --git a/src/service.ts b/src/service.ts index f6a571b5748..149b1ae02c7 100644 --- a/src/service.ts +++ b/src/service.ts @@ -19,7 +19,7 @@ export { decodeSchtasksOutput, setQuerySchtasksForTests, formatWindowsSchedulerS export type { WindowsSchedulerXmlState } from "./service/windows-taskxml"; export { buildWindowsServiceScript, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsLauncherVbs, buildWindowsTaskXml, buildWindowsTaskXmlDocument, windowsTaskRegistrationOwnedByAttempt, windowsTaskRegistrationHealthy, readWindowsSchedulerXmlState } from "./service/windows-taskxml"; export type { WindowsSchedulerRegistrationStageDeps, FreshWindowsSchedulerRegistrationDeps, RemoveNativeWindowsServiceDeps } from "./service/windows-ops"; -export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, stageElevatedSchedulerRegistration, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; +export { windowsListenPort, winswListenPort, writeServiceDefinitionFile, definitionCarriesCredential, stageWindowsSchedulerRegistrationXml, stageElevatedSchedulerRegistration, describeElevatedRegistrationFailure, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, assertWindowsNativeServiceAccountSupported, isWindowsSchedulerEndBenign, stopWindows, stopWindowsChecked, classifyWindowsServiceStop } from "./service/windows-ops"; export type { ServiceRepairVerb, RepairServiceDeps } from "./service/repair"; export { repairService } from "./service/repair"; export type { ServiceInstallPreparationDeps, FreshWindowsSchedulerInstallDeps, ServiceStopOutcome, ServiceUninstallOutcome } from "./service/orchestration"; diff --git a/src/service/windows-ops.ts b/src/service/windows-ops.ts index 75321fb9fcf..9206d6a8f9a 100644 --- a/src/service/windows-ops.ts +++ b/src/service/windows-ops.ts @@ -10,7 +10,7 @@ import { existsSync, mkdirSync, mkdtempSync, rmdirSync, unlinkSync } from "node: import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { getConfigDir } from "../config"; -import { runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError, type StagedWindowsTaskXml } from "../lib/windows-elevation"; +import { OCX_ELEVATED_STAGING_UNREADABLE, runWindowsElevatedScheduledTaskRegistration, WindowsSchtasksError, type StagedWindowsTaskXml } from "../lib/windows-elevation"; import { defaultWinswEntry, installWinswService, statusWinswRaw, uninstallWinswService, WINSW_SERVICE_ID, type WinswStatus } from "../lib/winsw"; import { forgetEphemeralSecretDir, forgetEphemeralSecretPath, hardenSecretDir } from "../lib/windows-secret-acl"; import { recordOwnedConfigPath } from "../lib/config-ownership"; @@ -288,6 +288,31 @@ export function stageElevatedSchedulerRegistration( } } +/** + * Turn an elevated registration exit code into something an operator can act on. + * + * The elevated process runs hidden, so nothing it writes survives; only the exit code + * crosses back. That makes an unexplained code the whole user-facing error, which is + * exactly what made the ENAMETOOLONG in #4692 expensive to diagnose. Staging introduces + * one new failure of its own — the payload is readable only by the account that created + * it, so an elevation answered with a different administrator's credentials cannot open + * it — and that one gets named along with its remedy rather than surfacing as a number. + */ +export function describeElevatedRegistrationFailure( + failureLabel: string, + exitCode: number, + stageDir: string, +): string { + if (exitCode === OCX_ELEVATED_STAGING_UNREADABLE) { + return `${failureLabel}: the elevated process could not read the staged task definition in ` + + `${stageDir}. That directory is readable only by the account that staged it, so this ` + + "happens when the UAC prompt was answered with a different administrator account. " + + "Approve the prompt as the signed-in user, or run the command again from a session " + + "already elevated as that user."; + } + return `${failureLabel} with exit code ${exitCode}.`; +} + /** * Stage, elevate, and clean up — on every exit, including UAC cancellation and a * synchronous spawn failure. @@ -312,7 +337,9 @@ async function runStagedElevatedSchedulerRegistration( replace, staged.expectedExisting, ); - if (exitCode !== 0) failure = new Error(`${failureLabel} with exit code ${exitCode}.`); + if (exitCode !== 0) { + failure = new Error(describeElevatedRegistrationFailure(failureLabel, exitCode, dirname(staged.xml.path))); + } } catch (error) { failure = error; } diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index 3b17cd6e93e..e3870a59393 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -15,6 +15,7 @@ import { buildWinswXml } from "../../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../../src/lib/service-secrets"; import { WindowsSchtasksError } from "../../src/lib/windows-elevation"; +import { OCX_ELEVATED_STAGING_UNREADABLE } from "../../src/lib/windows-elevation"; import { resolveCurrentWindowsPrincipal, setWindowsPrincipalRunnerForTests } from "../../src/lib/windows-user-principal"; import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; import type { OcxConfig } from "../../src/types"; @@ -2192,6 +2193,30 @@ describe("service lifecycle cleanup ordering", () => { } }); + test("an unreadable staged payload is reported with its cause and its remedy", () => { + // The elevated process runs hidden, so nothing it writes survives and the exit code is + // the entire user-facing error. Staging adds exactly one new failure -- the payload is + // readable only by the account that created it, so an elevation answered with another + // administrator's credentials cannot open it -- and reporting that as a bare number + // would reproduce what made #4692 expensive to diagnose in the first place. + const message = serviceModule.describeElevatedRegistrationFailure( + "Background service install failed", + OCX_ELEVATED_STAGING_UNREADABLE, + "C:\\Temp\\opencodex-service-stage-aaaaaa", + ); + expect(message).toContain("could not read the staged task definition"); + expect(message).toContain("C:\\Temp\\opencodex-service-stage-aaaaaa"); + expect(message).toContain("different administrator account"); + expect(message).toContain("Approve the prompt as the signed-in user"); + expect(message).not.toMatch(/exit code \d+/); + + // Every other code keeps the plain form; this is a named cause, not a catch-all. + for (const code of [1, 10, 13, 1223]) { + expect(serviceModule.describeElevatedRegistrationFailure("Task Scheduler rollback failed", code, "C:\\Temp\\x")) + .toBe("Task Scheduler rollback failed with exit code " + code + "."); + } + }); + test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { const calls: string[] = []; mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/windows/windows-elevation-spawn.test.ts b/tests/windows/windows-elevation-spawn.test.ts index fa0058f51db..4c905a49af1 100644 --- a/tests/windows/windows-elevation-spawn.test.ts +++ b/tests/windows/windows-elevation-spawn.test.ts @@ -6,6 +6,7 @@ import { OCX_ELEVATED_PROTOCOL_FAILED, OCX_ELEVATED_RUN_FAILED_ROLLBACK_FAILED, OCX_ELEVATED_RUN_FAILED_ROLLED_BACK, + OCX_ELEVATED_STAGING_UNREADABLE, OCX_ELEVATED_SUCCESS, OCX_ELEVATED_UAC_CANCELLED, WindowsElevationError, @@ -223,6 +224,15 @@ describe("runWindowsElevated spawn contract", () => { expect(elevatedScript).toContain("[IO.File]::ReadAllBytes($path)"); expect(elevatedScript).toContain("$sha.ComputeHash($bytes)"); expect(elevatedScript).toContain("Task Scheduler staged payload failed its integrity check."); + // #4692 follow-up: the one failure this staging design introduces has to be readable. + // A hidden elevated process has nowhere to print, so an unreadable payload rides its + // own exit code instead of collapsing into a generic non-zero status. + expect(elevatedScript).toContain("catch [System.UnauthorizedAccessException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " }"); + expect(elevatedScript).toContain("catch [System.Security.SecurityException] { exit " + OCX_ELEVATED_STAGING_UNREADABLE + " }"); + // It is not part of the create-and-run transaction's alphabet, and cannot be mistaken + // for UAC denial. + expect(OCX_ELEVATED_PROTOCOL_CODES).not.toContain(OCX_ELEVATED_STAGING_UNREADABLE); + expect(OCX_ELEVATED_STAGING_UNREADABLE).not.toBe(OCX_ELEVATED_UAC_CANCELLED); expect(elevatedScript.indexOf("-cne $expectedHash")) .toBeLessThan(elevatedScript.indexOf("[Text.Encoding]::Unicode.GetString($bytes)")); // No payload rides the command line any more, in either encoding layer. From 6a2b148f5ab4cc762573317c18b15d924273e6d3 Mon Sep 17 00:00:00 2001 From: JUN Date: Wed, 16 Sep 2026 12:14:45 +0900 Subject: [PATCH 5/5] test(service): move elevated-staging cover out of the ratcheted service 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. --- src/service/windows-ops.ts | 2 +- tests/service/service.test.ts | 139 ----------------- tests/windows/windows-elevation-spawn.test.ts | 146 ++++++++++++++++++ 3 files changed, 147 insertions(+), 140 deletions(-) diff --git a/src/service/windows-ops.ts b/src/service/windows-ops.ts index 9206d6a8f9a..703e9420613 100644 --- a/src/service/windows-ops.ts +++ b/src/service/windows-ops.ts @@ -235,7 +235,7 @@ export function stageElevatedSchedulerRegistration( forgetEphemeralSecretPath(file); } catch (error) { if ((error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") forgetEphemeralSecretPath(file); - else failure ??= error; + else if (failure === undefined) failure = error; } } try { diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index e3870a59393..5daaa02ec9c 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -1,7 +1,6 @@ import { afterAll, afterEach, describe, expect, spyOn, test } from "bun:test"; import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; -import { createHash } from "node:crypto"; import { tmpdir } from "node:os"; import { delimiter, isAbsolute, join, posix, win32 } from "node:path"; import { pathToFileURL } from "node:url"; @@ -15,7 +14,6 @@ import { buildWinswXml } from "../../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../../src/lib/service-secrets"; import { WindowsSchtasksError } from "../../src/lib/windows-elevation"; -import { OCX_ELEVATED_STAGING_UNREADABLE } from "../../src/lib/windows-elevation"; import { resolveCurrentWindowsPrincipal, setWindowsPrincipalRunnerForTests } from "../../src/lib/windows-user-principal"; import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; import type { OcxConfig } from "../../src/types"; @@ -2080,143 +2078,6 @@ describe("service lifecycle cleanup ordering", () => { } }); - /** - * #4692: a file an administrator process will read is itself a privilege-escalation - * surface, so access, redirection and tamper-evidence each have to hold. - */ - test("elevated staging hardens before writing, digests the exact bytes, and cleans up", () => { - const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-")); - const stageDir = join(parent, "private-stage"); - const calls: string[] = []; - try { - const staged = serviceModule.stageElevatedSchedulerRegistration( - "new", - "previous", - { - createStageDir: () => { - mkdirSync(stageDir, { mode: 0o700 }); - calls.push("create-stage-dir"); - return stageDir; - }, - hardenDir: () => { calls.push("harden-dir"); }, - writePayload: (path, bytes) => { - calls.push("write:" + path.slice(stageDir.length + 1)); - writeFileSync(path, bytes, { flag: "wx" }); - }, - hardenPath: path => { calls.push("harden:" + path.slice(stageDir.length + 1)); }, - }, - ); - - // The directory is private before anything is written into it; hardening after the - // write would leave a window where the payload is readable by another account. - expect(calls).toEqual([ - "create-stage-dir", - "harden-dir", - "write:register.xml", - "harden:register.xml", - "write:expected.xml", - "harden:expected.xml", - ]); - - // The digest covers exactly the bytes on disk, and those bytes are UTF-16LE with no - // BOM: the elevated process decodes them straight into Register-ScheduledTask, so - // what is hashed here is what gets registered, with no trimming step in between. - for (const [payload, value] of [ - [staged.xml, "new"], - [staged.expectedExisting!, "previous"], - ] as const) { - const onDisk = readFileSync(payload.path); - expect(onDisk.equals(Buffer.from(value, "utf16le"))).toBe(true); - expect(onDisk[0]).not.toBe(0xff); - expect(payload.sha256).toBe(createHash("sha256").update(onDisk).digest("hex")); - expect(payload.sha256).toMatch(/^[0-9a-f]{64}$/); - } - expect(staged.xml.sha256).not.toBe(staged.expectedExisting!.sha256); - - staged.cleanup(); - expect(existsSync(stageDir)).toBe(false); - // Idempotent: the success path calls it once, but a failure path may race it. - expect(() => staged.cleanup()).not.toThrow(); - } finally { - removeTreeWithRetry(parent); - } - }); - - test("elevated staging refuses a redirected path and leaves nothing behind", () => { - const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-reparse-")); - const stageDir = join(parent, "private-stage"); - try { - // A staged payload reached through a reparse point is a payload somebody else chose - // the destination for. Exclusive creation already refuses an existing name, so this - // is the check that keeps the guarantee from resting on a reading of O_EXCL. - expect(() => serviceModule.stageElevatedSchedulerRegistration("", undefined, { - createStageDir: () => { - mkdirSync(stageDir, { mode: 0o700 }); - return stageDir; - }, - hardenDir: () => {}, - writePayload: (path, bytes) => { writeFileSync(path, bytes, { flag: "wx" }); }, - hardenPath: () => { throw new Error("must not harden a redirected payload"); }, - inspect: path => ({ - isSymbolicLink: () => path !== stageDir, - isFile: () => true, - isDirectory: () => path === stageDir, - }), - })).toThrow("redirected path"); - expect(existsSync(stageDir)).toBe(false); - } finally { - removeTreeWithRetry(parent); - } - }); - - test("elevated staging cleans up when a payload write fails partway", () => { - const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-partial-")); - const stageDir = join(parent, "private-stage"); - try { - // The predecessor is the second payload, so this leaves a real file behind unless - // cleanup walks everything it created rather than only the one that failed. - expect(() => serviceModule.stageElevatedSchedulerRegistration("", "", { - createStageDir: () => { - mkdirSync(stageDir, { mode: 0o700 }); - return stageDir; - }, - hardenDir: () => {}, - writePayload: (path, bytes) => { - if (path.endsWith("expected.xml")) throw new Error("synthetic predecessor write failure"); - writeFileSync(path, bytes, { flag: "wx" }); - }, - hardenPath: () => {}, - })).toThrow("synthetic predecessor write failure"); - expect(existsSync(stageDir)).toBe(false); - } finally { - removeTreeWithRetry(parent); - } - }); - - test("an unreadable staged payload is reported with its cause and its remedy", () => { - // The elevated process runs hidden, so nothing it writes survives and the exit code is - // the entire user-facing error. Staging adds exactly one new failure -- the payload is - // readable only by the account that created it, so an elevation answered with another - // administrator's credentials cannot open it -- and reporting that as a bare number - // would reproduce what made #4692 expensive to diagnose in the first place. - const message = serviceModule.describeElevatedRegistrationFailure( - "Background service install failed", - OCX_ELEVATED_STAGING_UNREADABLE, - "C:\\Temp\\opencodex-service-stage-aaaaaa", - ); - expect(message).toContain("could not read the staged task definition"); - expect(message).toContain("C:\\Temp\\opencodex-service-stage-aaaaaa"); - expect(message).toContain("different administrator account"); - expect(message).toContain("Approve the prompt as the signed-in user"); - expect(message).not.toMatch(/exit code \d+/); - - // Every other code keeps the plain form; this is a named cause, not a catch-all. - for (const code of [1, 10, 13, 1223]) { - expect(serviceModule.describeElevatedRegistrationFailure("Task Scheduler rollback failed", code, "C:\\Temp\\x")) - .toBe("Task Scheduler rollback failed with exit code " + code + "."); - } - }); - test("UAC cancellation removes only staged XML and never enters cleanup or asset publication", async () => { const calls: string[] = []; mkdirSync(TEST_DIR, { recursive: true }); diff --git a/tests/windows/windows-elevation-spawn.test.ts b/tests/windows/windows-elevation-spawn.test.ts index 4c905a49af1..d2550a44bc7 100644 --- a/tests/windows/windows-elevation-spawn.test.ts +++ b/tests/windows/windows-elevation-spawn.test.ts @@ -1,5 +1,9 @@ import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; import { EventEmitter } from "node:events"; +import { createHash } from "node:crypto"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { OCX_ELEVATED_CREATE_FAILED, OCX_ELEVATED_PROTOCOL_CODES, @@ -26,8 +30,150 @@ import { finalizeWindowsSchedulerServiceRegistration, schedulerVerificationMaySettle, setFinalizeWindowsSchedulerHooksForTests, + stageElevatedSchedulerRegistration, + describeElevatedRegistrationFailure, } from "../../src/service"; import type { WindowsSchedulerInstallVerification } from "../../src/service"; +import { removeTreeWithRetry } from "../helpers/remove-tree"; + +/** + * #4692: a file an administrator process will read is itself a privilege-escalation + * surface, so access, redirection and tamper-evidence each have to hold. + */ +describe("elevated Task Scheduler payload staging", () => { + test("hardens before writing, digests the exact bytes, and cleans up", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-")); + const stageDir = join(parent, "private-stage"); + const calls: string[] = []; + try { + const staged = stageElevatedSchedulerRegistration( + "new", + "previous", + { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + calls.push("create-stage-dir"); + return stageDir; + }, + hardenDir: () => { calls.push("harden-dir"); }, + writePayload: (path, bytes) => { + calls.push("write:" + path.slice(stageDir.length + 1)); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: path => { calls.push("harden:" + path.slice(stageDir.length + 1)); }, + }, + ); + + // The directory is private before anything is written into it; hardening after the + // write would leave a window where the payload is readable by another account. + expect(calls).toEqual([ + "create-stage-dir", + "harden-dir", + "write:register.xml", + "harden:register.xml", + "write:expected.xml", + "harden:expected.xml", + ]); + + // The digest covers exactly the bytes on disk, and those bytes are UTF-16LE with no + // BOM: the elevated process decodes them straight into Register-ScheduledTask, so + // what is hashed here is what gets registered, with no trimming step in between. + for (const [payload, value] of [ + [staged.xml, "new"], + [staged.expectedExisting!, "previous"], + ] as const) { + const onDisk = readFileSync(payload.path); + expect(onDisk.equals(Buffer.from(value, "utf16le"))).toBe(true); + expect(onDisk[0]).not.toBe(0xff); + expect(payload.sha256).toBe(createHash("sha256").update(onDisk).digest("hex")); + expect(payload.sha256).toMatch(/^[0-9a-f]{64}$/); + } + expect(staged.xml.sha256).not.toBe(staged.expectedExisting!.sha256); + + staged.cleanup(); + expect(existsSync(stageDir)).toBe(false); + // Idempotent: the success path calls it once, but a failure path may race it. + expect(() => staged.cleanup()).not.toThrow(); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("refuses a redirected path and leaves nothing behind", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-reparse-")); + const stageDir = join(parent, "private-stage"); + try { + // A staged payload reached through a reparse point is a payload somebody else chose + // the destination for. Exclusive creation already refuses an existing name, so this + // is the check that keeps the guarantee from resting on a reading of O_EXCL. + expect(() => stageElevatedSchedulerRegistration("", undefined, { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { writeFileSync(path, bytes, { flag: "wx" }); }, + hardenPath: () => { throw new Error("must not harden a redirected payload"); }, + inspect: path => ({ + isSymbolicLink: () => path !== stageDir, + isFile: () => true, + isDirectory: () => path === stageDir, + }), + })).toThrow("redirected path"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("cleans up when a payload write fails partway", () => { + const parent = mkdtempSync(join(tmpdir(), "ocx-elevated-stage-partial-")); + const stageDir = join(parent, "private-stage"); + try { + // The predecessor is the second payload, so this leaves a real file behind unless + // cleanup walks everything it created rather than only the one that failed. + expect(() => stageElevatedSchedulerRegistration("", "", { + createStageDir: () => { + mkdirSync(stageDir, { mode: 0o700 }); + return stageDir; + }, + hardenDir: () => {}, + writePayload: (path, bytes) => { + if (path.endsWith("expected.xml")) throw new Error("synthetic predecessor write failure"); + writeFileSync(path, bytes, { flag: "wx" }); + }, + hardenPath: () => {}, + })).toThrow("synthetic predecessor write failure"); + expect(existsSync(stageDir)).toBe(false); + } finally { + removeTreeWithRetry(parent); + } + }); + + test("an unreadable staged payload is reported with its cause and its remedy", () => { + // The elevated process runs hidden, so nothing it writes survives and the exit code is + // the entire user-facing error. Staging adds exactly one new failure -- the payload is + // readable only by the account that created it, so an elevation answered with another + // administrator's credentials cannot open it -- and reporting that as a bare number + // would reproduce what made #4692 expensive to diagnose in the first place. + const message = describeElevatedRegistrationFailure( + "Background service install failed", + OCX_ELEVATED_STAGING_UNREADABLE, + "C:\\Temp\\opencodex-service-stage-aaaaaa", + ); + expect(message).toContain("could not read the staged task definition"); + expect(message).toContain("C:\\Temp\\opencodex-service-stage-aaaaaa"); + expect(message).toContain("different administrator account"); + expect(message).toContain("Approve the prompt as the signed-in user"); + expect(message).not.toMatch(/exit code \d+/); + + // Every other code keeps the plain form; this is a named cause, not a catch-all. + for (const code of [1, 10, 13, 1223]) { + expect(describeElevatedRegistrationFailure("Task Scheduler rollback failed", code, "C:\\Temp\\x")) + .toBe("Task Scheduler rollback failed with exit code " + code + "."); + } + }); +}); /** Linux CI fakes win32 without a real System32; keep elevation paths production-shaped. */ const FAKE_TRUSTED_ELEVATION_EXES = {