fix(cron): narrow accountId spoof guard to explicit mismatch only

Addresses codex P1 review on PR #69940: the previous guard rejected
targets that simply omitted accountId, but message-tool fills accountId
from the agent's bound account at exec time (message-tool.ts:730-733),
so account-bound cron jobs legitimately start with target.accountId
undefined. Rejecting that case lost skipMessagingToolDelivery, causing
dispatchCronDelivery to double-send.

Now we only reject when the tool explicitly names a *different*
accountId — which is the real CWE-284 spoof vector. Omission matches.

Tests updated accordingly:
- matcher unit test: flipped "omit accountId" case from false to true;
  "accountIds differ" case preserved as the real spoof guard
- integration tests: one legitimate-default case (rewrite happens),
  one explicit-mismatch case (rewrite suppressed)

658 cron tests pass.
This commit is contained in:
davehappyminion
2026-04-21 22:19:09 -04:00
committed by Ayaan Zaidi
parent 851bef9c25
commit 9db67e79a5
3 changed files with 72 additions and 23 deletions

View File

@@ -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", () => {

View File

@@ -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.

View File

@@ -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" }],
}),
);
});