From ae420a355c2b2729fbbcbb81cd67664aaa1e6311 Mon Sep 17 00:00:00 2001 From: Gustavo Madeira Santana Date: Thu, 2 Apr 2026 14:29:24 -0400 Subject: [PATCH] Matrix: tighten native approval gating --- extensions/matrix/src/approval-native.test.ts | 56 ++++++++++++++ extensions/matrix/src/approval-native.ts | 20 ++++- .../matrix/src/exec-approvals-handler.test.ts | 33 +++++++++ .../matrix/src/exec-approvals-handler.ts | 4 + extensions/matrix/src/exec-approvals.test.ts | 38 ++++++++++ extensions/matrix/src/exec-approvals.ts | 73 ++++++++++++++++++- extensions/slack/src/approval-native.test.ts | 2 + src/channels/plugins/types.adapters.ts | 1 + src/infra/exec-approval-forwarder.ts | 23 +++++- src/infra/exec-approval-reply.test.ts | 27 +++++++ src/infra/exec-approval-reply.ts | 12 +++ .../approval-delivery-helpers.test.ts | 18 +++++ src/plugin-sdk/approval-delivery-helpers.ts | 4 + 13 files changed, 301 insertions(+), 10 deletions(-) diff --git a/extensions/matrix/src/approval-native.test.ts b/extensions/matrix/src/approval-native.test.ts index 93fd637b437..5aae388fe68 100644 --- a/extensions/matrix/src/approval-native.test.ts +++ b/extensions/matrix/src/approval-native.test.ts @@ -95,6 +95,62 @@ describe("matrix native approval adapter", () => { expect(targets).toEqual([{ to: "user:@owner:example.org" }]); }); + it("keeps plugin forwarding fallback active when native delivery is exec-only", () => { + const shouldSuppress = matrixNativeApprovalAdapter.delivery?.shouldSuppressForwardingFallback; + if (!shouldSuppress) { + throw new Error("delivery suppression helper unavailable"); + } + + expect( + shouldSuppress({ + cfg: buildConfig(), + approvalKind: "plugin", + target: { + channel: "matrix", + to: "room:!ops:example.org", + accountId: "default", + }, + request: { + id: "req-1", + request: { + command: "echo hi", + turnSourceChannel: "matrix", + turnSourceTo: "room:!ops:example.org", + turnSourceAccountId: "default", + }, + createdAtMs: 0, + expiresAtMs: 1000, + }, + }), + ).toBe(false); + }); + + it("preserves room-id case when matching Matrix origin targets", async () => { + const target = await matrixNativeApprovalAdapter.native?.resolveOriginTarget?.({ + cfg: buildConfig(), + accountId: "default", + approvalKind: "exec", + request: { + id: "req-1", + request: { + command: "echo hi", + turnSourceChannel: "matrix", + turnSourceTo: "room:!Ops:Example.org", + turnSourceThreadId: "$thread", + turnSourceAccountId: "default", + sessionKey: "agent:main:matrix:channel:!Ops:Example.org", + }, + createdAtMs: 0, + expiresAtMs: 1000, + }, + }); + + expect(target).toEqual({ + to: "room:!Ops:Example.org", + threadId: "$thread", + }); + }); + it("keeps plugin approval auth independent from exec approvers", () => { const cfg = buildConfig({ dm: { allowFrom: ["@owner:example.org"] }, diff --git a/extensions/matrix/src/approval-native.ts b/extensions/matrix/src/approval-native.ts index f06e797f21b..e33dd72ba81 100644 --- a/extensions/matrix/src/approval-native.ts +++ b/extensions/matrix/src/approval-native.ts @@ -27,7 +27,10 @@ function normalizeComparableTarget(value: string): string { if (!target) { return value.trim().toLowerCase(); } - return `${target.kind}:${target.id}`.toLowerCase(); + if (target.kind === "user") { + return `user:${normalizeMatrixUserId(target.id)}`; + } + return `${target.kind.toLowerCase()}:${target.id}`; } function resolveMatrixNativeTarget(raw: string): string | null { @@ -127,6 +130,17 @@ const splitMatrixApprovalCapability = splitChannelApprovalCapability( matrixNativeApprovalCapability, ); const matrixBaseNativeApprovalAdapter = splitMatrixApprovalCapability.native; +const matrixBaseDeliveryAdapter = splitMatrixApprovalCapability.delivery; +type MatrixForwardingSuppressionParams = Parameters< + NonNullable["shouldSuppressForwardingFallback"]> +>[0]; +const matrixDeliveryAdapter = matrixBaseDeliveryAdapter && { + ...matrixBaseDeliveryAdapter, + shouldSuppressForwardingFallback: (params: MatrixForwardingSuppressionParams) => + params.approvalKind === "plugin" + ? false + : (matrixBaseDeliveryAdapter.shouldSuppressForwardingFallback?.(params) ?? false), +}; const matrixExecOnlyNativeApprovalAdapter = matrixBaseNativeApprovalAdapter && { describeDeliveryCapabilities: ( params: Parameters[0], @@ -176,7 +190,7 @@ export const matrixApprovalCapability = createChannelApprovalCapability({ ); }, approvals: { - delivery: matrixNativeApprovalCapability.delivery, + delivery: matrixDeliveryAdapter, native: matrixExecOnlyNativeApprovalAdapter, render: matrixNativeApprovalCapability.render, }, @@ -187,7 +201,7 @@ export const matrixNativeApprovalAdapter = { authorizeActorAction: matrixApprovalCapability.authorizeActorAction, getActionAvailabilityState: matrixApprovalCapability.getActionAvailabilityState, }, - delivery: matrixApprovalCapability.delivery, + delivery: matrixDeliveryAdapter, render: matrixApprovalCapability.render, native: matrixExecOnlyNativeApprovalAdapter, }; diff --git a/extensions/matrix/src/exec-approvals-handler.test.ts b/extensions/matrix/src/exec-approvals-handler.test.ts index e29c848763c..2bd07b6af64 100644 --- a/extensions/matrix/src/exec-approvals-handler.test.ts +++ b/extensions/matrix/src/exec-approvals-handler.test.ts @@ -223,4 +223,37 @@ describe("MatrixExecApprovalHandler", () => { }), ); }); + + it("honors request decision constraints in pending approval text", async () => { + const cfg = { + channels: { + matrix: { + homeserver: "https://matrix.example.org", + userId: "@bot:example.org", + accessToken: "tok", + execApprovals: { + enabled: true, + approvers: ["@owner:example.org"], + target: "channel", + }, + }, + }, + } as OpenClawConfig; + const { handler, sendMessage } = createHandler(cfg); + + await handler.handleRequested({ + ...baseRequest, + request: { + ...baseRequest.request, + ask: "always", + allowedDecisions: ["allow-once", "deny"], + }, + }); + + expect(sendMessage).toHaveBeenCalledWith( + "room:!ops:example.org", + expect.not.stringContaining("allow-always"), + expect.anything(), + ); + }); }); diff --git a/extensions/matrix/src/exec-approvals-handler.ts b/extensions/matrix/src/exec-approvals-handler.ts index f7f684ce6fe..e2a514c5f72 100644 --- a/extensions/matrix/src/exec-approvals-handler.ts +++ b/extensions/matrix/src/exec-approvals-handler.ts @@ -65,11 +65,15 @@ function buildPendingApprovalText(params: { request: ApprovalRequest; nowMs: num approvalId: params.request.id, approvalSlug: params.request.id.slice(0, 8), approvalCommandId: params.request.id, + ask: params.request.request.ask ?? undefined, + agentId: params.request.request.agentId ?? undefined, + allowedDecisions: params.request.request.allowedDecisions, command: resolveExecApprovalCommandDisplay((params.request as ExecApprovalRequest).request) .commandText, cwd: (params.request as ExecApprovalRequest).request.cwd ?? undefined, host: (params.request as ExecApprovalRequest).request.host === "node" ? "node" : "gateway", nodeId: (params.request as ExecApprovalRequest).request.nodeId ?? undefined, + sessionKey: params.request.request.sessionKey ?? undefined, expiresAtMs: params.request.expiresAtMs, nowMs: params.nowMs, }).text!; diff --git a/extensions/matrix/src/exec-approvals.test.ts b/extensions/matrix/src/exec-approvals.test.ts index 0d9ba48e814..18a2d5a1162 100644 --- a/extensions/matrix/src/exec-approvals.test.ts +++ b/extensions/matrix/src/exec-approvals.test.ts @@ -32,6 +32,11 @@ function buildConfig( describe("matrix exec approvals", () => { it("requires enablement and an explicit or inferred approver", () => { expect(isMatrixExecApprovalClientEnabled({ cfg: buildConfig() })).toBe(false); + expect( + isMatrixExecApprovalClientEnabled({ + cfg: buildConfig(undefined, { dm: { allowFrom: ["@owner:example.org"] } }), + }), + ).toBe(false); expect(isMatrixExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true }) })).toBe(false); expect( isMatrixExecApprovalClientEnabled({ @@ -56,6 +61,13 @@ describe("matrix exec approvals", () => { expect(isMatrixExecApprovalApprover({ cfg, senderId: "@owner:example.org" })).toBe(false); }); + it("ignores wildcard allowlist entries when inferring exec approvers", () => { + const cfg = buildConfig({ enabled: true }, { dm: { allowFrom: ["*"] } }); + + expect(getMatrixExecApprovalApprovers({ cfg })).toEqual([]); + expect(isMatrixExecApprovalClientEnabled({ cfg })).toBe(false); + }); + it("defaults target to dm", () => { expect( resolveMatrixExecApprovalTarget({ @@ -102,6 +114,8 @@ describe("matrix exec approvals", () => { execApproval: { approvalId: "req-1", approvalSlug: "req-1", + agentId: "ops-agent", + sessionKey: "agent:ops-agent:matrix:channel:!ops:example.org", }, }, }; @@ -121,6 +135,30 @@ describe("matrix exec approvals", () => { ).toBe(false); }); + it("keeps local prompts when filters exclude the request", () => { + const payload = { + channelData: { + execApproval: { + approvalId: "req-1", + approvalSlug: "req-1", + agentId: "other-agent", + sessionKey: "agent:other-agent:matrix:channel:!ops:example.org", + }, + }, + }; + + expect( + shouldSuppressLocalMatrixExecApprovalPrompt({ + cfg: buildConfig({ + enabled: true, + approvers: ["@owner:example.org"], + agentFilter: ["ops-agent"], + }), + payload, + }), + ).toBe(false); + }); + it("normalizes prefixed approver ids", () => { expect(normalizeMatrixApproverId("matrix:@owner:example.org")).toBe("@owner:example.org"); expect(normalizeMatrixApproverId("user:@owner:example.org")).toBe("@owner:example.org"); diff --git a/extensions/matrix/src/exec-approvals.ts b/extensions/matrix/src/exec-approvals.ts index f62a30fae6f..14b7f7d69e4 100644 --- a/extensions/matrix/src/exec-approvals.ts +++ b/extensions/matrix/src/exec-approvals.ts @@ -1,11 +1,13 @@ import { createChannelExecApprovalProfile, + getExecApprovalReplyMetadata, isChannelExecApprovalTargetRecipient, resolveApprovalRequestAccountId, resolveApprovalApprovers, } from "openclaw/plugin-sdk/approval-runtime"; import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import type { ExecApprovalRequest, PluginApprovalRequest } from "openclaw/plugin-sdk/infra-runtime"; +import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; import { normalizeAccountId } from "openclaw/plugin-sdk/routing"; import { resolveMatrixAccount } from "./matrix/accounts.js"; import { normalizeMatrixUserId } from "./matrix/monitor/allowlist.js"; @@ -17,6 +19,25 @@ export function normalizeMatrixApproverId(value: string | number): string | unde return normalized || undefined; } +function normalizeMatrixExecApproverId(value: string | number): string | undefined { + const normalized = normalizeMatrixApproverId(value); + return normalized === "*" ? undefined : normalized; +} + +function resolveMatrixExecApprovalConfig(params: { + cfg: OpenClawConfig; + accountId?: string | null; +}) { + const config = resolveMatrixAccount(params).config.execApprovals; + if (!config) { + return { enabled: false } as const; + } + return { + ...config, + enabled: config.enabled === true, + }; +} + export function getMatrixExecApprovalApprovers(params: { cfg: OpenClawConfig; accountId?: string | null; @@ -25,7 +46,7 @@ export function getMatrixExecApprovalApprovers(params: { return resolveApprovalApprovers({ explicit: account.execApprovals?.approvers, allowFrom: account.dm?.allowFrom, - normalizeApprover: normalizeMatrixApproverId, + normalizeApprover: normalizeMatrixExecApproverId, }); } @@ -44,7 +65,7 @@ export function isMatrixExecApprovalTargetRecipient(params: { } const matrixExecApprovalProfile = createChannelExecApprovalProfile({ - resolveConfig: (params) => resolveMatrixAccount(params).config.execApprovals, + resolveConfig: resolveMatrixExecApprovalConfig, resolveApprovers: getMatrixExecApprovalApprovers, normalizeSenderId: normalizeMatrixApproverId, isTargetRecipient: isMatrixExecApprovalTargetRecipient, @@ -68,5 +89,49 @@ export const isMatrixExecApprovalApprover = matrixExecApprovalProfile.isApprover export const isMatrixExecApprovalAuthorizedSender = matrixExecApprovalProfile.isAuthorizedSender; export const resolveMatrixExecApprovalTarget = matrixExecApprovalProfile.resolveTarget; export const shouldHandleMatrixExecApprovalRequest = matrixExecApprovalProfile.shouldHandleRequest; -export const shouldSuppressLocalMatrixExecApprovalPrompt = - matrixExecApprovalProfile.shouldSuppressLocalPrompt; + +function buildFilterCheckRequest(params: { + approvalId: string; + payload: ReplyPayload; +}): ExecApprovalRequest | null { + const metadata = getExecApprovalReplyMetadata(params.payload); + if (!metadata) { + return null; + } + return { + id: params.approvalId, + request: { + command: "", + agentId: metadata.agentId ?? null, + sessionKey: metadata.sessionKey ?? null, + }, + createdAtMs: 0, + expiresAtMs: 0, + }; +} + +export function shouldSuppressLocalMatrixExecApprovalPrompt(params: { + cfg: OpenClawConfig; + accountId?: string | null; + payload: ReplyPayload; +}): boolean { + if (!matrixExecApprovalProfile.shouldSuppressLocalPrompt(params)) { + return false; + } + const metadata = getExecApprovalReplyMetadata(params.payload); + if (!metadata) { + return false; + } + const request = buildFilterCheckRequest({ + approvalId: metadata.approvalId, + payload: params.payload, + }); + if (!request) { + return false; + } + return shouldHandleMatrixExecApprovalRequest({ + cfg: params.cfg, + accountId: params.accountId, + request, + }); +} diff --git a/extensions/slack/src/approval-native.test.ts b/extensions/slack/src/approval-native.test.ts index fe18afd91f7..bfac4de1eb3 100644 --- a/extensions/slack/src/approval-native.test.ts +++ b/extensions/slack/src/approval-native.test.ts @@ -309,6 +309,7 @@ describe("slack native approval adapter", () => { expect( shouldSuppress({ cfg: buildConfig(), + approvalKind: "exec", target: { channel: "slack", to: "channel:C123ROOM", accountId: "default" }, request: { id: "approval-1", @@ -326,6 +327,7 @@ describe("slack native approval adapter", () => { expect( shouldSuppress({ cfg: buildConfig(), + approvalKind: "exec", target: { channel: "slack", to: "channel:C123ROOM", accountId: "default" }, request: { id: "approval-1", diff --git a/src/channels/plugins/types.adapters.ts b/src/channels/plugins/types.adapters.ts index 20a0dccdb2f..975628c48ba 100644 --- a/src/channels/plugins/types.adapters.ts +++ b/src/channels/plugins/types.adapters.ts @@ -512,6 +512,7 @@ export type ChannelApprovalDeliveryAdapter = { hasConfiguredDmRoute?: (params: { cfg: OpenClawConfig }) => boolean; shouldSuppressForwardingFallback?: (params: { cfg: OpenClawConfig; + approvalKind: ChannelApprovalKind; target: ChannelApprovalForwardTarget; request: ExecApprovalRequest; }) => boolean; diff --git a/src/infra/exec-approval-forwarder.ts b/src/infra/exec-approval-forwarder.ts index 106b97c1cca..a4acff6e515 100644 --- a/src/infra/exec-approval-forwarder.ts +++ b/src/infra/exec-approval-forwarder.ts @@ -169,6 +169,7 @@ function buildSyntheticApprovalRequest(routeRequest: ApprovalRouteRequest): Exec } function shouldSkipForwardingFallback(params: { + approvalKind: "exec" | "plugin"; target: ExecApprovalForwardTarget; cfg: OpenClawConfig; routeRequest: ApprovalRouteRequest; @@ -181,6 +182,7 @@ function shouldSkipForwardingFallback(params: { return ( adapter?.delivery?.shouldSuppressForwardingFallback?.({ cfg: params.cfg, + approvalKind: params.approvalKind, target: params.target, request: buildSyntheticApprovalRequest(params.routeRequest), }) ?? false @@ -502,8 +504,15 @@ function createApprovalHandlers< resolveSessionTarget: params.resolveSessionTarget, }) : []), - ].filter((target) => !shouldSkipForwardingFallback({ target, cfg, routeRequest })); - + ].filter( + (target) => + !shouldSkipForwardingFallback({ + approvalKind: params.strategy.kind, + target, + cfg, + routeRequest, + }), + ); if (filteredTargets.length === 0) { return false; } @@ -598,7 +607,15 @@ function createApprovalHandlers< resolveSessionTarget: params.resolveSessionTarget, }) : []), - ].filter((target) => !shouldSkipForwardingFallback({ target, cfg, routeRequest })); + ].filter( + (target) => + !shouldSkipForwardingFallback({ + approvalKind: params.strategy.kind, + target, + cfg, + routeRequest, + }), + ); } } if (!targets?.length) { diff --git a/src/infra/exec-approval-reply.test.ts b/src/infra/exec-approval-reply.test.ts index 5cbb7905b91..ec7c1dc4287 100644 --- a/src/infra/exec-approval-reply.test.ts +++ b/src/infra/exec-approval-reply.test.ts @@ -76,14 +76,18 @@ describe("exec approval reply helpers", () => { execApproval: { approvalId: " req-1 ", approvalSlug: " slug-1 ", + agentId: " agent-1 ", allowedDecisions: ["allow-once", "bad", "deny", "allow-always", 3], + sessionKey: " session-1 ", }, }, }), ).toEqual({ approvalId: "req-1", approvalSlug: "slug-1", + agentId: "agent-1", allowedDecisions: ["allow-once", "deny", "allow-always"], + sessionKey: "session-1", }); }); @@ -104,7 +108,9 @@ describe("exec approval reply helpers", () => { execApproval: { approvalId: "req-1", approvalSlug: "slug-1", + agentId: undefined, allowedDecisions: ["allow-once", "allow-always", "deny"], + sessionKey: undefined, }, }); expect(payload.interactive).toEqual({ @@ -180,6 +186,27 @@ describe("exec approval reply helpers", () => { }); }); + it("stores agent and session metadata for downstream suppression checks", () => { + const payload = buildExecApprovalPendingReplyPayload({ + approvalId: "req-meta", + approvalSlug: "slug-meta", + agentId: "ops-agent", + sessionKey: "agent:ops-agent:matrix:channel:!room:example.org", + command: "echo ok", + host: "gateway", + }); + + expect(payload.channelData).toEqual({ + execApproval: { + approvalId: "req-meta", + approvalSlug: "slug-meta", + agentId: "ops-agent", + allowedDecisions: ["allow-once", "allow-always", "deny"], + sessionKey: "agent:ops-agent:matrix:channel:!room:example.org", + }, + }); + }); + it("uses a longer fence for commands containing triple backticks", () => { const payload = buildExecApprovalPendingReplyPayload({ approvalId: "req-2", diff --git a/src/infra/exec-approval-reply.ts b/src/infra/exec-approval-reply.ts index 9583d420351..1f4c4189644 100644 --- a/src/infra/exec-approval-reply.ts +++ b/src/infra/exec-approval-reply.ts @@ -15,7 +15,9 @@ export type ExecApprovalUnavailableReason = export type ExecApprovalReplyMetadata = { approvalId: string; approvalSlug: string; + agentId?: string; allowedDecisions?: readonly ExecApprovalReplyDecision[]; + sessionKey?: string; }; export type ExecApprovalActionDescriptor = { @@ -31,11 +33,13 @@ export type ExecApprovalPendingReplyParams = { approvalSlug: string; approvalCommandId?: string; ask?: string | null; + agentId?: string | null; allowedDecisions?: readonly ExecApprovalReplyDecision[]; command: string; cwd?: string; host: ExecHost; nodeId?: string; + sessionKey?: string | null; expiresAtMs?: number; nowMs?: number; }; @@ -231,10 +235,16 @@ export function getExecApprovalReplyMetadata( value === "allow-once" || value === "allow-always" || value === "deny", ) : undefined; + const agentId = + typeof record.agentId === "string" ? record.agentId.trim() || undefined : undefined; + const sessionKey = + typeof record.sessionKey === "string" ? record.sessionKey.trim() || undefined : undefined; return { approvalId, approvalSlug, + agentId, allowedDecisions, + sessionKey, }; } @@ -297,7 +307,9 @@ export function buildExecApprovalPendingReplyPayload( execApproval: { approvalId: params.approvalId, approvalSlug: params.approvalSlug, + agentId: params.agentId?.trim() || undefined, allowedDecisions, + sessionKey: params.sessionKey?.trim() || undefined, }, }, }; diff --git a/src/plugin-sdk/approval-delivery-helpers.test.ts b/src/plugin-sdk/approval-delivery-helpers.test.ts index a5cf871ed01..d26563378d6 100644 --- a/src/plugin-sdk/approval-delivery-helpers.test.ts +++ b/src/plugin-sdk/approval-delivery-helpers.test.ts @@ -165,6 +165,7 @@ describe("createApproverRestrictedNativeApprovalAdapter", () => { expect( shouldSuppressForwardingFallback({ cfg: {} as never, + approvalKind: "exec", target: { channel: "telegram", to: "target-1" }, request: { request: { @@ -179,6 +180,7 @@ describe("createApproverRestrictedNativeApprovalAdapter", () => { expect( shouldSuppressForwardingFallback({ cfg: {} as never, + approvalKind: "exec", target: { channel: "telegram", to: "target-1" }, request: { request: { @@ -193,6 +195,7 @@ describe("createApproverRestrictedNativeApprovalAdapter", () => { expect( shouldSuppressForwardingFallback({ cfg: {} as never, + approvalKind: "exec", target: { channel: "slack", to: "target-1" }, request: { request: { @@ -208,6 +211,21 @@ describe("createApproverRestrictedNativeApprovalAdapter", () => { cfg: {} as never, accountId: "topic-1", }); + + expect( + shouldSuppressForwardingFallback({ + cfg: {} as never, + approvalKind: "plugin", + target: { channel: "telegram", to: "target-1" }, + request: { + request: { + command: "pwd", + turnSourceChannel: "telegram", + turnSourceAccountId: "topic-1", + }, + } as never, + }), + ).toBe(false); }); }); diff --git a/src/plugin-sdk/approval-delivery-helpers.ts b/src/plugin-sdk/approval-delivery-helpers.ts index f6e6288ad18..abc80e7cb0c 100644 --- a/src/plugin-sdk/approval-delivery-helpers.ts +++ b/src/plugin-sdk/approval-delivery-helpers.ts @@ -18,6 +18,7 @@ type ApprovalAdapterParams = { type DeliverySuppressionParams = { cfg: OpenClawConfig; + approvalKind: ApprovalKind; target: { channel: string; accountId?: string | null }; request: { request: { turnSourceChannel?: string | null; turnSourceAccountId?: string | null } }; }; @@ -126,6 +127,9 @@ function buildApproverRestrictedNativeApprovalCapability( (resolvedAccountId === undefined ? input.target.accountId?.trim() : resolvedAccountId.trim()) || undefined; + if (input.approvalKind === "plugin") { + return false; + } return params.isNativeDeliveryEnabled({ cfg: input.cfg, accountId }); }, },