mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-21 10:16:44 +00:00
Enforce Slack plugin approval button authorization [AI] (#80899)
* fix: enforce slack plugin approval button authorization * fix: enforce slack plugin approval button authorization * addressing codex review * addressing codex review * docs: add changelog entry for PR merge
This commit is contained in:
Pavan Kumar Gondhi
@@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai
|
||||
|
||||
### Fixes
|
||||
|
||||
- Enforce Slack plugin approval button authorization [AI]. (#80899) Thanks @pgondhi987.
|
||||
- Recognize PowerShell -ec inline commands [AI]. (#80893) Thanks @pgondhi987.
|
||||
- fix(qqbot): authorize approval button callbacks [AI]. (#80892) Thanks @pgondhi987.
|
||||
- Telegram: render supported HTML tags in streamed and durable replies instead of showing literal markup. (#80977)
|
||||
|
||||
@@ -6,10 +6,8 @@ import { resolveCommandAuthorization } from "openclaw/plugin-sdk/command-auth-na
|
||||
import { requestHeartbeat } from "openclaw/plugin-sdk/heartbeat-runtime";
|
||||
import { normalizeOptionalString } from "openclaw/plugin-sdk/string-coerce-runtime";
|
||||
import { enqueueSystemEvent } from "openclaw/plugin-sdk/system-event-runtime";
|
||||
import {
|
||||
isSlackExecApprovalApprover,
|
||||
isSlackExecApprovalAuthorizedSender,
|
||||
} from "../../exec-approvals.js";
|
||||
import { isSlackApprovalAuthorizedSender } from "../../approval-auth.js";
|
||||
import { isSlackExecApprovalAuthorizedSender } from "../../exec-approvals.js";
|
||||
import { dispatchSlackPluginInteractiveHandler } from "../../interactive-dispatch.js";
|
||||
import {
|
||||
SLACK_REPLY_BUTTON_ACTION_ID,
|
||||
@@ -544,7 +542,7 @@ async function handleSlackExecApprovalInteraction(params: {
|
||||
if (!approval) {
|
||||
return false;
|
||||
}
|
||||
const pluginApprovalAuthorizedSender = isSlackExecApprovalApprover({
|
||||
const pluginApprovalAuthorizedSender = isSlackApprovalAuthorizedSender({
|
||||
cfg: params.ctx.cfg,
|
||||
accountId: params.ctx.accountId,
|
||||
senderId: params.parsed.userId,
|
||||
@@ -555,9 +553,13 @@ async function handleSlackExecApprovalInteraction(params: {
|
||||
senderId: params.parsed.userId,
|
||||
});
|
||||
const isPluginApproval = approval.approvalId.startsWith("plugin:");
|
||||
const resolveUnprefixedAsPlugin =
|
||||
!isPluginApproval && !execApprovalAuthorizedSender && pluginApprovalAuthorizedSender;
|
||||
const authorized = isPluginApproval
|
||||
? pluginApprovalAuthorizedSender
|
||||
: execApprovalAuthorizedSender || pluginApprovalAuthorizedSender;
|
||||
: execApprovalAuthorizedSender || resolveUnprefixedAsPlugin;
|
||||
const allowPluginFallback =
|
||||
!isPluginApproval && execApprovalAuthorizedSender && pluginApprovalAuthorizedSender;
|
||||
if (!authorized) {
|
||||
params.ctx.runtime.log?.(
|
||||
`slack:interaction drop exec approval user=${params.parsed.userId} (not authorized)`,
|
||||
@@ -572,7 +574,8 @@ async function handleSlackExecApprovalInteraction(params: {
|
||||
approvalId: approval.approvalId,
|
||||
decision: approval.decision,
|
||||
senderId: params.parsed.userId,
|
||||
allowPluginFallback: pluginApprovalAuthorizedSender,
|
||||
allowPluginFallback,
|
||||
...(resolveUnprefixedAsPlugin ? { resolveMethod: "plugin" as const } : {}),
|
||||
clientDisplayName: `Slack approval (${params.parsed.userId.trim() || "unknown"})`,
|
||||
});
|
||||
} catch (error) {
|
||||
|
||||
@@ -952,7 +952,7 @@ describe("registerSlackInteractionEvents", () => {
|
||||
approvalId: "req-123",
|
||||
decision: "allow-once",
|
||||
senderId: "U123",
|
||||
allowPluginFallback: true,
|
||||
allowPluginFallback: false,
|
||||
clientDisplayName: "Slack approval (U123)",
|
||||
});
|
||||
expect(resolvePluginConversationBindingApprovalMock).not.toHaveBeenCalled();
|
||||
@@ -967,6 +967,220 @@ describe("registerSlackInteractionEvents", () => {
|
||||
expect(respond).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("resolves plugin approval buttons from plugin approvers", async () => {
|
||||
const { ctx, app, getHandler } = createContext({
|
||||
cfg: {
|
||||
channels: {
|
||||
slack: {
|
||||
accounts: {
|
||||
default: {
|
||||
allowFrom: ["U123OWNER"],
|
||||
execApprovals: {
|
||||
enabled: true,
|
||||
approvers: ["U999EXEC"],
|
||||
target: "both",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
registerSlackInteractionEvents({ ctx: ctx as never });
|
||||
|
||||
const handler = getHandler();
|
||||
|
||||
const ack = vi.fn().mockResolvedValue(undefined);
|
||||
const respond = vi.fn().mockResolvedValue(undefined);
|
||||
await handler({
|
||||
ack,
|
||||
respond,
|
||||
body: {
|
||||
user: { id: "U123OWNER" },
|
||||
channel: { id: "C1" },
|
||||
container: { channel_id: "C1", message_ts: "100.200" },
|
||||
message: {
|
||||
ts: "100.200",
|
||||
text: "Plugin approval required",
|
||||
blocks: [
|
||||
{
|
||||
type: "actions",
|
||||
block_id: "plugin_actions",
|
||||
elements: [{ type: "button", action_id: "openclaw:reply_button" }],
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
action: {
|
||||
type: "button",
|
||||
action_id: "openclaw:reply_button",
|
||||
block_id: "plugin_actions",
|
||||
value: "/approve plugin:req-123 allow-always",
|
||||
text: { type: "plain_text", text: "Always allow" },
|
||||
},
|
||||
});
|
||||
|
||||
expect(ack).toHaveBeenCalled();
|
||||
expect(resolveApprovalOverGatewayMock).toHaveBeenCalledWith({
|
||||
cfg: ctx.cfg,
|
||||
approvalId: "plugin:req-123",
|
||||
decision: "allow-always",
|
||||
senderId: "U123OWNER",
|
||||
allowPluginFallback: false,
|
||||
clientDisplayName: "Slack approval (U123OWNER)",
|
||||
});
|
||||
expect(resolvePluginConversationBindingApprovalMock).not.toHaveBeenCalled();
|
||||
expect(dispatchPluginInteractiveHandlerMock).not.toHaveBeenCalled();
|
||||
expect(enqueueSystemEventMock).not.toHaveBeenCalled();
|
||||
expectRecordFields(chatUpdateCall(app), {
|
||||
channel: "C1",
|
||||
ts: "100.200",
|
||||
text: "Plugin approval required",
|
||||
blocks: [],
|
||||
});
|
||||
expect(respond).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows unprefixed plugin approval fallback from plugin approvers", async () => {
|
||||
const { ctx, app, getHandler } = createContext({
|
||||
cfg: {
|
||||
channels: {
|
||||
slack: {
|
||||
accounts: {
|
||||
default: {
|
||||
allowFrom: ["U123OWNER"],
|
||||
execApprovals: {
|
||||
enabled: true,
|
||||
approvers: ["U999EXEC"],
|
||||
target: "both",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
registerSlackInteractionEvents({ ctx: ctx as never });
|
||||
|
||||
const handler = getHandler();
|
||||
|
||||
const ack = vi.fn().mockResolvedValue(undefined);
|
||||
const respond = vi.fn().mockResolvedValue(undefined);
|
||||
await handler({
|
||||
ack,
|
||||
respond,
|
||||
body: {
|
||||
user: { id: "U123OWNER" },
|
||||
channel: { id: "C1" },
|
||||
container: { channel_id: "C1", message_ts: "100.200" },
|
||||
message: {
|
||||
ts: "100.200",
|
||||
text: "Plugin approval required",
|
||||
blocks: [
|
||||
{
|
||||
type: "actions",
|
||||
block_id: "plugin_actions",
|
||||
elements: [{ type: "button", action_id: "openclaw:reply_button" }],
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
action: {
|
||||
type: "button",
|
||||
action_id: "openclaw:reply_button",
|
||||
block_id: "plugin_actions",
|
||||
value: "/approve req-legacy allow-once",
|
||||
text: { type: "plain_text", text: "Allow once" },
|
||||
},
|
||||
});
|
||||
|
||||
expect(ack).toHaveBeenCalled();
|
||||
expect(resolveApprovalOverGatewayMock).toHaveBeenCalledWith({
|
||||
cfg: ctx.cfg,
|
||||
approvalId: "req-legacy",
|
||||
decision: "allow-once",
|
||||
senderId: "U123OWNER",
|
||||
allowPluginFallback: false,
|
||||
resolveMethod: "plugin",
|
||||
clientDisplayName: "Slack approval (U123OWNER)",
|
||||
});
|
||||
expect(resolvePluginConversationBindingApprovalMock).not.toHaveBeenCalled();
|
||||
expect(dispatchPluginInteractiveHandlerMock).not.toHaveBeenCalled();
|
||||
expect(enqueueSystemEventMock).not.toHaveBeenCalled();
|
||||
expectRecordFields(chatUpdateCall(app), {
|
||||
channel: "C1",
|
||||
ts: "100.200",
|
||||
text: "Plugin approval required",
|
||||
blocks: [],
|
||||
});
|
||||
expect(respond).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects plugin approval buttons from exec-only approvers", async () => {
|
||||
const { ctx, app, getHandler } = createContext({
|
||||
cfg: {
|
||||
channels: {
|
||||
slack: {
|
||||
accounts: {
|
||||
default: {
|
||||
allowFrom: ["U123OWNER"],
|
||||
execApprovals: {
|
||||
enabled: true,
|
||||
approvers: ["U999EXEC"],
|
||||
target: "both",
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
registerSlackInteractionEvents({ ctx: ctx as never });
|
||||
|
||||
const handler = getHandler();
|
||||
|
||||
const ack = vi.fn().mockResolvedValue(undefined);
|
||||
const respond = vi.fn().mockResolvedValue(undefined);
|
||||
await handler({
|
||||
ack,
|
||||
respond,
|
||||
body: {
|
||||
user: { id: "U999EXEC" },
|
||||
channel: { id: "C1" },
|
||||
container: { channel_id: "C1", message_ts: "100.200" },
|
||||
message: {
|
||||
ts: "100.200",
|
||||
text: "Plugin approval required",
|
||||
blocks: [
|
||||
{
|
||||
type: "actions",
|
||||
block_id: "plugin_actions",
|
||||
elements: [{ type: "button", action_id: "openclaw:reply_button" }],
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
action: {
|
||||
type: "button",
|
||||
action_id: "openclaw:reply_button",
|
||||
block_id: "plugin_actions",
|
||||
value: "/approve plugin:req-123 allow-always",
|
||||
text: { type: "plain_text", text: "Always allow" },
|
||||
},
|
||||
});
|
||||
|
||||
expect(ack).toHaveBeenCalled();
|
||||
expect(resolveApprovalOverGatewayMock).not.toHaveBeenCalled();
|
||||
expect(resolvePluginConversationBindingApprovalMock).not.toHaveBeenCalled();
|
||||
expect(dispatchPluginInteractiveHandlerMock).not.toHaveBeenCalled();
|
||||
expect(enqueueSystemEventMock).not.toHaveBeenCalled();
|
||||
expect(app.client.chat.update).not.toHaveBeenCalled();
|
||||
expect(respond).toHaveBeenCalledWith({
|
||||
text: "You are not authorized to approve this request.",
|
||||
response_type: "ephemeral",
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps exec approval buttons when gateway resolution fails", async () => {
|
||||
resolveApprovalOverGatewayMock.mockRejectedValueOnce(new Error("gateway down"));
|
||||
const { ctx, app, getHandler } = createContext();
|
||||
|
||||
@@ -66,6 +66,21 @@ describe("resolveApprovalOverGateway", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("routes explicit plugin resolution through plugin.approval.resolve", async () => {
|
||||
await resolveApprovalOverGateway({
|
||||
cfg: {} as never,
|
||||
approvalId: "approval-1",
|
||||
decision: "allow-once",
|
||||
resolveMethod: "plugin",
|
||||
});
|
||||
|
||||
expect(hoisted.clientRequest).toHaveBeenCalledTimes(1);
|
||||
expect(hoisted.clientRequest).toHaveBeenCalledWith("plugin.approval.resolve", {
|
||||
id: "approval-1",
|
||||
decision: "allow-once",
|
||||
});
|
||||
});
|
||||
|
||||
it("falls back to plugin.approval.resolve only for not-found exec approvals when enabled", async () => {
|
||||
const notFoundError = Object.assign(new Error("unknown or expired approval id"), {
|
||||
gatewayCode: "APPROVAL_NOT_FOUND",
|
||||
|
||||
@@ -9,6 +9,7 @@ type ResolveApprovalOverGatewayParams = {
|
||||
decision: ExecApprovalDecision;
|
||||
senderId?: string | null;
|
||||
allowPluginFallback?: boolean;
|
||||
resolveMethod?: "plugin";
|
||||
gatewayUrl?: string;
|
||||
clientDisplayName?: string;
|
||||
};
|
||||
@@ -32,7 +33,7 @@ export async function resolveApprovalOverGateway(
|
||||
decision: params.decision,
|
||||
});
|
||||
};
|
||||
if (params.approvalId.startsWith("plugin:")) {
|
||||
if (params.resolveMethod === "plugin" || params.approvalId.startsWith("plugin:")) {
|
||||
await requestResolve("plugin.approval.resolve");
|
||||
return;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user