From 2664f5951916f374907d693132f5315207f62fb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=85=B0=E4=B9=8B?= Date: Tue, 2 Jun 2026 18:10:19 +0800 Subject: [PATCH] fix(cron): reject blank delivery targets Reject whitespace-only cron delivery target strings before cron input normalization can trim and drop them, so bad delivery targets return INVALID_REQUEST instead of behaving as omitted fields. Keep explicit null update clears for delivery, failure destination, and completion destination fields. Co-authored-by: gaozixiang1 Co-authored-by: Lanzhi --- .../src/cron-validators.test.ts | 50 +++++++++++ packages/gateway-protocol/src/schema/cron.ts | 28 +++--- src/agents/tools/cron-tool.test.ts | 77 ++++++++++++++++ src/agents/tools/cron-tool.ts | 3 + src/cron/delivery-target-validation.ts | 36 ++++++++ src/gateway/server-methods/cron.ts | 10 ++- .../server-methods/cron.validation.test.ts | 89 +++++++++++++++++++ 7 files changed, 279 insertions(+), 14 deletions(-) create mode 100644 src/cron/delivery-target-validation.ts diff --git a/packages/gateway-protocol/src/cron-validators.test.ts b/packages/gateway-protocol/src/cron-validators.test.ts index db08ea1f3f0d..6d21296c863c 100644 --- a/packages/gateway-protocol/src/cron-validators.test.ts +++ b/packages/gateway-protocol/src/cron-validators.test.ts @@ -132,6 +132,56 @@ describe("cron protocol validators", () => { ).toBe(true); }); + it("rejects blank cron delivery target strings", () => { + expect( + validateCronAddParams({ + ...minimalAddParams, + delivery: { + mode: "announce", + channel: "telegram", + to: " ", + }, + }), + ).toBe(false); + + expect( + validateCronUpdateParams({ + id: "job-1", + patch: { + delivery: { + channel: "\t", + }, + }, + }), + ).toBe(false); + + expect( + validateCronUpdateParams({ + id: "job-1", + patch: { + delivery: { + failureDestination: { + channel: null, + to: " ", + }, + }, + }, + }), + ).toBe(false); + + expect( + validateCronUpdateParams({ + id: "job-1", + patch: { + failureAlert: { + channel: "last", + to: "\n\t", + }, + }, + }), + ).toBe(false); + }); + it("accepts remove params for id and jobId selectors", () => { expect(validateCronRemoveParams({ id: "job-1" })).toBe(true); expect(validateCronRemoveParams({ jobId: "job-2" })).toBe(true); diff --git a/packages/gateway-protocol/src/schema/cron.ts b/packages/gateway-protocol/src/schema/cron.ts index aa8c6da02294..0c0096aa681e 100644 --- a/packages/gateway-protocol/src/schema/cron.ts +++ b/packages/gateway-protocol/src/schema/cron.ts @@ -75,6 +75,8 @@ const CronDeliveryStatusSchema = Type.Union([ Type.Literal("unknown"), Type.Literal("not-requested"), ]); +const NonBlankString = Type.String({ minLength: 1, pattern: "\\S" }); +const CronAnnounceChannelSchema = Type.Union([Type.Literal("last"), NonBlankString]); const CronFailoverReasonSchema = Type.Union([ Type.Literal("auth"), Type.Literal("auth_permanent"), @@ -215,8 +217,8 @@ export const CronPayloadPatchSchema = Type.Union([ export const CronFailureAlertSchema = Type.Object( { after: Type.Optional(Type.Integer({ minimum: 1 })), - channel: Type.Optional(Type.Union([Type.Literal("last"), NonEmptyString])), - to: Type.Optional(Type.String()), + channel: Type.Optional(CronAnnounceChannelSchema), + to: Type.Optional(NonBlankString), cooldownMs: Type.Optional(Type.Integer({ minimum: 0 })), includeSkipped: Type.Optional(Type.Boolean()), mode: Type.Optional(Type.Union([Type.Literal("announce"), Type.Literal("webhook")])), @@ -227,8 +229,8 @@ export const CronFailureAlertSchema = Type.Object( export const CronFailureDestinationSchema = Type.Object( { - channel: Type.Optional(Type.Union([Type.Literal("last"), NonEmptyString])), - to: Type.Optional(Type.String()), + channel: Type.Optional(CronAnnounceChannelSchema), + to: Type.Optional(NonBlankString), accountId: Type.Optional(NonEmptyString), mode: Type.Optional(Type.Union([Type.Literal("announce"), Type.Literal("webhook")])), }, @@ -237,8 +239,8 @@ export const CronFailureDestinationSchema = Type.Object( const CronFailureDestinationPatchSchema = Type.Object( { - channel: Type.Optional(Type.Union([Type.Literal("last"), NonEmptyString, Type.Null()])), - to: Type.Optional(Type.Union([Type.String(), Type.Null()])), + channel: Type.Optional(Type.Union([CronAnnounceChannelSchema, Type.Null()])), + to: Type.Optional(Type.Union([NonBlankString, Type.Null()])), accountId: Type.Optional(Type.Union([NonEmptyString, Type.Null()])), mode: Type.Optional( Type.Union([Type.Literal("announce"), Type.Literal("webhook"), Type.Null()]), @@ -250,13 +252,13 @@ const CronFailureDestinationPatchSchema = Type.Object( export const CronCompletionDestinationSchema = Type.Object( { mode: Type.Literal("webhook"), - to: NonEmptyString, + to: NonBlankString, }, { additionalProperties: false }, ); const CronDeliverySharedProperties = { - channel: Type.Optional(Type.Union([Type.Literal("last"), NonEmptyString])), + channel: Type.Optional(CronAnnounceChannelSchema), threadId: Type.Optional(Type.Union([Type.String(), Type.Number()])), accountId: Type.Optional(NonEmptyString), bestEffort: Type.Optional(Type.Boolean()), @@ -264,7 +266,7 @@ const CronDeliverySharedProperties = { }; const CronDeliveryPatchSharedProperties = { - channel: Type.Optional(Type.Union([Type.Literal("last"), NonEmptyString, Type.Null()])), + channel: Type.Optional(Type.Union([CronAnnounceChannelSchema, Type.Null()])), threadId: Type.Optional(Type.Union([Type.String(), Type.Number(), Type.Null()])), accountId: Type.Optional(Type.Union([NonEmptyString, Type.Null()])), bestEffort: Type.Optional(Type.Boolean()), @@ -275,7 +277,7 @@ const CronDeliveryNoopSchema = Type.Object( { mode: Type.Literal("none"), ...CronDeliverySharedProperties, - to: Type.Optional(Type.String()), + to: Type.Optional(NonBlankString), }, { additionalProperties: false }, ); @@ -285,7 +287,7 @@ const CronDeliveryAnnounceSchema = Type.Object( mode: Type.Literal("announce"), ...CronDeliverySharedProperties, completionDestination: Type.Optional(CronCompletionDestinationSchema), - to: Type.Optional(Type.String()), + to: Type.Optional(NonBlankString), }, { additionalProperties: false }, ); @@ -294,7 +296,7 @@ const CronDeliveryWebhookSchema = Type.Object( { mode: Type.Literal("webhook"), ...CronDeliverySharedProperties, - to: NonEmptyString, + to: NonBlankString, }, { additionalProperties: false }, ); @@ -314,7 +316,7 @@ export const CronDeliveryPatchSchema = Type.Object( completionDestination: Type.Optional( Type.Union([CronCompletionDestinationSchema, Type.Null()]), ), - to: Type.Optional(Type.Union([Type.String(), Type.Null()])), + to: Type.Optional(Type.Union([NonBlankString, Type.Null()])), }, { additionalProperties: false }, ); diff --git a/src/agents/tools/cron-tool.test.ts b/src/agents/tools/cron-tool.test.ts index 845a94b72461..a992aae85ccb 100644 --- a/src/agents/tools/cron-tool.test.ts +++ b/src/agents/tools/cron-tool.test.ts @@ -633,6 +633,34 @@ describe("cron tool", () => { expect(params?.failureAlert).toBe(false); }); + it.each([ + ["delivery.channel", { channel: " ", to: "chat-1" }], + ["delivery.to", { mode: "announce", channel: "telegram", to: " \t" }], + [ + "delivery.failureDestination.to", + { mode: "announce", failureDestination: { mode: "announce", to: " " } }, + ], + [ + "delivery.completionDestination.to", + { mode: "announce", completionDestination: { mode: "webhook", to: "\n" } }, + ], + ])("rejects blank cron.add %s before gateway normalization", async (field, delivery) => { + const tool = createTestCronTool(); + + await expect( + tool.execute("call-blank-delivery-add", { + action: "add", + job: { + name: "reminder", + schedule: { at: new Date(123).toISOString() }, + payload: { kind: "agentTurn", message: "hello" }, + delivery, + }, + }), + ).rejects.toThrow(`${field} must be a non-empty string`); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + it("recovers flattened add params for failureAlert and payload extras", async () => { const tool = createTestCronTool(); await tool.execute("call-flat-add-extras", { @@ -1336,6 +1364,55 @@ describe("cron tool", () => { expect(params?.patch?.enabled).toBe(false); }); + it.each([ + ["delivery.channel", { channel: " " }], + ["delivery.to", { to: " " }], + ["delivery.failureDestination.to", { failureDestination: { to: " " } }], + ["delivery.completionDestination.to", { completionDestination: { mode: "webhook", to: " " } }], + ])("rejects blank cron.update %s before gateway normalization", async (field, delivery) => { + const tool = createTestCronTool(); + + await expect( + tool.execute("call-blank-delivery-update", { + action: "update", + id: "job-blank-delivery", + patch: { delivery }, + }), + ).rejects.toThrow(`${field} must be a non-empty string`); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + + it("passes nullable cron.update delivery clears through to the gateway", async () => { + const tool = createTestCronTool(); + await tool.execute("call-null-delivery-update", { + action: "update", + id: "job-clear-delivery", + patch: { + delivery: { + channel: null, + to: null, + failureDestination: null, + completionDestination: null, + }, + }, + }); + + const params = expectSingleGatewayCallMethod("cron.update") as + | { id?: string; patch?: { delivery?: unknown } } + | undefined; + expect(params).toEqual({ + id: "job-clear-delivery", + patch: { + delivery: { + channel: null, + to: null, + failureDestination: null, + completionDestination: null, + }, + }, + }); + }); + it("recovers additional flat patch params for update action", async () => { callGatewayMock.mockResolvedValueOnce({ ok: true }); diff --git a/src/agents/tools/cron-tool.ts b/src/agents/tools/cron-tool.ts index 5db0a763d9d9..c846467953b0 100644 --- a/src/agents/tools/cron-tool.ts +++ b/src/agents/tools/cron-tool.ts @@ -2,6 +2,7 @@ import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/st import { Type, type TSchema } from "typebox"; import { getRuntimeConfig } from "../../config/config.js"; import { resolveCronCreationDelivery } from "../../cron/delivery-context.js"; +import { assertCronDeliveryInputNonBlankFields } from "../../cron/delivery-target-validation.js"; import { normalizeCronJobCreate, normalizeCronJobPatch } from "../../cron/normalize.js"; import type { CronDelivery } from "../../cron/types.js"; import { normalizeHttpWebhookUrl } from "../../cron/webhook-url.js"; @@ -651,6 +652,7 @@ Use jobId canonical; id accepted compat. contextMessages (0-10) adds previous me throw new Error("job required"); } const canonicalJob = canonicalizeCronToolObject(params.job as Record); + assertCronDeliveryInputNonBlankFields(canonicalJob.delivery); const job = normalizeCronJobCreate(canonicalJob, { sessionContext: { sessionKey: opts?.agentSessionKey }, @@ -767,6 +769,7 @@ Use jobId canonical; id accepted compat. contextMessages (0-10) adds previous me const canonicalPatch = canonicalizeCronToolObject( params.patch as Record, ); + assertCronDeliveryInputNonBlankFields(canonicalPatch.delivery); const patch = normalizeCronJobPatch(canonicalPatch) ?? canonicalPatch; if (recoveredFlatPatch && isEmptyRecoveredCronPatch(patch)) { throw new Error("patch required"); diff --git a/src/cron/delivery-target-validation.ts b/src/cron/delivery-target-validation.ts new file mode 100644 index 000000000000..2a769baa0219 --- /dev/null +++ b/src/cron/delivery-target-validation.ts @@ -0,0 +1,36 @@ +function assertNonBlankStringField(field: string, value: unknown) { + if (value === undefined || value === null || typeof value !== "string") { + return; + } + if (value.trim()) { + return; + } + throw new Error(`${field} must be a non-empty string`); +} + +export function assertCronDeliveryInputNonBlankFields(delivery: unknown, fieldPrefix = "delivery") { + if (!delivery || typeof delivery !== "object") { + return; + } + const deliveryRecord = delivery as { + channel?: unknown; + to?: unknown; + failureDestination?: unknown; + completionDestination?: unknown; + }; + assertNonBlankStringField(`${fieldPrefix}.channel`, deliveryRecord.channel); + assertNonBlankStringField(`${fieldPrefix}.to`, deliveryRecord.to); + + const failureDestination = deliveryRecord.failureDestination; + if (failureDestination && typeof failureDestination === "object") { + const failureRecord = failureDestination as { channel?: unknown; to?: unknown }; + assertNonBlankStringField(`${fieldPrefix}.failureDestination.channel`, failureRecord.channel); + assertNonBlankStringField(`${fieldPrefix}.failureDestination.to`, failureRecord.to); + } + + const completionDestination = deliveryRecord.completionDestination; + if (completionDestination && typeof completionDestination === "object") { + const completionRecord = completionDestination as { to?: unknown }; + assertNonBlankStringField(`${fieldPrefix}.completionDestination.to`, completionRecord.to); + } +} diff --git a/src/gateway/server-methods/cron.ts b/src/gateway/server-methods/cron.ts index 0122569767dd..efe0a05cd917 100644 --- a/src/gateway/server-methods/cron.ts +++ b/src/gateway/server-methods/cron.ts @@ -14,6 +14,7 @@ import { } from "../../../packages/gateway-protocol/src/index.js"; import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { resolveCronDeliveryPreviews } from "../../cron/delivery-preview.js"; +import { assertCronDeliveryInputNonBlankFields } from "../../cron/delivery-target-validation.js"; import { normalizeCronJobCreate, normalizeCronJobPatch } from "../../cron/normalize.js"; import { isInvalidCronRunLogJobIdError, @@ -364,6 +365,7 @@ export const cronHandlers: GatewayRequestHandlers = { : undefined; let normalized: unknown; try { + assertCronDeliveryInputNonBlankFields((params as { delivery?: unknown } | null)?.delivery); normalized = normalizeCronJobCreate(params, { sessionContext: { sessionKey }, @@ -441,7 +443,13 @@ export const cronHandlers: GatewayRequestHandlers = { "cron.update": async ({ params, respond, context }) => { let normalizedPatch: ReturnType; try { - normalizedPatch = normalizeCronJobPatch((params as { patch?: unknown } | null)?.patch); + const rawPatch = (params as { patch?: unknown } | null)?.patch; + assertCronDeliveryInputNonBlankFields( + rawPatch && typeof rawPatch === "object" + ? (rawPatch as { delivery?: unknown }).delivery + : undefined, + ); + normalizedPatch = normalizeCronJobPatch(rawPatch); } catch (err) { respond( false, diff --git a/src/gateway/server-methods/cron.validation.test.ts b/src/gateway/server-methods/cron.validation.test.ts index 56ba6c1aa34c..a0fcbf4a0e70 100644 --- a/src/gateway/server-methods/cron.validation.test.ts +++ b/src/gateway/server-methods/cron.validation.test.ts @@ -507,6 +507,48 @@ describe("cron method validation", () => { expectCronSuccess(respond); }); + it("rejects blank announce delivery fields before normalization", async () => { + const { context, respond } = await invokeCronAdd( + agentTurnCronParams({ + name: "blank delivery target", + delivery: { + mode: "announce", + channel: "telegram", + to: " ", + }, + }), + ); + + expect(context.cron.add).not.toHaveBeenCalled(); + expectResponseError(respond, { + code: "INVALID_REQUEST", + messageIncludes: "delivery.to must be a non-empty string", + }); + }); + + it("rejects blank failure destination fields before normalization", async () => { + const { context, respond } = await invokeCronAdd( + agentTurnCronParams({ + name: "blank failure target", + delivery: { + mode: "announce", + channel: "telegram", + to: "telegram:123", + failureDestination: { + mode: "announce", + channel: " ", + }, + }, + }), + ); + + expect(context.cron.add).not.toHaveBeenCalled(); + expectResponseError(respond, { + code: "INVALID_REQUEST", + messageIncludes: "delivery.failureDestination.channel must be a non-empty string", + }); + }); + it("rejects announce targets prefixed for a different explicit delivery channel", async () => { setRuntimeConfig(telegramSlackConfig()); @@ -602,6 +644,53 @@ describe("cron method validation", () => { expect(clearDelivery.completionDestination).toBeNull(); }); + it("rejects blank delivery target patches before normalization", async () => { + const { context, respond } = await invokeCronUpdate( + { + id: "cron-1", + patch: { + delivery: { + to: "\t", + }, + }, + }, + createCronJob({ + delivery: { mode: "announce", channel: "telegram", to: "telegram:123" }, + }), + ); + + expect(context.cron.update).not.toHaveBeenCalled(); + expectResponseError(respond, { + code: "INVALID_REQUEST", + messageIncludes: "delivery.to must be a non-empty string", + }); + }); + + it("rejects blank completion destination patches before normalization", async () => { + const { context, respond } = await invokeCronUpdate( + { + id: "cron-1", + patch: { + delivery: { + completionDestination: { + mode: "webhook", + to: " ", + }, + }, + }, + }, + createCronJob({ + delivery: { mode: "announce" }, + }), + ); + + expect(context.cron.update).not.toHaveBeenCalled(); + expectResponseError(respond, { + code: "INVALID_REQUEST", + messageIncludes: "delivery.completionDestination.to must be a non-empty string", + }); + }); + it("accepts nullable delivery target clears on update", async () => { const { context, respond } = await invokeCronUpdate( {