diff --git a/src/cron/isolated-agent/delivery-dispatch.named-agent.test.ts b/src/cron/isolated-agent/delivery-dispatch.named-agent.test.ts index ca1c1a49e60..690fb711dd2 100644 --- a/src/cron/isolated-agent/delivery-dispatch.named-agent.test.ts +++ b/src/cron/isolated-agent/delivery-dispatch.named-agent.test.ts @@ -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", () => { diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 0bc59d97834..959b354ea9e 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -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. diff --git a/src/cron/isolated-agent/run.message-tool-policy.test.ts b/src/cron/isolated-agent/run.message-tool-policy.test.ts index b482e264f1d..7dd2affdd9b 100644 --- a/src/cron/isolated-agent/run.message-tool-policy.test.ts +++ b/src/cron/isolated-agent/run.message-tool-policy.test.ts @@ -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({ diff --git a/src/cron/isolated-agent/run.ts b/src/cron/isolated-agent/run.ts index bf4bca218c7..71d6f325b88 100644 --- a/src/cron/isolated-agent/run.ts +++ b/src/cron/isolated-agent/run.ts @@ -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, });