From bed80ee645595247aa3fda0f2af587578a6482af Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 20 Aug 2026 22:42:06 -0700 Subject: [PATCH] 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 --- src/cron/service.declarative-jobs.test.ts | 28 ++++++ src/cron/service.jobs.test.ts | 88 +++++++++++++++++++ src/cron/service.trigger-eval.test.ts | 18 ++-- src/cron/service/jobs-validation.test.ts | 4 +- src/cron/service/jobs-validation.ts | 31 ++++--- src/cron/service/jobs.ts | 5 +- .../cron-error-classification.test.ts | 10 ++- .../cron-error-classification.ts | 1 + src/gateway/server.cron.test.ts | 45 +++++++++- ui/src/e2e/cron-trigger-authoring.e2e.test.ts | 88 +++++++++++++++---- ui/src/lib/cron/index.test.ts | 42 ++++++++- 11 files changed, 315 insertions(+), 45 deletions(-) diff --git a/src/cron/service.declarative-jobs.test.ts b/src/cron/service.declarative-jobs.test.ts index 82ef5015312e..7d411f9d3e7a 100644 --- a/src/cron/service.declarative-jobs.test.ts +++ b/src/cron/service.declarative-jobs.test.ts @@ -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); diff --git a/src/cron/service.jobs.test.ts b/src/cron/service.jobs.test.ts index e0ab3c27c898..f66671ff8ddc 100644 --- a/src/cron/service.jobs.test.ts +++ b/src/cron/service.jobs.test.ts @@ -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 = ( diff --git a/src/cron/service.trigger-eval.test.ts b/src/cron/service.trigger-eval.test.ts index 301d4fc4e1ad..f70c305afadb 100644 --- a/src/cron/service.trigger-eval.test.ts +++ b/src/cron/service.trigger-eval.test.ts @@ -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)); diff --git a/src/cron/service/jobs-validation.test.ts b/src/cron/service/jobs-validation.test.ts index e4e6aa6a9507..baa6e6cc4b6d 100644 --- a/src/cron/service/jobs-validation.test.ts +++ b/src/cron/service/jobs-validation.test.ts @@ -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", diff --git a/src/cron/service/jobs-validation.ts b/src/cron/service/jobs-validation.ts index d2c069790129..556f5f4fd0c0 100644 --- a/src/cron/service/jobs-validation.ts +++ b/src/cron/service/jobs-validation.ts @@ -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, @@ -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, - 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) { diff --git a/src/cron/service/jobs.ts b/src/cron/service/jobs.ts index daa8279ae683..22c25f38fd03 100644 --- a/src/cron/service/jobs.ts +++ b/src/cron/service/jobs.ts @@ -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, diff --git a/src/gateway/server-methods/cron-error-classification.test.ts b/src/gateway/server-methods/cron-error-classification.test.ts index 4beec525e210..f0cd0cf5d9ba 100644 --- a/src/gateway/server-methods/cron-error-classification.test.ts +++ b/src/gateway/server-methods/cron-error-classification.test.ts @@ -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( diff --git a/src/gateway/server-methods/cron-error-classification.ts b/src/gateway/server-methods/cron-error-classification.ts index 052a23a53de7..908d3395cd6d 100644 --- a/src/gateway/server-methods/cron-error-classification.ts +++ b/src/gateway/server-methods/cron-error-classification.ts @@ -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") || diff --git a/src/gateway/server.cron.test.ts b/src/gateway/server.cron.test.ts index 77a3b91dd58d..d5151ac369fa 100644 --- a/src/gateway/server.cron.test.ts +++ b/src/gateway/server.cron.test.ts @@ -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 }); } diff --git a/ui/src/e2e/cron-trigger-authoring.e2e.test.ts b/ui/src/e2e/cron-trigger-authoring.e2e.test.ts index 972b4573c878..10db025566fa 100644 --- a/ui/src/e2e/cron-trigger-authoring.e2e.test.ts +++ b/ui/src/e2e/cron-trigger-authoring.e2e.test.ts @@ -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); + }, + ); + }); }); diff --git a/ui/src/lib/cron/index.test.ts b/ui/src/lib/cron/index.test.ts index b6ea6c24bad2..7d541e410fe5 100644 --- a/ui/src/lib/cron/index.test.ts +++ b/ui/src/lib/cron/index.test.ts @@ -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",