mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 18:00:54 +00:00
fix(cron): rewrite generic message provider in trace + guard accountId spoof
When a cron job sends via the generic `message` tool, the delivery trace previously recorded `messageToolSentTo[i].channel = "message"` even though the send was resolved to a specific channel (e.g. telegram). This made `jq` diffing intended-vs-actual awkward for the happy path. Fix: - `normalizeMessagingToolTarget` now rewrites `channel: "message"` to the resolved channel when `matchesMessagingToolDeliveryTarget` confirms the tool send matches the resolved cron delivery target. Genuinely unmatched generic sends keep the literal "message" so audits can still flag them. - `matchesMessagingToolDeliveryTarget` now requires strict accountId equality whenever the resolved delivery carries an `accountId`. An omitted `target.accountId` previously short-circuited the guard and was treated as a wildcard, letting a generic send spoof attribution to any bot identity in the cron delivery trace (CWE-284). This was flagged by Aisle on #69771. Tests: - Unit: `matchesMessagingToolDeliveryTarget` rejects omitted-accountId against account-tied delivery; still matches same-accountId. - Integration: cron run trace rewrites generic "message" to the resolved channel, preserves accountId on both sides, and leaves the literal "message" provider in place when the tool send omits accountId against an account-tied delivery.
This commit is contained in:
committed by
Ayaan Zaidi
parent
e8f18f95d5
commit
851bef9c25
@@ -72,6 +72,25 @@ describe("matchesMessagingToolDeliveryTarget", () => {
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
it("rejects when delivery has accountId but target omits it (spoof guard)", () => {
|
||||
// Regression guard for CWE-284: an omitted target.accountId must NOT
|
||||
// count as a wildcard match against an account-tied delivery.
|
||||
expect(
|
||||
matchesMessagingToolDeliveryTarget(
|
||||
{ provider: "message", to: "123456" },
|
||||
{ channel: "telegram", to: "123456", accountId: "bot-a" },
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("matches when delivery and target carry the same accountId", () => {
|
||||
expect(
|
||||
matchesMessagingToolDeliveryTarget(
|
||||
{ provider: "telegram", to: "123456", accountId: "bot-a" },
|
||||
{ channel: "telegram", to: "123456", accountId: "bot-a" },
|
||||
),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("resolveCronDeliveryBestEffort", () => {
|
||||
|
||||
@@ -80,8 +80,18 @@ export function matchesMessagingToolDeliveryTarget(
|
||||
if (provider && provider !== "message" && provider !== channel) {
|
||||
return false;
|
||||
}
|
||||
if (target.accountId && delivery.accountId && target.accountId !== delivery.accountId) {
|
||||
return false;
|
||||
// Strict accountId matching when the resolved delivery is tied to a specific
|
||||
// account: require the tool-reported target to carry an equal accountId.
|
||||
// Omitting target.accountId must NOT count as a wildcard match, otherwise a
|
||||
// generic `message` send could spoof attribution to any bot identity in the
|
||||
// cron delivery trace (CWE-284).
|
||||
if (delivery.accountId) {
|
||||
if (!target.accountId) {
|
||||
return false;
|
||||
}
|
||||
if (target.accountId !== delivery.accountId) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
// Strip :topic:NNN from message targets and normalize Feishu/Lark prefixes on
|
||||
// both sides so cron duplicate suppression compares canonical IDs.
|
||||
|
||||
@@ -455,6 +455,134 @@ describe("runCronIsolatedAgentTurn message tool policy", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("rewrites generic message provider to resolved channel in delivery trace", async () => {
|
||||
mockRunCronFallbackPassthrough();
|
||||
resolveCronDeliveryPlanMock.mockReturnValue({
|
||||
requested: true,
|
||||
mode: "announce",
|
||||
channel: "telegram",
|
||||
to: "123",
|
||||
});
|
||||
runEmbeddedPiAgentMock.mockResolvedValue({
|
||||
payloads: [{ text: "sent" }],
|
||||
didSendViaMessagingTool: true,
|
||||
messagingToolSentTargets: [{ tool: "message", provider: "message", to: "123" }],
|
||||
meta: { agentMeta: { usage: { input: 10, output: 20 } } },
|
||||
});
|
||||
|
||||
const result = await runCronIsolatedAgentTurn({
|
||||
...makeParams(),
|
||||
job: {
|
||||
id: "message-tool-generic-target",
|
||||
name: "Message Tool Generic Target",
|
||||
schedule: { kind: "every", everyMs: 60_000 },
|
||||
sessionTarget: "isolated",
|
||||
payload: { kind: "agentTurn", message: "send a message" },
|
||||
delivery: { mode: "announce", channel: "telegram", to: "123" },
|
||||
} as never,
|
||||
});
|
||||
|
||||
expect(result.delivery).toEqual(
|
||||
expect.objectContaining({
|
||||
resolved: { ok: true, channel: "telegram", to: "123", source: "explicit" },
|
||||
messageToolSentTo: [{ channel: "telegram", to: "123" }],
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("preserves accountId when rewriting generic message provider to resolved channel", async () => {
|
||||
mockRunCronFallbackPassthrough();
|
||||
resolveCronDeliveryPlanMock.mockReturnValue({
|
||||
requested: true,
|
||||
mode: "announce",
|
||||
channel: "telegram",
|
||||
to: "123",
|
||||
accountId: "bot-a",
|
||||
});
|
||||
resolveDeliveryTargetMock.mockResolvedValue({
|
||||
ok: true,
|
||||
channel: "telegram",
|
||||
to: "123",
|
||||
accountId: "bot-a",
|
||||
threadId: undefined,
|
||||
mode: "explicit",
|
||||
});
|
||||
runEmbeddedPiAgentMock.mockResolvedValue({
|
||||
payloads: [{ text: "sent" }],
|
||||
didSendViaMessagingTool: true,
|
||||
messagingToolSentTargets: [
|
||||
{ tool: "message", provider: "message", to: "123", accountId: "bot-a" },
|
||||
],
|
||||
meta: { agentMeta: { usage: { input: 10, output: 20 } } },
|
||||
});
|
||||
|
||||
const result = await runCronIsolatedAgentTurn({
|
||||
...makeParams(),
|
||||
job: {
|
||||
id: "message-tool-generic-target-account",
|
||||
name: "Message Tool Generic Target (accountId)",
|
||||
schedule: { kind: "every", everyMs: 60_000 },
|
||||
sessionTarget: "isolated",
|
||||
payload: { kind: "agentTurn", message: "send a message" },
|
||||
delivery: { mode: "announce", channel: "telegram", to: "123", accountId: "bot-a" },
|
||||
} as never,
|
||||
});
|
||||
|
||||
expect(result.delivery).toEqual(
|
||||
expect.objectContaining({
|
||||
messageToolSentTo: [{ channel: "telegram", to: "123", accountId: "bot-a" }],
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("does not rewrite generic message provider when accountId is missing on tool send", async () => {
|
||||
// Regression guard: a tool send omitting accountId must NOT be attributed
|
||||
// to a specific account-tied delivery in the trace. Spoofing protection
|
||||
// for CronDeliveryTrace.messageToolSentTo[i].channel (CWE-284).
|
||||
mockRunCronFallbackPassthrough();
|
||||
resolveCronDeliveryPlanMock.mockReturnValue({
|
||||
requested: true,
|
||||
mode: "announce",
|
||||
channel: "telegram",
|
||||
to: "123",
|
||||
accountId: "bot-a",
|
||||
});
|
||||
resolveDeliveryTargetMock.mockResolvedValue({
|
||||
ok: true,
|
||||
channel: "telegram",
|
||||
to: "123",
|
||||
accountId: "bot-a",
|
||||
threadId: undefined,
|
||||
mode: "explicit",
|
||||
});
|
||||
runEmbeddedPiAgentMock.mockResolvedValue({
|
||||
payloads: [{ text: "sent" }],
|
||||
didSendViaMessagingTool: true,
|
||||
messagingToolSentTargets: [{ tool: "message", provider: "message", to: "123" }],
|
||||
meta: { agentMeta: { usage: { input: 10, output: 20 } } },
|
||||
});
|
||||
|
||||
const result = await runCronIsolatedAgentTurn({
|
||||
...makeParams(),
|
||||
job: {
|
||||
id: "message-tool-generic-target-account-spoof",
|
||||
name: "Message Tool Generic Target (account spoof guard)",
|
||||
schedule: { kind: "every", everyMs: 60_000 },
|
||||
sessionTarget: "isolated",
|
||||
payload: { kind: "agentTurn", message: "send a message" },
|
||||
delivery: { mode: "announce", channel: "telegram", to: "123", accountId: "bot-a" },
|
||||
} as never,
|
||||
});
|
||||
|
||||
expect(result.delivery).toEqual(
|
||||
expect.objectContaining({
|
||||
// Channel stays as "message" because the tool-reported target did not
|
||||
// carry an accountId matching the resolved delivery's bot-a binding.
|
||||
messageToolSentTo: [{ channel: "message", to: "123" }],
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("does not mark message tool delivery as matched when cron target resolution failed", async () => {
|
||||
mockRunCronFallbackPassthrough();
|
||||
resolveCronDeliveryPlanMock.mockReturnValue({
|
||||
|
||||
@@ -142,15 +142,37 @@ function normalizeCronTraceTarget(
|
||||
};
|
||||
}
|
||||
|
||||
type MessagingToolTargetMatcher = (
|
||||
target: { provider?: string; to?: string; accountId?: string },
|
||||
delivery: { channel?: string; to?: string; accountId?: string },
|
||||
) => boolean;
|
||||
|
||||
function normalizeMessagingToolTarget(
|
||||
target: MessagingToolSend,
|
||||
resolvedDelivery: ResolvedCronDeliveryTarget,
|
||||
matchesMessagingToolDeliveryTarget: MessagingToolTargetMatcher,
|
||||
): CronDeliveryTraceMessageTarget | undefined {
|
||||
const channel = target.provider?.trim();
|
||||
if (!channel) {
|
||||
return undefined;
|
||||
}
|
||||
// Rewrite the generic "message" provider to the resolved channel in the
|
||||
// trace when the tool send actually matches the resolved cron delivery
|
||||
// target. This makes `intended.channel === messageToolSentTo[i].channel`
|
||||
// diffable for the happy path, while genuine unmatched generic sends keep
|
||||
// the literal "message" provider so audits can still flag them.
|
||||
const traceChannel =
|
||||
channel === "message" &&
|
||||
resolvedDelivery.ok &&
|
||||
matchesMessagingToolDeliveryTarget(target, {
|
||||
channel: resolvedDelivery.channel,
|
||||
to: resolvedDelivery.to,
|
||||
accountId: resolvedDelivery.accountId,
|
||||
})
|
||||
? resolvedDelivery.channel
|
||||
: channel;
|
||||
return {
|
||||
channel,
|
||||
channel: traceChannel,
|
||||
...(target.to ? { to: target.to } : {}),
|
||||
...(target.accountId ? { accountId: target.accountId } : {}),
|
||||
...(target.threadId ? { threadId: target.threadId } : {}),
|
||||
@@ -161,6 +183,7 @@ function buildCronDeliveryTrace(params: {
|
||||
deliveryPlan: CronDeliveryPlan;
|
||||
resolvedDelivery: ResolvedCronDeliveryTarget;
|
||||
messagingToolSentTargets: MessagingToolSend[];
|
||||
matchesMessagingToolDeliveryTarget: MessagingToolTargetMatcher;
|
||||
fallbackUsed: boolean;
|
||||
delivered: boolean;
|
||||
}): CronDeliveryTrace {
|
||||
@@ -195,7 +218,13 @@ function buildCronDeliveryTrace(params: {
|
||||
error: params.resolvedDelivery.error.message,
|
||||
};
|
||||
const messageToolSentTo = params.messagingToolSentTargets
|
||||
.map((target) => normalizeMessagingToolTarget(target))
|
||||
.map((target) =>
|
||||
normalizeMessagingToolTarget(
|
||||
target,
|
||||
params.resolvedDelivery,
|
||||
params.matchesMessagingToolDeliveryTarget,
|
||||
),
|
||||
)
|
||||
.filter((target): target is CronDeliveryTraceMessageTarget => Boolean(target));
|
||||
return {
|
||||
...(intended ? { intended } : {}),
|
||||
@@ -836,6 +865,7 @@ async function finalizeCronRun(params: {
|
||||
deliveryPlan: prepared.deliveryPlan,
|
||||
resolvedDelivery: prepared.resolvedDelivery,
|
||||
messagingToolSentTargets,
|
||||
matchesMessagingToolDeliveryTarget,
|
||||
fallbackUsed: deliveryResult.deliveryAttempted && !skipMessagingToolDelivery,
|
||||
delivered: deliveryResult.delivered,
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user