Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib/windows-elevation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(" ");
Comment on lines 399 to 401

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Limit the non-elevation guidance to create failures

This guidance is selected for every access-denied schtasks invocation, not just /create; src/service.ts routes /run, /end, /query, and /delete failures through the same formatter. For example, an access denial during service uninstall now incorrectly says elevation cannot help and tells the user to retry installation, even though the scoped task definition is irrelevant to deletion and elevated permissions may be the remedy. Branch on operation and retain create-specific wording only for the owned create case.

Useful? React with 👍 / 👎.

if (operation === "create" && ownedCreateAccessDenied) {
return `${guidance}\n${WINDOWS_SCHTASKS_CREATE_ACCESS_DENIED_MARKER}`;
Expand Down
11 changes: 7 additions & 4 deletions src/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 <UserId>${taskXmlString(sessionTriggerUserId)}</UserId>`
: "";
Expand All @@ -2316,7 +2319,7 @@ export function buildWindowsTaskXml(
</RegistrationInfo>
<Triggers>
<LogonTrigger>
<Enabled>true</Enabled>
<Enabled>true</Enabled>${sessionUserIdElement}
</LogonTrigger>
Comment on lines 2321 to 2323

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Update the owned service architecture documents

This changes the Windows Task Scheduler service contract in src/service.ts, but neither structure/runtime.md nor structure/ops/docs-and-release.md—the documents mapped to this source file by structure/INDEX.md—is updated. Document the newly required account-scoped logon trigger and its compatibility implications in the same change.

AGENTS.md reference: src/AGENTS.md:L10-L11

Useful? React with 👍 / 👎.

${WINDOWS_SESSION_RECOVERY_STATE_CHANGES.map(stateChange => `<SessionStateChangeTrigger>
<Enabled>true</Enabled>${sessionUserIdElement}
Expand Down
44 changes: 37 additions & 7 deletions tests/service/service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ const buildWindowsTaskXml = (...args: Parameters<typeof buildWindowsTaskXmlProdu
const windowsTaskRegistrationHealthy = (...args: Parameters<typeof windowsTaskRegistrationHealthyProduction>) =>
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 =
`<LogonTrigger>\n <Enabled>true</Enabled>\n <UserId>${TEST_WINDOWS_TASK_SID}</UserId>\n </LogonTrigger>`;

const TEST_DIR = join(import.meta.dir, ".tmp-service-test");
const previousOpenCodexHome = process.env.OPENCODEX_HOME;
const previousCodexHome = process.env.CODEX_HOME;
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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 = /<LogonTrigger>[\s\S]*?<\/LogonTrigger>/i.exec(scoped)?.[0] ?? "";
expect(logon).toContain("<Enabled>true</Enabled>");
expect(logon).toContain("<UserId>MACHINE\\installer</UserId>");
expect(logon.indexOf("<Enabled>")).toBeLessThan(logon.indexOf("<UserId>"));

// 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 = /<LogonTrigger>[\s\S]*?<\/LogonTrigger>/i.exec(unscoped)?.[0] ?? "";
expect(unscopedLogon).toContain("<Enabled>true</Enabled>");
expect(unscopedLogon).not.toContain("<UserId>");
});

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";
Expand Down Expand Up @@ -931,10 +958,13 @@ describe("Windows service task", () => {
const scoped = buildWindowsTaskXml("ignored.cmd", guardLauncher, undefined, "MACHINE\\installer")
.replace(/<Command>.*?<\/Command>/, `<Command>${guardWscript}</Command>`);
const duplicateScope = scoped.replace(
"<UserId>MACHINE\\installer</UserId>",
"<UserId>MACHINE\\installer</UserId><UserId>MACHINE\\installer</UserId>",
"<UserId>MACHINE\\installer</UserId>\n <StateChange>",
"<UserId>MACHINE\\installer</UserId><UserId>MACHINE\\installer</UserId>\n <StateChange>",
);
const emptyScope = scoped.replace(
"<UserId>MACHINE\\installer</UserId>\n <StateChange>",
"<UserId></UserId>\n <StateChange>",
);
const emptyScope = scoped.replace("MACHINE\\installer", "");
expect(windowsTaskRegistrationHealthy(duplicateScope, guardWscript, guardLauncher, "MACHINE\\installer")).toBe(false);
expect(windowsTaskRegistrationHealthy(emptyScope, guardWscript, guardLauncher, "MACHINE\\installer")).toBe(false);

Expand Down Expand Up @@ -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("<LogonTrigger>\n <Enabled>true</Enabled>\n </LogonTrigger>", "<LogonTrigger />")
.replace(LOGON_TRIGGER_BLOCK, "<LogonTrigger />")
.replace(" <RunLevel>LeastPrivilege</RunLevel>\n", "")
.replace(" <Enabled>true</Enabled>\n <Hidden>", " <Hidden>");
expect(canonical).toContain("<LogonTrigger />");
Expand Down Expand Up @@ -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>.*?<\/Command>/, `<Command>${wscript}</Command>`);
const bootOnly = xml.replace("<LogonTrigger>\n <Enabled>true</Enabled>\n </LogonTrigger>", "<BootTrigger />");
const bootOnly = xml.replace(LOGON_TRIGGER_BLOCK, "<BootTrigger />");

// The schema allows arbitrary XML under Task/Data, and comments could smuggle a
// decoy too — neither may stand in for a real logon trigger.
Expand Down
7 changes: 5 additions & 2 deletions tests/windows/windows-elevation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: "",
Expand All @@ -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);
});
Expand Down
Loading