From 39bcf695dce42dd81f6e6994776d27692e1c13b0 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 4 Apr 2026 20:13:23 +0900 Subject: [PATCH] fix(cron): reject unsafe custom session targets earlier --- CHANGELOG.md | 1 + src/cron/normalize.test.ts | 17 ++++++ src/cron/normalize.ts | 10 ++-- src/cron/service.jobs.test.ts | 28 ++++++++++ src/cron/service/jobs.ts | 4 ++ src/cron/session-target.ts | 12 +++++ src/gateway/server-cron.ts | 6 +-- src/gateway/server-methods/cron.ts | 36 +++++++++++-- src/gateway/server.cron.test.ts | 84 ++++++++++++++++++++++++++++++ 9 files changed, 183 insertions(+), 15 deletions(-) create mode 100644 src/cron/session-target.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 58e5a0055c2..743abd19f46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -188,6 +188,7 @@ Docs: https://docs.openclaw.ai - Browser/profiles: reject remote browser profile `cdpUrl` values that violate strict SSRF policy before saving config, with clearer validation errors for blocked endpoints. (#60477) Thanks @eleqtrizit. - Browser/screenshots: stop sending `fromSurface: false` on CDP screenshots so managed Chrome 146+ browsers can capture images again. (#60682) Thanks @mvanhorn. - Mattermost/slash commands: harden native slash-command callback token validation to use constant-time secret comparison, matching the existing interaction-token path. +- Cron/security: reject unsafe custom `sessionTarget: "session:..."` IDs earlier during cron add, update, and execution so malformed custom session keys fail closed with clear errors. ## 2026.4.1 diff --git a/src/cron/normalize.test.ts b/src/cron/normalize.test.ts index 50b9faac9ad..9caedefcd14 100644 --- a/src/cron/normalize.test.ts +++ b/src/cron/normalize.test.ts @@ -593,6 +593,23 @@ describe("normalizeCronJobCreate", () => { expect(normalized.sessionTarget).toBe("session:MySessionID"); }); + + it("rejects custom session ids with path separators", () => { + expect(() => + normalizeCronJobCreate({ + name: "bad-custom-session", + schedule: { kind: "cron", expr: "* * * * *" }, + sessionTarget: "session:../../outside", + payload: { kind: "agentTurn", message: "hello" }, + }), + ).toThrow("invalid cron sessionTarget session id"); + + expect(() => + normalizeCronJobPatch({ + sessionTarget: "session:..\\outside", + }), + ).toThrow("invalid cron sessionTarget session id"); + }); }); describe("normalizeCronJobPatch", () => { diff --git a/src/cron/normalize.ts b/src/cron/normalize.ts index 49d32224986..75bfb9c89f5 100644 --- a/src/cron/normalize.ts +++ b/src/cron/normalize.ts @@ -8,6 +8,7 @@ import { } from "./delivery-field-schemas.js"; import { parseAbsoluteTimeMs } from "./parse.js"; import { inferLegacyName } from "./service/normalize.js"; +import { assertSafeCronSessionTargetId } from "./session-target.js"; import { normalizeCronStaggerMs, resolveDefaultCronStaggerMs } from "./stagger.js"; import type { CronJobCreate, CronJobPatch } from "./types.js"; @@ -321,10 +322,7 @@ function normalizeSessionTarget(raw: unknown) { } // Support custom session IDs with "session:" prefix if (lower.startsWith("session:")) { - const sessionId = trimmed.slice(8).trim(); - if (sessionId) { - return `session:${sessionId}`; - } + return `session:${assertSafeCronSessionTargetId(trimmed.slice(8))}`; } return undefined; } @@ -539,7 +537,7 @@ export function normalizeCronJobInput( const sessionKey = options.sessionContext.sessionKey.trim(); if (sessionKey) { // Store as session:customId format for persistence - next.sessionTarget = `session:${sessionKey}`; + next.sessionTarget = `session:${assertSafeCronSessionTargetId(sessionKey)}`; } } // If "current" wasn't resolved, fall back to "isolated" behavior @@ -551,7 +549,7 @@ export function normalizeCronJobInput( if (next.sessionTarget === "current") { const sessionKey = options.sessionContext?.sessionKey?.trim(); if (sessionKey) { - next.sessionTarget = `session:${sessionKey}`; + next.sessionTarget = `session:${assertSafeCronSessionTargetId(sessionKey)}`; } else { next.sessionTarget = "isolated"; } diff --git a/src/cron/service.jobs.test.ts b/src/cron/service.jobs.test.ts index 937258be992..eacb6cb1b48 100644 --- a/src/cron/service.jobs.test.ts +++ b/src/cron/service.jobs.test.ts @@ -475,6 +475,20 @@ describe("createJob rejects sessionTarget main for non-default agents", () => { ).not.toThrow(); }); + it("rejects custom session targets with path separators", () => { + const state = createMockState(now, { defaultAgentId: "main" }); + expect(() => + createJob(state, { + name: "bad-custom-session", + enabled: true, + schedule: { kind: "every", everyMs: 60_000 }, + sessionTarget: "session:../../outside", + wakeMode: "now", + payload: { kind: "agentTurn", message: "hello" }, + }), + ).toThrow("invalid cron sessionTarget session id"); + }); + it("rejects failureDestination on main jobs without webhook delivery mode", () => { const state = createMockState(now, { defaultAgentId: "main" }); expect(() => @@ -526,6 +540,20 @@ describe("applyJobPatch rejects sessionTarget main for non-default agents", () = } expect(() => applyJobPatch(job, patch, { defaultAgentId: "main" })).not.toThrow(); }); + + it("rejects patching to a custom session target with path separators", () => { + const job = createMainJob(); + expect(() => + applyJobPatch( + job, + { + sessionTarget: "session:..\\outside", + payload: { kind: "agentTurn", message: "hello" }, + }, + { defaultAgentId: "main" }, + ), + ).toThrow("invalid cron sessionTarget session id"); + }); }); describe("cron stagger defaults", () => { diff --git a/src/cron/service/jobs.ts b/src/cron/service/jobs.ts index 8c9a103730c..3569539dc20 100644 --- a/src/cron/service/jobs.ts +++ b/src/cron/service/jobs.ts @@ -6,6 +6,7 @@ import { computeNextRunAtMs, computePreviousRunAtMs, } from "../schedule.js"; +import { assertSafeCronSessionTargetId } from "../session-target.js"; import { normalizeCronStaggerMs, resolveCronStaggerMs, @@ -136,6 +137,9 @@ export function assertSupportedJobSpec(job: Pick { - const normalizedPatch = normalizeCronJobPatch((params as { patch?: unknown } | null)?.patch); + let normalizedPatch: ReturnType; + try { + normalizedPatch = normalizeCronJobPatch((params as { patch?: unknown } | null)?.patch); + } catch (err) { + respond( + false, + undefined, + errorShape( + ErrorCodes.INVALID_REQUEST, + `invalid cron.update params: ${err instanceof Error ? err.message : String(err)}`, + ), + ); + return; + } const candidate = normalizedPatch && typeof params === "object" && params !== null ? { ...params, patch: normalizedPatch } diff --git a/src/gateway/server.cron.test.ts b/src/gateway/server.cron.test.ts index c3a752b67fe..8742d595588 100644 --- a/src/gateway/server.cron.test.ts +++ b/src/gateway/server.cron.test.ts @@ -462,6 +462,52 @@ describe("gateway server cron", () => { } }); + test("rejects unsafe custom session ids on add and update", async () => { + const { prevSkipCron } = await setupCronTestRun({ + tempPrefix: "openclaw-gw-cron-bad-session-target-", + cronEnabled: false, + }); + + const { server, ws } = await startServerWithClient(); + await connectOk(ws); + + try { + const addRes = await rpcReq(ws, "cron.add", { + name: "bad custom session", + enabled: true, + schedule: { kind: "every", everyMs: 60_000 }, + sessionTarget: "session:../../outside", + wakeMode: "now", + payload: { kind: "agentTurn", message: "hello" }, + }); + expect(addRes.ok).toBe(false); + expect(addRes.error?.message).toContain("invalid cron sessionTarget session id"); + + const validRes = await rpcReq(ws, "cron.add", { + name: "good custom session", + enabled: true, + schedule: { kind: "every", everyMs: 60_000 }, + sessionTarget: "session:project-alpha:ops", + wakeMode: "now", + payload: { kind: "agentTurn", message: "hello" }, + }); + expect(validRes.ok).toBe(true); + const jobId = (validRes.payload as { id?: unknown } | null)?.id; + expect(typeof jobId).toBe("string"); + + const updateRes = await rpcReq(ws, "cron.update", { + id: jobId, + patch: { + sessionTarget: "session:..\\outside", + }, + }); + expect(updateRes.ok).toBe(false); + expect(updateRes.error?.message).toContain("invalid cron sessionTarget session id"); + } finally { + await cleanupCronTestRun({ ws, server, prevSkipCron }); + } + }); + test("writes cron run history and auto-runs due jobs", async () => { const { prevSkipCron } = await setupCronTestRun({ tempPrefix: "openclaw-gw-cron-log-", @@ -559,6 +605,44 @@ describe("gateway server cron", () => { } }, 45_000); + test("fails closed for persisted unsafe custom session ids", async () => { + const now = Date.now(); + const { prevSkipCron } = await setupCronTestRun({ + tempPrefix: "openclaw-gw-cron-persisted-bad-session-target-", + cronEnabled: false, + jobs: [ + { + id: "bad-custom-session-job", + name: "bad custom session job", + enabled: true, + createdAtMs: now, + updatedAtMs: now, + schedule: { kind: "every", everyMs: 60_000 }, + sessionTarget: "session:../../outside", + wakeMode: "now", + payload: { kind: "agentTurn", message: "hello" }, + state: {}, + }, + ], + }); + + cronIsolatedRun.mockClear(); + const { server, ws } = await startServerWithClient(); + await connectOk(ws); + + try { + const runRes = await rpcReq(ws, "cron.run", { + id: "bad-custom-session-job", + mode: "force", + }); + expect(runRes.ok).toBe(false); + expect(runRes.error?.message).toContain("invalid cron sessionTarget session id"); + expect(cronIsolatedRun).not.toHaveBeenCalled(); + } finally { + await cleanupCronTestRun({ ws, server, prevSkipCron }); + } + }); + test("returns from cron.run immediately while isolated work continues in background", async () => { const { prevSkipCron } = await setupCronTestRun({ tempPrefix: "openclaw-gw-cron-run-detached-",