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 690fb711dd2..4400fcb988e 100644 --- a/src/cron/isolated-agent/delivery-dispatch.named-agent.test.ts +++ b/src/cron/isolated-agent/delivery-dispatch.named-agent.test.ts @@ -72,15 +72,19 @@ 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. + it("matches when delivery has accountId and target omits it (tool fills accountId at exec)", () => { + // message-tool resolves accountId from the agent's bound account at + // execution time (message-tool.ts: `accountId ?? agentAccountId`), so + // an absent target.accountId is equivalent to the delivery's bound + // account. Rejecting this case caused duplicate sends for account-bound + // cron jobs (codex review on PR #69940). CWE-284 spoofing is still + // prevented by the "accountIds differ" case above. expect( matchesMessagingToolDeliveryTarget( { provider: "message", to: "123456" }, { channel: "telegram", to: "123456", accountId: "bot-a" }, ), - ).toBe(false); + ).toBe(true); }); it("matches when delivery and target carry the same accountId", () => { diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 959b354ea9e..a1bcb8ef567 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -80,18 +80,13 @@ export function matchesMessagingToolDeliveryTarget( if (provider && provider !== "message" && provider !== channel) { 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; - } + // CWE-284: when the tool-reported target explicitly names a different + // accountId than the account-bound delivery, reject so attribution cannot + // be spoofed to another bot identity. An omitted target.accountId is + // legitimate — message-tool fills accountId from the agent's bound account + // at exec time, which equals delivery.accountId for account-bound jobs. + if (delivery.accountId && target.accountId && 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 7dd2affdd9b..877d746f5e0 100644 --- a/src/cron/isolated-agent/run.message-tool-policy.test.ts +++ b/src/cron/isolated-agent/run.message-tool-policy.test.ts @@ -535,10 +535,13 @@ describe("runCronIsolatedAgentTurn message tool policy", () => { ); }); - 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). + it("rewrites generic message provider when tool send omits accountId (tool fills at exec)", async () => { + // message-tool resolves accountId from the agent's bound account at exec + // time (message-tool.ts: `accountId ?? agentAccountId`), so a tool call + // that omits accountId is the common path for account-bound cron jobs. + // The trace rewrite must still happen here, otherwise cron's + // delivery-suppression flag is lost and dispatchCronDelivery would + // double-send for account-bound jobs (codex review on PR #69940). mockRunCronFallbackPassthrough(); resolveCronDeliveryPlanMock.mockReturnValue({ requested: true, @@ -562,6 +565,53 @@ describe("runCronIsolatedAgentTurn message tool policy", () => { meta: { agentMeta: { usage: { input: 10, output: 20 } } }, }); + const result = await runCronIsolatedAgentTurn({ + ...makeParams(), + job: { + id: "message-tool-generic-target-account-default", + name: "Message Tool Generic Target (accountId default)", + 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" }], + }), + ); + }); + + it("does not rewrite generic message provider when tool names a different accountId (spoof guard)", async () => { + // CWE-284: a tool that explicitly sets a foreign accountId must not be + // attributed to this account-bound delivery in the trace. + 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-b" }, + ], + meta: { agentMeta: { usage: { input: 10, output: 20 } } }, + }); + const result = await runCronIsolatedAgentTurn({ ...makeParams(), job: { @@ -576,9 +626,9 @@ describe("runCronIsolatedAgentTurn message tool policy", () => { 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" }], + // Channel stays as "message" because the tool named bot-b, which does + // not match the resolved delivery's bot-a binding. + messageToolSentTo: [{ channel: "message", to: "123", accountId: "bot-b" }], }), ); });