From d8a1ebbb492749fa56f47393fe8438dae6e03306 Mon Sep 17 00:00:00 2001 From: Vatsal Garg Date: Fri, 14 Aug 2026 13:11:03 +0530 Subject: [PATCH] fix(approvals): prevent cross-channel exec approval leak (#122517) Reject unbound foreign-channel fallback at shared approval-account selection while preserving recorded bindings and explicit forwarding targets. Cover Telegram and Matrix routing contracts. Co-authored-by: vatsalgargg Co-authored-by: Ayaan Zaidi --- extensions/matrix/src/exec-approvals.test.ts | 47 ++++++---------- .../telegram/src/exec-approvals.test.ts | 54 ++++++++----------- src/infra/approval-request-account-binding.ts | 4 ++ .../exec-approval-session-target.test.ts | 33 ++++++++++++ 4 files changed, 76 insertions(+), 62 deletions(-) diff --git a/extensions/matrix/src/exec-approvals.test.ts b/extensions/matrix/src/exec-approvals.test.ts index 128583040d1c..a87908247226 100644 --- a/extensions/matrix/src/exec-approvals.test.ts +++ b/extensions/matrix/src/exec-approvals.test.ts @@ -111,10 +111,11 @@ function buildMultiAccountMatrixConfig(params: { } as OpenClawConfig; } -function makeForeignChannelApprovalRequest(params: { +function makeChannelApprovalRequest(params: { id: string; sessionKey?: string; agentId?: string; + turnSourceChannel?: string; }): MatrixExecApprovalRequest { return { id: params.id, @@ -122,7 +123,7 @@ function makeForeignChannelApprovalRequest(params: { command: "echo hi", agentId: params.agentId ?? "ops-agent", sessionKey: params.sessionKey ?? "agent:ops-agent:missing", - turnSourceChannel: "slack", + turnSourceChannel: params.turnSourceChannel ?? "slack", turnSourceTo: "channel:C123", }, createdAtMs: 0, @@ -401,7 +402,7 @@ describe("matrix exec approvals", () => { }, }); const cfg = buildMultiAccountMatrixConfig({ sessionStorePath: storePath }); - const request = makeForeignChannelApprovalRequest({ + const request = makeChannelApprovalRequest({ id: "req-3", sessionKey: "agent:ops-agent:matrix:channel:!room:example.org", }); @@ -422,26 +423,6 @@ describe("matrix exec approvals", () => { ).toBe(true); }); - it("reports each eligible foreign-channel account as a raw route candidate", () => { - const cfg = buildMultiAccountMatrixConfig({}); - const request = makeForeignChannelApprovalRequest({ id: "req-4" }); - - expect( - shouldHandleMatrixExecApprovalRequest({ - cfg, - accountId: "default", - request, - }), - ).toBe(true); - expect( - shouldHandleMatrixExecApprovalRequest({ - cfg, - accountId: "ops", - request, - }), - ).toBe(true); - }); - it("reports each eligible same-channel account as a raw route candidate", () => { const cfg = buildMultiAccountMatrixConfig({}); const request: MatrixExecApprovalRequest = { @@ -457,14 +438,14 @@ describe("matrix exec approvals", () => { expect(shouldHandleMatrixExecApprovalRequest({ cfg, accountId: "ops", request })).toBe(true); }); - it("allows unbound foreign-channel approvals when only one matrix account can handle them", () => { + it("rejects unbound foreign-channel approvals even when only one matrix account can handle them", () => { const cfg = buildMultiAccountMatrixConfig({ opsExecApprovals: { enabled: false, approvers: ["@owner:example.org"], }, }); - const request = makeForeignChannelApprovalRequest({ id: "req-5" }); + const request = makeChannelApprovalRequest({ id: "req-5" }); expect( shouldHandleMatrixExecApprovalRequest({ @@ -472,7 +453,7 @@ describe("matrix exec approvals", () => { accountId: "default", request, }), - ).toBe(true); + ).toBe(false); expect( shouldHandleMatrixExecApprovalRequest({ cfg, @@ -482,7 +463,7 @@ describe("matrix exec approvals", () => { ).toBe(false); }); - it("uses request filters when checking foreign-channel matrix ambiguity", () => { + it("uses request filters when checking unbound matrix account eligibility", () => { const cfg = buildMultiAccountMatrixConfig({ defaultExecApprovals: { enabled: true, @@ -495,7 +476,10 @@ describe("matrix exec approvals", () => { agentFilter: ["other-agent"], }, }); - const request = makeForeignChannelApprovalRequest({ id: "req-6" }); + const request = makeChannelApprovalRequest({ + id: "req-6", + turnSourceChannel: "matrix", + }); expect( shouldHandleMatrixExecApprovalRequest({ @@ -513,11 +497,14 @@ describe("matrix exec approvals", () => { ).toBe(false); }); - it("ignores disabled matrix accounts when checking foreign-channel ambiguity", () => { + it("ignores disabled matrix accounts when checking unbound account eligibility", () => { const cfg = buildMultiAccountMatrixConfig({ opsOverrides: { enabled: false }, }); - const request = makeForeignChannelApprovalRequest({ id: "req-7" }); + const request = makeChannelApprovalRequest({ + id: "req-7", + turnSourceChannel: "matrix", + }); expect( shouldHandleMatrixExecApprovalRequest({ diff --git a/extensions/telegram/src/exec-approvals.test.ts b/extensions/telegram/src/exec-approvals.test.ts index 8746b4582c04..c95006669641 100644 --- a/extensions/telegram/src/exec-approvals.test.ts +++ b/extensions/telegram/src/exec-approvals.test.ts @@ -95,16 +95,17 @@ function buildMultiAccountTelegramConfig(params: { } as OpenClawConfig; } -function makeForeignChannelApprovalRequest(params: { +function makeChannelApprovalRequest(params: { id: string; sessionKey?: string; + turnSourceChannel?: string; }): TelegramExecApprovalRequest { return { id: params.id, request: { command: "echo hi", sessionKey: params.sessionKey ?? "agent:ops:missing", - turnSourceChannel: "slack", + turnSourceChannel: params.turnSourceChannel ?? "slack", turnSourceTo: "channel:C123", }, createdAtMs: 0, @@ -168,7 +169,7 @@ describe("telegram exec approvals", () => { expect(isTelegramExecApprovalApprover({ cfg, senderId: "67890" })).toBe(true); }); - it("does not require explicit Telegram exec approvers when command owner identifies the Telegram operator", () => { + it("does not require explicit Telegram exec approvers when command owner identifies the operator", () => { const cfg = { ...buildConfig(), commands: { @@ -182,7 +183,10 @@ describe("telegram exec approvals", () => { expect( shouldHandleTelegramExecApprovalRequest({ cfg, - request: makeForeignChannelApprovalRequest({ id: "discord-diagnostics" }), + request: makeChannelApprovalRequest({ + id: "telegram-diagnostics", + turnSourceChannel: "telegram", + }), }), ).toBe(true); }); @@ -252,7 +256,7 @@ describe("telegram exec approvals", () => { }, }); const cfg = buildMultiAccountTelegramConfig({ sessionStorePath: storePath }); - const request = makeForeignChannelApprovalRequest({ + const request = makeChannelApprovalRequest({ id: "req-2", sessionKey: "agent:ops:telegram:direct:123", }); @@ -273,26 +277,6 @@ describe("telegram exec approvals", () => { ).toBe(true); }); - it("reports each eligible foreign-channel account as a raw route candidate", () => { - const cfg = buildMultiAccountTelegramConfig({}); - const request = makeForeignChannelApprovalRequest({ id: "req-3" }); - - expect( - shouldHandleTelegramExecApprovalRequest({ - cfg, - accountId: "default", - request, - }), - ).toBe(true); - expect( - shouldHandleTelegramExecApprovalRequest({ - cfg, - accountId: "ops", - request, - }), - ).toBe(true); - }); - it("reports each eligible same-channel account as a raw route candidate", () => { const cfg = buildMultiAccountTelegramConfig({}); const request: TelegramExecApprovalRequest = { @@ -312,11 +296,11 @@ describe("telegram exec approvals", () => { expect(shouldHandleTelegramExecApprovalRequest({ cfg, accountId: "ops", request })).toBe(true); }); - it("allows unbound foreign-channel approvals when only one telegram account can handle them", () => { + it("rejects unbound foreign-channel approvals even when only one telegram account can handle them", () => { const cfg = buildMultiAccountTelegramConfig({ opsExecApprovals: { enabled: false, approvers: ["123"] }, }); - const request = makeForeignChannelApprovalRequest({ id: "req-4" }); + const request = makeChannelApprovalRequest({ id: "req-4" }); expect( shouldHandleTelegramExecApprovalRequest({ @@ -324,7 +308,7 @@ describe("telegram exec approvals", () => { accountId: "default", request, }), - ).toBe(true); + ).toBe(false); expect( shouldHandleTelegramExecApprovalRequest({ cfg, @@ -334,7 +318,7 @@ describe("telegram exec approvals", () => { ).toBe(false); }); - it("uses request filters when checking foreign-channel telegram ambiguity", () => { + it("uses request filters when checking unbound telegram account eligibility", () => { const cfg = buildMultiAccountTelegramConfig({ defaultExecApprovals: { enabled: true, @@ -347,7 +331,10 @@ describe("telegram exec approvals", () => { agentFilter: ["other"], }, }); - const request = makeForeignChannelApprovalRequest({ id: "req-5" }); + const request = makeChannelApprovalRequest({ + id: "req-5", + turnSourceChannel: "telegram", + }); expect( shouldHandleTelegramExecApprovalRequest({ @@ -459,9 +446,12 @@ describe("telegram exec approvals", () => { ).toBe(false); }); - it("ignores disabled telegram accounts when checking foreign-channel ambiguity", () => { + it("ignores disabled telegram accounts when checking unbound account eligibility", () => { const cfg = buildMultiAccountTelegramConfig({ opsOverrides: { enabled: false } }); - const request = makeForeignChannelApprovalRequest({ id: "req-6" }); + const request = makeChannelApprovalRequest({ + id: "req-6", + turnSourceChannel: "telegram", + }); expect( shouldHandleTelegramExecApprovalRequest({ diff --git a/src/infra/approval-request-account-binding.ts b/src/infra/approval-request-account-binding.ts index d905a8692696..0c238075156a 100644 --- a/src/infra/approval-request-account-binding.ts +++ b/src/infra/approval-request-account-binding.ts @@ -261,6 +261,10 @@ export function doesApprovalRequestSelectChannelAccount(params: { if (boundAccountId || forwardAccountIds.length > 0) { return false; } + const turnSourceChannel = normalizeOptionalChannel(params.request.request.turnSourceChannel); + if (turnSourceChannel && turnSourceChannel !== normalizeOptionalChannel(params.channel)) { + return false; + } const eligibleAccountIds = params.eligibleAccountIds .map(normalizeOptionalAccountId) .filter((candidate): candidate is string => Boolean(candidate)); diff --git a/src/infra/exec-approval-session-target.test.ts b/src/infra/exec-approval-session-target.test.ts index 39cd4308e13b..2e20488faee1 100644 --- a/src/infra/exec-approval-session-target.test.ts +++ b/src/infra/exec-approval-session-target.test.ts @@ -90,6 +90,39 @@ describe("native approval account selection", () => { ).toBe(false); }); + it("rejects foreign-channel fallback but preserves explicit forwarding", () => { + const request = buildRequest({ turnSourceChannel: "whatsapp" }); + const explicitTargetConfig: OpenClawConfig = { + approvals: { + exec: { + enabled: true, + mode: "targets", + targets: [{ channel: "telegram", to: "owner" }], + }, + }, + }; + expect( + doesApprovalRequestSelectChannelAccount({ + cfg: {}, + request, + channel: "telegram", + accountId: "default", + defaultAccountId: "default", + eligibleAccountIds: ["default"], + }), + ).toBe(false); + expect( + doesApprovalRequestSelectChannelAccount({ + cfg: explicitTargetConfig, + request, + channel: "telegram", + accountId: "default", + defaultAccountId: "default", + eligibleAccountIds: ["default"], + }), + ).toBe(true); + }); + it("selects the recorded account even when several accounts are eligible", () => { const request = buildRequest({ turnSourceChannel: "telegram",