From 0a105c0900de701d2ee9f1abc96b017afbd0afdd Mon Sep 17 00:00:00 2001 From: Pavan Kumar Gondhi Date: Mon, 13 Apr 2026 12:00:13 +0530 Subject: [PATCH] fix(approval-auth): prevent empty approver list from granting explicit approval authorization [AI] (#65714) * fix: address issue * fix: address PR review feedback * fix: address PR review feedback * docs: add changelog entry for PR merge --- CHANGELOG.md | 1 + src/auto-reply/reply/commands-approve.test.ts | 59 ++++++++++++++++++- src/infra/channel-approval-auth.test.ts | 39 ++++++++++++ src/infra/channel-approval-auth.ts | 8 ++- src/plugin-sdk/approval-auth-helpers.test.ts | 37 +++++++++++- src/plugin-sdk/approval-auth-helpers.ts | 37 +++++++++++- 6 files changed, 177 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38e1a6a2ac8..c02fad47ef7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(approval-auth): prevent empty approver list from granting explicit approval authorization [AI]. (#65714) Thanks @pgondhi987. - fix(security): broaden shell-wrapper detection and block env-argv assignment injection [AI-assisted]. (#65717) Thanks @pgondhi987. - Gateway/startup: defer scheduled services until sidecars finish, gate chat history and model listing during sidecar resume, and let Control UI retry startup-gated history loads so Sandbox wake resumes channels first. (#65365) Thanks @lml2468. - Control UI/chat: load the live gateway slash-command catalog into the composer and command palette so dock commands, plugin commands, and direct skill aliases appear in chat, while keeping trusted local commands authoritative and bounding remote command metadata. (#65620) Thanks @BunsDev. diff --git a/src/auto-reply/reply/commands-approve.test.ts b/src/auto-reply/reply/commands-approve.test.ts index 06ad62a9906..e219d5c49a7 100644 --- a/src/auto-reply/reply/commands-approve.test.ts +++ b/src/auto-reply/reply/commands-approve.test.ts @@ -115,7 +115,8 @@ const signalApproveTestPlugin: ChannelPlugin = { approvalCapability: createResolvedApproverActionAuthAdapter({ channelLabel: "Signal", resolveApprovers: ({ cfg, accountId }) => { - const signal = accountId ? cfg.channels?.signal?.accounts?.[accountId] : cfg.channels?.signal; + const scopedSignal = accountId ? cfg.channels?.signal?.accounts?.[accountId] : undefined; + const signal = scopedSignal ?? cfg.channels?.signal; return resolveApprovalApprovers({ allowFrom: signal?.allowFrom, defaultTo: signal?.defaultTo, @@ -643,6 +644,62 @@ describe("handleApproveCommand", () => { expect(callGatewayMock).not.toHaveBeenCalled(); }); + it("does not allow empty helper approvers to bypass unauthorized sender checks", async () => { + const params = buildApproveParams( + "/approve abc12345 allow-once", + { + commands: { text: true }, + channels: { + signal: { + allowFrom: [], + }, + }, + } as OpenClawConfig, + { + Provider: "signal", + Surface: "signal", + SenderId: "+15551239999", + }, + ); + params.command.isAuthorizedSender = false; + + const result = await handleApproveCommand(params, true); + expect(result?.shouldContinue).toBe(false); + expect(result?.reply).toBeUndefined(); + expect(callGatewayMock).not.toHaveBeenCalled(); + }); + + it("keeps same-chat /approve available to authorized senders when helper approvers are empty", async () => { + callGatewayMock.mockResolvedValue({ ok: true }); + const params = buildApproveParams( + "/approve abc12345 allow-once", + { + commands: { text: true }, + channels: { + signal: { + allowFrom: [], + }, + }, + } as OpenClawConfig, + { + Provider: "signal", + Surface: "signal", + SenderId: "+15551239999", + }, + ); + params.command.isAuthorizedSender = true; + + const result = await handleApproveCommand(params, true); + expect(result?.shouldContinue).toBe(false); + expect(result?.reply?.text).toContain("Approval allow-once submitted"); + expect(callGatewayMock).toHaveBeenCalledWith( + expect.objectContaining({ + method: "exec.approval.resolve", + params: { id: "abc12345", decision: "allow-once" }, + }), + ); + }); + it("accepts Telegram /approve from exec target recipients when native approvals are disabled", async () => { const params = buildApproveParams( "/approve abc12345 allow-once", diff --git a/src/infra/channel-approval-auth.test.ts b/src/infra/channel-approval-auth.test.ts index d2908a4d1c3..e502b5f8b97 100644 --- a/src/infra/channel-approval-auth.test.ts +++ b/src/infra/channel-approval-auth.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createResolvedApproverActionAuthAdapter } from "../plugin-sdk/approval-auth-helpers.js"; const getChannelPluginMock = vi.hoisted(() => vi.fn()); @@ -116,4 +117,42 @@ describe("resolveApprovalCommandAuthorization", () => { approvalKind: "exec", }); }); + + it("keeps empty approver fallback implicit without bypassing channel sender auth", () => { + getChannelPluginMock.mockReturnValue({ + approvalCapability: createResolvedApproverActionAuthAdapter({ + channelLabel: "Signal", + resolveApprovers: () => [], + }), + }); + + expect( + resolveApprovalCommandAuthorization({ + cfg: {} as never, + channel: "signal", + accountId: "work", + senderId: "uuid:attacker", + kind: "exec", + }), + ).toEqual({ authorized: true, explicit: false }); + }); + + it("keeps configured approvers explicit when sender matches", () => { + getChannelPluginMock.mockReturnValue({ + approvalCapability: createResolvedApproverActionAuthAdapter({ + channelLabel: "Signal", + resolveApprovers: () => ["uuid:owner"], + }), + }); + + expect( + resolveApprovalCommandAuthorization({ + cfg: {} as never, + channel: "signal", + accountId: "work", + senderId: "uuid:owner", + kind: "exec", + }), + ).toEqual({ authorized: true, explicit: true }); + }); }); diff --git a/src/infra/channel-approval-auth.ts b/src/infra/channel-approval-auth.ts index ff3b045fa98..6793fde928b 100644 --- a/src/infra/channel-approval-auth.ts +++ b/src/infra/channel-approval-auth.ts @@ -1,5 +1,6 @@ import { getChannelPlugin, resolveChannelApprovalCapability } from "../channels/plugins/index.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { isImplicitSameChatApprovalAuthorization } from "../plugin-sdk/approval-auth-helpers.js"; import { normalizeMessageChannel } from "../utils/message-channel.js"; export type ApprovalCommandAuthorization = { @@ -30,6 +31,9 @@ export function resolveApprovalCommandAuthorization(params: { if (!resolved) { return { authorized: true, explicit: false }; } + // Keep `resolved` by reference; cloning before this check would drop the + // non-enumerable implicit-fallback marker. + const implicitSameChatAuthorization = isImplicitSameChatApprovalAuthorization(resolved); const availability = approvalCapability?.getActionAvailabilityState?.({ cfg: params.cfg, accountId: params.accountId, @@ -39,6 +43,8 @@ export function resolveApprovalCommandAuthorization(params: { return { authorized: resolved.authorized, reason: resolved.reason, - explicit: resolved.authorized ? availability?.kind !== "disabled" : true, + explicit: resolved.authorized + ? !implicitSameChatAuthorization && availability?.kind !== "disabled" + : true, }; } diff --git a/src/plugin-sdk/approval-auth-helpers.test.ts b/src/plugin-sdk/approval-auth-helpers.test.ts index cc00b88666e..c43846cb8ae 100644 --- a/src/plugin-sdk/approval-auth-helpers.test.ts +++ b/src/plugin-sdk/approval-auth-helpers.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from "vitest"; -import { createResolvedApproverActionAuthAdapter } from "./approval-auth-helpers.js"; +import { + createResolvedApproverActionAuthAdapter, + isImplicitSameChatApprovalAuthorization, +} from "./approval-auth-helpers.js"; describe("createResolvedApproverActionAuthAdapter", () => { it.each([ @@ -55,4 +58,36 @@ describe("createResolvedApproverActionAuthAdapter", () => { ).toEqual(testCase.expected); } }); + + it("marks empty-approver fallback auth as implicit", () => { + const auth = createResolvedApproverActionAuthAdapter({ + channelLabel: "Signal", + resolveApprovers: () => [], + }); + const result = auth.authorizeActorAction({ + cfg: {}, + senderId: "uuid:attacker", + action: "approve", + approvalKind: "exec", + }); + + expect(result).toEqual({ authorized: true }); + expect(isImplicitSameChatApprovalAuthorization(result)).toBe(true); + }); + + it("does not mark configured-approver auth as implicit", () => { + const auth = createResolvedApproverActionAuthAdapter({ + channelLabel: "Signal", + resolveApprovers: () => ["uuid:owner"], + }); + const result = auth.authorizeActorAction({ + cfg: {}, + senderId: "uuid:owner", + action: "approve", + approvalKind: "exec", + }); + + expect(result).toEqual({ authorized: true }); + expect(isImplicitSameChatApprovalAuthorization(result)).toBe(false); + }); }); diff --git a/src/plugin-sdk/approval-auth-helpers.ts b/src/plugin-sdk/approval-auth-helpers.ts index fdf0690bdc0..a15f8ed48c6 100644 --- a/src/plugin-sdk/approval-auth-helpers.ts +++ b/src/plugin-sdk/approval-auth-helpers.ts @@ -2,6 +2,40 @@ import { normalizeOptionalString } from "../shared/string-coerce.js"; import type { OpenClawConfig } from "./config-runtime.js"; type ApprovalKind = "exec" | "plugin"; +type ApprovalAuthorizationResult = { + authorized: boolean; + reason?: string; +}; +const IMPLICIT_SAME_CHAT_APPROVAL_AUTHORIZATION = Symbol( + "openclaw.implicitSameChatApprovalAuthorization", +); + +function markImplicitSameChatApprovalAuthorization( + result: ApprovalAuthorizationResult, +): ApprovalAuthorizationResult { + // Keep this non-enumerable to avoid changing auth payload shape. + // Consumers must pass the same object reference to + // `isImplicitSameChatApprovalAuthorization`; spread/Object.assign/JSON clones + // drop this marker. + Object.defineProperty(result, IMPLICIT_SAME_CHAT_APPROVAL_AUTHORIZATION, { + value: true, + enumerable: false, + }); + return result; +} + +export function isImplicitSameChatApprovalAuthorization( + result: ApprovalAuthorizationResult | null | undefined, +): boolean { + return Boolean( + result && + ( + result as ApprovalAuthorizationResult & { + [IMPLICIT_SAME_CHAT_APPROVAL_AUTHORIZATION]?: true; + } + )[IMPLICIT_SAME_CHAT_APPROVAL_AUTHORIZATION], + ); +} export function createResolvedApproverActionAuthAdapter(params: { channelLabel: string; @@ -25,7 +59,8 @@ export function createResolvedApproverActionAuthAdapter(params: { }) { const approvers = params.resolveApprovers({ cfg, accountId }); if (approvers.length === 0) { - return { authorized: true } as const; + // Empty approver sets are implicit same-chat fallback, not explicit approver bypass. + return markImplicitSameChatApprovalAuthorization({ authorized: true }); } const normalizedSenderId = senderId ? normalizeSenderId(senderId) : undefined; if (normalizedSenderId && approvers.includes(normalizedSenderId)) {