fix(cron): reject malformed condition scripts before saving (#127004)

* fix(cron): reject malformed condition triggers

Validate newly authored cron condition scripts before persistence, return actionable Gateway errors, and preserve maintenance of malformed triggers shipped in existing releases.

* fix(cron): reject blank condition triggers
This commit is contained in:
Peter Steinberger
2026-08-20 22:42:06 -07:00
committed by GitHub
parent 6c667989a8
commit bed80ee645
11 changed files with 315 additions and 45 deletions
+28
View File
@@ -83,6 +83,34 @@ function ownedDeclaration(params: {
}
describe("CronService declarative jobs", () => {
it("rejects malformed declared triggers without changing persisted jobs", async () => {
const { storePath } = await makeStorePath();
const cron = createCronService(storePath);
await cron.start();
try {
const invalidTrigger = { script: "const x = ;" };
const expectedError =
"cron trigger script has a syntax error: Unexpected token (line 1, column 10)";
await expect(cron.add(declaration({ trigger: invalidTrigger }))).rejects.toThrow(
expectedError,
);
expect(await cron.list()).toEqual([]);
const validTrigger = { script: "return { fire: true }" };
const created = declarativeResult(await cron.add(declaration({ trigger: validTrigger })));
await expect(cron.add(declaration({ trigger: invalidTrigger }))).rejects.toThrow(
expectedError,
);
expect(await cron.readJob(created.id)).toMatchObject({ trigger: validTrigger });
expect(await cron.list()).toEqual([expect.objectContaining({ trigger: validTrigger })]);
} finally {
cron.stop();
}
});
it("creates, no-ops, and converges in place while preserving state and enablement", async () => {
const { storePath } = await makeStorePath();
const cron = createCronService(storePath);
+88
View File
@@ -868,6 +868,94 @@ describe("cron tool authority defaults", () => {
});
});
describe("condition trigger syntax validation", () => {
const now = Date.parse("2026-07-18T12:00:00.000Z");
const malformedScript = "const x = ;";
const input = (script = "return { fire: true }") => ({
name: "condition-job",
enabled: true,
schedule: { kind: "every" as const, everyMs: 60_000 },
sessionTarget: "main" as const,
wakeMode: "now" as const,
payload: { kind: "systemEvent" as const, text: "changed" },
trigger: { script },
});
it.each([
["creation", "create"],
["replacement", "patch"],
["declarative convergence", "declarative"],
] as const)("rejects malformed trigger scripts on %s", (_name, mutation) => {
const state = createMockState(now, { scriptPayloadsEnabled: true });
const mutate = (script: string) => {
if (mutation === "create") {
createJob(state, input(script));
return;
}
const job = createJob(state, input());
if (mutation === "patch") {
applyJobPatch(job, { trigger: { script } });
return;
}
applyDeclarativeJobSpec(job, input(script), {
enabledExplicit: true,
nowMs: now,
});
};
expect(() => mutate(malformedScript)).toThrow(
"cron trigger script has a syntax error: Unexpected token (line 1, column 10)",
);
expect(() => mutate(" ")).toThrow("cron trigger script must not be empty");
});
it.each([
["top-level await", "await tools.wait(1); return { fire: true }"],
["top-level return", "return { fire: true }"],
])("accepts %s in trigger scripts", (_name, script) => {
expect(() => createJob(createMockState(now), input(script))).not.toThrow();
});
it.each([
["rename", { name: "renamed condition" }],
["disable", { enabled: false }],
["clear", { trigger: null }],
] as const)("allows %s for legacy malformed triggers while triggers are disabled", (_, patch) => {
const job = createJob(createMockState(now), input());
job.trigger = { script: malformedScript };
applyJobPatch(job, patch, { cronConfig: { triggers: { enabled: false } } });
if (!("trigger" in patch)) {
expect(job).toMatchObject(patch);
}
expect(job.trigger).toEqual("trigger" in patch ? undefined : { script: malformedScript });
});
it("replaces a legacy malformed trigger with a valid script", () => {
const job = createJob(createMockState(now), input());
job.trigger = { script: malformedScript };
applyJobPatch(job, { trigger: { script: "return { fire: false }" } });
expect(job.trigger).toEqual({ script: "return { fire: false }" });
});
it("clears a legacy malformed trigger when a disabled declaration omits it", () => {
const job = createJob(createMockState(now), input());
job.trigger = { script: malformedScript };
const { trigger: _trigger, ...declaration } = input();
expect(() =>
applyDeclarativeJobSpec(job, declaration, {
enabledExplicit: false,
nowMs: now,
cronConfig: { triggers: { enabled: false } },
}),
).not.toThrow();
expect(job.trigger).toBeUndefined();
});
});
describe("script payload validation", () => {
const now = Date.parse("2026-07-18T12:00:00.000Z");
const input = (
+10 -8
View File
@@ -356,13 +356,13 @@ describe("cron trigger evaluation", () => {
}),
});
try {
const originalTrigger = { script: "original trigger", once: true };
const originalTrigger = { script: 'return "original"', once: true };
const job = await harness.cron.add(watcher({ trigger: originalTrigger }));
const run = runWhenDue(harness.cron, job.id);
await started.promise;
await harness.cron.update(job.id, {
trigger: { script: "replacement trigger", once: true },
trigger: { script: 'return "replacement"', once: true },
state: { triggerState: { owner: "latest edit" } },
});
if (restore) {
@@ -374,7 +374,7 @@ describe("cron trigger evaluation", () => {
const stored = harness.cron.getJob(job.id);
expect(stored?.enabled).toBe(true);
expect(stored?.trigger).toEqual(
restore ? originalTrigger : { script: "replacement trigger", once: true },
restore ? originalTrigger : { script: 'return "replacement"', once: true },
);
expect(stored?.state.triggerState).toEqual(restore ? undefined : { owner: "latest edit" });
expect(stored?.state.lastTriggerEvalAtMs).toBeUndefined();
@@ -406,7 +406,7 @@ describe("cron trigger evaluation", () => {
const originalState = { owner: "original" };
const job = await harness.cron.add(
watcher({
trigger: { script: "unchanged trigger", once: true },
trigger: { script: 'return "unchanged"', once: true },
state: { triggerState: originalState },
}),
);
@@ -422,7 +422,7 @@ describe("cron trigger evaluation", () => {
const stored = harness.cron.getJob(job.id);
expect(stored?.enabled).toBe(true);
expect(stored?.trigger).toEqual({ script: "unchanged trigger", once: true });
expect(stored?.trigger).toEqual({ script: 'return "unchanged"', once: true });
expect(stored?.state.triggerState).toEqual(
restore ? originalState : { owner: "latest edit" },
);
@@ -508,19 +508,21 @@ describe("cron trigger evaluation", () => {
});
const harness = await createHarness({ evaluateCronTrigger });
try {
const job = await harness.cron.add(watcher({ trigger: { script: "old trigger" } }));
const job = await harness.cron.add(watcher({ trigger: { script: 'return "old"' } }));
const run = runWhenDue(harness.cron, job.id);
await started.promise;
await harness.cron.update(job.id, {
...(replace ? { trigger: { script: "replacement trigger" } } : {}),
...(replace ? { trigger: { script: 'return "replacement"' } } : {}),
state: { triggerState: { owner: "latest edit" } },
});
evaluation.resolve({ kind: "evaluated", fire: false, state: { owner: "obsolete" } });
expect(await run).toEqual({ ok: true, ran: true });
const stored = harness.cron.getJob(job.id);
expect(stored?.trigger).toEqual({ script: replace ? "replacement trigger" : "old trigger" });
expect(stored?.trigger).toEqual({
script: replace ? 'return "replacement"' : 'return "old"',
});
expect(stored?.state.triggerState).toEqual({ owner: "latest edit" });
expect(stored?.state.lastTriggerEvalAtMs).toBeUndefined();
expect(stored?.state.nextRunAtMs).toEqual(expect.any(Number));
+2 -2
View File
@@ -11,7 +11,7 @@ describe("cron trigger enablement", () => {
expect(() =>
assertTriggerSupport(triggerJob, {
cronConfig: {},
requireEnabled: true,
validateAuthoredTrigger: true,
}),
).not.toThrow();
});
@@ -20,7 +20,7 @@ describe("cron trigger enablement", () => {
expect(() =>
assertTriggerSupport(triggerJob, {
cronConfig: { triggers: { enabled: false } },
requireEnabled: true,
validateAuthoredTrigger: true,
}),
).toThrow(
"cron triggers are disabled because the operator set cron.triggers.enabled: false; remove it or set it to true",
+20 -11
View File
@@ -11,6 +11,18 @@ import { assertSafeCronSessionTargetId } from "../session-target.js";
import type { CronDelivery, CronJob, CronJobPatch } from "../types.js";
import { normalizeHttpWebhookUrl } from "../webhook-url.js";
function assertCronScriptSyntax(script: string, subject: "script payload" | "trigger script") {
if (!script.trim()) {
throw new Error(`cron ${subject} must not be empty`);
}
const parsed = parseCodeModeScriptSyntax(script);
if (!parsed.ok) {
throw new Error(
`cron ${subject} has a syntax error: ${parsed.message} (line ${parsed.line}, column ${parsed.column})`,
);
}
}
/** Validates that session target and payload kind form a supported cron job shape. */
export function assertSupportedJobSpec(
job: Pick<CronJob, "schedule" | "sessionTarget" | "payload">,
@@ -61,16 +73,10 @@ export function assertScriptPayloadSupport(
if (job.payload.kind !== "script") {
return;
}
if (!job.payload.script.trim()) {
throw new Error("cron script payload must not be empty");
}
if (opts?.validateSyntax !== false) {
const parsed = parseCodeModeScriptSyntax(job.payload.script);
if (!parsed.ok) {
throw new Error(
`cron script payload has a syntax error: ${parsed.message} (line ${parsed.line}, column ${parsed.column})`,
);
}
assertCronScriptSyntax(job.payload.script, "script payload");
} else if (!job.payload.script.trim()) {
throw new Error("cron script payload must not be empty");
}
if (job.trigger) {
// Both script kinds expose trigger.state, so composing them would give one
@@ -86,12 +92,12 @@ export function assertScriptPayloadSupport(
export function assertTriggerSupport(
job: Pick<CronJob, "schedule" | "trigger">,
opts?: { cronConfig?: CronConfig; requireEnabled?: boolean },
opts?: { cronConfig?: CronConfig; validateAuthoredTrigger?: boolean },
) {
if (!job.trigger) {
return;
}
if (opts?.requireEnabled && opts.cronConfig?.triggers?.enabled === false) {
if (opts?.validateAuthoredTrigger && opts.cronConfig?.triggers?.enabled === false) {
throw new Error(
"cron triggers are disabled because the operator set cron.triggers.enabled: false; remove it or set it to true",
);
@@ -107,6 +113,9 @@ export function assertTriggerSupport(
if (job.schedule.kind === "every" && job.schedule.everyMs < minIntervalMs) {
throw new Error(`cron trigger every interval must be at least ${minIntervalMs}ms`);
}
if (opts?.validateAuthoredTrigger) {
assertCronScriptSyntax(job.trigger.script, "trigger script");
}
}
export function assertPacingSupport(job: Pick<CronJob, "schedule" | "pacing">) {
+4 -1
View File
@@ -169,7 +169,10 @@ function validateFullJob(
context.patch.enabled === true ||
context.patch.schedule?.kind === "stream";
const validateCapabilities = () => {
assertTriggerSupport(job, { cronConfig, requireEnabled: triggerTouched });
assertTriggerSupport(job, {
cronConfig,
validateAuthoredTrigger: triggerTouched,
});
assertScriptPayloadSupport(job, {
cronConfig,
requireEnabled: scriptTouched,
@@ -4,6 +4,7 @@ import { isCronInvalidRequestError } from "./cron-error-classification.js";
describe("isCronInvalidRequestError", () => {
it.each([
"cron script payload has a syntax error: Unexpected token (line 1, column 10)",
"cron trigger script has a syntax error: Unexpected token (line 1, column 10)",
"cron script payload must not be empty",
"cron script payloads cannot be combined with a condition trigger",
"cron script payloads are disabled because the operator set cron.triggers.enabled: false; remove it or set it to true to allow unattended scripts",
@@ -11,9 +12,12 @@ describe("isCronInvalidRequestError", () => {
expect(isCronInvalidRequestError(new Error(message))).toBe(true);
});
it("does not classify unrelated script runtime failures", () => {
expect(isCronInvalidRequestError(new Error("cron script payload runtime failed"))).toBe(false);
});
it.each(["cron script payload runtime failed", "cron trigger runtime failed"])(
"does not classify unrelated script runtime failures: %s",
(message) => {
expect(isCronInvalidRequestError(new Error(message))).toBe(false);
},
);
it("classifies ambiguous announce delivery validation", () => {
expect(
@@ -10,6 +10,7 @@ export function isCronInvalidRequestError(err: unknown): boolean {
message.includes("cron displayName") ||
message.includes("cron announce delivery requires an explicit channel") ||
message.includes("cron script payload has a syntax error") ||
message.includes("cron trigger script has a syntax error") ||
message.includes("cron script payload must not be empty") ||
message.includes("cron script payloads cannot be combined") ||
message.includes("cron script payloads are disabled") ||
+44 -1
View File
@@ -723,7 +723,7 @@ describe("gateway server cron", () => {
}
});
test("returns INVALID_REQUEST for malformed cron scripts", async () => {
test("rejects malformed cron payload and trigger scripts before persistence", async () => {
const { prevSkipCron } = await setupCronTestRun({
tempPrefix: "openclaw-gw-cron-script-syntax-",
cronEnabled: true,
@@ -745,6 +745,49 @@ describe("gateway server cron", () => {
expect(response.error?.message).toContain(
"cron script payload has a syntax error: Unexpected token (line 1, column 10)",
);
const triggerInput = {
name: "condition watcher",
enabled: true,
schedule: { kind: "every", everyMs: 60_000 },
sessionTarget: "main",
wakeMode: "now",
payload: { kind: "systemEvent", text: "changed" },
};
const invalidTrigger = { script: "const x = ;" };
const expectedTriggerError =
"cron trigger script has a syntax error: Unexpected token (line 1, column 10)";
const invalidCreate = await directCronReq(cronState, "cron.add", {
...triggerInput,
trigger: invalidTrigger,
});
expect(invalidCreate.ok).toBe(false);
expect(invalidCreate.error).toMatchObject({
code: "INVALID_REQUEST",
message: expect.stringContaining(expectedTriggerError),
});
expect((await loadCronStore(cronState.storePath)).jobs).toEqual([]);
const validTrigger = { script: "return { fire: true }" };
const created = await directCronReq(cronState, "cron.add", {
...triggerInput,
trigger: validTrigger,
});
const jobId = expectCronJobIdFromResponse(created);
const invalidUpdate = await directCronReq(cronState, "cron.update", {
id: jobId,
patch: { trigger: invalidTrigger },
});
expect(invalidUpdate.ok).toBe(false);
expect(invalidUpdate.error).toMatchObject({
code: "INVALID_REQUEST",
message: expect.stringContaining(expectedTriggerError),
});
expect((await loadCronStore(cronState.storePath)).jobs).toEqual([
expect.objectContaining({ id: jobId, trigger: validTrigger }),
]);
} finally {
await cleanupCronTestRun({ cronState, prevSkipCron });
}
+70 -18
View File
@@ -43,6 +43,27 @@ function listResponse(jobs: unknown[]) {
};
}
function cronMethodResponses(jobs: unknown[]) {
return {
"cron.add": { id: "new-automation" },
"cron.list": {
cases: [
{ match: { lastRunStatus: "error" }, response: listResponse([]) },
{ response: listResponse(jobs) },
],
},
"cron.runs": {
entries: [],
total: 0,
offset: 0,
limit: 50,
hasMore: false,
nextOffset: null,
},
"cron.status": { enabled: true, triggersEnabled: true, jobs: jobs.length, nextWakeAtMs: null },
};
}
async function captureProof(page: Page, name: string) {
if (!proofDirectory) {
return;
@@ -76,24 +97,7 @@ suite.define(() => {
{ locale: "en-US", serviceWorkers: "block", viewport: { height: 1_050, width: 1_440 } },
async ({ page }) => {
const gateway = await installMockGateway(page, {
methodResponses: {
"cron.add": { id: "new-automation" },
"cron.list": {
cases: [
{ match: { lastRunStatus: "error" }, response: listResponse([]) },
{ response: listResponse([scriptJob]) },
],
},
"cron.runs": {
entries: [],
total: 0,
offset: 0,
limit: 50,
hasMore: false,
nextOffset: null,
},
"cron.status": { enabled: true, triggersEnabled: true, jobs: 1, nextWakeAtMs: null },
},
methodResponses: cronMethodResponses([scriptJob]),
});
await page.goto(`${suite.server.baseUrl}cron`);
@@ -285,4 +289,52 @@ suite.define(() => {
},
);
});
it("keeps a rejected trigger draft visible without reloading inventory", async () => {
await suite.withPage(
{ locale: "en-US", serviceWorkers: "block", viewport: { height: 1_050, width: 1_440 } },
async ({ page }) => {
const gateway = await installMockGateway(page, {
methodResponses: cronMethodResponses([scriptJob]),
});
await page.goto(`${suite.server.baseUrl}cron`);
const existingRow = page.locator('[data-test-id="cron-row-existing-script-automation"]');
await existingRow.waitFor();
await page.locator('[data-test-id="cron-new-task"]').click();
await page.locator("#cron-name").fill("Malformed condition");
await page.locator("#cron-payload-text").fill("Run when the condition matches");
await page.locator("details.cron-advanced > summary").click();
await page
.locator(".settings-row--toggle")
.filter({ hasText: "Condition trigger" })
.click();
const triggerScript = page.locator("#cron-trigger-script");
await triggerScript.fill("const x = ;");
const listsBeforeSave = (await gateway.getRequests("cron.list")).length;
const submit = page.locator('[data-test-id="cron-submit"]');
await gateway.deferNext("cron.add");
await submit.click();
const request = await gateway.waitForRequest("cron.add");
expect(request.params).toMatchObject({
name: "Malformed condition",
trigger: { script: "const x = ;", once: false },
});
const message = "Condition script is invalid";
await gateway.rejectDeferred("cron.add", { code: "INVALID_REQUEST", message });
const errorBanner = page.locator(".cron-error-banner");
await errorBanner.waitFor({ state: "visible" });
expect(await errorBanner.textContent()).toContain(message);
expect(await triggerScript.inputValue()).toBe("const x = ;");
expect(await gateway.getRequests("cron.list")).toHaveLength(listsBeforeSave);
await errorBanner.scrollIntoViewIfNeeded();
await captureProof(page, "05-malformed-trigger-rejected");
await page.locator('[data-test-id="cron-back"]').click();
await existingRow.waitFor();
expect(await gateway.getRequests("cron.list")).toHaveLength(listsBeforeSave);
},
);
});
});
+41 -1
View File
@@ -142,7 +142,7 @@ function createCronSubmitHarness(
const result = await addCronJob(state);
return { call: findRequestCall(request.mock.calls, method), result };
};
return { state, submit };
return { request, state, submit };
}
function createCronEditHarness(job: CronJob) {
@@ -1331,6 +1331,46 @@ describe("cron controller", () => {
});
});
it.each(["cron.add", "cron.update"] as const)(
"preserves the trigger draft and inventory when %s rejects its syntax",
async (method) => {
const existingJob = createCronJob({
id: "job-condition-syntax",
name: "Conditional job",
trigger: { script: "return { fire: true }" },
});
const { request, state } = createCronSubmitHarness(existingJob.id, {
method,
jobs: [existingJob],
state: { cronCreateOpen: method === "cron.add" },
form: {
name: "Conditional job",
scheduleKind: "every",
everyAmount: "1",
everyUnit: "minutes",
payloadText: "run",
triggerEnabled: true,
triggerScript: "const x = ;",
},
});
const originalInventory = structuredClone(state.cronJobs);
const message =
"cron trigger script has a syntax error: Unexpected token (line 1, column 10)";
request.mockRejectedValue(new Error(message));
await expect(addCronJob(state)).resolves.toEqual({ saved: false });
expect(state.cronError).toBe(message);
expect(state.cronForm.triggerScript).toBe("const x = ;");
expect(state.cronJobs).toEqual(originalInventory);
expect(state.cronCreateOpen).toBe(method === "cron.add");
if (method === "cron.update") {
expect(state.cronEditingJobId).toBe(existingJob.id);
expect(state.cronEditingJob?.trigger).toEqual(existingJob.trigger);
}
},
);
it("clears an existing condition trigger from cron.update", async () => {
const job = createCronJob({
id: "job-clear-trigger",