mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-10 08:41:13 +00:00
fix(cron): reject unsafe custom session targets earlier
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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";
|
||||
}
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -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<CronJob, "sessionTarget" | "pay
|
||||
job.sessionTarget === "isolated" ||
|
||||
job.sessionTarget === "current" ||
|
||||
job.sessionTarget.startsWith("session:");
|
||||
if (job.sessionTarget.startsWith("session:")) {
|
||||
assertSafeCronSessionTargetId(job.sessionTarget.slice(8));
|
||||
}
|
||||
if (job.sessionTarget === "main" && job.payload.kind !== "systemEvent") {
|
||||
throw new Error('main cron jobs require payload.kind="systemEvent"');
|
||||
}
|
||||
|
||||
12
src/cron/session-target.ts
Normal file
12
src/cron/session-target.ts
Normal file
@@ -0,0 +1,12 @@
|
||||
const INVALID_CRON_SESSION_TARGET_ID_ERROR = "invalid cron sessionTarget session id";
|
||||
|
||||
export function assertSafeCronSessionTargetId(sessionId: string): string {
|
||||
const trimmed = sessionId.trim();
|
||||
if (!trimmed) {
|
||||
throw new Error(INVALID_CRON_SESSION_TARGET_ID_ERROR);
|
||||
}
|
||||
if (trimmed.includes("/") || trimmed.includes("\\") || trimmed.includes("\0")) {
|
||||
throw new Error(INVALID_CRON_SESSION_TARGET_ID_ERROR);
|
||||
}
|
||||
return trimmed;
|
||||
}
|
||||
@@ -18,6 +18,7 @@ import {
|
||||
resolveCronRunLogPruneOptions,
|
||||
} from "../cron/run-log.js";
|
||||
import { CronService } from "../cron/service.js";
|
||||
import { assertSafeCronSessionTargetId } from "../cron/session-target.js";
|
||||
import { resolveCronStorePath } from "../cron/store.js";
|
||||
import { normalizeHttpWebhookUrl } from "../cron/webhook-url.js";
|
||||
import { formatErrorMessage } from "../infra/errors.js";
|
||||
@@ -287,10 +288,7 @@ export function buildGatewayCronService(params: {
|
||||
const { agentId, cfg: runtimeConfig } = resolveCronAgent(job.agentId);
|
||||
let sessionKey = `cron:${job.id}`;
|
||||
if (job.sessionTarget.startsWith("session:")) {
|
||||
const customSessionId = job.sessionTarget.slice(8).trim();
|
||||
if (customSessionId) {
|
||||
sessionKey = customSessionId;
|
||||
}
|
||||
sessionKey = assertSafeCronSessionTargetId(job.sessionTarget.slice(8));
|
||||
}
|
||||
try {
|
||||
return await runCronIsolatedAgentTurn({
|
||||
|
||||
@@ -93,10 +93,23 @@ export const cronHandlers: GatewayRequestHandlers = {
|
||||
typeof (params as { sessionKey?: unknown } | null)?.sessionKey === "string"
|
||||
? (params as { sessionKey: string }).sessionKey
|
||||
: undefined;
|
||||
const normalized =
|
||||
normalizeCronJobCreate(params, {
|
||||
sessionContext: { sessionKey },
|
||||
}) ?? params;
|
||||
let normalized: unknown;
|
||||
try {
|
||||
normalized =
|
||||
normalizeCronJobCreate(params, {
|
||||
sessionContext: { sessionKey },
|
||||
}) ?? params;
|
||||
} catch (err) {
|
||||
respond(
|
||||
false,
|
||||
undefined,
|
||||
errorShape(
|
||||
ErrorCodes.INVALID_REQUEST,
|
||||
`invalid cron.add params: ${err instanceof Error ? err.message : String(err)}`,
|
||||
),
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (!validateCronAddParams(normalized)) {
|
||||
respond(
|
||||
false,
|
||||
@@ -123,7 +136,20 @@ export const cronHandlers: GatewayRequestHandlers = {
|
||||
respond(true, job, undefined);
|
||||
},
|
||||
"cron.update": async ({ params, respond, context }) => {
|
||||
const normalizedPatch = normalizeCronJobPatch((params as { patch?: unknown } | null)?.patch);
|
||||
let normalizedPatch: ReturnType<typeof normalizeCronJobPatch>;
|
||||
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 }
|
||||
|
||||
@@ -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-",
|
||||
|
||||
Reference in New Issue
Block a user