diff --git a/CHANGELOG.md b/CHANGELOG.md index c1bf6b12351..9852d3be03a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- fix(qqbot): authorize approval button callbacks [AI]. (#80892) Thanks @pgondhi987. - Scrub streamable MCP redirect headers [AI]. (#80906) Thanks @pgondhi987. - fix(memory-wiki): require admin scope for ingest [AI]. (#80897) Thanks @pgondhi987. - memory-wiki: require write scope for Obsidian search [AI]. (#80904) Thanks @pgondhi987. diff --git a/extensions/qqbot/src/bridge/approval/capability.ts b/extensions/qqbot/src/bridge/approval/capability.ts index 3bdec806003..0beb76f566a 100644 --- a/extensions/qqbot/src/bridge/approval/capability.ts +++ b/extensions/qqbot/src/bridge/approval/capability.ts @@ -1,10 +1,9 @@ /** * QQ Bot Approval Capability — entry point. * - * QQBot uses a simpler approval model than Telegram/Slack: any user who - * can see the inline-keyboard buttons can approve. No explicit approver - * list is required — the bot simply sends the approval message to the - * originating conversation and whoever clicks the button resolves it. + * QQBot uses a simpler approval model than Telegram/Slack: when no + * approver list is configured, the bot sends the approval message to the + * originating conversation and any participant can approve from there. * * When `execApprovals` IS configured, it gates which requests are * handled natively and who is authorized. When it is NOT configured, @@ -23,9 +22,8 @@ import { isQQBotExecApprovalClientEnabled, matchesQQBotApprovalAccount, shouldHandleQQBotExecApprovalRequest, - isQQBotExecApprovalAuthorizedSender, - isQQBotExecApprovalApprover, resolveQQBotExecApprovalConfig, + authorizeQQBotApprovalAction, } from "../../exec-approvals.js"; import { ensurePlatformAdapter } from "../bootstrap.js"; import { resolveQQBotAccount } from "../config.js"; @@ -103,18 +101,8 @@ function canResolveTarget(request: { function createQQBotApprovalCapability(): ChannelApprovalCapability { return createChannelApprovalCapability({ - authorizeActorAction: ({ cfg, accountId, senderId, approvalKind }) => { - if (hasExecApprovalConfig({ cfg, accountId })) { - const authorized = - approvalKind === "plugin" - ? isQQBotExecApprovalApprover({ cfg, accountId, senderId }) - : isQQBotExecApprovalAuthorizedSender({ cfg, accountId, senderId }); - return authorized - ? { authorized: true } - : { authorized: false, reason: "You are not authorized to approve this request." }; - } - return { authorized: true }; - }, + authorizeActorAction: ({ cfg, accountId, senderId, approvalKind }) => + authorizeQQBotApprovalAction({ cfg, accountId, senderId, approvalKind }), getActionAvailabilityState: ({ cfg, diff --git a/extensions/qqbot/src/engine/gateway/gateway.ts b/extensions/qqbot/src/engine/gateway/gateway.ts index 66d505f2c57..fa8b0908609 100644 --- a/extensions/qqbot/src/engine/gateway/gateway.ts +++ b/extensions/qqbot/src/engine/gateway/gateway.ts @@ -181,7 +181,9 @@ export async function startGateway(ctx: CoreGatewayContext): Promise { } }; - const handleInteraction = createInteractionHandler(account, ctx.runtime, log); + const handleInteraction = createInteractionHandler(account, ctx.runtime, log, { + getActiveCfg: () => activeCfgProvider.getActiveCfg(), + }); const connection = new GatewayConnection({ account, diff --git a/extensions/qqbot/src/engine/gateway/interaction-handler.test.ts b/extensions/qqbot/src/engine/gateway/interaction-handler.test.ts new file mode 100644 index 00000000000..d2601628a1d --- /dev/null +++ b/extensions/qqbot/src/engine/gateway/interaction-handler.test.ts @@ -0,0 +1,206 @@ +import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { registerPlatformAdapter, type PlatformAdapter } from "../adapter/index.js"; +import type { InteractionEvent } from "../types.js"; +import { createInteractionHandler } from "./interaction-handler.js"; +import type { GatewayAccount, GatewayPluginRuntime } from "./types.js"; + +const acknowledgeInteractionMock = vi.hoisted(() => vi.fn(async () => undefined)); + +vi.mock("../messaging/sender.js", () => ({ + accountToCreds: (account: GatewayAccount) => ({ + appId: account.appId, + clientSecret: account.clientSecret, + }), + acknowledgeInteraction: acknowledgeInteractionMock, +})); + +const resolveApprovalMock = vi.fn(async () => true); + +const account: GatewayAccount = { + accountId: "default", + appId: "app", + clientSecret: "secret", + markdownSupport: false, + config: {}, +}; + +const runtime = {} as GatewayPluginRuntime; + +function makeRestrictedCfg(approvers: string[]): OpenClawConfig { + return { + channels: { + qqbot: { + appId: "app", + clientSecret: "secret", + execApprovals: { + enabled: true, + approvers, + }, + }, + }, + } as OpenClawConfig; +} + +function makeUnrestrictedCfg(): OpenClawConfig { + return { + channels: { + qqbot: { + appId: "app", + clientSecret: "secret", + }, + }, + } as OpenClawConfig; +} + +function makeApprovalEvent(overrides: Partial = {}): InteractionEvent { + return { + id: "interaction-1", + type: 11, + chat_type: 1, + group_openid: "group-1", + group_member_openid: "ATTACKER_OPENID", + version: 1, + data: { + type: 11, + resolved: { + button_data: "approve:exec:abc12345:allow-once", + user_id: "ATTACKER_USER_ID", + }, + }, + ...overrides, + }; +} + +function installPlatformAdapter(): void { + registerPlatformAdapter({ + validateRemoteUrl: vi.fn(async () => undefined), + resolveSecret: vi.fn(async (value: unknown) => (typeof value === "string" ? value : undefined)), + downloadFile: vi.fn(async () => "/tmp/file"), + fetchMedia: vi.fn(async () => { + throw new Error("unused"); + }), + getTempDir: () => "/tmp", + hasConfiguredSecret: (value: unknown) => typeof value === "string" && value.length > 0, + normalizeSecretInputString: (value: unknown) => (typeof value === "string" ? value : undefined), + resolveSecretInputString: ({ value }: { value: unknown }) => + typeof value === "string" ? value : undefined, + resolveApproval: resolveApprovalMock, + } as PlatformAdapter); +} + +describe("createInteractionHandler approval buttons", () => { + beforeEach(() => { + vi.clearAllMocks(); + installPlatformAdapter(); + }); + + it("rejects approval button clicks from users outside the configured approvers", async () => { + const handler = createInteractionHandler(account, runtime, undefined, { + getActiveCfg: () => makeRestrictedCfg(["OWNER_OPENID"]), + }); + + handler(makeApprovalEvent()); + + await vi.waitFor(() => expect(acknowledgeInteractionMock).toHaveBeenCalled()); + + expect(acknowledgeInteractionMock).toHaveBeenCalledWith( + { appId: "app", clientSecret: "secret" }, + "interaction-1", + 0, + { content: "You are not authorized to approve this request." }, + ); + expect(resolveApprovalMock).not.toHaveBeenCalled(); + }); + + it("does not authorize from resolved user id when the actor openid is not approved", async () => { + const handler = createInteractionHandler(account, runtime, undefined, { + getActiveCfg: () => makeRestrictedCfg(["OWNER_OPENID"]), + }); + + handler( + makeApprovalEvent({ + data: { + type: 11, + resolved: { + button_data: "approve:exec:abc12345:allow-once", + user_id: "OWNER_OPENID", + }, + }, + }), + ); + + await vi.waitFor(() => expect(acknowledgeInteractionMock).toHaveBeenCalled()); + + expect(acknowledgeInteractionMock).toHaveBeenCalledWith( + { appId: "app", clientSecret: "secret" }, + "interaction-1", + 0, + { content: "You are not authorized to approve this request." }, + ); + expect(resolveApprovalMock).not.toHaveBeenCalled(); + }); + + it("resolves approval button clicks from configured approvers", async () => { + const handler = createInteractionHandler(account, runtime, undefined, { + getActiveCfg: () => makeRestrictedCfg(["OWNER_OPENID"]), + }); + + handler(makeApprovalEvent({ group_member_openid: "OWNER_OPENID" })); + + await vi.waitFor(() => + expect(resolveApprovalMock).toHaveBeenCalledWith("exec:abc12345", "allow-once"), + ); + }); + + it("uses the direct user openid when a group member openid is unavailable", async () => { + const handler = createInteractionHandler(account, runtime, undefined, { + getActiveCfg: () => makeRestrictedCfg(["OWNER_OPENID"]), + }); + + handler( + makeApprovalEvent({ + chat_type: 2, + group_openid: undefined, + group_member_openid: undefined, + user_openid: "OWNER_OPENID", + }), + ); + + await vi.waitFor(() => + expect(resolveApprovalMock).toHaveBeenCalledWith("exec:abc12345", "allow-once"), + ); + }); + + it("allows approval button clicks when exec approvals are not configured", async () => { + const handler = createInteractionHandler(account, runtime, undefined, { + getActiveCfg: () => makeUnrestrictedCfg(), + }); + + handler(makeApprovalEvent()); + + await vi.waitFor(() => + expect(resolveApprovalMock).toHaveBeenCalledWith("exec:abc12345", "allow-once"), + ); + }); + + it("rejects approval button clicks when active config cannot be loaded", async () => { + const handler = createInteractionHandler(account, runtime, undefined, { + getActiveCfg: () => { + throw new Error("config unavailable"); + }, + }); + + handler(makeApprovalEvent()); + + await vi.waitFor(() => expect(acknowledgeInteractionMock).toHaveBeenCalled()); + + expect(acknowledgeInteractionMock).toHaveBeenCalledWith( + { appId: "app", clientSecret: "secret" }, + "interaction-1", + 0, + { content: "Approval is unavailable." }, + ); + expect(resolveApprovalMock).not.toHaveBeenCalled(); + }); +}); diff --git a/extensions/qqbot/src/engine/gateway/interaction-handler.ts b/extensions/qqbot/src/engine/gateway/interaction-handler.ts index bf5120c7b53..6cffcf0f62e 100644 --- a/extensions/qqbot/src/engine/gateway/interaction-handler.ts +++ b/extensions/qqbot/src/engine/gateway/interaction-handler.ts @@ -5,12 +5,14 @@ * * 1. **Config query** (type=2001) — reads config, ACKs with `claw_cfg`. * 2. **Config update** (type=2002) — writes config, ACKs with updated snapshot. - * 3. **Approval button** (other) — ACKs, resolves approval via PlatformAdapter. + * 3. **Approval button** (other) — ACKs, resolves authorized approval actions. * * Config query/update require `runtime.config`. When unavailable, those * branches fall through to a bare ACK (backward-compatible). */ +import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts"; +import { authorizeQQBotApprovalAction } from "../../exec-approvals.js"; import { resolveQQBotEffectivePolicies } from "../access/resolve-policy.js"; import { getPlatformAdapter } from "../adapter/index.js"; import { parseApprovalButtonData } from "../approval/index.js"; @@ -151,6 +153,7 @@ export function createInteractionHandler( account: GatewayAccount, runtime: GatewayPluginRuntime, log?: EngineLogger, + options?: { getActiveCfg?: () => OpenClawConfig }, ): (event: InteractionEvent) => void { return (event) => { const creds = accountToCreds(account); @@ -174,33 +177,155 @@ export function createInteractionHandler( } // ---- Approval button / other ---- - void acknowledgeInteraction(creds, event.id).catch((err) => { - log?.error(`Interaction ACK failed: ${err instanceof Error ? err.message : String(err)}`); - }); - const parsed = parseApprovalButtonData(event.data?.resolved?.button_data ?? ""); if (!parsed) { + void acknowledgeInteraction(creds, event.id).catch((err) => { + log?.error(`Interaction ACK failed: ${err instanceof Error ? err.message : String(err)}`); + }); return; } - const adapter = getPlatformAdapter(); - if (!adapter.resolveApproval) { - log?.error("resolveApproval not available on PlatformAdapter"); - return; - } - - void adapter.resolveApproval(parsed.approvalId, parsed.decision).then((ok) => { - if (ok) { - log?.info(`Approval resolved: id=${parsed.approvalId}, decision=${parsed.decision}`); - } else { - log?.error(`Approval resolve failed: id=${parsed.approvalId}`); - } + void handleApprovalButtonInteraction({ + accountId: account.accountId, + creds, + event, + getActiveCfg: options?.getActiveCfg ?? runtime.config?.current, + log, + parsed, }); }; } // ============ Helpers ============ +async function handleApprovalButtonInteraction(params: { + accountId: string; + creds: { appId: string; clientSecret: string }; + event: InteractionEvent; + getActiveCfg?: () => OpenClawConfig | Record; + log?: EngineLogger; + parsed: { approvalId: string; decision: "allow-once" | "allow-always" | "deny" }; +}): Promise { + if (!params.getActiveCfg) { + await acknowledgeApprovalInteraction(params.creds, params.event, params.log, { + content: "Approval is unavailable.", + }); + params.log?.error("Approval button rejected: active config is unavailable"); + return; + } + + let cfg: OpenClawConfig; + try { + cfg = params.getActiveCfg() as OpenClawConfig; + } catch (err) { + await acknowledgeApprovalInteraction(params.creds, params.event, params.log, { + content: "Approval is unavailable.", + }); + params.log?.error( + `Approval button rejected: active config failed to load: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + return; + } + + const authorization = authorizeApprovalButtonActor({ + cfg, + accountId: params.accountId, + event: params.event, + approvalKind: resolveApprovalKind(params.parsed.approvalId), + }); + if (!authorization.authorized) { + await acknowledgeApprovalInteraction(params.creds, params.event, params.log, { + content: authorization.reason ?? "You are not authorized to approve this request.", + }); + params.log?.info(`Approval button rejected: id=${params.parsed.approvalId}`); + return; + } + + await acknowledgeApprovalInteraction(params.creds, params.event, params.log); + + const adapter = getPlatformAdapter(); + if (!adapter.resolveApproval) { + params.log?.error("resolveApproval not available on PlatformAdapter"); + return; + } + + try { + const ok = await adapter.resolveApproval(params.parsed.approvalId, params.parsed.decision); + if (ok) { + params.log?.info( + `Approval resolved: id=${params.parsed.approvalId}, decision=${params.parsed.decision}`, + ); + } else { + params.log?.error(`Approval resolve failed: id=${params.parsed.approvalId}`); + } + } catch (err) { + params.log?.error( + `Approval resolve failed: id=${params.parsed.approvalId}: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } +} + +async function acknowledgeApprovalInteraction( + creds: { appId: string; clientSecret: string }, + event: InteractionEvent, + log: EngineLogger | undefined, + data?: Record, +): Promise { + try { + await acknowledgeInteraction(creds, event.id, 0, data); + } catch (err) { + log?.error(`Interaction ACK failed: ${err instanceof Error ? err.message : String(err)}`); + } +} + +function authorizeApprovalButtonActor(params: { + cfg: OpenClawConfig; + accountId: string; + event: InteractionEvent; + approvalKind: "exec" | "plugin"; +}): { authorized: boolean; reason?: string } { + const senderIds = resolveApprovalActorSenderIds(params.event); + if (senderIds.length === 0) { + return authorizeQQBotApprovalAction({ + cfg: params.cfg, + accountId: params.accountId, + senderId: null, + approvalKind: params.approvalKind, + }); + } + + let denial: { authorized: boolean; reason?: string } | undefined; + for (const senderId of senderIds) { + const result = authorizeQQBotApprovalAction({ + cfg: params.cfg, + accountId: params.accountId, + senderId, + approvalKind: params.approvalKind, + }); + if (result.authorized) { + return result; + } + denial ??= result; + } + return denial ?? { authorized: false, reason: "You are not authorized to approve this request." }; +} + +function resolveApprovalActorSenderIds(event: InteractionEvent): string[] { + const ids = [event.group_member_openid, event.user_openid].flatMap((value) => { + const normalized = typeof value === "string" ? value.trim() : ""; + return normalized ? [normalized] : []; + }); + return Array.from(new Set(ids)); +} + +function resolveApprovalKind(approvalId: string): "exec" | "plugin" { + return approvalId.toLowerCase().startsWith("plugin:") ? "plugin" : "exec"; +} + /** Execute an async handler, ACK with the result, and handle errors. */ async function handleWithAck( creds: { appId: string; clientSecret: string }, diff --git a/extensions/qqbot/src/exec-approvals.ts b/extensions/qqbot/src/exec-approvals.ts index 42ed163bb07..96a7f1d71ad 100644 --- a/extensions/qqbot/src/exec-approvals.ts +++ b/extensions/qqbot/src/exec-approvals.ts @@ -216,3 +216,22 @@ export const isQQBotExecApprovalClientEnabled = qqbotExecApprovalProfile.isClien export const isQQBotExecApprovalApprover = qqbotExecApprovalProfile.isApprover; export const isQQBotExecApprovalAuthorizedSender = qqbotExecApprovalProfile.isAuthorizedSender; export const shouldHandleQQBotExecApprovalRequest = qqbotExecApprovalProfile.shouldHandleRequest; + +export function authorizeQQBotApprovalAction(params: { + cfg: OpenClawConfig; + accountId?: string | null; + senderId?: string | null; + approvalKind: "exec" | "plugin"; +}): { authorized: boolean; reason?: string } { + if (resolveQQBotExecApprovalConfig(params) === undefined) { + return { authorized: true }; + } + + const authorized = + params.approvalKind === "plugin" + ? isQQBotExecApprovalApprover(params) + : isQQBotExecApprovalAuthorizedSender(params); + return authorized + ? { authorized: true } + : { authorized: false, reason: "You are not authorized to approve this request." }; +}