Skip to content

Commit 97398e0

Browse files
ferminquantsteipete
ferminquant
authored and
steipete
committed
fix(cron): accept opaque session target keys
1 parent 3a4f2b1 commit 97398e0

7 files changed

Lines changed: 117 additions & 50 deletions

File tree

‎src/cron/normalize.test.ts‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -682,21 +682,33 @@ describe("normalizeCronJobCreate", () => {
682682
expect(normalized.sessionTarget).toBe("session:MySessionID");
683683
});
684684

685-
it("rejects custom session ids with path separators", () => {
685+
it("preserves custom session ids with channel-native separators", () => {
686+
const created = normalizeCronJobCreate({
687+
name: "dingtalk-group",
688+
schedule: { kind: "cron", expr: "* * * * *" },
689+
sessionTarget: "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==",
690+
payload: { kind: "agentTurn", message: "hello" },
691+
}) as unknown as Record<string, unknown>;
692+
693+
expect(created.sessionTarget).toBe(
694+
"session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==",
695+
);
696+
697+
const patched = normalizeCronJobPatch({
698+
sessionTarget: "session:..\\outside",
699+
}) as unknown as Record<string, unknown>;
700+
expect(patched.sessionTarget).toBe("session:..\\outside");
701+
});
702+
703+
it("rejects null bytes in custom session ids", () => {
686704
expect(() =>
687705
normalizeCronJobCreate({
688-
name: "bad-custom-session",
706+
name: "null-byte-session",
689707
schedule: { kind: "cron", expr: "* * * * *" },
690-
sessionTarget: "session:../../outside",
708+
sessionTarget: "session:bad\0id",
691709
payload: { kind: "agentTurn", message: "hello" },
692710
}),
693711
).toThrow("invalid cron sessionTarget session id");
694-
695-
expect(() =>
696-
normalizeCronJobPatch({
697-
sessionTarget: "session:..\\outside",
698-
}),
699-
).toThrow("invalid cron sessionTarget session id");
700712
});
701713
});
702714

‎src/cron/service.jobs.test.ts‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -481,14 +481,28 @@ describe("createJob rejects sessionTarget main for non-default agents", () => {
481481
expect(job.sessionTarget).toBe("isolated");
482482
});
483483

484-
it("rejects custom session targets with path separators", () => {
484+
it("accepts custom session targets with channel-native separators", () => {
485+
const state = createMockState(now, { defaultAgentId: "main" });
486+
const sessionTarget = "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==";
487+
const job = createJob(state, {
488+
name: "dingtalk-group-session",
489+
enabled: true,
490+
schedule: { kind: "every", everyMs: 60_000 },
491+
sessionTarget,
492+
wakeMode: "now",
493+
payload: { kind: "agentTurn", message: "hello" },
494+
});
495+
expect(job.sessionTarget).toBe(sessionTarget);
496+
});
497+
498+
it("rejects null bytes in custom session targets", () => {
485499
const state = createMockState(now, { defaultAgentId: "main" });
486500
expect(() =>
487501
createJob(state, {
488502
name: "bad-custom-session",
489503
enabled: true,
490504
schedule: { kind: "every", everyMs: 60_000 },
491-
sessionTarget: "session:../../outside",
505+
sessionTarget: "session:bad\0id",
492506
wakeMode: "now",
493507
payload: { kind: "agentTurn", message: "hello" },
494508
}),
@@ -548,13 +562,27 @@ describe("applyJobPatch rejects sessionTarget main for non-default agents", () =
548562
expect(job.agentId).toBe("main");
549563
});
550564

551-
it("rejects patching to a custom session target with path separators", () => {
565+
it("accepts patching to a custom session target with channel-native separators", () => {
566+
const job = createMainJob();
567+
const sessionTarget = "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==";
568+
applyJobPatch(
569+
job,
570+
{
571+
sessionTarget,
572+
payload: { kind: "agentTurn", message: "hello" },
573+
},
574+
{ defaultAgentId: "main" },
575+
);
576+
expect(job.sessionTarget).toBe(sessionTarget);
577+
});
578+
579+
it("rejects patching to a custom session target with null bytes", () => {
552580
const job = createMainJob();
553581
expect(() =>
554582
applyJobPatch(
555583
job,
556584
{
557-
sessionTarget: "session:..\\outside",
585+
sessionTarget: "session:bad\0id",
558586
payload: { kind: "agentTurn", message: "hello" },
559587
},
560588
{ defaultAgentId: "main" },

‎src/cron/service/store.test.ts‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -360,17 +360,18 @@ describe("cron service store seam coverage", () => {
360360
expect(after).toBe(before);
361361
});
362362

363-
it("loads persisted jobs with unsafe custom session ids so run paths can fail closed", async () => {
363+
it("loads persisted jobs with opaque custom session ids containing separators", async () => {
364364
const { storePath } = await makeStorePath();
365+
const sessionTarget = "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==";
365366

366367
await writeSingleJobStore(storePath, {
367-
id: "unsafe-session-target-job",
368-
name: "unsafe session target job",
368+
id: "opaque-session-target-job",
369+
name: "opaque session target job",
369370
enabled: true,
370371
createdAtMs: STORE_TEST_NOW - 60_000,
371372
updatedAtMs: STORE_TEST_NOW - 60_000,
372373
schedule: { kind: "every", everyMs: 60_000 },
373-
sessionTarget: "session:../../outside",
374+
sessionTarget,
374375
wakeMode: "now",
375376
payload: { kind: "agentTurn", message: "ping" },
376377
state: {},
@@ -380,13 +381,18 @@ describe("cron service store seam coverage", () => {
380381

381382
await ensureLoaded(state, { skipRecompute: true });
382383

383-
const job = findJobOrThrow(state, "unsafe-session-target-job");
384-
expect(job.sessionTarget).toBe("session:../../outside");
385-
expectWarnedJob({
386-
storePath,
387-
jobId: "unsafe-session-target-job",
388-
message: "invalid persisted sessionTarget",
389-
});
384+
const job = findJobOrThrow(state, "opaque-session-target-job");
385+
expect(job.sessionTarget).toBe(sessionTarget);
386+
const warnCalls = logger.warn.mock.calls as unknown as Array<
387+
[{ storePath?: string; jobId?: string }, string]
388+
>;
389+
expect(
390+
warnCalls.some(
391+
([metadata, message]) =>
392+
metadata.jobId === "opaque-session-target-job" &&
393+
message.includes("invalid persisted sessionTarget"),
394+
),
395+
).toBe(false);
390396
});
391397

392398
it("clears stale nextRunAtMs after force reload when cron schedule expression changes", async () => {

‎src/cron/session-target.test.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,17 @@ describe("cron session target helpers", () => {
1414
);
1515
});
1616

17-
it("rejects unsafe persistent session targets", () => {
18-
expect(() => resolveCronSessionTargetSessionKey("session:../../outside")).toThrow(
17+
it("preserves opaque persistent session targets with native separators", () => {
18+
expect(
19+
resolveCronSessionTargetSessionKey(
20+
"session: agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a== ",
21+
),
22+
).toBe("agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==");
23+
expect(resolveCronSessionTargetSessionKey("session:..\\outside")).toBe("..\\outside");
24+
});
25+
26+
it("rejects null bytes in persistent session targets", () => {
27+
expect(() => resolveCronSessionTargetSessionKey("session:bad\0id")).toThrow(
1928
"invalid cron sessionTarget session id",
2029
);
2130
});
@@ -24,9 +33,9 @@ describe("cron session target helpers", () => {
2433
expect(
2534
resolveCronCurrentSessionTarget({
2635
sessionTarget: "current",
27-
sessionKey: " agent:main:telegram:direct:123 ",
36+
sessionKey: " agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a== ",
2837
}),
29-
).toBe("session:agent:main:telegram:direct:123");
38+
).toBe("session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==");
3039
});
3140

3241
it("falls back current targets to isolated without a creator session key", () => {

‎src/cron/session-target.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ export function assertSafeCronSessionTargetId(sessionId: string): string {
99
if (!trimmed) {
1010
throw new Error(INVALID_CRON_SESSION_TARGET_ID_ERROR);
1111
}
12-
if (trimmed.includes("/") || trimmed.includes("\\") || trimmed.includes("\0")) {
12+
if (trimmed.includes("\0")) {
1313
throw new Error(INVALID_CRON_SESSION_TARGET_ID_ERROR);
1414
}
1515
return trimmed;

‎src/gateway/server-cron.test.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,7 +1024,7 @@ describe("buildGatewayCronService", () => {
10241024
}
10251025
});
10261026

1027-
it("passes custom session targets through to isolated cron runs", async () => {
1027+
it("passes opaque custom session targets through to isolated cron runs", async () => {
10281028
const tmpDir = path.join(os.tmpdir(), `server-cron-custom-session-${Date.now()}`);
10291029
const cfg = {
10301030
session: {
@@ -1042,20 +1042,21 @@ describe("buildGatewayCronService", () => {
10421042
broadcast: () => {},
10431043
});
10441044
try {
1045+
const sessionKey = "agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==";
10451046
const job = await state.cron.add({
10461047
name: "custom-session",
10471048
enabled: true,
10481049
schedule: { kind: "at", at: new Date(1).toISOString() },
1049-
sessionTarget: "session:project-alpha-monitor",
1050+
sessionTarget: `session:${sessionKey}`,
10501051
wakeMode: "next-heartbeat",
10511052
payload: { kind: "agentTurn", message: "hello" },
10521053
});
10531054

10541055
await state.cron.run(job.id, "force");
10551056

1056-
const options = expectIsolatedRunFields({ sessionKey: "project-alpha-monitor" });
1057+
const options = expectIsolatedRunFields({ sessionKey });
10571058
expect(requireRecord(options.job, "isolated job").id).toBe(job.id);
1058-
expectCleanupForSessionKeys(["project-alpha-monitor"]);
1059+
expectCleanupForSessionKeys([sessionKey]);
10591060
} finally {
10601061
state.cron.stop();
10611062
}

‎src/gateway/server.cron.test.ts‎

Lines changed: 28 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -784,28 +784,30 @@ describe("gateway server cron", () => {
784784
}
785785
});
786786

787-
test("rejects unsafe custom session ids on add and update", async () => {
787+
test("accepts opaque custom session ids on add and update", async () => {
788788
const { prevSkipCron } = await setupCronTestRun({
789-
tempPrefix: "openclaw-gw-cron-bad-session-target-",
789+
tempPrefix: "openclaw-gw-cron-opaque-session-target-",
790790
cronEnabled: false,
791791
});
792792

793793
const cronState = await createDirectCronState();
794794

795795
try {
796796
const addRes = await directCronReq(cronState, "cron.add", {
797-
name: "bad custom session",
797+
name: "dingtalk group session",
798798
enabled: true,
799799
schedule: { kind: "every", everyMs: 60_000 },
800-
sessionTarget: "session:../../outside",
800+
sessionTarget: "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==",
801801
wakeMode: "now",
802802
payload: { kind: "agentTurn", message: "hello" },
803803
});
804-
expect(addRes.ok).toBe(false);
805-
expect(addRes.error?.message).toContain("invalid cron sessionTarget session id");
804+
expect(addRes.ok).toBe(true);
805+
expectRecordFields(addRes.payload, {
806+
sessionTarget: "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==",
807+
});
806808

807809
const validRes = await directCronReq(cronState, "cron.add", {
808-
name: "good custom session",
810+
name: "custom session to patch",
809811
enabled: true,
810812
schedule: { kind: "every", everyMs: 60_000 },
811813
sessionTarget: "session:project-alpha:ops",
@@ -822,8 +824,8 @@ describe("gateway server cron", () => {
822824
sessionTarget: "session:..\\outside",
823825
},
824826
});
825-
expect(updateRes.ok).toBe(false);
826-
expect(updateRes.error?.message).toContain("invalid cron sessionTarget session id");
827+
expect(updateRes.ok).toBe(true);
828+
expectRecordFields(updateRes.payload, { sessionTarget: "session:..\\outside" });
827829
} finally {
828830
await cleanupCronTestRun({ cronState, prevSkipCron });
829831
}
@@ -1136,20 +1138,21 @@ describe("gateway server cron", () => {
11361138
}
11371139
}, 45_000);
11381140

1139-
test("fails closed for persisted unsafe custom session ids", async () => {
1141+
test("runs persisted opaque custom session ids with native separators", async () => {
11401142
const now = Date.now();
1143+
const sessionTarget = "session:agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==";
11411144
const { prevSkipCron } = await setupCronTestRun({
1142-
tempPrefix: "openclaw-gw-cron-persisted-bad-session-target-",
1145+
tempPrefix: "openclaw-gw-cron-persisted-opaque-session-target-",
11431146
cronEnabled: false,
11441147
jobs: [
11451148
{
1146-
id: "bad-custom-session-job",
1147-
name: "bad custom session job",
1149+
id: "opaque-custom-session-job",
1150+
name: "opaque custom session job",
11481151
enabled: true,
11491152
createdAtMs: now,
11501153
updatedAtMs: now,
11511154
schedule: { kind: "every", everyMs: 60_000 },
1152-
sessionTarget: "session:../../outside",
1155+
sessionTarget,
11531156
wakeMode: "now",
11541157
payload: { kind: "agentTurn", message: "hello" },
11551158
state: {},
@@ -1162,13 +1165,21 @@ describe("gateway server cron", () => {
11621165
await connectOk(ws);
11631166

11641167
try {
1168+
const finished = waitForCronEvent(
1169+
ws,
1170+
(payload) =>
1171+
payload?.jobId === "opaque-custom-session-job" && payload?.action === "finished",
1172+
);
11651173
const runRes = await rpcReq(ws, "cron.run", {
1166-
id: "bad-custom-session-job",
1174+
id: "opaque-custom-session-job",
11671175
mode: "force",
11681176
});
11691177
expect(runRes.ok).toBe(true);
1170-
expect(runRes.payload).toEqual({ ok: true, ran: false, reason: "invalid-spec" });
1171-
expect(cronIsolatedRun).not.toHaveBeenCalled();
1178+
expectEnqueuedRunPayload(runRes.payload);
1179+
await finished;
1180+
expect(cronIsolatedRun).toHaveBeenCalledTimes(1);
1181+
const call = cronIsolatedRun.mock.calls.at(0)?.[0] as { sessionKey?: unknown } | undefined;
1182+
expect(call?.sessionKey).toBe("agent:main:dingtalk:group:cid3tmd4xb19xjfk/wogxwy2a==");
11721183
} finally {
11731184
await cleanupCronTestRun({ ws, server, prevSkipCron });
11741185
}

0 commit comments

Comments
 (0)