diff --git a/src/lib/windows-elevation.ts b/src/lib/windows-elevation.ts index 0e7d1757868..b2d02b81234 100644 --- a/src/lib/windows-elevation.ts +++ b/src/lib/windows-elevation.ts @@ -397,7 +397,7 @@ export function formatWindowsSchtasksError(error: unknown, args: string[]): stri const guidance = [ "Windows access denied while running Task Scheduler.", `Command: schtasks ${argsText}`, - "Approve the Windows UAC prompt to install the background service, or run `ocx service install` from an elevated PowerShell window.", + "The OpenCodex task definition is scoped to the installing account and normally registers without elevation, so this denial is not fixed by approving a UAC prompt. Check `ocx service status`: if an existing `opencodex-proxy` task belongs to a different account, remove it from that account and retry `ocx service install`.", ].join(" "); if (operation === "create" && ownedCreateAccessDenied) { return `${guidance}\n${WINDOWS_SCHTASKS_CREATE_ACCESS_DENIED_MARKER}`; diff --git a/src/service.ts b/src/service.ts index 85c5b3abc56..08e04ef49c6 100644 --- a/src/service.ts +++ b/src/service.ts @@ -2303,9 +2303,12 @@ export function buildWindowsTaskXml( // keeps spaces intact, and /b (batch mode) suppresses script error popups. const escapedLauncherArgs = taskXmlString(`/b /nologo "${launcher}"`); // `UserId` is optional in the schema, and omitting it makes a SessionStateChangeTrigger - // fire for ANY account's session change. Production registration resolves and passes the - // installing account SID explicitly; the optional parameter remains only for deterministic - // builders/tests, and the live validator rejects an unscoped recovery trigger. + // fire for ANY account's session change. An unscoped LogonTrigger is read as "any user's + // logon", which a non-elevated user may not register: schtasks /create answers "Access is + // denied" for a task whose design (InteractiveToken + LeastPrivilege) needs no elevation + // (#4425). Production registration resolves and passes the installing account SID + // explicitly; the optional parameter remains only for deterministic builders/tests, and + // the live validator rejects an unscoped recovery trigger. const sessionUserIdElement = sessionTriggerUserId ? `\n ${taskXmlString(sessionTriggerUserId)}` : ""; @@ -2316,7 +2319,7 @@ export function buildWindowsTaskXml( - true + true${sessionUserIdElement} ${WINDOWS_SESSION_RECOVERY_STATE_CHANGES.map(stateChange => ` true${sessionUserIdElement} diff --git a/tests/service/service.test.ts b/tests/service/service.test.ts index 677866fa498..71666961f6a 100644 --- a/tests/service/service.test.ts +++ b/tests/service/service.test.ts @@ -45,6 +45,10 @@ const buildWindowsTaskXml = (...args: Parameters) => windowsTaskRegistrationHealthyProduction(args[0], args[1], args[2], args[3] === undefined ? TEST_WINDOWS_TASK_SID : args[3]); +/** The exact LogonTrigger block the builder emits for the default test SID (#4425). */ +const LOGON_TRIGGER_BLOCK = + `\n true\n ${TEST_WINDOWS_TASK_SID}\n `; + const TEST_DIR = join(import.meta.dir, ".tmp-service-test"); const previousOpenCodexHome = process.env.OPENCODEX_HOME; const previousCodexHome = process.env.CODEX_HOME; @@ -850,8 +854,8 @@ describe("Windows service task", () => { * `UserId` is optional in the schema, and omitting it makes a SessionStateChangeTrigger fire * for any account's session change. Scope it to the installing account when that account is * known. The builder is synchronous and cannot force an account lookup, so an unknown - * account degrades to the unscoped trigger — the same position the pre-existing - * `LogonTrigger` is already in, and still better than having no recovery trigger at all. + * account degrades every trigger to the unscoped form — still better than having no + * recovery trigger at all, and the position `LogonTrigger` was in before #4425. */ test("scopes session-recovery triggers to the installing account when it is known", () => { const scoped = buildWindowsTaskXml("s.cmd", "l.vbs", undefined, "MACHINE\\installer"); @@ -885,6 +889,29 @@ describe("Windows service task", () => { } }); + /** + * #4425: an unscoped LogonTrigger means "any user's logon", which a non-elevated user may + * not register — schtasks /create answers "Access is denied" even though the task design + * (InteractiveToken + LeastPrivilege) needs no elevation, and the old diagnostic then + * misread that denial as a privilege problem. The logon trigger consumes the same + * scoped-user element as its sibling session triggers, and logonTriggerType orders + * Enabled before UserId. + */ + test("scopes the logon trigger to the installing account so a non-elevated create succeeds", () => { + const scoped = buildWindowsTaskXml("s.cmd", "l.vbs", undefined, "MACHINE\\installer"); + const logon = /[\s\S]*?<\/LogonTrigger>/i.exec(scoped)?.[0] ?? ""; + expect(logon).toContain("true"); + expect(logon).toContain("MACHINE\\installer"); + expect(logon.indexOf("")).toBeLessThan(logon.indexOf("")); + + // Unknown account: unscoped rather than absent — the pre-#4425 shape, which only an + // elevated shell can register. Production always resolves a SID before staging. + const unscoped = buildWindowsTaskXml("s.cmd", "l.vbs", undefined, ""); + const unscopedLogon = /[\s\S]*?<\/LogonTrigger>/i.exec(unscoped)?.[0] ?? ""; + expect(unscopedLogon).toContain("true"); + expect(unscopedLogon).not.toContain(""); + }); + test("accepts an explicit session scope only for the known matching identity", () => { const wscript = "C:\\Windows\\System32\\wscript.exe"; const launcher = "C:\\Users\\Test\\.opencodex\\service-launcher.vbs"; @@ -931,10 +958,13 @@ describe("Windows service task", () => { const scoped = buildWindowsTaskXml("ignored.cmd", guardLauncher, undefined, "MACHINE\\installer") .replace(/.*?<\/Command>/, `${guardWscript}`); const duplicateScope = scoped.replace( - "MACHINE\\installer", - "MACHINE\\installerMACHINE\\installer", + "MACHINE\\installer\n ", + "MACHINE\\installerMACHINE\\installer\n ", + ); + const emptyScope = scoped.replace( + "MACHINE\\installer\n ", + "\n ", ); - const emptyScope = scoped.replace("MACHINE\\installer", ""); expect(windowsTaskRegistrationHealthy(duplicateScope, guardWscript, guardLauncher, "MACHINE\\installer")).toBe(false); expect(windowsTaskRegistrationHealthy(emptyScope, guardWscript, guardLauncher, "MACHINE\\installer")).toBe(false); @@ -962,7 +992,7 @@ describe("Windows service task", () => { // Windows drops elements equal to their schema default when it exports a task: // Trigger/Settings Enabled default to true and RunLevel defaults to LeastPrivilege. const canonical = xml - .replace("\n true\n ", "") + .replace(LOGON_TRIGGER_BLOCK, "") .replace(" LeastPrivilege\n", "") .replace(" true\n ", " "); expect(canonical).toContain(""); @@ -1076,7 +1106,7 @@ describe("Windows service task", () => { const launcher = "C:\\Users\\Test\\.opencodex\\service-launcher.vbs"; const xml = buildWindowsTaskXml("ignored.cmd", launcher) .replace(/.*?<\/Command>/, `${wscript}`); - const bootOnly = xml.replace("\n true\n ", ""); + const bootOnly = xml.replace(LOGON_TRIGGER_BLOCK, ""); // The schema allows arbitrary XML under Task/Data, and comments could smuggle a // decoy too — neither may stand in for a real logon trigger. diff --git a/tests/windows/windows-elevation.test.ts b/tests/windows/windows-elevation.test.ts index dc2f207b7b9..9ac35dfbe57 100644 --- a/tests/windows/windows-elevation.test.ts +++ b/tests/windows/windows-elevation.test.ts @@ -39,7 +39,7 @@ describe("windows elevation helpers", () => { expect(isWindowsAccessDeniedError(error)).toBe(true); }); - test("formats schtasks create access-denied errors with marker and UAC guidance", () => { + test("formats schtasks create access-denied errors with marker and non-elevation guidance", () => { const error = Object.assign(new Error("Command failed"), { stderr: "FEHLER: Zugriff verweigert\r\n", stdout: "", @@ -55,7 +55,10 @@ describe("windows elevation helpers", () => { ]); expect(message).toContain("Windows access denied while running Task Scheduler."); expect(message).toContain("schtasks /create /tn opencodex-proxy /xml task.xml /f"); - expect(message).toContain("UAC prompt"); + // #4425: the scoped task definition registers without elevation, so the diagnostic + // must not send the user to approve a UAC prompt as the fix. + expect(message).toContain("normally registers without elevation"); + expect(message).not.toContain("Approve the Windows UAC prompt"); expect(message).toContain(WINDOWS_SCHTASKS_CREATE_ACCESS_DENIED_MARKER); expect(isWindowsSchtasksCreateAccessDenied(message)).toBe(true); });